Closed Bug 353851 Opened 18 years ago Closed 16 years ago

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

Categories

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

x86
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla1.9beta4

People

(Reporter: dbaron, Assigned: bent.mozilla)

Details

(Keywords: memory-leak, Whiteboard: late-compat)

Attachments

(2 files)

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)
...
I wonder whether we should really support window.opener for chrome.
Summary: accumulation of outer chrome windows in mOpener chains → accumulation of outer chrome windows in mOpener chains (window.opener)
[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
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.
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.
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.
Flags: blocking1.9?
Assignee: general → bent.mozilla
Flags: blocking1.9? → blocking1.9+
Priority: -- → P1
QA Contact: ian → general
Attached patch Patch, v1Splinter Review
Use weak refs for mOpener as we discussed.
Attachment #301562 - Flags: review?(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.
Attachment #301562 - Flags: superreview+
Attachment #301562 - Flags: review?(jst)
Attachment #301562 - Flags: review+
Fixed. Changed that to an assertion.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Whiteboard: late-compat
Target Milestone: --- → mozilla1.9beta4
Backed out because this caused test failures.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Turns out there was a small typo, needed this:

-  nsCOMPtr<nsPIDOMWindow> openerPwin(do_QueryInterface(mOpener));
+  nsCOMPtr<nsPIDOMWindow> openerPwin(do_QueryInterface(opener));
Fixed again.
Status: REOPENED → RESOLVED
Closed: 16 years ago16 years ago
Resolution: --- → FIXED
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!");
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to comment #12)

Pushed assertion fix.
In the future it would be more helpful to file new bugs instead of reopening.
Status: REOPENED → RESOLVED
Closed: 16 years ago16 years ago
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: