Closed Bug 1396904 Opened 7 years ago Closed 7 years ago

Assertion failure: !inUnsafeRegion ([AutoAssertNoGC] possible GC in GC-unsafe region)

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- wontfix
firefox55 --- wontfix
firefox56 --- wontfix
firefox57 --- fixed

People

(Reporter: jkratzer, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase)

Attachments

(4 files)

Attached file Testcase
Testcase found while fuzzing mozilla-central rev 20170904-1401e3eec44d.
Flags: in-testsuite?
Attached file Stack
Group: core-security → javascript-core-security
Looks more like a DOM bug, could be a uaf
Component: JavaScript Engine → DOM
Boris, any ideas? It looks like the interesting part of the stack from the log is we're removing a script blocker, which calls into nsXULCommandDispatcher::UpdateCommands, and then we try to dispatch an event, which has a pending exception, so we try to create a proxy object, but for some reason we're not expecting that to happen here.

0|2|libxul.so|js::ProxyObject::create|hg:hg.mozilla.org/mozilla-central:js/src/vm/ProxyObject.cpp:3ecda4678c49|174|0x16
...
0|10|libxul.so|JS_GetPendingException|hg:hg.mozilla.org/mozilla-central:js/src/jsapi.cpp:3ecda4678c49|6759|0xb
0|12|libxul.so|mozilla::dom::AutoEntryScript::AutoEntryScript|hg:hg.mozilla.org/mozilla-central:dom/script/ScriptSettings.cpp:3ecda4678c49|674|0x5
0|13|libxul.so|mozilla::dom::CallbackObject::CallSetup::CallSetup|hg:hg.mozilla.org/mozilla-central:mfbt/Maybe.h:3ecda4678c49|459|0x5
0|14|libxul.so|mozilla::JSEventHandler::HandleEvent|s3:gecko-generated-sources:d0cd062a8f2e61a1842e705e1539dafa6e2559f266e0cf39cc24ecbc1de67828060d8f7d4015631bcfe7c22d53ac77e562e84587fe96e76b41b346b32ed4aeb6/dist/include/mozilla/dom/EventHandlerBinding.h:|352|0x29
...
0|19|libxul.so|mozilla::EventDispatcher::Dispatch|hg:hg.mozilla.org/mozilla-central:dom/events/EventDispatcher.cpp:3ecda4678c49|823|0x5
0|20|libxul.so|nsXULCommandDispatcher::UpdateCommands|hg:hg.mozilla.org/mozilla-central:dom/xul/nsXULCommandDispatcher.cpp:3ecda4678c49|402|0x1f
0|21|libxul.so|nsContentUtils::RemoveScriptBlocker|hg:hg.mozilla.org/mozilla-central:dom/base/nsContentUtils.cpp:3ecda4678c49|5681|0xe
0|22|libxul.so|nsDocument::EndUpdate|hg:hg.mozilla.org/mozilla-central:dom/base/nsDocument.cpp:3ecda4678c49|5357|0x5
0|23|libxul.so|nsHTMLDocument::EndUpdate|hg:hg.mozilla.org/mozilla-central:dom/html/nsHTMLDocument.cpp:3ecda4678c49|2510|0x5
0|24|libxul.so|mozAutoDocUpdate::~mozAutoDocUpdate|hg:hg.mozilla.org/mozilla-central:dom/base/mozAutoDocUpdate.h:3ecda4678c49|40|0x14
Flags: needinfo?(bzbarsky)
So...

We're firing an event.  That is, we're about to run script.  So in general, at this stackframe:

  0|14|libxul.so|mozilla::JSEventHandler::HandleEvent

it better be ok to allocate, gc, whatever.  So far so good.

The assert is coming from the fact that in CallSetup::CallSetup we have a block with a JS::AutoSuppressGCAnalysis at the start.  Inside that block we do mAutoEntryScript.emplace which leads to the rest of the stack.

So obvious thoughts:

1)  It's not clear to me that we need this AutoSuppressGCAnalysis at all.  I think it used to be the case that we had a bunch more virtual function calls there, and uses of gcthings after those calls, but I think that's all gone.  So I think we can take out the AutoSuppressGCAnalysis entirely.  I pushed https://treeherder.mozilla.org/#/jobs?repo=try&revision=e23e70332598b990f29bc49d7248a1ecef68537c to verify this theory.

2)  If we did not hit this assert, then note that we're doing this JS_GetPendingException call:

  0|10|libxul.so|JS_GetPendingException|hg:hg.mozilla.org/mozilla-central:js/src/jsapi.cpp:3ecda4678c49|6759|0xb

from a debug-only block that is just gathering information so it can fatally assert in the most informative possible way at <http://searchfox.org/mozilla-central/rev/67f38de2443e6b613d874fcf4d2cd1f2fc3d5e97/dom/script/ScriptSettings.cpp#446>.  The fact that we're in this block at all (i.e. there _is_ a pending exception when we call AutoJSAPI::InitInternal) is a bug.  In this case, I expect it's basically the same as bug 1393806.  At least the testcase has the same structure, with infinite recursion in mutation events.  I'll double-check that, though.
Note that I haven't been able to reproduce the assertion with the testcase here.  But looking at it more closely, it's definitely got the "infinite recursion during adopt" thing from bug 1393806 going on...
Hazard run is green, as expected.
Flags: needinfo?(bzbarsky)
Group: javascript-core-security
Depends on: 1393806
Summary: Assertion failure: !inUnsafeRegion ([AutoAssertNoGC] possible GC in GC-unsafe region) → (bogus) Assertion failure: !inUnsafeRegion ([AutoAssertNoGC] possible GC in GC-unsafe region)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Comment on attachment 8905644 [details] [diff] [review]
Remove bogus and no longer needed AutoSuppressGCAnalysis

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

Awesome, thanks!
Attachment #8905644 - Flags: review?(continuation) → review+
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd62e8a31942
Remove bogus and no longer needed AutoSuppressGCAnalysis.  r=mccr8
https://hg.mozilla.org/mozilla-central/rev/bd62e8a31942
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
To be clear, the assertion failure was not bogus.  It was a real assertion failure as far as it went: we had turned off analysis and were doing gc.

It happens there were no live things across that block, which is convenient.  And the code that triggered GC only ran in debug builds, which is also convenient.
Summary: (bogus) Assertion failure: !inUnsafeRegion ([AutoAssertNoGC] possible GC in GC-unsafe region) → Assertion failure: !inUnsafeRegion ([AutoAssertNoGC] possible GC in GC-unsafe region)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.