41608 - how to destroy named roots when we have no context

Status: VERIFIED FIXED

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

Description

[Alec Flett]

The bug is in nsGlobalWindow.cpp.. the issue is basically that during
construction of nsGlobalWindowImpl, we add a named root, "window_object" with
the current script context. Shaver just helped me find that we weren't ever
removing the named reference, the mScriptObject when the object cleans up. This
really screwed with the JS engine's GC and I got alot of fatal assertions about
root_points_to_gcArenaPool

***PDT: This eventually leads to a crash in the mail account wizard if you open
it more than one or two times. ***

So we added this to nsGlobalWindowImpl::Cleanup()

  if (mContext)
    mContext->RemoveReference(&mScriptObject, mScriptObject);

shaver and jst seem to agree this is the right thing to do... filing this bug so
that it gets marked nsbeta2+ so I can check in.

Comment 1

[Alec Flett]

add nsbeta2 and crash keyword, and dogfood so it gets quick attention,
reassigning to myself

Comment 2

[scottputterman]

marking M17.

Comment 3

[scottputterman]

marking critical

Comment 4

[Jim Roskind]

Approving for beta2 plus checkin.
Sounds like a small and valuable fix

Comment 5

[Brendan Eich [:brendan]]

What's the fix?  JS_RemoveRootRT is ready and waiting in the JS API -- it's been
there for a while.

/be

Comment 6

[Alec Flett]

well, the fix is to add mContext->RemoveReference(), which I assume will call a 
JS_* API of some sort.. after talking with jst and shaver s'more, I've also 
added this to GlobalWindowImp::SetContext... here, I'll attach a patch..

Comment 7

[Alec Flett]

Attachment: fix for removal of global window root

Comment 8

[Alec Flett]

ok, that patch is in.

Comment 9

[Prashant Desale]

Verified with 2000-07-06-08.

Comment 10

[alla]

There is still a way this can happen. If you open a dialog window (File->Open
File) and close it (click on cancel) the code in
nsJSUtils::nsConvertObjectToJSVal() (called by WindowOpenDialog()) will call
GlobalWindowImpl::GetScriptObject() on the Closed window. This will allocate a
new ScriptObject, which is returned. This object is never RemoveReferenced(),
since mContext == NULL in CleanUp(). 

In fact the whole GlobalWindow is leaked, so CleanUp() is never called. But it
wouldn't work even if it weren't leaked.

Comment 11

[Brendan Eich [:brendan]]

I think this is a dup of bug 43466 and bug 38951.  They're all dups of each
other if you ask me.

Alex correctly identified one part of the problem behind all these bugs, that is
indeed still with us: the premature nulling of mScriptObject in
GlobalWindowImpl::SetDocShell(nsnull) -- more travis damage that danm and I
worked all day yesterday to undo.  Dan has a fix in sight, which involves
handing off the window's mScriptObject root to its nsJSContext when the
SetDocShell(nsnull) happens.  If it fixes all these bugs, we should close them. 
I don't think we need to reopen this bug, however.

/be