1287547 - twitter ghost window leaked by persistent rooted HTMLScriptElement

Status: RESOLVED FIXED

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

Description

[Ben Kelly [:bkelly, not reviewing]]

Breaking this out from bug 1277376 comment 12.  I got a ghost window on a fresh nightly today that seems to be due to a rooted HTMLScriptElement wrapper:

0000019B7D651920 [JS Object (Window)]
    --[UnwrapDOMObject(obj)]--> 0000019B545D6C00 [nsGlobalWindow # 2147484160 inner https://twitter.
com/i/cards/tfw/v1/745247585036869632?advertiser_name=GoodMad&cardname=promo_website&is_following_ad
vertiser=false&impression_id=3f954d1eff53bc19&lang=en&card_height=271#xdm_e=https%3A%2F%2Ftwitter.co
m&xdm_c=default2752&xdm_p=1]

    Root 0000019B7D651920 is a marked GC object.

via persistent-Object :
0000019B76A490A0 [HTMLScriptElement <no private>]
    --[shape]--> 0000019B83C88588 [shape]
    --[base]--> 0000019B7ADE1380 [base_shape]
    --[global]--> 0000019B7D651920 [Window <no private>]

Note, I did have mozilla-tree-status addon installed since I was testing how well bug 1267693 fixed things.  I'm not trying to reproduce without the addon.

Comment 1

[Andrew McCreight [:mccr8]]

I think the persistent- means that the reflector is in a PersistentRooted<> variable. I'm not sure what would do that for a random element.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

I got this again today without any addons installed.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

I'm trying to instrument PersistentRooted<> to see where we ever root HTMLScriptElement.  I'm kind of wondering if its in CycleCollectedJSRuntime::NurseryWrapperPreserved().

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

I'm 95% sure this is due to the window being closed or navigated while there is an async script ParseTask running.

The main place I see PersistentRooted<> being applied to HTMLScriptElement is in stacks like:

>	xul.dll!JS_GetSimpleObjectName(JSObject * aValue) Line 287	C++
 	xul.dll!JS::PersistentRooted<JSObject * __ptr64>::log(JSObject * aValue) Line 1095	C++
 	xul.dll!JS::PersistentRooted<JSObject * __ptr64>::set<JSObject * __ptr64 const & __ptr64>(JSObject * const & value) Line 1087	C++
 	xul.dll!JS::PersistentRooted<JSObject * __ptr64>::operator=(JSObject * const & p) Line 1067	C++
 	xul.dll!JS::OwningCompileOptions::setElement(JSObject * e) Line 3856	C++
 	xul.dll!JS::OwningCompileOptions::copy(JSContext * cx, const JS::ReadOnlyCompileOptions & rhs) Line 3823	C++
 	xul.dll!js::ParseTask::init(JSContext * cx, const JS::ReadOnlyCompileOptions & options) Line 222	C++
 	xul.dll!js::StartOffThreadParseScript(JSContext * cx, const JS::ReadOnlyCompileOptions & options, const char16_t * chars, unsigned __int64 length, void(*)(void *, void *) callback, void * callbackData) Line 501	C++
 	xul.dll!JS::CompileOffThread(JSContext * cx, const JS::ReadOnlyCompileOptions & options, const char16_t * chars, unsigned __int64 length, void(*)(void *, void *) callback, void * callbackData) Line 4081	C++
 	xul.dll!nsScriptLoader::AttemptAsyncScriptCompile(nsScriptLoadRequest * aRequest) Line 1691	C++
 	xul.dll!nsScriptLoader::PrepareLoadedRequest(nsScriptLoadRequest * aRequest, nsIIncrementalStreamLoader * aLoader, nsresult aStatus, mozilla::Vector<char16_t,0,mozilla::MallocAllocPolicy> & aString) Line 2538	C++
 	xul.dll!nsScriptLoader::OnStreamComplete(nsIIncrementalStreamLoader * aLoader, nsISupports * aContext, nsresult aChannelStatus, nsresult aSRIStatus, mozilla::Vector<char16_t,0,mozilla::MallocAllocPolicy> & aString, mozilla::dom::SRICheckDataVerifier * aSRIDataVerifier) Line 2332	C++
 	xul.dll!nsScriptLoadHandler::OnStreamComplete(nsIIncrementalStreamLoader * aLoader, nsISupports * aContext, nsresult aStatus, unsigned int aDataLength, const unsigned char * aData) Line 2846	C++
 	xul.dll!nsIncrementalStreamLoader::OnStopRequest(nsIRequest * request, nsISupports * ctxt, nsresult aStatus) Line 98	C++
 	xul.dll!mozilla::net::InterceptFailedOnStop::OnStopRequest(nsIRequest * aRequest, nsISupports * aContext, nsresult aStatusCode) Line 898	C++
 	xul.dll!mozilla::net::nsHTTPCompressConv::OnStopRequest(nsIRequest * request, nsISupports * aContext, nsresult aStatus) Line 156	C++
 	xul.dll!mozilla::net::nsStreamListenerTee::OnStopRequest(nsIRequest * request, nsISupports * context, nsresult status) Line 51	C++
 	xul.dll!mozilla::net::nsHttpChannel::OnStopRequest(nsIRequest * request, nsISupports * ctxt, nsresult status) Line 6547	C++
 	xul.dll!nsInputStreamPump::OnStateStop() Line 715	C++
 	xul.dll!nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream * stream) Line 433	C++
 	xul.dll!nsInputStreamReadyEvent::Run() Line 97	C++

I instrumented ParseTask.init() and ~ParseTask() in a debug build.  It ran slow enough that I was able to close the twitter tab when I saw the ParseTask get initialized.  The twitter window then leaked through shutdown.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Note, I also instrumented js::CancelOffThreadParses(JSRuntime* rt) and it doesn't seem to get called here.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Note, this leaked things straight through shutdown:

WARNING: YOU ARE LEAKING THE WORLD (at least one JSRuntime and everything alive inside it, that is) AT
JS_ShutDown TIME.  FIX THIS!

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

So I tried to force this to happen again by doing this:

1) Set a breakpoint in ScriptParseTask::parse()
2) Freeze the parse thread
3) Resume the rest of the threads
4) Close the window
5) Thaw the parse thread

In this case, though, everything cleaned up correctly.

It seems this leak occurs when the window is closed before the parse thread actually starts parsing the script.  I think this is most likely when its waiting to parse on the next GC.

Jon, should GlobalHelperThreadState::cancelParseTask() inspect the contents of parseWorklist() and parseWaitingOnGC() queues?  It seems we don't cleanup pending parse tasks.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

After further debugging I don't think thats the issue.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

We are pretty sure that the nsScriptLoadRequest is being destroyed without cleaning up its mOffThreadToken (which is the ParseTask).

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Don't leak HTMLScriptElement wrapper when page is closed during off-thread scr ipt parsing. r=bz

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8774481 [details] [diff] [review]
Don't leak HTMLScriptElement wrapper when page is closed during off-thread scr ipt parsing. r=bz

Try is looking reasonably green:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9477aad31221

This implements the cleanup code in the Cancel() method and destructor.  It also adds an assert in the destructor to try to prevent relying on it.

I verified this fixed the one reliable way I had to trigger this bug locally.  Not sure how to write a test for something requiring this kind of timing.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8774481 [details] [diff] [review]
Don't leak HTMLScriptElement wrapper when page is closed during off-thread scr ipt parsing. r=bz

r=me.  Thanks for hunting this down!

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a40fff81778
Don't leak HTMLScriptElement wrapper when page is closed during off-thread script parsing. r=bz

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7a40fff81778