D14666: Fixes memory leak in KUrlComboBox::setUrl

[David Hallas]

hallas closed this revision.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14666

To: hallas, dfaure
Cc: dfaure, kde-frameworks-devel, michaelh, ngraham, bruns

D14666: Fixes memory leak in KUrlComboBox::setUrl

[David Hallas]

hallas updated this revision to Diff 39393.
hallas added a comment.


  Rebased

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14666?vs=39265=39393

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D14666

AFFECTED FILES
  autotests/kurlcomboboxtest.cpp
  autotests/kurlcomboboxtest.h
  src/widgets/kurlcombobox.cpp

To: hallas, dfaure
Cc: dfaure, kde-frameworks-devel, michaelh, ngraham, bruns

D14666: Fixes memory leak in KUrlComboBox::setUrl

[David Faure]

dfaure added a comment.


  Yes, I think you should apply for one. You can quote my name when the form 
asks who recommended you.

REPOSITORY
  R241 KIO

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D14666

To: hallas, dfaure
Cc: dfaure, kde-frameworks-devel, michaelh, ngraham, bruns

D14666: Fixes memory leak in KUrlComboBox::setUrl

[David Faure]

dfaure accepted this revision.
dfaure added a comment.
This revision is now accepted and ready to land.


  Very nice work, thanks!
  
  Do you have a KDE contributor account, to push this commit?

REPOSITORY
  R241 KIO

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D14666

To: hallas, dfaure
Cc: dfaure, kde-frameworks-devel, michaelh, ngraham, bruns

D14666: Fixes memory leak in KUrlComboBox::setUrl

[David Hallas]

hallas added a comment.


  In D14666#305209 , @dfaure wrote:
  
  > Very nice work, thanks!
  >
  > Do you have a KDE contributor account, to push this commit?
  
  
  No, not yet - do you think I should apply for one? I would really like to 
continue posting patches :)

REPOSITORY
  R241 KIO

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D14666

To: hallas, dfaure
Cc: dfaure, kde-frameworks-devel, michaelh, ngraham, bruns

D14666: Fixes memory leak in KUrlComboBox::setUrl

[David Hallas]

hallas added inline comments.

INLINE COMMENTS

> dfaure wrote in kurlcombobox.cpp:358
> problem: `i` shouldn't be in increased after this...
> 
> Maybe this should use iterators instead?
> 
> A unitttest is missing for this code path, in any case.

In general this class is not covered very well by unit test, I have added a 
test case that covers most of the removeUrl functionality, but I think I will 
do a different patch with more unit test.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14666

To: hallas, dfaure
Cc: dfaure, kde-frameworks-devel, michaelh, ngraham, bruns

D14666: Fixes memory leak in KUrlComboBox::setUrl

[David Hallas]

hallas updated this revision to Diff 39265.
hallas marked 2 inline comments as done.
hallas added a comment.


  Use remove_if for removing entries from itemList and defaultList.
  Added unit test of KUrlComboBox::removeUrl.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14666?vs=39252=39265

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D14666

AFFECTED FILES
  autotests/kurlcomboboxtest.cpp
  autotests/kurlcomboboxtest.h
  src/widgets/kurlcombobox.cpp

To: hallas, dfaure
Cc: dfaure, kde-frameworks-devel, michaelh, ngraham, bruns

D14666: Fixes memory leak in KUrlComboBox::setUrl

[David Faure]

dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.


  Thanks ;)

INLINE COMMENTS

> kurlcombobox.cpp:358
> +if (d->itemList.at(i).get() == mit.value()) {
> +d->itemList.erase(d->itemList.begin() + i);
> +break;

problem: `i` shouldn't be in increased after this...

Maybe this should use iterators instead?

A unitttest is missing for this code path, in any case.

> kurlcombobox.cpp:365
> +if (d->defaultList.at(i).get() == mit.value()) {
> +d->defaultList.erase(d->defaultList.begin() + i);
> +break;

same here.

Maybe erase(remove_if()) would be best.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14666

To: hallas, dfaure
Cc: dfaure, kde-frameworks-devel, michaelh, ngraham, bruns

D14666: Fixes memory leak in KUrlComboBox::setUrl

[David Hallas]

hallas updated this revision to Diff 39252.
hallas added a comment.


  Refactored to use std::vector and std::unique_ptr

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14666?vs=39249=39252

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D14666

AFFECTED FILES
  autotests/kurlcomboboxtest.cpp
  autotests/kurlcomboboxtest.h
  src/widgets/kurlcombobox.cpp

To: hallas
Cc: dfaure, kde-frameworks-devel, michaelh, ngraham, bruns

D14666: Fixes memory leak in KUrlComboBox::setUrl

[David Faure]

dfaure resigned from this revision.
dfaure added a comment.


  The ownership was clear in the original code: itemList and defaultList own 
the items, the rest (like itemMapper) doesn't.
  So unique_ptrs in itemList and defaultList, and raw pointers elsewhere, would 
be perfectly fine and much clearer.
  Don't fall into the "no raw pointers" trap. The correct recommendation is "no 
raw pointers that own the object".

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14666

To: hallas
Cc: dfaure, kde-frameworks-devel, michaelh, ngraham, bruns

D14666: Fixes memory leak in KUrlComboBox::setUrl

[David Hallas]

hallas added a comment.


  In D14666#304848 , @dfaure wrote:
  
  > Why shared? I don't see any ownership sharing happening. The list owns the 
pointers. I'm always wary of shared_ptr because it's often overused as "we 
don't really know who's responsible for deleting this, so let's just refcount 
it". But there's no refcounting needed here. The ownership is very clear here.
  
  
  My first approach was to use unique_ptr, but then I realized that pointers 
are shared between the defaultList and the itemMapper (see for example line 
164) and also between the itemList and the itemMapper (see line 278, 291 and 
others), but I can't see any sharing between the defaultList and itemList. I 
thought of using a std::weak_ptr in the itemMapper, but I am not sure that 
entries are always removed from itemMapper when the are removed from the 
defaultList or itemMapper, also if the itemMapper should be able to hold 
objects from both lists I think it is more sane to use a shared_ptr.
  
  Let me know what you think

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14666

To: hallas, dfaure
Cc: dfaure, kde-frameworks-devel, michaelh, ngraham, bruns

D14666: Fixes memory leak in KUrlComboBox::setUrl

[David Faure]

dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.


  Why shared? I don't see any ownership sharing happening. The list owns the 
pointers. I'm always wary of shared_ptr because it's often overused as "we 
don't really know who's responsible for deleting this, so let's just refcount 
it". But there's no refcounting needed here. The ownership is very clear here.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14666

To: hallas, dfaure
Cc: dfaure, kde-frameworks-devel, michaelh, ngraham, bruns

D14666: Fixes memory leak in KUrlComboBox::setUrl

[David Hallas]

hallas updated this revision to Diff 39249.
hallas added a comment.


  Refactored the code to use std::shared pointers instead.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14666?vs=39240=39249

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D14666

AFFECTED FILES
  autotests/kurlcomboboxtest.cpp
  autotests/kurlcomboboxtest.h
  src/widgets/kurlcombobox.cpp

To: hallas, dfaure
Cc: dfaure, kde-frameworks-devel, michaelh, ngraham, bruns

D14666: Fixes memory leak in KUrlComboBox::setUrl

[David Faure]

dfaure added a comment.


  std::unique_ptr sounds fine indeed.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14666

To: hallas, dfaure
Cc: dfaure, kde-frameworks-devel, michaelh, ngraham, bruns

D14666: Fixes memory leak in KUrlComboBox::setUrl

[David Hallas]

hallas added inline comments.

INLINE COMMENTS

> dfaure wrote in kurlcombobox.cpp:363
> Doesn't this have the same issue? delete missing

Sure has :) I was thinking if a nicer solution would be to use a smart pointer 
in the itemList instead? That way we wouldn't need to go hunting for this and 
risk introducing leaks again? Do we have any favorite smart pointer for this? 
Or would a std::unique_ptr be fine?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14666

To: hallas, dfaure
Cc: dfaure, kde-frameworks-devel, michaelh, ngraham, bruns

D14666: Fixes memory leak in KUrlComboBox::setUrl

[David Faure]

dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> kurlcombobox.cpp:279
>  Q_ASSERT(!d->itemList.isEmpty());
> +delete d->itemList.last();
>  d->itemList.removeLast();

This could be merged with the next line, by doing

  delete d->itemList.takeLast();

> kurlcombobox.cpp:363
>  if (url.toString(QUrl::StripTrailingSlash) == 
> mit.value()->url.toString(QUrl::StripTrailingSlash)) {
>  if (!d->itemList.removeAll(mit.value()) && checkDefaultURLs) {
>  d->defaultList.removeAll(mit.value());

Doesn't this have the same issue? delete missing

> kurlcombobox.cpp:364
>  if (!d->itemList.removeAll(mit.value()) && checkDefaultURLs) {
>  d->defaultList.removeAll(mit.value());
>  }

same here, right?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14666

To: hallas, dfaure
Cc: dfaure, kde-frameworks-devel, michaelh, ngraham, bruns

D14666: Fixes memory leak in KUrlComboBox::setUrl

[David Hallas]

hallas created this revision.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: kde-frameworks-devel.
hallas requested review of this revision.

REVISION SUMMARY
  Subject: Fixes memory leak in KUrlComboBox::setUrl
  
  Details:
  
  If setUrl is called multiple times it first removes the previous item
  but fails to delete the item before removing it causing a memory leak.

TEST PLAN
  Unit test

REPOSITORY
  R241 KIO

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D14666

AFFECTED FILES
  autotests/kurlcomboboxtest.cpp
  autotests/kurlcomboboxtest.h
  src/widgets/kurlcombobox.cpp

To: hallas
Cc: kde-frameworks-devel, michaelh, ngraham, bruns