QML object access through model crashes

[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.

[JoeCFD]

@kshegunov why is the unique pointer even needed in the first place?
to @mzimmers: is OutcomeModel a subclass of Qt class? I can not see it.

[kshegunov]

@JoeCFD said in QML object access through model crashes:

@kshegunov why is the unique pointer even needed in the first place?

It isn't, strictly speaking. If it were me, I'd've implemented it with regular ol' raw pointers. The difference is only in the semantics - std::unique_ptr means "I own the stuff" (also implying you can't have two different std::unique_ptr instances pointing to the same object, hence the copy constructor being deleted).

to @mzimmers: is OutcomeModel a subclass of Qt class? I can not see it.

Both the model and the Outcome class are derived from Qt classes. I imagine the OutcomeModel is derived from QAbstractTableModel, where as the Outcome is derived from QObject (or something akin).

[mzimmers]

@JoeCFD said in QML object access through model crashes:

is OutcomeModel a subclass of Qt class?

Yes, a subclass of QAbstractListModel. And Outcome is a subclass of QObject.

[JoeCFD]

@kshegunov Ok. Qt has its own garbage collection mechanism. Therefore, any derived classes from Qt classes may not need any shared or unique pointers unless they are needed to be deleted immediately for example inside one func call(sometimes I do it). I guess in most cases, raw pointers are good enough while Qt will take care of memory release.
In his case, the pointer is used(returned). I guess a raw pointer is good enough.

[GrecKo]

@JoeCFD the advantage of using an owning smart pointer in a container is that when removing an element from a container, the pointed-to object will be automatically deleted.

@mzimmers the question is now why are you using this getOutcome() function at all in your model?

How do you expose the uuid in the first place? Do you get it from your OutcomeList? Can't you directly add a role returning an Outcome*?

[JoeCFD]

@GrecKo That is ok. This case is same as I do in a func call. But, I do not think his usage of shared or unique pointer in this func is correct.

raw pointers will work for mzimmers as well. He got confused with shared or unique pointers.

[mzimmers]

@GrecKo I'll do my best to explain this briefly yet clearly.

My app has several models. One model is the SpaceModel. The SpaceModel contains a list of Space objects. Each Space contains a list of Outcome UUIDs, which reference items in the OutcomeModel.

I did this because both Spaces and Outcomes are distinct, independent entities in my system (neither "owns" the other), and this was the best I could think of.

So, the getOutcome() is to give the Space objects visibility into the Outcome properties.

I have no doubt there's a better way to do this, but this was the best I could come up with.

[GrecKo]

@mzimmers alright, that seems sensible enough.

@JoeCFD said in QML object access through model crashes:

@GrecKo That is ok. This case is same as I do in a func call. But, I do not think his usage of shared or unique pointer in this func is correct.
raw pointers will work for mzimmers as well. He got confused with shared or unique pointers.

I didn't understand what you mean.
If the OutcomeModel class actually owns the Outcome instances, using a container with owning smart pointers looks correct to me.

[mzimmers]

Hi all -

Sorry, but I have to re-open this; problem still exists after all.

Summary (as brief as I can make it):

I have a model for a class I've defined:

class Outcome : public QObject
{
    Q_OBJECT
    QML_ELEMENT

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

class OutcomeModel : public QAbstractListModel
{
    Q_OBJECT
    QML_ELEMENT

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

These outcome objects have a N:N relation with another class Space. I maintain the relations in a list in my Space class:

class Space : public QObject {
    Q_OBJECT
    QML_ELEMENT
    QList<QUuid> m_outcomeList;
    Q_PROPERTY(QList<QUuid> outcomeList READ outcomeList WRITE setOutcomeList NOTIFY outcomeListChanged FINAL)
    // all the stuff for above Q_PROPERTY is defined and seems to work.

The size of m_outcomeList can be from 0 to arbitrarily large. (I also have a Space model.)

In QML, I have a Space detail card with the following:

Pane {
    id: spaceDetail
    property Space space
    property var outcomeList: space.outcomeList
    ListView {
        model: spaceDetail.outcomeList
        delegate: outcomeComponent
        Component {
            id: outcomeComponent
            RowLayout {
                property Outcome outcome: outcomeModel.getOutcome(modelData)
                Label {
                    text: outcome !== null ? outcome.name : "no name available"
                }
                // and so on.

When I navigate to a space detail for a space that has a non-0 outcome list, everything works fine. When I then navigate to another space detail, two bad things happen:

1. the Outcome destructor is invoked for a valid outcome (should not be invoked)
2. the program crashes after a few seconds. The stack pointer points to stuff within my destructor (which does nothing other than a qDebug().
   

I have tried this with smart pointers and raw pointers, with the same result.

I know this is a lot to digest, but...can anyone see ANYTHING wrong with what I'm doing?

[GrecKo]

@mzimmers it appears you have an ownership problem.

Who owns the std::shared_ptr<Outcome> in OutcomeModel and who are they shared with? Do they have a QObject parent?