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

RESOLVED FIXED in mozilla1.9beta4

Status

()

Core
DOM
P1
normal
RESOLVED FIXED
12 years ago
9 years ago

People

(Reporter: dbaron, Assigned: Ben Turner (not reading bugmail, use the needinfo flag!))

Tracking

({memory-leak})

Trunk
mozilla1.9beta4
x86
Linux
memory-leak
Points:
---
Bug Flags:
blocking1.9 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: late-compat)

Attachments

(2 attachments)

(Reporter)

Description

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

Comment 1

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

Comment 2

12 years ago
[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

12 years ago
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

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

Comment 5

10 years ago
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
Created attachment 301562 [details] [diff] [review]
Patch, v1

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
Last Resolved: 10 years ago
Resolution: --- → FIXED
Whiteboard: late-compat
Target Milestone: --- → mozilla1.9beta4
Backed out because this caused test failures.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Created attachment 302177 [details] [diff] [review]
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));
Fixed again.
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED

Comment 12

9 years ago
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
Last Resolved: 10 years ago9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.