[Libreoffice-bugs] [Bug 115420] Crash using "Close" button for Extension Manager -> Check for Updates dialog

[bugzilla-daemon at bugs.documentfoundation.org]

https://bugs.documentfoundation.org/show_bug.cgi?id=115420

--- Comment #42 from Jan-Marek Glogowski <glogow at fbihome.de> ---
(In reply to Michael Meeks from comment #39)
> Hmm - then again ... looking at the code:
> 
> vcl/win/window/salframe.cxx
> 
> It puzzles me:
> 
> WinSalFrame::~WinSalFrame()
> {
> 
>     // Release Cache DC
>     if ( mpGraphics2 &&
>          mpGraphics2->getHDC() )
>         ReleaseGraphics( mpGraphics2 );
That's fine - should be sufficient to just check for mpGraphics2, as
ReleaseGraphics does the rest.

> Doesn't seem to do anything of the sort; unless mpGraphics2 == mpGraphics.
> 
> Indeed the condition:
> 
> void WinSalFrame::ReleaseGraphics( SalGraphics* pGraphics )
> {
>     if ( mpGraphics2 == pGraphics )
>     {
> 
> Looks deeply counter-intuitive; surely that should be mpGraphics2 !=
> pGraphics - ie. only free it if it is different ?
No - we just free the cached / shared / threaded DC (not the object), if we
want to release it.

> I'm left puzzled by who frees mpGraphics2 - which (after all) has this timer
> associated with it. Of course, that is not related to GL, and perhaps is a
> leak but only when used threaded:
> 
>     WinSalGraphics*         mpGraphics;             // current frame graphics
>     WinSalGraphics*         mpGraphics2;            // current frame
> graphics for other threads
> 
> The rather grim extensions code being high on the awful threaded, graphical
> code list of horror.
> 
> Thoughts much appreciated; I append a blind fix in case someone can
> reproduce this and can compile that.
> 
> diff --git a/vcl/win/window/salframe.cxx b/vcl/win/window/salframe.cxx
> index 64b073f99139..fee8440cc700 100644
> --- a/vcl/win/window/salframe.cxx
> +++ b/vcl/win/window/salframe.cxx
> @@ -926,6 +926,17 @@ WinSalFrame::~WinSalFrame()
>           mpGraphics2->getHDC() )
>          ReleaseGraphics( mpGraphics2 );
>  
> +    // Why did we never do this ?
> +    if (mpGraphics2 != mpGraphics)
This is always true. Don't confuse the mpGraphics with pGraphics. I did it
quite some times when reading the code.
But we seem to leak the mpGraphics2 object, which is assigned once, but never
freed.

I'm now definitely preferring an *m_* prefix for member variables.

(In reply to Aron Budea from comment #40)
> (In reply to Michael Meeks from comment #39)
> After applying the patch, I'm getting assertion failed at startup:
> "vcl/win/app/salint.cxx Line: 611
> 
> Expression: !pInst->mbNoYieldLock"

Probably we need something like the fix in
4baec725e0dc0713f0d47003e9b10bc3b62f56ff here.
Seems we switch to the main thread and then to an other thread again here.
But that is just a guess.

commit 4baec725e0dc0713f0d47003e9b10bc3b62f56ff
Author: Jan-Marek Glogowski <glogow at fbihome.de>
Date:   Mon Aug 28 19:58:32 2017 +0200

    WIN run main thread redirects ignoring SolarMutex

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/libreoffice-bugs/attachments/20180307/928d3dd7/attachment.html>