Closed Bug 416939 Opened 14 years ago Closed 14 years ago

Broadcast the destruction of DOM windows

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla1.9beta4

People

(Reporter: bent.mozilla, Assigned: bent.mozilla)

References

Details

(Keywords: memory-leak)

Attachments

(1 file)

Attached patch Patch, v1Splinter Review
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.
Flags: blocking1.9?
Attachment #302745 - Flags: review?(jst)
Keywords: mlk
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"?
Flags: blocking1.9? → blocking1.9+
Priority: -- → P1
(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 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.
Attachment #302745 - Flags: superreview+
Attachment #302745 - Flags: review?(jst)
Attachment #302745 - Flags: review+
Fixed with comments addressed.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Depends on: 417430
Depends on: 417840
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.
I fixed that by making the decl ifdef DEBUG.
(In reply to comment #6)
> I fixed that by making the decl ifdef DEBUG.

Thanks Jesse. Oops.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.