1181619 - Assertion failure: !JS_IsExceptionPending(cx), at c:\users\mozilla\debug-builds\ mozilla-central\firefox-debug\dom\bindings\MutationObserverBinding.cpp:1292

Status: RESOLVED FIXED

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

Description

[Carsten Book [:Tomcat]]

Attachment: windows stack

Found by bughunter and reproduced on a windows 7 trunk debug build based on m-c tip


Steps to reproduce:
--> Load http://lite.almasryalyoum.com/lists/38392/
---> Assertion failure: !JS_IsExceptionPending(cx), at c:\users\mozilla\debug-builds\ mozilla-central\firefox-debug\dom\bindings\MutationObserverBinding.cpp:1292

.  0  Id: 198.d64 Suspend: 1 Teb: bffdf000 Unfrozen
ChildEBP RetAddr  
001ae2d4 5a65ef41 xul!mozilla::dom::MutationRecordBinding::get_type(struct JSContext * cx = 0x0ad59460, class JS::Handle<JSObject *> obj = class JS::Handle<JSObject *>, class nsDOMMutationRecord * self = 0x11cfee80, class JSJitGetterCallArgs args = class JSJitGetterCallArgs)+0x61 [c:\users\mozilla\debug-builds\mozilla-central\firefox-debug\dom\bindings\mutationobserverbinding.cpp @ 1292]
001ae328 5c4a1cc6 xul!mozilla::dom::GenericBindingGetter(struct JSContext * cx = 0x0ad59460, unsigned int argc = 0, class JS::Value * vp = 0x001ae360)+0x115 [c:\users\mozilla\debug-builds\mozilla-central\dom\bindings\bindingutils.cpp @ 2490]
001ae37c 24495971 xul!js::jit::DoCallNativeGetter(struct JSContext * cx = 0x0ad59460, class JS::Handle<JSFunction *> callee = class JS::Handle<JSFunction *>, class JS::Handle<JSObject *> obj = class JS::Handle<JSObject *>, class JS::MutableHandle<JS::Value> result = class JS::MutableHandle<JS::Value>)+0xd6 [c:\users\mozilla\debug-builds\mozilla-central\js\src\jit\baselineic.cpp @ 916]
WARNING: Frame IP not in any known module. Following frames may be wrong.
001ae4a0 5be18690 0x24495971
001ae4a8 5bde57cd xul!js::gc::TenuredCell::arenaHeader(void)+0xc0 [c:\users\mozilla\debug-builds\mozilla-central\js\src\gc\heap.h @ 1393]
001ae4b8 5be06138 xul!js::gc::AssertValidToSkipBarrier(class js::gc::TenuredCell * thing = 0x001ae4d0)+0xcd [c:\users\mozilla\debug-builds\mozilla-central\js\src\gc\heap.h @ 1468]
001ae4f4 001ae56c xul!NewFunctionClone(struct JSContext * cx = 0x001ae4d0, class JS::Handle<JSFunction *> fun = class JS::Handle<JSFunction *>, js::NewObjectKind newKind = GenericObject (0n0), js::gc::AllocKind allocKind = 0n1762552 (No matching enumerant), class JS::Handle<JSObject *> proto = class JS::Handle<JSObject *>)+0x1a8 [c:\users\mozilla\debug-builds\mozilla-central\js\src\jsfun.cpp @ 2105]
001ae50c 0456c248 0x1ae56c
001ae510 ffffff88 0x456c248
001ae514 0456c218 0xffffff88
001ae518 ffffff88 0x456c218
001ae51c 0d4e1970 0xffffff88
001ae520 ffffff88 0xd4e1970
001ae524 00000000 0xffffff88

Comment 1

[Boris Zbarsky [:bzbarsky]]

Some data on this pending exception:

1)  It's a JSEXN_REFERENCEERR.
2)  It was thrown at http://lite.almasryalyoum.com/lists/38392/ line 75 column 5.  That line is:

  GS_googleAddAdSenseService("ca-pub-4450440688621183");
3)  Unsurprisingly, the message is "GS_googleAddAdSenseService is not defined".

Comment 2

[Boris Zbarsky [:bzbarsky]]

[Tracking Requested - why for this release]: Regression that can break websites.

The good news is that this reproduces easily if I just save the page and add a <base> to it.  So it's simple enough to just breakpoint on the relevant bit of JS.

At the point when we throw this exception, the C++ stack looks like this:

#1  0x000000010758e638 in js::CallJSNative (cx=0x135caae30, native=0x107330d60 <js::math_sin(JSContext*, unsigned int, JS::Value*)>, args=@0x7fff5fbf8440) at jscntxtinlines.h:235
#2  0x000000010751be9e in js::Invoke (cx=0x135caae30, args={<JS::detail::CallArgsBase<0>> = {<JS::CallReceiver> = {<JS::detail::CallReceiverBase<0>> = {<JS::detail::UsedRvalBase<0>> = {usedRval_ = false}, argv_ = 0x119b4f0b8}, <No data fields>}, argc_ = 0, constructing_ = false}, <No data fields>}, construct=js::NO_CONSTRUCT) at Interpreter.cpp:711
#3  0x00000001075368cf in Interpret (cx=0x135caae30, state=@0x7fff5fbfb490) at Interpreter.cpp:2959
#4  0x0000000107528db2 in js::RunScript (cx=0x135caae30, state=@0x7fff5fbfb490) at Interpreter.cpp:655
#5  0x0000000107544103 in js::ExecuteKernel (cx=0x135caae30, script={<js::HandleBase<JSScript *>> = {<No data fields>}, ptr = 0x7fff5fbfb7f8}, scopeChainArg=@0x137c591a0, thisv=@0x7fff5fbfb680, newTargetValue=@0x7fff5fbfb670, type=js::EXECUTE_GLOBAL, evalInFrame={ptr_ = 0}, result=0x0) at Interpreter.cpp:895
#6  0x000000010754447f in js::Execute (cx=0x135caae30, script={<js::HandleBase<JSScript *>> = {<No data fields>}, ptr = 0x7fff5fbfb7f8}, scopeChainArg=@0x137c591a0, rval=0x0) at Interpreter.cpp:928
#7  0x0000000107c1e64a in Evaluate (cx=0x135caae30, scope={<js::HandleBase<JSObject *>> = {<No data fields>}, ptr = 0x7fff5fbfba10}, staticScope={<js::HandleBase<js::ScopeObject *>> = {<No data fields>}, ptr = 0x7fff5fbfb9e8}, optionsArg=@0x7fff5fbfbef0, srcBuf=@0x7fff5fbfc220, rval={<js::MutableHandleBase<JS::Value>> = {<js::MutableValueOperations<JS::MutableHandle<JS::Value> >> = {<js::ValueOperations<JS::MutableHandle<JS::Value> >> = {<No data fields>}, <No data fields>}, <No data fields>}, ptr = 0x7fff5fbfbd98}) at jsapi.cpp:4408
#8  0x0000000107c1e979 in Evaluate (cx=0x135caae30, scopeChain=@0x7fff5fbfbc70, optionsArg=@0x7fff5fbfbef0, srcBuf=@0x7fff5fbfc220, rval={<js::MutableHandleBase<JS::Value>> = {<js::MutableValueOperations<JS::MutableHandle<JS::Value> >> = {<js::ValueOperations<JS::MutableHandle<JS::Value> >> = {<No data fields>}, <No data fields>}, <No data fields>}, ptr = 0x7fff5fbfbd98}) at jsapi.cpp:4435
#9  0x0000000107c1e80d in JS::Evaluate (cx=0x135caae30, scopeChain=@0x7fff5fbfbc70, optionsArg=@0x7fff5fbfbef0, srcBuf=@0x7fff5fbfc220, rval={<js::MutableHandleBase<JS::Value>> = {<js::MutableValueOperations<JS::MutableHandle<JS::Value> >> = {<js::ValueOperations<JS::MutableHandle<JS::Value> >> = {<No data fields>}, <No data fields>}, <No data fields>}, ptr = 0x7fff5fbfbd98}) at jsapi.cpp:4490
#10 0x000000010320bcee in nsJSUtils::EvaluateString (aCx=0x135caae30, aSrcBuf=@0x7fff5fbfc220, aEvaluationGlobal={<js::HandleBase<JSObject *>> = {<No data fields>}, ptr = 0x7fff5fbfbff0}, aCompileOptions=@0x7fff5fbfbef0, aEvaluateOptions=@0x7fff5fbfbdd0, aRetValue={<js::MutableHandleBase<JS::Value>> = {<js::MutableValueOperations<JS::MutableHandle<JS::Value> >> = {<js::ValueOperations<JS::MutableHandle<JS::Value> >> = {<No data fields>}, <No data fields>}, <No data fields>}, ptr = 0x7fff5fbfbd98}, aOffThreadToken=0x0) at nsJSUtils.cpp:223
#11 0x000000010320c2ff in nsJSUtils::EvaluateString (aCx=0x135caae30, aSrcBuf=@0x7fff5fbfc220, aEvaluationGlobal={<js::HandleBase<JSObject *>> = {<No data fields>}, ptr = 0x7fff5fbfbff0}, aCompileOptions=@0x7fff5fbfbef0, aOffThreadToken=0x0) at nsJSUtils.cpp:285
#12 0x0000000103250a4d in nsScriptLoader::EvaluateScript (this=0x13acfd120, aRequest=0x13da28210, aSrcBuf=@0x7fff5fbfc220, aOffThreadToken=0x0) at nsScriptLoader.cpp:1142

At the point when we try to call into the mutation observer, the stack looks like this:

#22 0x0000000102ef22ad in nsDOMMutationObserver::HandleMutations () at nsDOMMutationObserver.h:528
#23 0x0000000102ee22a9 in nsContentUtils::PerformMainThreadMicroTaskCheckpoint () at ../../../mozilla/dom/base/nsContentUtils.cpp:5229
#24 0x0000000102ee2245 in nsContentUtils::LeaveMicroTask () at ../../../mozilla/dom/base/nsContentUtils.cpp:5198
#25 0x0000000103239351 in nsAutoMicroTask::~nsAutoMicroTask (this=0x7fff5fbfbc20) at nsContentUtils.h:2597
#26 0x000000010322d0a5 in nsAutoMicroTask::~nsAutoMicroTask (this=0x7fff5fbfbc20) at nsContentUtils.h:2596
#27 0x000000010320bf33 in nsJSUtils::EvaluateString (aCx=0x135caae30, aSrcBuf=@0x7fff5fbfc220, aEvaluationGlobal={<js::HandleBase<JSObject *>> = {<No data fields>}, ptr = 0x7fff5fbfbff0}, aCompileOptions=@0x7fff5fbfbef0, aEvaluateOptions=@0x7fff5fbfbdd0, aRetValue={<js::MutableHandleBase<JS::Value>> = {<js::MutableValueOperations<JS::MutableHandle<JS::Value> >> = {<js::ValueOperations<JS::MutableHandle<JS::Value> >> = {<No data fields>}, <No data fields>}, <No data fields>}, ptr = 0x7fff5fbfbd98}, aOffThreadToken=0x0) at nsJSUtils.cpp:248
#28 0x000000010320c2ff in nsJSUtils::EvaluateString (aCx=0x135caae30, aSrcBuf=@0x7fff5fbfc220, aEvaluationGlobal={<js::HandleBase<JSObject *>> = {<No data fields>}, ptr = 0x7fff5fbfbff0}, aCompileOptions=@0x7fff5fbfbef0, aOffThreadToken=0x0) at nsJSUtils.cpp:285
#29 0x0000000103250a4d in nsScriptLoader::EvaluateScript (this=0x13acfd120, aRequest=0x13da28210, aSrcBuf=@0x7fff5fbfc220, aOffThreadToken=0x0) at nsScriptLoader.cpp:1142

So this is a regression from bug 1174486.  The fundamental issue is that the nsAutoMicroTask is in EvaluateString, not its callers, so it comes off the stack before we have reported the exception.  I really wish we had a way to assert in this situation...

I can obviously fix this up for EvaluateString by passing in the AutoJSAPI instead of the JSContext and going back to reporting the exception inside the EvaluateString call.  But this seems like a general footgun in the take-ownership-of-error-reporting functionality: if we're not already in an automicrotask, and someone inside us enters one, we will get calls into script while exceptions are pending.

It seems to me that taking ownership of error reporting needs to imply an automicrotask or something.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Pretty minimal testcase

Comment 4

[Bobby Holley (:bholley)]

We discussed this on IRC. Ideally we'd like LeaveMicroTask to trigger the exception reporting before it does its thing. There are varying ways to do that.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Some notes from the IRC discussion:

1) We can't generally just have nsAutoMicroTask take an AutoJSAPI as a constructor argument and report on it in the destructor, because in the case of an AutoEntryScript we want to AutoEntryScript to come off the stack before LeaveMicroTask.

2) We don't have an automatic way for an nsAutoMicroTask to get the top AutoJSAPI to report on it.

But given #1, the desired ordering is really that we enter the nsAutoMicroTask, then set up the AutoJSAPI anyway.  That's what we want to enforce.

So the plan is to have EnterMicroTask assert !GetCurrentJSContext() when going from 0 to 1, and hoist the nsAutoMicroTask out of EvaluateString into whichever callers don't already have it.

Comment 6

[Boris Zbarsky [:bzbarsky]]

And that of course fails any time someone executes script without entering a microtask first.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Make sure we've entered a microtask before we call nsJSUtils::EvaluateString, and put those microtasks outside the relevant AutoEntryScripts so we report any possible exceptions before doing the microtask checkpoint

Comment 8

[Boris Zbarsky [:bzbarsky]]

I filed bug 1181740 on a more general solution here.

Comment 9

[Bobby Holley (:bholley)]

Comment on attachment 8631209 [details] [diff] [review]
Make sure we've entered a microtask before we call nsJSUtils::EvaluateString, and put those microtasks outside the relevant AutoEntryScripts so we report any possible exceptions before doing the microtask checkpoint

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

Implicit in this change is the removal of the microtask for XBL field evaluation, right? Worth mentioning in the commit message, since it's not obvious.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> Implicit in this change is the removal of the microtask for XBL field evaluation, right? 

Nope.  nsXBLProtoImplField::InstallField already has an nsAutoMicroTask on the stack before it does anything else.  So no behavior change there at all.

Comment 11

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ed167ab17003

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ed167ab17003

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8631209 [details] [diff] [review]
Make sure we've entered a microtask before we call nsJSUtils::EvaluateString, and put those microtasks outside the relevant AutoEntryScripts so we report any possible exceptions before doing the microtask checkpoint

Approval Request Comment
[Feature/regressing bug #]: Bug 1174486
[User impact if declined]: Could have some sites that use mutation observers end
   up with weird behavior where code doesn't run when it should.
[Describe test coverage new/current, TreeHerder]: Decent.
[Risks and why]: I think this is very low-risk.
[String/UUID change made/needed]: None.

Comment 14

[Carsten Book [:Tomcat]]

i can confirm that this checkin fix urls that bughunter reported for this crash \o/

Comment 15

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8631209 [details] [diff] [review]
Make sure we've entered a microtask before we call nsJSUtils::EvaluateString, and put those microtasks outside the relevant AutoEntryScripts so we report any possible exceptions before doing the microtask checkpoint

Approved for uplift to aurora so that we don't ship this regression with 41.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cc5082dd3f73