Ubuntu Online Accounts API

Merge lp://staging/~michihenning/online-accounts-api/leak-fix into lp://staging/online-accounts-api

leak-fix
Merge into trunk

Proposed by Michi Henning on 2016-12-21

Status:

Merged

Approved by:

Alberto Mardegan on 2016-12-21

Approved revision:

Merged at revision:

Proposed branch:

lp://staging/~michihenning/online-accounts-api/leak-fix

Merge into:

lp://staging/online-accounts-api

Diff against target:

42 lines (+4/-2)

3 files modified

src/lib/OnlineAccounts/pending_call.h (+2/-1)
src/lib/OnlineAccountsDaemon/i18n.cpp (+1/-1)
src/lib/Ubuntu/OnlineAccounts.2/account.cpp (+1/-0)

To merge this branch:

bzr merge lp://staging/~michihenning/online-accounts-api/leak-fix

Medium

Confirmed

Link a bug report

Reviewer	Review Type	Date Requested	Status
Alberto Mardegan (community)		2016-12-21	Approve on 2016-12-21
Review via email: mp+313667@code.staging.launchpad.net

Commit message

Fixed memory leak in PendingCallWatcher.
Got rid of compiler warning.
Added missing \endlist to documentation.

Description of the change

Fixed memory leak in PendingCallWatcher.
Got rid of compiler warning.
Added missing \endlist to documentation.

This would not have happened with std::unique_ptr or QScopedPointer. I strongly recommend to never use raw pointers. As this particular example demonstrates, it is easy to get even the most simple case wrong. It took me considerable time to track this down; much better to not allow this type of error to ever arise in the first place.

Revision history for this message

Alberto Mardegan (mardy) wrote on 2016-12-21:

Hi Michi! I still do prefer an explicit delete, when the lifetime of the object is as simple as in this case (and yet I forgot it! :-) ).
If you still decide to go for the QScopedPointer, please add its include file to pending_call.h (now it works even without it, but compilation might break on a future Qt release).

review: Needs Fixing

lp://staging/~michihenning/online-accounts-api/leak-fix updated on 2016-12-21

44. By Michi Henning on 2016-12-21: Added include for QScopedPointer.

Revision history for this message

Michi Henning (michihenning) wrote on 2016-12-21:

Well, if this doesn't convince you, I don't know what will. This has cost us quite a few hours. And, by using QScopedPointer or unique_ptr, this bug could never have happened in the first place.

I cannot for the life of me see what writing the code with a raw pointer and an explicit delete is preferable when, with a smart pointer, no code is needed at all.

Revision history for this message

Alberto Mardegan (mardy) wrote on 2016-12-21:

It's just that old habits are hard to die, but hey, I'm trying to change :-)

review: Approve

Revision history for this message

Michi Henning (michihenning) wrote on 2016-12-21:

The bug was introduced here: https://code.launchpad.net/~mardy/online-accounts-api/style/+merge/249168