Re: mshtml: suppress module unloading while gecko is loaded

[Paul Bryan Roberts]

> Dan Kegel wrote:
> Thanks to Mike Hearn for pointing out that mshtml reference
> counting was wrong, and Jacek for pointing out how to fix
> it for now.
>
> Once gecko is loaded, don't let mshtml unload.
> This fixes a crash in starting Sketchup (bug 16164),
> and probably fixes a number of other apps that
> went pear-shaped after unloading mshtml...

Bug 14065 is another instance of the same thing.

I did a quick survey of DllCanUnloadNow() on the Wine 1.1.8 code base a 
couple of weeks ago ...

55 DllCanUnloadNow routines (in some 272 dll subdirectories)

11 FIXME stubs return S_FALSE
   7 FIXME stubs return S_OK

3 without FIXME return S_FALSE
   2 without FIXME return S_OK

32 appear to be real implementations

However, the first in the list (alphabetically) is in astream/main.c.  
It's reference counter (ddl_ref) is not incremented or decremented 
anywhere.  I count four pairs of inc/dec omissions.

In mshtml, I count 7 dll reference count inc/dec pairs, including the 
server lock pair, in main.c, htmldoc.c and protocol.c   I count 28 pairs 
of InterlockedDecrement / InterlockedIncrement calls around the creation 
/ destruction of active objects throughout the dll.  That would seem to 
imply 22 missing reference count inc/dec pairs.

If I finish my script, it will report how many pairs are missing for 
which dlls and in which source files, which might help make a decision 
whether the take up Mike Hearn's suggestion of never unloading 
anything.  Even that should involve work taking out the reference counts 
that are already there.

I do not see how the classic WinAPI regression tests can catch this kind 
of bug other than being a tautologous test of the creation/destruction 
all the known active objects.  If Mike Hearn's suggestion is not taken 
up, the script might also be useful for regression test purposes.

Cheers,

mshtml: suppress module unloading while gecko is loaded

[Dan Kegel]

On Mon, Dec 1, 2008 at 12:08 AM, Paul Bryan Roberts
 wrote:
> In mshtml, I count 7 dll reference count inc/dec pairs, including the server
> lock pair, in main.c, htmldoc.c and protocol.c   I count 28 pairs of
> InterlockedDecrement / InterlockedIncrement calls around the creation /
> destruction of active objects throughout the dll.  That would seem to imply
> 22 missing reference count inc/dec pairs.

I suspect Alexandre didn't commit my lame patch because he wants us to
have a go at getting CanDLLUnloadNow right.  Thanks for the analysis.
> If I finish my script, it will report how many pairs are missing for which
> dlls and in which source files

That sounds quite handy.
> which might help make a decision whether the
> take up Mike Hearn's suggestion of never unloading anything.  Even that
> should involve work taking out the reference counts that are already there.

I have a feeling we're not quite in the "memory is free" era yet...
> I do not see how the classic WinAPI regression tests can catch this kind of
> bug other than being a tautologous test of the creation/destruction all the
> known active objects.  If Mike Hearn's suggestion is not taken up, the
> script might also be useful for regression test purposes.

Your script is doing static analysis ... maybe it should be integrated into
http://people.redhat.com/mstefani/wine/smatch/
Making it a smatch rule might be easier than writing it from scratch.
- Dan