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

Status: VERIFIED FIXED

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

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.)

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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).

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

My guess is that we're failing to clear scope on the inner windows held in the fastback cache.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This is a regression *since* the 1.8 branch.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

In ~WindowStateHolder, on this line:

    mInnerWindow->FreeInnerObjects(GetScriptContextFromJSContext(cx));

GetScriptContextFromJSContext returns null.  (Should it, for the safe context?)

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(And the argument being null causes the ClearScope call to be skipped, which is what causes the leak.)

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

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.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> (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 9

[Johnny Stenback (:jst)]

Comment on attachment 238922 [details] [diff] [review]
patch

sr=jst

Comment 10

[Mark Hammond [:markh] [:mhammond]]

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)

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Checked in to trunk.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

*** Bug 353658 has been marked as a duplicate of this bug. ***

Comment 14

[Adam Guthrie]

*** Bug 353114 has been marked as a duplicate of this bug. ***

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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...

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: 1.8 branch patch

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.

Comment 17

[Johnny Stenback (:jst)]

Comment on attachment 244505 [details] [diff] [review]
1.8 branch patch

r+sr=jst

Comment 18

[Daniel Veditz [:dveditz]]

Comment on attachment 244505 [details] [diff] [review]
1.8 branch patch

approved for 1.8/1.8.0 branches, a=dveditz for drivers

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Checked in to MOZILLA_1_8_BRANCH.

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 21

[Jay Patel [:jay]]

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.

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

For the tool whose output was in comment 0, see:

http://www.squarefree.com/2006/01/13/memory-leak-detection-tool/
http://dbaron.org/log/2006-01#e20060110a
http://dbaron.org/log/2006-01#e20060114a

Comment 23

[Carsten Book [:Tomcat]]

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