416939 - Broadcast the destruction of DOM windows

Status: RESOLVED FIXED

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

Description

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

Attachment: Patch, v1

DOM windows are being held alive far too long due to some quirks with the XULTemplateBuilder/QueryProcessor. We're going to add a backdoor release via an observer topic to get those windows cleaned up.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

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

If I read this correctly, seems like nsXULTemplateBuilder::Observe calls QueryInterface (and AddRef) on an object which is being deleted:

>+nsXULTemplateBuilder::Observe(nsISupports* aSubject,
...
>+        nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(aSubject);
This addrefs

> nsGlobalWindow::~nsGlobalWindow()
...
>+  // Make sure that this is called before we null out the document.
>+  NotifyDOMWindowDestroyed(this);
>+
Isn't global window refcount 0 here? Or stabilized to 1.

Not a problem in this case, but what if someone else starts to use 
"dom-window-destroyed" and creates a long-living strong ref to aSubject.
That might lead to a crash.

Maybe the subject could be the nsIDocument object and topic something like
"document-detached-from-window"?

Comment 2

[Johnny Stenback (:jst)]

(In reply to comment #1)
> Not a problem in this case, but what if someone else starts to use 
> "dom-window-destroyed" and creates a long-living strong ref to aSubject.
> That might lead to a crash.
> 
> Maybe the subject could be the nsIDocument object and topic something like
> "document-detached-from-window"?

Or maybe we could just not call this from the destructor (or any of the methods called from the dtor. The key here is to let things know that a window is being torn down, not that it's actually being deleted.

Comment 3

[Johnny Stenback (:jst)]

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

- In nsGlobalWindow::nsGlobalWindow():

 #ifdef DEBUG
-  printf("++DOMWINDOW == %d\n", gRefCnt);
+  printf("++DOMWINDOW == %d (%p) [Outer = %p]\n", gRefCnt,
+         static_cast<nsIScriptGlobalObject*>(this), aOuterWindow);
+  mSerial = gRefCnt;

This doesn't seem right, shouldn't the serial always be incremented here rather than set to gRefCnt? With this code, the window with the serial number 2 (or whatever) can be created many times over. Seems like this would be more helpful for debugging this stuff if it's a true serial count, incremented each time a window is created. And that means the serial number should be printed out here too (during startup before windows start dying mSerial and gRefCnt will be the same, but not so later on once windows start going away).

- In nsGlobalWindow::~nsGlobalWindow():

 #ifdef DEBUG
-  printf("--DOMWINDOW == %d\n", gRefCnt);
+  printf("--DOMWINDOW == %d (%p) [gRefCnt = %d] [Outer = %p]\n", mSerial,
+         static_cast<nsIScriptGlobalObject*>(this), gRefCnt, mOuterWindow);

Leave the first %d be gRefCnt, and add a mSerial inside brackets here, to match what the ctor prints out.

r+sr=jst with that, as this patch doesn't actually call the NotifyDOMWindowDestroyed() function from the dtor, as I thought it did based on Smaugs comment.

Comment 4

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

Fixed with comments addressed.

Comment 5

[Tuukka Tolvanen (sp3000)]

ah, one line of warnings ++

At global scope:
http://mxr.mozilla.org/mozilla/source/dom/src/base/nsGlobalWindow.cpp#212
warning: ‘gSerialCounter’ defined but not used

...because the use is DEBUG and the decl is not.

Comment 6

[Jesse Ruderman]

I fixed that by making the decl ifdef DEBUG.

Comment 7

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

(In reply to comment #6)
> I fixed that by making the decl ifdef DEBUG.

Thanks Jesse. Oops.