Closed
Bug 1287547
Opened 8 years ago
Closed 8 years ago
twitter ghost window leaked by persistent rooted HTMLScriptElement
Categories
(Core :: DOM: Core & HTML, defect, P1)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla50
Tracking | Status | |
---|---|---|
firefox50 | --- | fixed |
People
(Reporter: bkelly, Assigned: bkelly)
References
(Blocks 1 open bug)
Details
(Whiteboard: [MemShrink])
Attachments
(1 file)
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
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.
Updated•8 years ago
|
Blocks: GhostWindows
Comment 1•8 years ago
|
||
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.
Updated•8 years ago
|
Priority: -- → P1
Assignee | ||
Comment 3•8 years ago
|
||
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
Assignee | ||
Comment 4•8 years ago
|
||
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.
Assignee | ||
Comment 5•8 years ago
|
||
Note, I also instrumented js::CancelOffThreadParses(JSRuntime* rt) and it doesn't seem to get called here.
Assignee | ||
Comment 6•8 years ago
|
||
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!
Assignee | ||
Comment 7•8 years ago
|
||
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)
Assignee | ||
Comment 8•8 years ago
|
||
After further debugging I don't think thats the issue.
Flags: needinfo?(jcoppeard)
Assignee | ||
Comment 9•8 years ago
|
||
We are pretty sure that the nsScriptLoadRequest is being destroyed without cleaning up its mOffThreadToken (which is the ParseTask).
Assignee | ||
Comment 10•8 years ago
|
||
Assignee | ||
Comment 11•8 years ago
|
||
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 12•8 years ago
|
||
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+
Comment 13•8 years ago
|
||
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
Comment 14•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7a40fff81778
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Assignee | ||
Updated•8 years ago
|
Assignee | ||
Updated•8 years ago
|
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•