Closed Bug 1287547 Opened 3 years ago Closed 3 years ago

twitter ghost window leaked by persistent rooted HTMLScriptElement

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: bkelly, Assigned: bkelly)

References

(Blocks 1 open bug)

Details

(Whiteboard: [MemShrink])

Attachments

(1 file)

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.
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.
Priority: -- → P1
I got this again today without any addons installed.
I'm trying to instrument PersistentRooted<> to see where we ever root HTMLScriptElement.  I'm kind of wondering if its in CycleCollectedJSRuntime::NurseryWrapperPreserved().
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
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.
Note, I also instrumented js::CancelOffThreadParses(JSRuntime* rt) and it doesn't seem to get called here.
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!
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.
Flags: needinfo?(jcoppeard)
After further debugging I don't think thats the issue.
Flags: needinfo?(jcoppeard)
We are pretty sure that the nsScriptLoadRequest is being destroyed without cleaning up its mOffThreadToken (which is the ParseTask).
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.
Attachment #8774481 - Flags: review?(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!
Attachment #8774481 - Flags: review?(bzbarsky) → review+
Pushed by bkelly@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/7a40fff81778
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
See Also: → 1279757
See Also: 1279757
Duplicate of this bug: 1279757
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.