Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes for memory leaks and QObject* ownership bugs #201

Open
wants to merge 62 commits into
base: master
Choose a base branch
from

Conversation

412b
Copy link
Contributor

@412b 412b commented May 20, 2015

Due to returning QObject* in calls to C++ objects from QML (e.g.
getItem(i)), those objects were changing ownership to QML and were
subject to gc. That could've been noticed during switching between
journey results and journey details results (instantly switch back and
forth to create enough garbage).
var item = result.getItem(i);
item would be null after some time. So it was ending up in page
showing loader and not doing anything or in a bit more rare cases with
segfault.

Due to returning QObject* in calls to C++ objects from QML (e.g.
getItem(i)), those objects were changing ownership to QML and were
subject to gc. That could've been noticed during switching between
journey results and journey details results (instantly switch back and
forth to create enough garbage).
`var item = result.getItem(i);`
`item` would be null after some time. So it was ending up in page
showing loader and not doing anything or in a bit more rare cases with
segfault.
Requires:
- sailfishsilica-qt5 >= 0.10.9
- qt5-qtdeclarative-import-positioning
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, my guess is, this also auto installs the position lib in emulator.
Only worry is, that this also get catched by harbour store validator :) (as far as i know using gps is still against the harbour rules)

If you have any insights on this, or should we wait and see? :D

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this change relevant to QObject ownership change at all?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that change got there by mistake mostly, but w/o it you can not run it in emu w/o manually installing packages. it can also affect users, which have no positioning dependant apps installed (if that is possible at all :))
that said, that change can be undone.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@leppa
Copy link
Collaborator

leppa commented May 21, 2015

It looks like we were leaking memory after each search before, and will be leaking even more with this change. At the end everything gets parented to lastJourneyResultList, however, we never delete it - we just assign NULL. I think that adding delete lastJourneyResultList before calling lastJourneyResultList = NULL should be enough, but it might make sense to do some memory profiling.

@412b
Copy link
Contributor Author

412b commented May 21, 2015

This changes wasn't intented to fix memory leaks in c++ part of the app.
calling delete will help only with a proper destructor in place and that one is missing at the moment.
i can push the needed changes from my other branch.

@leppa
Copy link
Collaborator

leppa commented May 21, 2015

Not completely true. QObject deletes all its children when it's deleted, and you do parent all JourneyResultItem instances to lastJourneyResultList and all JourneyDetailResultItem instances to corresponding JourneyResultItem instances. So, deleting lastJourneyResultList would delete all of them.

@412b
Copy link
Contributor Author

412b commented May 21, 2015

I was talking about a generic way.
As long as setting parent is properly done it is ok in this particular case.
As far as I remember, when you're building classes' hierarchy (like QObject -> Class A -> Class B), then for the children of type B to be deleted you need to have a virtual destructor in A.

@leppa
Copy link
Collaborator

leppa commented May 21, 2015

Not sure. All of the classes in the created object tree inherit from QObject. As they don't have a destructor, QObject's destructor (which is virtual) will be called for all of them, and it will delete all it's children.

A more proper solution would, of course, be destructors that call qDeleteAll() on JourneyResultList::m_items and JourneyDetailResultList::m_items.

I was referring to this particular case and your change. The one-liner I suggest would already prevent quite a big leak. I'm pretty sure that there are other places where we leak memory and they need to be investigated, too.

412b and others added 23 commits May 25, 2015 00:46
Due to returning QObject* in calls to C++ objects from QML (e.g.
getItem(i)), those objects were changing ownership to QML and were
subject to gc. That could've been noticed during switching between
journey results and journey details results (instantly switch back and
forth to create enough garbage).
`var item = result.getItem(i);`
`item` would be null after some time. So it was ending up in page
showing loader and not doing anything or in a bit more rare cases with
segfault.
@412b 412b changed the title Fix for QObject* ownership bugs Fixes for memory leaks and QObject* ownership bugs Jun 19, 2015
@412b
Copy link
Contributor Author

412b commented Jun 19, 2015

@smurfy seems like at this point I better stop :) I've done more or less basic checks, no segfaults during that period. though there is plenty of things to improve, for this pr there are too many commmits already.

@412b
Copy link
Contributor Author

412b commented Jun 23, 2015

there is a regression in hafas parser, which I will adress in next commit.

@412b
Copy link
Contributor Author

412b commented Jun 23, 2015

seems to be it for this PR :)

@balcy
Copy link

balcy commented Apr 15, 2019

this is PR is definately needed for Ubuntu touch, without it the app crashes if you switch between the connections overview and the connections details for a few times.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants