Closed Bug 1396904 Opened 3 years ago Closed 3 years ago
Assertion failure: !in
Unsafe Region ([Auto Assert No GC] possible GC in GC-unsafe region)
772 bytes, text/html
360.73 KB, text/plain
6.95 KB, patch
|Details | Diff | Splinter Review|
2.81 KB, patch
|Details | Diff | Splinter Review|
Testcase found while fuzzing mozilla-central rev 20170904-1401e3eec44d.
Looks more like a DOM bug, could be a uaf
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
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.
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 email@example.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/bd62e8a31942 Remove bogus and no longer needed AutoSuppressGCAnalysis. r=mccr8
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)
You need to log in before you can comment on or make changes to this bug.