353851 - accumulation of outer chrome windows in mOpener chains (window.opener)

Status: RESOLVED FIXED

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

Description

[David Baron :dbaron:]

When repeatedly closing and opening browser windows in Firefox, the outer window objects of the windows that have disappeared remain around because they're entrained by |nsGlobalWindow::mOpener|.

Steps to reproduce:
 1. set the environment variables in mozilla/tools/footprint/leak-gauge.pl
 2. start firefox
 3. repeatedly:
    a. run leak-gauge.pl on the log file you named in step (1)
    b. Press Ctrl-N to open a new browser window
    c. close the old browser window
    d. wait 3 seconds

Actual results:  Each time step (3a) is done, the number of DOM Windows reported as leaked increases by one.

Expected results:  Count of windows, documents, and docshells on the heap should remain constant.

What appears to be going on here is that the outer window for the browser is being kept alive through the mOpener member variable of the window opened from it.  On shutdown after just 1.5 iterations (3a-3d, 3a), the window that looked through leak-gauge like it should have gone away (since it was the outer without an inner) was destroyed with the stack:

<nsGlobalWindow> 0x08A072A0 3 Destroy
NS_LogRelease_P (/builds/trunk/mozilla/xpcom/base/nsTraceRefcntImpl.cpp:1069)
nsGlobalWindow::Release() (/builds/trunk/mozilla/dom/src/base/nsGlobalWindow.cpp:666)
nsGlobalChromeWindow::Release() (/builds/trunk/mozilla/dom/src/base/nsGlobalWindow.cpp:7356)
nsCOMPtr<nsIDOMWindowInternal>::assign_assuming_AddRef(nsIDOMWindowInternal*) (../../dist/include/xpcom/nsCOMPtr.h:568)
nsCOMPtr<nsIDOMWindowInternal>::assign_with_AddRef(nsISupports*) (../../dist/include/xpcom/nsCOMPtr.h:1224)
nsCOMPtr<nsIDOMWindowInternal>::operator=(nsIDOMWindowInternal*) (../../dist/include/xpcom/nsCOMPtr.h:714)
nsGlobalWindow::CleanUp() (/builds/trunk/mozilla/dom/src/base/nsGlobalWindow.cpp:553)
~nsGlobalWindow (/builds/trunk/mozilla/dom/src/base/nsGlobalWindow.cpp:523)
~nsGlobalChromeWindow (/builds/trunk/mozilla/dom/src/base/nsGlobalWindow.h:694)
nsGlobalWindow::Release() (/builds/trunk/mozilla/dom/src/base/nsGlobalWindow.cpp:666)
nsGlobalChromeWindow::Release() (/builds/trunk/mozilla/dom/src/base/nsGlobalWindow.cpp:7356)
XPCJSRuntime::GCCallback(JSContext*, JSGCStatus) (/builds/trunk/mozilla/js/src/xpconnect/src/xpcjsruntime.cpp:579)
DOMGCCallback (/builds/trunk/mozilla/dom/src/base/nsJSEnvironment.cpp:3232)
js_GC (/builds/trunk/mozilla/js/src/jsgc.c:3083)
JS_GC (/builds/trunk/mozilla/js/src/jsapi.c:1944)
nsDOMScriptObjectFactory::Observe(nsISupports*, char const*, unsigned short const*) (/builds/trunk/mozilla/dom/src/base/nsDOMScriptObjectFactory.cpp:288)
nsObserverList::NotifyObservers(nsISupports*, char const*, unsigned short const*) (/builds/trunk/mozilla/xpcom/ds/nsObserverList.cpp:127)
nsObserverService::NotifyObservers(nsISupports*, char const*, unsigned short const*) (/builds/trunk/mozilla/xpcom/ds/nsObserverService.cpp:177)
NS_ShutdownXPCOM_P (/builds/trunk/mozilla/xpcom/build/nsXPComInit.cpp:717)
~ScopedXPCOMStartup (/builds/trunk/mozilla/toolkit/xre/nsAppRunner.cpp:588)
...

Comment 1

[David Baron :dbaron:]

I wonder whether we should really support window.opener for chrome.

Comment 2

[David Baron :dbaron:]

[03 10:44:42] <timeless> dbaron: i know i've actively used window.opener in chrome
[03 10:45:04] <timeless> not doing it means announcing fairly publicly that you're not

Comment 3

[timeless]

i know i've actively used window.opener in chrome. not doing it means announcing fairly publicly that you're not.

note that in general, i consider anything beyond the first closed window.opener to be something that shouldn't be reachable, i.e. i mostly agree that a chain is a leak and want it fixed. certainly the code i was involved in only cared about window.opener chains for pairs of existing windows since it was trying to actually do useful work, not resurrect ancient history.

Comment 4

[Jesse Ruderman]

Instead of not supporting it, how about making it opt-in with "hasopener=yes" in the third parameter to window.open()?  Or opt-out, if we're really worried about breaking extensions.

Comment 5

[David Baron :dbaron:]

I think this is potentially a much bigger issue now with the cycle collector.  In any cases where we're relying on the cycle collector to clean things (such as a cycle including both a window and content nodes) up, this bug would prevent that cycle from being cleaned up.  So I think we need to figure out something here for 1.9.

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v1

Use weak refs for mOpener as we discussed.

Comment 7

[Johnny Stenback (:jst)]

Comment on attachment 301562 [details] [diff] [review]
Patch, v1

- In nsGlobalWindow::SetOpenerWindow():

-  mOpener = aOpener;
+  
+  mOpener = do_GetWeakReference(aOpener);
+  if (!mOpener)
+    return;
+

There's debug code that depends on this function running to completion, so don't return early here.

r+sr=jst with that.

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Fixed. Changed that to an assertion.

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Backed out because this caused test failures.

Comment 10

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch I checked in

Turns out there was a small typo, needed this:

-  nsCOMPtr<nsPIDOMWindow> openerPwin(do_QueryInterface(mOpener));
+  nsCOMPtr<nsPIDOMWindow> openerPwin(do_QueryInterface(opener));

Comment 11

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Fixed again.

Comment 12

[timeless]

bent: the patch you checked in is causing many assertions in normal use. your assertion was bad.

  NS_ASSERTION(aOpener || !aOriginalOpener,
               "Shouldn't set mHadOriginalOpener if aOpener is null");

  mOpener = do_GetWeakReference(aOpener);
  NS_ASSERTION(mOpener, "Opener must support weak references!");

you can't assert mOpener will be non null here, the correct assertion is:
  NS_ASSERTION(!aOpener || mOpener, "Opener must support weak references!");

Comment 13

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to comment #12)

Pushed assertion fix.

Comment 14

[Ben Turner (not reading bugmail, use the needinfo flag!)]

In the future it would be more helpful to file new bugs instead of reopening.