Closed Bug 1181619 Opened 4 years ago Closed 4 years ago

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

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox40 --- unaffected
firefox41 + fixed
firefox42 + fixed
firefox-esr38 --- unaffected
b2g-v2.1S --- unaffected
b2g-v2.2 --- unaffected
b2g-v2.2r --- unaffected
b2g-master --- fixed

People

(Reporter: cbook, Assigned: bzbarsky)

References

(Blocks 2 open bugs, )

Details

(Keywords: assertion)

Attachments

(3 files)

Attached file 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
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".
[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.
Blocks: 1174486
Component: JavaScript Engine → DOM
Flags: needinfo?(bobbyholley)
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.
Flags: needinfo?(bobbyholley)
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.
And that of course fails any time someone executes script without entering a microtask first.
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
I filed bug 1181740 on a more general solution here.
Blocks: 1181740
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.
Attachment #8631209 - Flags: review?(bobbyholley) → review+
> 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.
https://hg.mozilla.org/mozilla-central/rev/ed167ab17003
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Flags: in-testsuite? → in-testsuite+
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.
Attachment #8631209 - Flags: approval-mozilla-aurora?
i can confirm that this checkin fix urls that bughunter reported for this crash \o/
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.
Attachment #8631209 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Group: core-security → core-security-release
Group: core-security-release
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.