JS contexts are not cleaned up properly on window close




DOM: Core & HTML
19 years ago
18 years ago


(Reporter: Simon Fraser, Assigned: vidur (gone))


Mac System 8.5

Firefox Tracking Flags

(Not tracked)





19 years ago
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.

Comment 1

19 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

19 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

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

(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.

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.

Comment 5

19 years ago
In an attempt to get my bug list in order again, marking all the bugs I have
currently as ASSIGNED.
Not critical for M15, moving to M17.
Target Milestone: M15 → M17
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.
Last Resolved: 18 years ago
Resolution: --- → FIXED

Comment 8

18 years ago
You need to log in before you can comment on or make changes to this bug.