8555 - JS contexts are not cleaned up properly on window close

Status: VERIFIED FIXED

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

Description

[Simon Fraser [no longer active]]

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

[Simon Fraser [no longer active]]

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

[brendan-obsolete]

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

Comment 3

[Mike Shaver (:shaver emeritus)]

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.

Comment 4

[Mike Shaver (:shaver emeritus)]

Moving all non-DOM[012], non-crash bugs to M15.

Comment 5

[vidur (gone)]

In an attempt to get my bug list in order again, marking all the bugs I have
currently as ASSIGNED.

Comment 6

[Johnny Stenback (:jst)]

Not critical for M15, moving to M17.

Comment 7

[Johnny Stenback (:jst)]

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.

Comment 8

[Prashant Desale]

Verified.