Closed Bug 975419 Opened 6 years ago Closed 6 years ago

Assertion failure in mozilla:dom:ScriptSettingsStackEntry::ScriptSettingsStackEntry

Categories

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

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: Irving, Assigned: bholley)

Details

Attachments

(3 files)

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
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.
Flags: needinfo?(bzbarsky)
#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.
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.
Flags: needinfo?(bzbarsky)
(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.
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.
> 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.
> or does it also affect window contents?

Seems to me like it can affect any global.
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?
Flags: needinfo?(terrence)
Flags: needinfo?(continuation)
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.)
Flags: needinfo?(continuation)
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
Flags: needinfo?(continuation)
I think you should just be able to dig out the JS object and stick it in the array with the rest of them.
Flags: needinfo?(continuation)
Oh, gross, you have to add a new array, I guess.
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
Flags: needinfo?(terrence)
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 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
Attachment #8383479 - Flags: review?(bzbarsky) → review+
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?
Attachment #8383476 - Flags: review?(continuation) → review+
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.
Attachment #8383477 - Flags: review?(terrence)
Attachment #8383477 - Flags: review?(continuation)
Attachment #8383477 - Flags: review+
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.
Attachment #8383479 - Flags: review?(continuation) → review+
(In reply to Andrew McCreight [:mccr8] from comment #20)
> These are both weak references, like the global on nsGlobalWindow?

Correct.
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
Attachment #8383477 - Flags: review?(terrence) → review+
Wow, that totally wasn't all-platform.

https://tbpl.mozilla.org/?tree=Try&rev=8d8b86542e25
https://hg.mozilla.org/mozilla-central/rev/4d8af8698173
https://hg.mozilla.org/mozilla-central/rev/058ed6c240bb
https://hg.mozilla.org/mozilla-central/rev/1742ecba9c09
Assignee: nobody → bobbyholley
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.