Review Request 121161: Set KIO::Integration::AccessManager to null so we don't crash on close.

[Thomas Lübking]

> On Nov. 17, 2014, 9:45 nachm., Thomas Lübking wrote:
> > attica-kde/kdeplugin/kdeplatformdependent.cpp, line 56
> > <https://git.reviewboard.kde.org/r/121161/diff/2/?file=328928#file328928line56>
> >
> >     is
> >     
> >     KdePlatformDependent::~KdePlatformDependent()
> >     {
> >        if (QCoreApplication::closingDown())
> >           m_accessManager->setParent(nullptr);
> >     }
> >     
> >     sufficient?
> 
> Albert Astals Cid wrote:
>     Isn't that a more complex way of doing the same?
> 
> Thomas Lübking wrote:
>     Depends on whether KdePlatformDependent is ever deleted in a running application (where the leak would really matter)
>     If not, then yes: superfluous complication.
> 
> Jeremy Whiting wrote:
>     Did I hear a "Ship it!" in there somewhere?
> 
> Thomas Lübking wrote:
>     Leaving aside that don't maintain the attica plugin, you have a "+1" from here, IF
>     a) the conditional reparent on destruction is not sufficient (doesn't work)
>        OR
>     b) KdePlatformDependent object(s) are not supposed to be deleted during the runtime of the application
>     
>     Ie. if the leak is inevitable or only theoretical.
> 
> Jeremy Whiting wrote:
>     Yep, I'm one of the attica and knewstuff maintainers and got the plugin to load recently, then hit this crash on close, so trying to fix it. The code in the plugin is only built with the plugin, so shouldn't be used besides as a plugin.
>     
>     The conditional reparent probably works too, but as Albert said it's just a more complicated way of doing the same thing.

> The code in the plugin is only built with the plugin, so shouldn't be used besides as a plugin.

That's not what I asked.

This:
int main(int, char**) {
   KdePlatformDependent dep; // lives as long as the process
}

does not actually "leak".

This:
int main(int, char**) {
   for (int i = 0; i < 100; ++i)
      KdePlatformDependent dep; // lives for the iteration only
}

does.


If the change causes an *actual* leak and that's avoidable, I'd suggest to avoid it.
If not, than it really doesn't matter.


- Thomas


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121161/#review70542
-----------------------------------------------------------


On Nov. 17, 2014, 9:11 nachm., Jeremy Whiting wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/121161/
> -----------------------------------------------------------
> 
> (Updated Nov. 17, 2014, 9:11 nachm.)
> 
> 
> Review request for kdelibs and Plasma.
> 
> 
> Repository: plasma-desktop
> 
> 
> Description
> -------
> 
> Plugin unload order was making the attica_kde plugin crash on close,
> this works around it by leaking one AccessManager.
> 
> 
> Diffs
> -----
> 
>   attica-kde/kdeplugin/kdeplatformdependent.cpp 489c03b18b7bb940007ab51cb197105fbc25de9f 
> 
> Diff: https://git.reviewboard.kde.org/r/121161/diff/
> 
> 
> Testing
> -------
> 
> knewstuff tests no longer crash on close.
> 
> 
> Thanks,
> 
> Jeremy Whiting
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20141118/1049f0a1/attachment.htm>