Closed Bug 353022 Opened 13 years ago Closed 13 years ago

leak when loading google firefox homepage, clicking bookmark, and closing window

Categories

(Core :: DOM: Core & HTML, defect, critical)

x86
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla1.9alpha1

People

(Reporter: dbaron, Assigned: dbaron)

References

Details

(Keywords: memory-leak, regression, verified1.8.1.1, Whiteboard: [patch][need testcase])

Attachments

(2 files)

Steps to reproduce:
 1. create a clean firefox profile
 2. load https://bugzilla.mozilla.org/show_bug.cgi?id=352911
 3. drag the URL to the bookmarks toolbar
 4. quit firefox
 5. start firefox (which loads http://www.google.com/firefox?client=firefox-a&rls=org.mozilla:en-US:official )
 6. click the bookmark created in (3)
 7. close the browser window

Actual results:

Leaked outer window 9334470 at address 9334470.
Leaked inner window 957e238 (outer 9334470) at address 957e238.
 ... with URI "https://bugzilla.mozilla.org/show_bug.cgi?id=352911".
Leaked inner window 9552e48 (outer 9334470) at address 9552e48.
 ... with URI "http://www.google.com/firefox?client=firefox-a&rls=org.mozilla:en-US:official".
Leaked document at address 92abdd0.
 ... with URI "http://www.google.com/firefox?client=firefox-a&rls=org.mozilla:en-US:official".
Summary:
Leaked 3 out of 8 DOM Windows
Leaked 1 out of 36 documents
Leaked 0 out of 3 docshells

Expected results:
no leaks

I see this leak both with and without the patch to bug 352911.

The HTMLDocument seems to be kept alive through its wrapper, which is kept alive by:
08c571d0 object 0x8ac0758 HTMLDocument via locked object(Window @ 0x08c56cb8).document(HTMLDocument @ 0x08c571d0).
(That's a trace from with the patch to bug 352911.)
So it doesn't matter what page is bookmarked, but it does matter that it's clicking a bookmark rather than entering a URL in the URL bar.

However, it does matter that the homepage loaded is the Google/Firefox homepage (the one we use for en-US release builds).
Summary: leak on bugzilla page → leak when loading google firefox homepage, clicking bookmark, and closing window
My guess is that we're failing to clear scope on the inner windows held in the fastback cache.
This is a regression *since* the 1.8 branch.
Version: 1.8 Branch → Trunk
In ~WindowStateHolder, on this line:

    mInnerWindow->FreeInnerObjects(GetScriptContextFromJSContext(cx));

GetScriptContextFromJSContext returns null.  (Should it, for the safe context?)
Assignee: dbaron → general
Blocks: dom-agnostic
Severity: major → critical
Flags: blocking1.9?
(And the argument being null causes the ClearScope call to be skipped, which is what causes the leak.)
Perhaps it would help if the entries were released earlier.  I tried replacing ~WindowStateHolder with:

  if (mInnerWindow) {
    // This window was left in the bfcache and is now going away. We need to
    // free it up.
    PRUint32 lang_id;
    NS_STID_FOR_ID(lang_id) {
      // Note that langCtx comes from an outer window for which
      // mInnerWindow is not the current inner window.
      nsIScriptContext *langCtx =
        mInnerWindow->GetScriptContextInternal(lang_id);
      if (langCtx)
        // FreeInnerObjects calls ClearScope, which does the JSAutoRequest
        // thing for JS
        mInnerWindow->FreeInnerObjects(langCtx);
    }
  }


but it doesn't work since all the script contexts on the outer window are null.  But that's probably happening because the whole thing is happening here:

#1  0x015ba23f in ~WindowStateHolder (this=0xaa20f70)
    at /builds/trunk/mozilla/dom/src/base/nsGlobalWindow.cpp:1037
#2  0x015ba388 in WindowStateHolder::Release (this=0xaa20f70)
    at /builds/trunk/mozilla/dom/src/base/nsGlobalWindow.cpp:1046
#3  0x00b668ec in nsCOMPtr_base::assign_assuming_AddRef (this=0xa81b510,
    newPtr=0x0) at /builds/trunk/mozilla/xpcom/build/../glue/nsCOMPtr.h:531
#4  0x00b66656 in nsCOMPtr_base::assign_with_AddRef (this=0xa81b510,
    rawPtr=0x0) at nsCOMPtr.cpp:89
#5  0x00a87695 in nsCOMPtr<nsISupports>::operator= (this=0xa81b510, rhs=0x0)
    at ../../dist/include/xpcom/nsCOMPtr.h:1033
#6  0x00ac6950 in nsSHEntry::DropPresentationState (this=0xa81b4a0)
    at /builds/trunk/mozilla/docshell/shistory/src/nsSHEntry.cpp:635
#7  0x00ac739e in ~nsSHEntry (this=0xa81b4a0)
    at /builds/trunk/mozilla/docshell/shistory/src/nsSHEntry.cpp:118
#8  0x00ac7558 in nsSHEntry::Release (this=0xa81b4a0)
    at /builds/trunk/mozilla/docshell/shistory/src/nsSHEntry.cpp:128
#9  0x00a7e24a in ~nsCOMPtr (this=0xa82e748)
    at ../../dist/include/xpcom/nsCOMPtr.h:583
#10 0x00ac8ef0 in ~nsSHTransaction (this=0xa82e730)
    at /builds/trunk/mozilla/docshell/shistory/src/nsSHTransaction.cpp:54
#11 0x00ac8d78 in nsSHTransaction::Release (this=0xa82e730)
    at /builds/trunk/mozilla/docshell/shistory/src/nsSHTransaction.cpp:61
#12 0x00ac917a in ~nsCOMPtr (this=0xa371ee4)
    at ../../../dist/include/xpcom/nsCOMPtr.h:583
#13 0x00acc786 in ~nsSHistory (this=0xa371ec8)
    at /builds/trunk/mozilla/docshell/shistory/src/nsSHistory.cpp:142
#14 0x00acc34b in nsSHistory::Release (this=0xa371ec8)
    at /builds/trunk/mozilla/docshell/shistory/src/nsSHistory.cpp:149
#15 0x005e8246 in XPCJSRuntime::GCCallback (cx=0xa789588, status=JSGC_END)
    at /builds/trunk/mozilla/js/src/xpconnect/src/xpcjsruntime.cpp:590
#16 0x0159f978 in DOMGCCallback (cx=0xa789588, status=JSGC_END)
    at /builds/trunk/mozilla/dom/src/base/nsJSEnvironment.cpp:3209
#17 0x004d4100 in js_GC (cx=0xa789588, gckind=GC_NORMAL)
    at /builds/trunk/mozilla/js/src/jsgc.c:3077
#18 0x0049d732 in JS_GC (cx=0xa789588)
    at /builds/trunk/mozilla/js/src/jsapi.c:1938
#19 0x00a1c98a in nsXREDirProvider::DoShutdown (this=0xbf8780a4)
    at /builds/trunk/mozilla/toolkit/xre/nsXREDirProvider.cpp:731
#20 0x00a0f300 in ~ScopedXPCOMStartup (this=0xbf878174)
    at /builds/trunk/mozilla/toolkit/xre/nsAppRunner.cpp:583
#21 0x00a12fa2 in XRE_main (argc=Variable "argc" is not available.)
    at /builds/trunk/mozilla/toolkit/xre/nsAppRunner.cpp:2504

That's probably also why the safe context doesn't work -- I'm guessing the hidden window has been destroyed.  And I'm guessing it's why this is particular to clicking on a bookmark -- which would mean it's probably only a shutdown leak.

I'm still not happy about this code not looping through all languages, nor about it happening from GC rather than from Close on the outer window.
Attached patch patchSplinter Review
So this fixes the leak for me.  It's the second part of the patch that really fixes the leak; however, I think the first is probably a good idea as well.

I'm not all that familiar with this code, so please do review this carefully to make sure I'm not doing something I shouldn't be.
Assignee: general → dbaron
Status: NEW → ASSIGNED
Attachment #238922 - Flags: superreview?(jst)
Attachment #238922 - Flags: review?(mhammond)
Whiteboard: [patch]
Target Milestone: --- → mozilla1.9alpha
> (Should it, for the safe context?)

There are two types of safe contexts.  There's the XPConnect safe context, which is not associated with a window and hence has no nsIScriptGlobalObject or nsIScriptContext attached to it.  Then there's the hidden window context, which is created by appshell.  This happens to be attached to the hidden window and hence has the standard DOM paraphernalia.

Now during startup, appshell tells XPConnect to use the hidden window context as the safe context.  During appshell shutdown, we tear down the hidden window, and at the same time tell XPConnect to go back to using the "normal" safe context.

So if the ~WindowStateHolder happens after hidden window teardown at shutdown it would be trying to use the XPConnect safe context, with this bug resulting.

In general, assuming that the safe JS context has an associated nsIScriptContext is bogus in our current code...

It bothers me that session history waits until it's destroyed to drop bfcache.  It should probably observe some sort of shutdown notification...  Might want to file as separate bug on that.
Comment on attachment 238922 [details] [diff] [review]
patch

sr=jst
Attachment #238922 - Flags: superreview?(jst) → superreview+
Comment on attachment 238922 [details] [diff] [review]
patch

I've no problem with that patch - but it isn't obvious to me that the DOM_AGNOSTIC work broke anything here - ie, its not clear how DOM_AGNOSTIC work caused this leak.  I'm only guessing that you believe DOM_AGNOSTIC was involved though, so the above may not be relevant.  I'd recommend having jst give it a review too (which he may have already done as part of the sr)
Attachment #238922 - Flags: review?(mhammond) → review+
Pre-DOM-Agnostic, all that was required was a JSContext.  Now a JSContext that has an associated nsIScriptContext is required, and we don't necessarily have that during shutdown.  That said, it's better to do this earlier rather than later since the SHEntry objects are themselves garbage-collected, and it's better not to take multiple cycles of GC to clean things up if we can easily avoid it.
Checked in to trunk.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
*** Bug 353658 has been marked as a duplicate of this bug. ***
*** Bug 353114 has been marked as a duplicate of this bug. ***
No longer blocks: 353658
So I'm thinking I might want to try to get the second chunk of this patch in for 1.8.1.1 as well, since it could cause things to get cleaned up more quickly, or make leaks of SHEntries caused by extensions less severe, or prevent something that an extension does from causing a cycle...
Flags: blocking1.8.1.1?
Attached patch 1.8 branch patchSplinter Review
This patch could be a substantial leak improvement, although it's somewhat hard to tell for sure without some good automated tests.  It should be pretty low risk.
Attachment #244505 - Flags: superreview?(jst)
Attachment #244505 - Flags: review?(jst)
Attachment #244505 - Flags: approval1.8.1.1?
Attachment #244505 - Flags: approval1.8.0.9?
Comment on attachment 244505 [details] [diff] [review]
1.8 branch patch

r+sr=jst
Attachment #244505 - Flags: superreview?(jst)
Attachment #244505 - Flags: superreview+
Attachment #244505 - Flags: review?(jst)
Attachment #244505 - Flags: review+
Flags: blocking1.8.1.1?
Flags: blocking1.8.1.1+
Flags: blocking1.8.0.9-
Comment on attachment 244505 [details] [diff] [review]
1.8 branch patch

approved for 1.8/1.8.0 branches, a=dveditz for drivers
Attachment #244505 - Flags: approval1.8.1.1?
Attachment #244505 - Flags: approval1.8.1.1+
Attachment #244505 - Flags: approval1.8.0.9?
Attachment #244505 - Flags: approval1.8.0.9+
Checked in to MOZILLA_1_8_BRANCH.
Keywords: fixed1.8.1.1
Comment on attachment 244505 [details] [diff] [review]
1.8 branch patch

I'm not sure why I requested 1.8.0.9 approval here, so clearing approval flag.
Attachment #244505 - Flags: approval1.8.0.9+
Adding [need testcase] since there doesn't appear be an easy way to verify/test this fix.   If anyone does come up with a testcase or a set of automated tests, please let us know.
Whiteboard: [patch] → [patch][need testcase]
Verified fixed for 1.8.1.1 with Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.1) Gecko/20061208 Firefox/2.0.0.1 ID:2006120814 on Fedora FC 6
Status: RESOLVED → VERIFIED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.