browserAppCore ownership model needs fixing

VERIFIED FIXED in M8

Status

SeaMonkey
UI Design
P1
normal
VERIFIED FIXED
19 years ago
13 years ago

People

(Reporter: Simon Fraser, Assigned: Bill Law)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

19 years ago
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.

Updated

19 years ago
OS: Mac System 8.5 → All
Priority: P3 → P1
Target Milestone: M8
(Assignee)

Updated

19 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 1

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

Updated

19 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 19 years ago
Resolution: --- → FIXED
(Assignee)

Comment 2

19 years ago
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.
(Reporter)

Comment 3

19 years ago
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?
(Assignee)

Updated

19 years ago
Status: RESOLVED → REOPENED
(Assignee)

Comment 4

19 years ago
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.

Updated

19 years ago
Resolution: FIXED → ---

Comment 5

19 years ago
Clearing Fixed resolution since it got re-opened.
(Assignee)

Comment 6

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

Comment 7

19 years ago
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 );
>     }
>   }
>
(Assignee)

Updated

19 years ago
Status: REOPENED → RESOLVED
Last Resolved: 19 years ago19 years ago
Resolution: --- → FIXED

Updated

19 years ago
Status: RESOLVED → VERIFIED

Comment 8

19 years ago
this was a code level fix and is verified per engineer, marking bug verified.
Product: Core → Mozilla Application Suite
You need to log in before you can comment on or make changes to this bug.