Closed
Bug 8555
Opened 25 years ago
Closed 24 years ago
JS contexts are not cleaned up properly on window close
Categories
(Core :: DOM: Core & HTML, defect, P3)
Tracking
()
VERIFIED
FIXED
M17
People
(Reporter: sfraser_bugs, Assigned: vidur)
References
()
Details
JS contexts are still not being cleaned up properly in apprunner, despite bug 8248 being fixed. I have tracked this down empirically to a change to nsGlobalWindow.cpp that vidur made on June 8, the diffs for which are in the URL above. Specifically, SetNewDocument() now no longer calls JS_ClearScope() when called with a null document. SetNewDocument() is called with a null document on window close, as part of the window tear-down, and I'm not sure that vidur was aware of this behaviour. So now JS_ClearScope() does not get called on window close. It is this which I suspect is the reason that JS contexts are not properly cleaned up. If I comment out the test for a null document at line 268 (file vers 1.92), then cleanup happens, and my xpconnected objects get released on window close. I'm going to check in this change (commenting out the test for a nil document when calling JS_ClearScope()), because it works, and seems to have no bad side-effects. But I'm going to leave this bug open so that someone who knows more about this can double-check that it's the right thing to do.
Reporter | ||
Comment 1•25 years ago
|
||
Here's the diff that I'm checking in: Index: nsGlobalWindow.cpp =================================================================== RCS file: /cvsroot/mozilla/dom/src/base/nsGlobalWindow.cpp,v retrieving revision 1.93 diff -c -2 -r1.93 nsGlobalWindow.cpp *** nsGlobalWindow.cpp 1999/06/19 22:15:14 1.93 --- nsGlobalWindow.cpp 1999/06/19 22:17:57 *************** *** 265,270 **** if ((nsnull != mScriptObject) && ! (nsnull != mContext) && ! (nsnull != aDocument)) { JS_ClearScope((JSContext *)mContext->GetNativeContext(), (JSObject *)mScriptObject); --- 265,270 ---- if ((nsnull != mScriptObject) && ! (nsnull != mContext) /* && ! (nsnull != aDocument) */ ) { JS_ClearScope((JSContext *)mContext->GetNativeContext(), (JSObject *)mScriptObject);
Comment 2•25 years ago
|
||
Careful, I think vidur made that change to "fix" things to openDialog works. Danm should verify that your reversal of vidur's change hasn't broken openDialog. The real fix, I argued at the time and still maintain, is to avoid overloading SetNewDocument's semantics to do both "clear the old document's JS scope, when there was an old document" and "initialize the new document's JS scope." The clearing needs to happen only when there was an old doc -- and higher layers of control flow can distinguish that from when this is the first doc. It's inefficient to clear when there's nothing to clear, but that's a secondary point. The primary point is that overloading precludes custom window property presetting. (openDialog needs to preset an arguments property in the new window, and have it survive SetNewDocument's second, unoverloaded sense: populate the standard JS scope; but not be cleared just after it's preset by the first unoverloaded sense: clear any old state.) At the time, we (danm, vidur, and I) believed that testing aDocument would tell us whether this was the first time, or a subsequent time, that SetNewDocument was called for a given window. Foolish us! SetNewDocument should be decomposed into primitives: ClearOldDocument and SetNewDocument. Or something similar that effectively disambiguates the "first document" case so that custom window property presetting can be done without the properties being immediately cleared. /be
Is this bug fixed, then? Did smfr's patch break openDialog? Are we clearing contexts properly? Would like to get this off vidur's list.
Moving all non-DOM[012], non-crash bugs to M15.
Assignee | ||
Comment 5•25 years ago
|
||
In an attempt to get my bug list in order again, marking all the bugs I have currently as ASSIGNED.
Comment 7•24 years ago
|
||
This old bug can be considerd fixed. We clean up on window close and openDialog *does* work. The current mechanism might have a bit of inefficiency in it, but w can live with that.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•