975419 - Assertion failure in mozilla:dom:ScriptSettingsStackEntry::ScriptSettingsStackEntry

Status: RESOLVED FIXED

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

Description

[:Irving Reid (No longer working on Firefox)]

Assertion failure: mGlobalObject->GetGlobalJSObject() (Must have an actual JS global for the duration on the stack), at ../../dist/include/mozilla/dom/ScriptSettings.h:79

The assertion failure happens shortly after opening about:addons, disabling a restartless add-on (Whimsy, in my tests), and then navigating away from about:addons.

Firefox Trunk build with --enable-debug, --disable-optimize, --enable-tests, --enable-profiling

Irvings-MozBook% clang --version
Apple LLVM version 5.0 (clang-500.2.75) (based on LLVM 3.3svn)
Target: x86_64-apple-darwin12.5.0
Thread model: posix

Process:         firefox [28906]
Path:            /Users/USER/*/NightlyDebug.app/Contents/MacOS/firefox
Identifier:      org.mozilla.nightlydebug
Version:         30.0a1 (3014.2.20)
Code Type:       X86-64 (Native)
Parent Process:  Python [28894]
User ID:         501

Date/Time:       2014-02-21 09:41:49.139 -0500
OS Version:      Mac OS X 10.8.5 (12F45)
Report Version:  10

Interval Since Last Report:          74390 sec
Crashes Since Last Report:           1
Per-App Interval Since Last Report:  967 sec
Per-App Crashes Since Last Report:   1
Anonymous UUID:                      897C5300-2DBD-1EBF-695C-45E14DF340BB

Crashed Thread:  0  Dispatch queue: com.apple.main-thread

Exception Type:  EXC_BAD_ACCESS (SIGSEGV)
Exception Codes: KERN_INVALID_ADDRESS at 0x0000000000000000

VM Regions Near 0:
--> 
    __TEXT                 0000000100000000-000000010000b000 [   44K] r-x/rwx SM=COW  /Users/USER/*/NightlyDebug.app/Contents/MacOS/firefox

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   XUL                           	0x000000010334cdf6 mozilla::dom::ScriptSettingsStackEntry::ScriptSettingsStackEntry(nsIGlobalObject*, bool) + 262 (ScriptSettings.h:78)
1   XUL                           	0x000000010332818c mozilla::dom::AutoIncumbentScript::AutoIncumbentScript(nsIGlobalObject*) + 44 (ScriptSettings.cpp:233)
2   XUL                           	0x000000010332814d mozilla::dom::AutoIncumbentScript::AutoIncumbentScript(nsIGlobalObject*) + 29 (ScriptSettings.cpp:237)
3   XUL                           	0x0000000102e8638c void mozilla::Maybe<mozilla::dom::AutoIncumbentScript>::construct<nsIGlobalObject*>(nsIGlobalObject* const&) + 156 (Maybe.h:55)
4   XUL                           	0x0000000102e7e4b6 mozilla::dom::CallbackObject::CallSetup::CallSetup(mozilla::dom::CallbackObject*, mozilla::ErrorResult&, mozilla::dom::CallbackObject::ExceptionHandling, JSCompartment*, bool) + 1222 (CallbackObject.cpp:138)
5   XUL                           	0x0000000102e7dfe4 mozilla::dom::CallbackObject::CallSetup::CallSetup(mozilla::dom::CallbackObject*, mozilla::ErrorResult&, mozilla::dom::CallbackObject::ExceptionHandling, JSCompartment*, bool) + 68 (CallbackObject.cpp:169)
6   XUL                           	0x000000010347524c void mozilla::dom::EventListener::HandleEvent<mozilla::dom::EventTarget*>(mozilla::dom::EventTarget* const&, nsDOMEvent&, mozilla::ErrorResult&, mozilla::dom::CallbackObject::ExceptionHandling) + 124 (EventListenerBinding.h:41)
7   XUL                           	0x0000000103466de6 nsEventListenerManager::HandleEventSubType(nsListenerStruct*, nsIDOMEvent*, mozilla::dom::EventTarget*) + 326 (nsEventListenerManager.cpp:957)
8   XUL                           	0x0000000103467142 nsEventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent**, mozilla::dom::EventTarget*, nsEventStatus*) + 706 (nsEventListenerManager.cpp:1020)
9   XUL                           	0x000000010347f8e9 nsEventListenerManager::HandleEvent(nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent**, mozilla::dom::EventTarget*, nsEventStatus*) + 345 (nsEventListenerManager.h:327)
10  XUL                           	0x0000000103473d0f nsEventTargetChainItem::HandleEvent(nsEventChainPostVisitor&, ELMCreationDetector&) + 479 (nsEventDispatcher.cpp:193)
11  XUL                           	0x000000010345f8e8 nsEventTargetChainItem::HandleEventTargetChain(nsTArray<nsEventTargetChainItem>&, nsEventChainPostVisitor&, nsDispatchingCallback*, ELMCreationDetector&) + 296 (nsEventDispatcher.cpp:263)
12  XUL                           	0x0000000103461221 nsEventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent*, nsEventStatus*, nsDispatchingCallback*, nsCOMArray<mozilla::dom::EventTarget>*) + 4801 (nsEventDispatcher.cpp:597)
13  XUL                           	0x00000001034615d8 nsEventDispatcher::DispatchDOMEvent(nsISupports*, mozilla::WidgetEvent*, nsIDOMEvent*, nsPresContext*, nsEventStatus*) + 376 (nsEventDispatcher.cpp:658)
14  XUL                           	0x000000010398b742 nsINode::DispatchEvent(nsIDOMEvent*, bool*) + 226 (nsINode.cpp:1165)
15  XUL                           	0x0000000103874c85 nsContentUtils::DispatchEvent(nsIDocument*, nsISupports*, nsAString_internal const&, bool, bool, bool, bool*) + 613 (nsContentUtils.cpp:3378)
16  XUL                           	0x0000000103874a14 nsContentUtils::DispatchTrustedEvent(nsIDocument*, nsISupports*, nsAString_internal const&, bool, bool, bool*) + 116 (nsContentUtils.cpp:3348)
17  XUL                           	0x0000000103825b5e nsDocument::DispatchContentLoadedEvents() + 430 (nsDocument.cpp:4694)
18  XUL                           	0x0000000103e58a8c mozilla::dom::XULDocument::DoneWalking() + 1356 (XULDocument.cpp:3233)
19  XUL                           	0x0000000103e4cd4e mozilla::dom::XULDocument::ResumeWalk() + 4302 (XULDocument.cpp:3163)
20  XUL                           	0x0000000103e599e7 mozilla::dom::XULDocument::OnScriptCompileComplete(JSScript*, tag_nsresult) + 567 (XULDocument.cpp:3631)
21  XUL                           	0x0000000103e5975b mozilla::dom::XULDocument::OnStreamComplete(nsIStreamLoader*, nsISupports*, tag_nsresult, unsigned int, unsigned char const*) + 1579 (XULDocument.cpp:3550)
22  XUL                           	0x0000000103e59b6d non-virtual thunk to mozilla::dom::XULDocument::OnStreamComplete(nsIStreamLoader*, nsISupports*, tag_nsresult, unsigned int, unsigned char const*) + 77 (Unified_cpp_content_xul_document_src0.cpp:3551)
23  XUL                           	0x00000001017f4ae3 nsStreamLoader::OnStopRequest(nsIRequest*, nsISupports*, tag_nsresult) + 211 (nsStreamLoader.cpp:100)
24  XUL                           	0x000000010177d951 nsBaseChannel::OnStopRequest(nsIRequest*, nsISupports*, tag_nsresult) + 257 (nsBaseChannel.cpp:732)
25  XUL                           	0x000000010177da3d non-virtual thunk to nsBaseChannel::OnStopRequest(nsIRequest*, nsISupports*, tag_nsresult) + 61 (Unified_cpp_netwerk_base_src0.cpp:745)
26  XUL                           	0x00000001017b8753 nsInputStreamPump::OnStateStop() + 1059 (nsInputStreamPump.cpp:704)
27  XUL                           	0x00000001017b7801 nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) + 433 (nsInputStreamPump.cpp:438)
28  XUL                           	0x00000001017b887f non-virtual thunk to nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) + 47 (Unified_cpp_netwerk_base_src1.cpp:490)
29  XUL                           	0x00000001016775a0 nsInputStreamReadyEvent::Run() + 144 (nsStreamUtils.cpp:85)
30  XUL                           	0x00000001016a1079 nsThread::ProcessNextEvent(bool, bool*) + 1561 (nsThread.cpp:644)
31  XUL                           	0x00000001015a2c4b NS_ProcessPendingEvents(nsIThread*, unsigned int) + 171 (nsThreadUtils.cpp:210)
32  XUL                           	0x000000010303d8c9 nsBaseAppShell::NativeEventCallback() + 201 (nsBaseAppShell.cpp:99)
33  XUL                           	0x0000000102fc9bdc nsAppShell::ProcessGeckoEvents(void*) + 428 (nsAppShell.mm:388)
34  com.apple.CoreFoundation      	0x00007fff91783b31 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17
35  com.apple.CoreFoundation      	0x00007fff91783455 __CFRunLoopDoSources0 + 245
36  com.apple.CoreFoundation      	0x00007fff917a67f5 __CFRunLoopRun + 789
37  com.apple.CoreFoundation      	0x00007fff917a60e2 CFRunLoopRunSpecific + 290
38  com.apple.HIToolbox           	0x00007fff89531eb4 RunCurrentEventLoopInMode + 209
39  com.apple.HIToolbox           	0x00007fff89531b94 ReceiveNextEventCommon + 166
40  com.apple.HIToolbox           	0x00007fff89531ae3 BlockUntilNextEventMatchingListInMode + 62
41  com.apple.AppKit              	0x00007fff8e55f533 _DPSNextEvent + 685
42  com.apple.AppKit              	0x00007fff8e55edf2 -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] + 128
43  XUL                           	0x0000000102fc8b47 -[GeckoNSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] + 119 (nsAppShell.mm:165)
44  com.apple.AppKit              	0x00007fff8e5561a3 -[NSApplication run] + 517
45  XUL                           	0x0000000102fca6c2 nsAppShell::Run() + 162 (nsAppShell.mm:742)
46  XUL                           	0x0000000104c73a1c nsAppStartup::Run() + 156 (nsAppStartup.cpp:276)
47  XUL                           	0x0000000104b2cb6c XREMain::XRE_mainRun() + 6044 (nsAppRunner.cpp:3954)
48  XUL                           	0x0000000104b2d379 XREMain::XRE_main(int, char**, nsXREAppData const*) + 697 (nsAppRunner.cpp:4021)
49  XUL                           	0x0000000104b2d7ed XRE_main + 77 (nsAppRunner.cpp:4231)
50  org.mozilla.nightlydebug      	0x0000000100002067 do_main(int, char**, nsIFile*) + 1639 (nsBrowserApp.cpp:282)
51  org.mozilla.nightlydebug      	0x00000001000015a1 main + 321 (nsBrowserApp.cpp:643)
52  org.mozilla.nightlydebug      	0x0000000100001004 start + 52

Comment 1

[Bobby Holley (:bholley)]

Ok. So this basically means that the incumbent global stashed on the callback object went away by the time we want to invoke that callback, even though the EventListener is still alive (presumably held alive by the EventTarget).

So we have 3 options, all of them kind of crappy:
(1) The Callback holds the incumbent global alive in addition to the callable object and its global.
(2) If the incumbent global has gone away, we don't fire the callback.
(3) If the incumbent global has gone away, we fire the callback without restoring the incumbent (and just use the entry script, which is the global object of the underlying callable).

(1) is the simplest, but it's annoying to hold things alive just for the sake of a somewhat-arcane spec concept. The other 2 are non-deterministics, which isn't great. (3) is kind of off the table IMO.

I might vote for (1), on the grounds that it's probably pretty rare to get into this situation, and this is the only situation where we'd be holding an additional global alive.

Thoughts Boris?

Whatever we decide, the spec probably needs to be updated to say something about this.

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

#1 means we're entraining an entirely new global which is suboptimal from a memory usage perspective.  It might be the least bad option though.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Per spec, nothing ever gets observably GCed, so the situation in question can't arise.

I'd probably be OK with #1 here; it's pretty rare to have an incumbent global for a callback that doesn't match the global of the callback _or_ the global of the thing that's holding on to the callback.

Comment 4

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] (reviews will be slow; ask someone else) from comment #3)
> Per spec, nothing ever gets observably GCed, so the situation in question
> can't arise.

That's not really the case IIUC. The spec definitely allows globals to get observably GCed. Consider the cases with pageshow vs onload, and the reconstruction of session history entries with dynamic iframes. When we talked about it, Hixie said he thought it was pretty hopeless to avoid that in the spec, and was working on making the interactions as simple as possible.

Now, whether that means that this itself is spec-relevant, I don't know. The spec could probably use clarifying on this point at the very least.

> I'd probably be OK with #1 here; it's pretty rare to have an incumbent
> global for a callback that doesn't match the global of the callback _or_ the
> global of the thing that's holding on to the callback.

Ok. I'll whip up a patch.

Comment 5

[:Irving Reid (No longer working on Firefox)]

Does this only arise in cases like XPIProvider, where we're explicitly unloading chrome code that may have left callbacks behind, or does it also affect window contents? For chrome code, if it's feasible, it would be helpful to get a warning or exception at unload time telling us we're leaving behind orphaned callbacks.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> Consider the cases with pageshow vs onload,

That's not a GC issue.  That's a "cached presentation" issue.  It no more exposes GC behavior than the HTTP cache does.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> or does it also affect window contents?

Seems to me like it can affect any global.

Comment 8

[Bobby Holley (:bholley)]

Hm. So in trying to trace the various JSObjects inside nsIGlobalObject implementations, I've run into the snag that TraceCallbacks only takes a JS::Heap<JSObject*>, not a JS::TenuredHeap<JSObject*>.

Here are some options I can think of:
* Add another overload of Trace(..) to TraceCallbacks. Yuck.
* Make TenuredHeap<T> inherit Heap<T> somehow.
* reinterpret_cast<Heap<T>>(mJSObject) (is this valid? Seems dicey).

Thoughts?

Comment 9

[Andrew McCreight [:mccr8]]

At a lower level, it looks like JS_CallTenuredObjectTracer needs to be called, but that doesn't really help when writing a TRACE thing.  I'd guess that adding a new Trace thing to TraceCallbacks is the most sensible option, but I think Terrence or Jon are better people to answer this.  (Though it looks like Jon is away at the moment.)

Comment 10

[Bobby Holley (:bholley)]

Ok. What do I do about SnowWhiteKiller::Trace? It looks like the PurpleBuffer contains an array of JS::Heap<JSObject*> instances.

http://dxr.mozilla.org/mozilla-central/source/xpcom/base/nsCycleCollector.cpp#2387

Comment 11

[Andrew McCreight [:mccr8]]

I think you should just be able to dig out the JS object and stick it in the array with the rest of them.

Comment 12

[Andrew McCreight [:mccr8]]

Oh, gross, you have to add a new array, I guess.

Comment 13

[Terrence Cole [:terrence]]

The only reason TraceCallbacks doesn't have any Tenuredheap<> overloads is that we haven't needed them yet. Let's just update the existing tracing infrastructure to support them.

The need for TenuredHeap at all is a bit sadfaces, but that's just the cost of performance. See the comment here for details: http://dxr.mozilla.org/mozilla-central/source/js/public/RootingAPI.h#290

Comment 14

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=8bc474fe740a

Comment 15

[Bobby Holley (:bholley)]

Attachment: Part 1 - Used a TenuredHeap pointer for SandboxPrivate and BackstagePass. v1

Comment 16

[Bobby Holley (:bholley)]

Attachment: Part 2 - Add a JS::TenuredHeap<JSObject*> overload to TraceCallbacks. v1

Comment 17

[Bobby Holley (:bholley)]

Attachment: Part 3 - Trace the Incumbent Global from a CallbackObject (but check it too, just to be safe). v1

Comment 18

[:Irving Reid (No longer working on Firefox)]

Built a new debug/profiling Trunk with these patches applied; my test no longer crashes, but it has started printing

JavaScript error: , line 0: can't access dead object

each time I navigate between pages after disabling the Whimsy extension.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8383479 [details] [diff] [review]
Part 3 - Trace the Incumbent Global from a CallbackObject (but check it too, just to be safe). v1

r=me

Comment 20

[Andrew McCreight [:mccr8]]

Comment on attachment 8383476 [details] [diff] [review]
Part 1 - Used a TenuredHeap pointer for SandboxPrivate and BackstagePass. v1

Review of attachment 8383476 [details] [diff] [review]:
-----------------------------------------------------------------

These are both weak references, like the global on nsGlobalWindow?

Comment 21

[Andrew McCreight [:mccr8]]

Comment on attachment 8383477 [details] [diff] [review]
Part 2 - Add a JS::TenuredHeap<JSObject*> overload to TraceCallbacks. v1

Review of attachment 8383477 [details] [diff] [review]:
-----------------------------------------------------------------

Terrence should review the RootingAPI.h change, as small as it is.

Comment 22

[Andrew McCreight [:mccr8]]

Comment on attachment 8383479 [details] [diff] [review]
Part 3 - Trace the Incumbent Global from a CallbackObject (but check it too, just to be safe). v1

Review of attachment 8383479 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

I don't understand the CallbackObject::CallSetup::CallSetup stuff, but presumably bz has that covered.

Thanks for hooking up the tenured heap tracing stuff!  You've saved somebody else some work in the future.

Comment 23

[Bobby Holley (:bholley)]

(In reply to Andrew McCreight [:mccr8] from comment #20)
> These are both weak references, like the global on nsGlobalWindow?

Correct.

Comment 24

[Terrence Cole [:terrence]]

Comment on attachment 8383477 [details] [diff] [review]
Part 2 - Add a JS::TenuredHeap<JSObject*> overload to TraceCallbacks. v1

Review of attachment 8383477 [details] [diff] [review]:
-----------------------------------------------------------------

The trace callback changes look good to me. r=me

Comment 25

[Bobby Holley (:bholley)]

Final all-platform try push:

https://tbpl.mozilla.org/?tree=Try&rev=809cc6459d6d

Comment 26

[Bobby Holley (:bholley)]

Wow, that totally wasn't all-platform.

https://tbpl.mozilla.org/?tree=Try&rev=8d8b86542e25

Comment 27

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4d8af8698173
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/058ed6c240bb
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1742ecba9c09

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4d8af8698173
https://hg.mozilla.org/mozilla-central/rev/058ed6c240bb
https://hg.mozilla.org/mozilla-central/rev/1742ecba9c09