About tdf#90566: Theme::disposing(void) memory leak: maChangeListeners is not properly disposed

classic Classic list List threaded Threaded
4 messages Options
julien2412 julien2412
Reply | Threaded
Open this post in threaded view
|

About tdf#90566: Theme::disposing(void) memory leak: maChangeListeners is not properly disposed

This post was updated on .
Hello,

I read about tdf#90566, Theme::disposing(void) memory leak: maChangeListeners is not properly disposed and wondered why we couldn't just  remove the 2 first lines of the disposing function (see http://opengrok.libreoffice.org/xref/core/sfx2/source/sidebar/Theme.cxx#390) + replace "aListeners" by "maChangeListeners"?
I mean why adding an intermediate variable instead of just using the initial variable?

I found this type of construct at different locations in sfx2, see http://opengrok.libreoffice.org/search?q=swap&project=core&defs=&refs=&path=sfx2%2Fsource%2Fsidebar&hist=
(EDIT: I don't mean there's a memory leak in these other locations but we also might simplify these)

Julien
sberg sberg
Reply | Threaded
Open this post in threaded view
|

Re: About tdf#90566: Theme::disposing(void) memory leak: maChangeListeners is not properly disposed

On 04/12/2015 08:50 AM, julien2412 wrote:
> I read about tdf#90566, Theme::disposing(void) memory leak:
> maChangeListeners is not properly disposed and wondered why we couldn't just
> remove the 2 first lines of the disposing function (see
> http://opengrok.libreoffice.org/xref/core/sfx2/source/sidebar/Theme.cxx#390)
> + replace "aListeners" by "maChangeListeners"?
> I mean why adding an intermediate variable instead of just using the initial
> variable?

The general reason for this construct is multi-threading:  Access to the
maChangeListeners member typically needs to be guarded by some mutex
(typically a member of the same class instance as maChangeListeners),
and the calls "out of that instance" to the

   (*iListener)->disposing(aEvent);

must be done with that mutex not locked (to avoid deadlock).  That is
why the generic skeleton is

   Listeners copy;
   {
     Guard g(mMutex);
     copy = mListeners;
   }
   for (i: copy) i->...

However, in this particular case, the access to maChangeListeners is not
guarded, but likely out of ignorance of the original author.

_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
julien2412 julien2412
Reply | Threaded
Open this post in threaded view
|

Re: About tdf#90566: Theme::disposing(void) memory leak: maChangeListeners is not properly disposed

sberg wrote
On 04/12/2015 08:50 AM, julien2412 wrote:
> ...
The general reason for this construct is multi-threading:  Access to the
maChangeListeners member typically needs to be guarded by some mutex
(typically a member of the same class instance as maChangeListeners),
and the calls "out of that instance" to the

   (*iListener)->disposing(aEvent);

must be done with that mutex not locked (to avoid deadlock).
...
However, in this particular case, the access to maChangeListeners is not
guarded, but likely out of ignorance of the original author.
Thank you Stephan for your feedback.

I've tried to find a similar case with a mutex but haven't found any.
For example, in a completely different part, http://opengrok.libreoffice.org/xref/core/sdext/source/presenter/PresenterWindowManager.cxx#943 no mutex.
Could you give an example so I could apply it to our initial case or do you prefer I just fix the variable use for the moment?

Julien
sberg sberg
Reply | Threaded
Open this post in threaded view
|

Re: About tdf#90566: Theme::disposing(void) memory leak: maChangeListeners is not properly disposed

On 04/13/2015 11:36 AM, julien2412 wrote:
> I've tried to find a similar case with a mutex but haven't found any.
> For example, in a completely different part,
> http://opengrok.libreoffice.org/xref/core/sdext/source/presenter/PresenterWindowManager.cxx#943
> no mutex.
> Could you give an example so I could apply it to our initial case or do you
> prefer I just fix the variable use for the moment?

An example is OInterfaceContainerHelper::disposeAndClear in
cppuhelper/source/interfacecontainer.cxx, but I would fix the apparent
variable confusion independently of any locking problems anyway.
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice