8537 - browserAppCore ownership model needs fixing

Status: VERIFIED FIXED

(SeaMonkey :: UI Design, defect, P1)

Description

[Simon Fraser [no longer active]]

The browserAppCore holds owning references to a whole bunch of things which
it should probably not. In addition, the EndObserving() call needs to
be called from someplace other than the destructor.

This is one of a suite of memory leak fixes that I have plans to check in
fixes for some of this in early M8.

Comment 1

[Bill Law]

I'm accepting this bug.  Note that some measures have been taken to ameliorate
things (EndObserving() is now called when the browser window closes).

Comment 2

[Bill Law]

I think Simon has fixed all these.  I'm waiting to check in something for the
finddialog, but that's really a "find dialog ownership model is broken" fix.

Simon, please slap me if there's still work to be done on this one.  Make sure
you claim credit if it's fixed.

Comment 3

[Simon Fraser [no longer active]]

I'm only willing to accept this as fixed if we can demonstrate the the destructor
of the browserAppCore gets called when closing a browser window. I don't think
that happens yet, does it?

Comment 4

[Bill Law]

I can't say.  It was crashing before the destructor could be called the last I
checked.  My concern is that broken ownershiip models *anywhere* are likely to
block it, since the JS context seems to be the last thing to go.

But your criteria is reasonable.  I'll look into the bigger picture tomorrow.

Comment 5

[leger]

Clearing Fixed resolution since it got re-opened.

Comment 6

[Bill Law]

nsBrowserAppCore mRefCnt drops to 3.  I think maybe one reference is held by
session history (and not released).  Will keep plugging away tomorrow.

Comment 7

[Bill Law]

I've coded and tested a fix.  The nsBrowserAppCore dtor gets called (and nothing
bad happens).  Just need an opportunity to check it in.

Here's the diffs:

Index: nsBrowserAppCore.cpp
===================================================================
RCS file: /cvsroot/mozilla/xpfe/AppCores/src/nsBrowserAppCore.cpp,v
retrieving revision 1.128
diff -r1.128 nsBrowserAppCore.cpp
162d161
<
1655c1654,1660
<       // session history is an instance, not a service
---
>   // Undo other stuff we did in SetContentWindow.
>   if ( mContentAreaWebShell ) {
>       mContentAreaWebShell->SetDocLoaderObserver( 0 );
>       mContentAreaWebShell->SetSessionHistory( 0 );
>   }
>
>   // session history is an instance, not a service
1656a1662,1664
>
>   // Release search context.
>   mSearchContext = 0;

Index: navigator.xul
===================================================================
RCS file: /cvsroot/mozilla/xpfe/browser/src/navigator.xul,v
retrieving revision 1.159
diff -r1.159 navigator.xul
292c292
<         onunload="if (appCore) appCore.close()"
---
>         onunload="Shutdown()"

Index: navigator.js
===================================================================
RCS file: /cvsroot/mozilla/xpfe/browser/src/navigator.js,v
retrieving revision 1.41
diff -r1.41 navigator.js
40a41,49
>   function Shutdown() {
>     // Close the app core.
>     if ( appCore ) {
>         appCore.close();
>         // Remove app core from app core manager.
>         XPAppCoresManager.Remove( appCore );
>     }
>   }
>

Comment 8

[rubydoo123]

this was a code level fix and is verified per engineer, marking bug verified.