Closed Bug 416939 Opened 13 years ago Closed 13 years ago
Broadcast the destruction of DOM windows
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.
Attachment #302745 - Flags: review?(jst)
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"?
(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.
Fixed with comments addressed.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
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.
13 years ago
Depends on: 417383
(In reply to comment #6) > I fixed that by making the decl ifdef DEBUG. Thanks Jesse. Oops.
You need to log in before you can comment on or make changes to this bug.