295983 - [FIXr]Leak when closing a tab or window that has a page loading in it

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

BUILD: Current trunk build

STEPS TO REPRODUCE:

1) env XPCOM_MEM_LEAK_LOG=/tmp/log.txt firefox
2) Open two tabs
3) Start
   http://lxr.mozilla.org/seamonkey/source/layout/base/nsCSSFrameConstructor.cpp
   loading in the first tab.
4) Close that first tab before the load completes (using Ctrl-W)
5) Quit firefox
6) Look at leak log

EXPECTED RESULTS: very few leaks

ACTUAL RESULTS: leaked nsDocument objects, etc.

What's going on is that when we stop the load we have script on the stack (the
event handler script for the <xul:key> that's closing the tab).  So we don't
null out the mParser in nsHTMLDocument::EndLoad; instead we set the termination
func on the context and return.  Before the script is done, though, it
dispatches a DOM event (a "select" event, to be precise).  This calls into
nsJSContext::CallEventHandler which nulls out the termination function.  So we
never break the document -> parser -> sink -> document cycle and leak the whole
thing.

I think we could fix that up by restoring the original func and arg instead of
just nulling them out unconditionally.  That would still leave a problem --
script could both mess with the document and call close() on the window.  In
that case the two would race to both set the termination func on the context and
either we leak like this or the window doesn't close properly...

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix

So the changes in this patch are as follows:

1)  Make the termination func a list of termination funcs to deal with both the
window and document setting their termination func from the same script.
2)  When executing script via nsJSContext save the termination funcs we have
posted before we run the script and restore them afterward.  This part fixes
the the leak on closing a tab (but not on closing a window, which also leaks at
the moment).
3)  Call ScriptEvaluated from all places that could be posting termination
functions (in particular, nsJSContext::EvaluateStringWithValue was never
calling it).
4)  Lock the return value of EvaluateStringWithValue so it won't get GC'ed
(this change necessitated by change #3 and depends on bug 296006).
5)  Change XBL to not let the wrapped native go away while the XBL binding is
installed (necessitated by change #3).
6)  Lock the return value of CallEventHandler, just because it's the right
thing to do.
7)  Call ScriptEvaluated _after_ popping the context stack.  This fixes the
leak on window close.  What was happening there was that the document load was
ended from inside ReallyCloseWindow (called by the window termination func). 
But since the window termination func was called before the stack was popped,
the document simply posted a termination func of its own instead of breaking
the cycle through the parser.  And this termination func just got ignored. 
Popping the context stack first makes the document actually null out its parser
in this situation.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Another possible fix would be to use nsCxPusher in this code, since that doesn't
call ScriptEvaluated for nested script invocations.  The problem there is
actually that it doesn't check whether the inner and outer script invocation are
running on different (ns)JSContexts; if they're not (eg chrome dispatching an
event from script and a content event handler running in response to that) it
looks like it can fail to call ScriptEvaluated when it really should be called.

Comment 3

[Johnny Stenback (:jst)]

Comment on attachment 184893 [details] [diff] [review]
Proposed fix

r+sr=jst

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 184893 [details] [diff] [review]
Proposed fix

Requesting 1.8b3 approval.  This isn't the safest patch in the world, but it's
the safest I can come up with for this problem, and without this we leak all
sorts of stuff any time a still-loading window/tab is closed...

Comment 5

[Boris Zbarsky [:bzbarsky]]

Fixed (with a small follow-up bustage fix).