QML object access through model crashes

[jeremy_k]

@mzimmers said in QML object access through model crashes:

Hi all -

My app is doing something rather strange - upon entering a QML component

What does entering mean?

Outcome* OutcomeModel::getOutcome(const QUuid &uuid)
{
    auto outcome {std::make_shared<Outcome>(this)};
    const auto i { getIndex(uuid) };
    if (i == NOT_IN_LIST) {
        qWarning() << __PRETTY_FUNCTION__ << "uuid not found in list.";
    } else {
        outcome = m_list.at(i);
    }
    return outcome.get();
}

This looks extremely suspicious. The QML engine doesn't know about shared_ptr, and the get() is stripping that information anyway. The engine presumes ownership of created objects unless told otherwise. That means that when the object goes out of scope, the engine will eventually invoke the Outcome destructor. Holding a shared pointer to it elsewhere won't prevent that.

If I understand the else section, under some circumstance the OutcomeModel factory will attempt to reuse a previously created object that may have been deleted by the engine.

[mzimmers]

@jeremy_k said in QML object access through model crashes:

What does entering mean?

I mean when a component is loaded.

In this case, the list itself is "good" -- that is, there have been only additions to it, no deletions. My list is declared like this:

typedef QList<std::shared_ptr<Outcome>> OutcomeList;
OutcomeList m_list;

I don't really need to maintain a list of pointers; my list could be the actual objects, but then code like this:

outcome = m_list.at(listIndex);

becomes problematic because, as a QObject-derived class, Outcome has no copy c'tor. So, I'm not sure what the best way to go is here.

[mzimmers]

@JoeCFD said in QML object access through model crashes:

you are returning the raw pointer of outcome from this func. And outcome is destroyed after this func call.

But isn't the pointer itself still valid (meaning it's pointing to the correct item in the list)?

[JoeCFD]

@mzimmers It is a dangled pointer after return.

[jeremy_k]

@mzimmers said in QML object access through model crashes:

@JoeCFD said in QML object access through model crashes:

you are returning the raw pointer of outcome from this func. And outcome is destroyed after this func call.

But isn't the pointer itself still valid (meaning it's pointing to the correct item in the list)?

Add some logging to the destructor to see when the object is destroyed.

[kshegunov]

Also, unless you're fan of pain and suffering don't hold QObjects in shared pointers. You're basically saying that the object owns itself (semantically) and have no guarantee when this object is going to be freed. This in turn may cause you to execute a slot on an object that's getting/got deleted.

If you want to own a QObject use std::unique_ptr/QScopedPointer, if you're going to hold a weak reference to the object (i.e. to hold a pointer without owning the actual object) then use QPointer.

[mzimmers]

I added a member to my OutcomeModel:

    Outcome *m_outcome;

and modified my get routine:

Outcome *OutcomeModel::getOutcome(const QUuid &uuid)
{
    bool found = false;
    m_outcome = nullptr;
    for (const auto &o: m_list) {
        if (o->uuid() == uuid) {
            m_outcome = o.get();
            found = true;
            break;
        }
    }
    if (!found) {
        m_outcome = m_list.at(0).get();
    }
    return m_outcome;
}

(I realize this isn't the desired ultimate behavior, but I wanted to try to ensure that m_outcome would always point to something valid - the list will always contain at least one item).

I'm getting the same behavior. Obviously I'm missing something, but I don't see what it is.

[kshegunov]

As usual, if we are tracking a segfault here, do provide a stack trace.

[mzimmers]

@kshegunov I can't - the error occurs somewhere within QObject and there's no RTTI information for it.

BTW: I'm not ignoring your suggestion about shared_ptr; I just want to get this figured out first.

[JonB]

@mzimmers said in QML object access through model crashes:

@kshegunov I can't - the error occurs somewhere within QObject and there's no RTTI information for it.

I know you mentioned this elsewhere, but this ought not stop you getting a stack trace from a seg fault. It might stop you seeing the internals of a QObject, but are you saying this is somehow an "error" in the debugger which prevents you accessing a stack trace? Even if it does inside a QObject, you should still see information about where it was in your or Qt code.

[kshegunov]

@mzimmers said in QML object access through model crashes:

BTW: I'm not ignoring your suggestion about shared_ptr; I just want to get this figured out first.

Substitute your shared_ptr with QPointer and when you fill in that list use new. At the point of crash if you are seeing dereferencing a nullptr, then something's not right with the ownership; that is to say something (probably QML) took ownership of the objects and freed them and you were left with dangling pointers all over the place.

PS.
You should almost never use shared_ptr::get, which erases the notion of the control block and who/when this object is going to be culled.

[mzimmers]

Thanks to your suggestions, I think I'm making progress. In my OutcomeModel, I've eliminated the Outcome element, and done the following:

typedef QPointer<Outcome> OutcomePtr;
typedef QList<OutcomePtr> OutcomeList;
OutcomePtr OutcomeModel::getOutcome(const QUuid &uuid)
{
   // auto outcome {std::make_shared<Outcome>(this)};
   OutcomePtr outcome { new Outcome };
   const auto i { getIndex(uuid) };
   if (i == NgaUI::NOT_IN_LIST) {
       qWarning() << __PRETTY_FUNCTION__ << "uuid not found in list."; // I never see this warning.
   } else {
       delete outcome;
       outcome = m_list.at(i);
   }
   return outcome.data();
}

I'm no longer getting segmentation faults, so I think we may have solved that problem. So, now the remaining issue is how to use the QPointer to access the Outcome properties from my QML? I tried this:

ListView {
    model: outcomeList // a list of UUIDs
    delegate: RowLayout {
        Label {
            text: "running: " + outcomeModel.getOutcome(modelData).isRunning

but the Label shows as undefined. I've verified that the getOutcome() routine seems to be returning good data. Any ideas what I'm doing wrong?

Thanks...

[JoeCFD]

@mzimmers delegate in ListView is a component from here:
https://doc.qt.io/qt-6/qml-qtquick-listview.html#delegate-prop
Is RowLayout a component?

[mzimmers]

@JoeCFD if I understand your point, I need to do this instead:

ListView {
    model: outcomeList
    delegate: rowComponent
    Component {
        id: rowComponent
        RowLayout {
            Label {
                id: runningLabel
                text: "running: " + outcomeModel.getOutcome(modelData).isRunning

Is this what you were getting at? The behavior seems the same.

Thanks...

[GrecKo]

Any ideas what I'm doing wrong?

To be blunt I'd say having trouble explaining what your issue is and blindly following strangers advices.

QPointer is an observing pointer. Don't use it to keep ownership.
shared_ptr was fine, unique_ptr may have been better. raw pointers could also be used if using the QObject parent ownership system, it would require to manually delete the object on removal though.

Your unusual code rules do not help you there (the "always init your variables at the start of a function" and "only one return").

Your getOutcome function is leaking (when not finding the uuid, doing unnecessary temporary allocation when finding it).

the Label shows as undefined

Is the text "undefined", "running: undefined", something else?
what does outcomeModel.getOutcome(modelData) returns?

The source issue of your problem was most likely that your object has no QObject::parent, thus the QML engine taking ownership of it when it access it from your Q_INVOKABLE.

https://doc.qt.io/qt-6/qtqml-cppintegration-data.html#data-ownership

I'd go back to using shared_ptr or unique_ptr and making sure to set a parent to your contained objects so the QML engine don't take ownership of it.

Keep in mind I'm also an internet stranger, so don't apply blindly what I'm claiming.

[mzimmers]

@GrecKo points taken.

@GrecKo said in QML object access through model crashes:

Your unusual code rules do not help you there (the "always init your variables at the start of a function" and "only one return").
Your getOutcome function is leaking (when not finding the uuid, doing unnecessary temporary allocation when finding it).

Is this better? EDIT: I can't do this with unique_ptr; the second return statement is invalid.

OutcomePtr OutcomeModel::getOutcome(const QUuid &uuid)
{
    const auto i { getIndex(uuid) };
    if (i == NgaUI::NOT_IN_LIST) {
        qWarning() << __PRETTY_FUNCTION__ << "uuid not found in list.";
        return nullptr;
    } else {
        return m_list.at(i);
    }
}

@GrecKo said in QML object access through model crashes:

Is the text "undefined", "running: undefined", something else?

"running: undefined"

@GrecKo said in QML object access through model crashes:

what does outcomeModel.getOutcome(modelData) returns?

I also notice that the address of outcome doesn't match the address of m_list[2], which I find somewhat strange (if these were old-fashioned pointers, it should, I believe).
EDIT: this screenshot might be more helpful (this was using a shared_ptr):

I do notice that now the addresses of outcome and m_list[2] agree, so I guess this is progress. Still having the "undefined" issue, though.

[mzimmers]

Update: I've got something now that works (doesn't crash and properly updates the QML). Here's the C++:

typedef std::shared_ptr<Outcome> OutcomePtr;
typedef QList<OutcomePtr> OutcomeList;

Outcome *OutcomeModel::getOutcome(const QUuid &uuid)
{
    const auto i { getIndex(uuid) };
    if (i == NgaUI::NOT_IN_LIST) {
        return nullptr;
    } else {
        OutcomePtr outcome(m_list.at(i));
        return outcome.get();
    }
}

And the QML:

ListView {
    model: outcomeList
    delegate: rowComponent
    Component {
        id: rowComponent
        RowLayout {
            property Outcome outcome: outcomeModel.getOutcome(modelData)
            Label {
                text: "running: " + outcome.isRunning
            }

So, I have 2 follow up questions:

1. if I were to use unique_ptrs instead of shared_ptrs, how do I code this line? I can't understand from the docs.

    OutcomePtr outcome(m_list.at(i));

2. any other comments/room for improvement that anyone can see?

Thanks to everyone for the help on this...

[kshegunov]

@GrecKo said in QML object access through model crashes:

To be blunt I'd say having trouble explaining what your issue is and blindly following strangers advices.

Hey, I take pride in my internet stranger's random advices.

QPointer is an observing pointer. Don't use it to keep ownership.

As already stated ...

@mzimmers said in QML object access through model crashes:

if I were to use unique_ptrs instead of shared_ptrs, how do I code this line? I can't understand from the docs.

You don't. unique_ptr/QScopedPointer is an owning wrapper. You'd return the underlying raw pointer instead (which you already do with shared_ptr).
I believe this is what you're looking for: https://en.cppreference.com/w/cpp/memory/unique_ptr/get

[mzimmers]

@kshegunov said in QML object access through model crashes:

You'd return the underlying raw pointer instead

Got it. Now, how about something like this:

auto outcome { std::make_unique<Outcome>(this) };
const auto listIndex { getIndex(uuid) };

if (listIndex == NgaUI::NOT_IN_LIST) { // will append to list below.
    outcome->setUuid(uuid);
} else {
    outcome = m_list.at(listIndex);
}

This is used in a function that updates the model, either by adding items to the model list, or updating the existing items. The last line won't compile. I could delete and re-create the pointer here, but I suspect there's a better way to go about this.

[kshegunov]

@mzimmers said in QML object access through model crashes:
Well assuming this is similar to the code we saw above:

if (listIndex != NgaUI::NOT_IN_LIST)
    return m_list.at(listIndex).get();

auto outcome = std::make_unique<Outcome>(this);
outcome->setUuid(uuid);
return m_list.emplaceBack(std::move(outcome)).get();

Or something of this sort, I imagine.