Closed Bug 1415761 Opened 2 years ago Closed 2 years ago

Assertion failure while we are invoking custom element reactions in the middle of throwing an exception

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox-esr52 --- unaffected
firefox56 --- unaffected
firefox57 --- unaffected
firefox58 --- disabled
firefox59 --- fixed

People

(Reporter: jkratzer, Assigned: edgar)

References

(Blocks 1 open bug, )

Details

(Keywords: assertion, testcase)

Attachments

(2 files)

Attached file trigger.html
Testcase found while fuzzing mozilla-central rev 7b75416fb54c.

0|0|libxul.so|mozilla::dom::AutoJSAPI::InitInternal|hg:hg.mozilla.org/mozilla-central:dom/script/ScriptSettings.cpp:7b75416fb54c|349|0x0
0|1|libxul.so|mozilla::dom::AutoEntryScript::AutoEntryScript|hg:hg.mozilla.org/mozilla-central:dom/script/ScriptSettings.cpp:7b75416fb54c|674|0x5
0|2|libxul.so|mozilla::dom::CustomElementReactionsStack::InvokeReactions|hg:hg.mozilla.org/mozilla-central:mfbt/Maybe.h:7b75416fb54c|459|0x8
0|3|libxul.so|mozilla::dom::CustomElementReactionsStack::PopAndInvokeElementQueue|hg:hg.mozilla.org/mozilla-central:dom/base/CustomElementRegistry.cpp:7b75416fb54c|1014|0xe
0|4|libxul.so|mozilla::Maybe<mozilla::dom::AutoCEReaction>::reset|hg:hg.mozilla.org/mozilla-central:dom/base/CustomElementRegistry.h:7b75416fb54c|520|0xd
0|5|libxul.so|mozilla::dom::CharacterDataBinding::before|hg:hg.mozilla.org/mozilla-central:mfbt/Maybe.h:7b75416fb54c|100|0xc
0|6|libxul.so|mozilla::dom::GenericBindingMethod|hg:hg.mozilla.org/mozilla-central:dom/bindings/BindingUtils.cpp:7b75416fb54c|3039|0x9
0|7|libxul.so|js::CallJSNative|hg:hg.mozilla.org/mozilla-central:js/src/jscntxtinlines.h:7b75416fb54c|291|0x9
0|8|libxul.so|js::InternalCallOrConstruct|hg:hg.mozilla.org/mozilla-central:js/src/vm/Interpreter.cpp:7b75416fb54c|473|0xf
0|9|libxul.so|InternalCall|hg:hg.mozilla.org/mozilla-central:js/src/vm/Interpreter.cpp:7b75416fb54c|522|0xd
0|10|libxul.so|Interpret|hg:hg.mozilla.org/mozilla-central:js/src/vm/Interpreter.cpp:7b75416fb54c|528|0xf
0|11|libxul.so|js::RunScript|hg:hg.mozilla.org/mozilla-central:js/src/vm/Interpreter.cpp:7b75416fb54c|423|0xb
0|12|libxul.so|js::InternalCallOrConstruct|hg:hg.mozilla.org/mozilla-central:js/src/vm/Interpreter.cpp:7b75416fb54c|495|0xf
0|13|libxul.so|InternalCall|hg:hg.mozilla.org/mozilla-central:js/src/vm/Interpreter.cpp:7b75416fb54c|522|0xd
0|14|libxul.so|js::Call|hg:hg.mozilla.org/mozilla-central:js/src/vm/Interpreter.cpp:7b75416fb54c|541|0x5
0|15|libxul.so|js::ForwardingProxyHandler::call|hg:hg.mozilla.org/mozilla-central:js/src/proxy/Wrapper.cpp:7b75416fb54c|175|0x9
0|16|libxul.so|js::CrossCompartmentWrapper::call|hg:hg.mozilla.org/mozilla-central:js/src/proxy/CrossCompartmentWrapper.cpp:7b75416fb54c|359|0x13
0|17|libxul.so|js::Proxy::call|hg:hg.mozilla.org/mozilla-central:js/src/proxy/Proxy.cpp:7b75416fb54c|512|0x15
0|18|libxul.so|js::proxy_Call|hg:hg.mozilla.org/mozilla-central:js/src/proxy/Proxy.cpp:7b75416fb54c|787|0x9
0|19|libxul.so|js::CallJSNative|hg:hg.mozilla.org/mozilla-central:js/src/jscntxtinlines.h:7b75416fb54c|291|0x9
0|20|libxul.so|js::InternalCallOrConstruct|hg:hg.mozilla.org/mozilla-central:js/src/vm/Interpreter.cpp:7b75416fb54c|455|0x12
0|21|libxul.so|InternalCall|hg:hg.mozilla.org/mozilla-central:js/src/vm/Interpreter.cpp:7b75416fb54c|522|0xd
0|22|libxul.so|js::Call|hg:hg.mozilla.org/mozilla-central:js/src/vm/Interpreter.cpp:7b75416fb54c|541|0x5
0|23|libxul.so|JS::Call|hg:hg.mozilla.org/mozilla-central:js/src/jsapi.cpp:7b75416fb54c|3021|0x1c
0|24|libxul.so|mozilla::dom::EventHandlerNonNull::Call|s3:gecko-generated-sources:45c7673d6cdcb9ad297632b42609e09b31819dc7edd399050135d90eee920939c623d2fafd32c6b684bb31a39ce12b69569fa2cdaed88536fb4c8b46f692a33d/dom/bindings/EventHandlerBinding.cpp:|260|0x5
0|25|libxul.so|mozilla::JSEventHandler::HandleEvent|s3:gecko-generated-sources:d0cd062a8f2e61a1842e705e1539dafa6e2559f266e0cf39cc24ecbc1de67828060d8f7d4015631bcfe7c22d53ac77e562e84587fe96e76b41b346b32ed4aeb6/dist/include/mozilla/dom/EventHandlerBinding.h:|362|0x1f
0|26|libxul.so|mozilla::EventListenerManager::HandleEventSubType|hg:hg.mozilla.org/mozilla-central:dom/events/EventListenerManager.cpp:7b75416fb54c|1118|0x14
0|27|libxul.so|mozilla::EventListenerManager::HandleEventInternal|hg:hg.mozilla.org/mozilla-central:dom/events/EventListenerManager.cpp:7b75416fb54c|1295|0x15
0|28|libxul.so|mozilla::EventTargetChainItem::HandleEvent|hg:hg.mozilla.org/mozilla-central:dom/events/EventListenerManager.h:7b75416fb54c|376|0x13
0|29|libxul.so|mozilla::EventTargetChainItem::HandleEventTargetChain|hg:hg.mozilla.org/mozilla-central:dom/events/EventDispatcher.cpp:7b75416fb54c|462|0xf
0|30|libxul.so|mozilla::EventDispatcher::Dispatch|hg:hg.mozilla.org/mozilla-central:dom/events/EventDispatcher.cpp:7b75416fb54c|823|0x5
0|31|libxul.so|nsDocumentViewer::LoadComplete|hg:hg.mozilla.org/mozilla-central:layout/base/nsDocumentViewer.cpp:7b75416fb54c|1064|0x25
0|32|libxul.so|nsDocShell::EndPageLoad|hg:hg.mozilla.org/mozilla-central:docshell/base/nsDocShell.cpp:7b75416fb54c|7756|0x18
0|33|libxul.so|nsDocShell::OnStateChange|hg:hg.mozilla.org/mozilla-central:docshell/base/nsDocShell.cpp:7b75416fb54c|7554|0x18
0|34|libxul.so|nsDocLoader::DoFireOnStateChange|hg:hg.mozilla.org/mozilla-central:uriloader/base/nsDocLoader.cpp:7b75416fb54c|1320|0x2b
0|35|libxul.so|nsDocLoader::doStopDocumentLoad|hg:hg.mozilla.org/mozilla-central:uriloader/base/nsDocLoader.cpp:7b75416fb54c|861|0x22
0|36|libxul.so|nsDocLoader::DocLoaderIsEmpty|hg:hg.mozilla.org/mozilla-central:uriloader/base/nsDocLoader.cpp:7b75416fb54c|750|0xf
0|37|libxul.so|nsDocLoader::OnStopRequest|hg:hg.mozilla.org/mozilla-central:uriloader/base/nsDocLoader.cpp:7b75416fb54c|632|0x16
0|38|libxul.so|mozilla::net::nsLoadGroup::RemoveRequest|hg:hg.mozilla.org/mozilla-central:netwerk/base/nsLoadGroup.cpp:7b75416fb54c|629|0x1f
0|39|libxul.so|nsDocument::DoUnblockOnload|hg:hg.mozilla.org/mozilla-central:dom/base/nsDocument.cpp:7b75416fb54c|9364|0x20
0|40|libxul.so|nsDocument::UnblockOnload|hg:hg.mozilla.org/mozilla-central:dom/base/nsDocument.cpp:7b75416fb54c|9286|0x5
0|41|libxul.so|nsDocument::DispatchContentLoadedEvents|hg:hg.mozilla.org/mozilla-central:dom/base/nsDocument.cpp:7b75416fb54c|5641|0x11
0|42|libxul.so|mozilla::detail::RunnableMethodImpl<nsDocument*, void (nsDocument::*)(), true, (mozilla::RunnableKind)0u>::Run|hg:hg.mozilla.org/mozilla-central:xpcom/threads/nsThreadUtils.h:7b75416fb54c|1142|0x13
0|43|libxul.so|nsThread::ProcessNextEvent|hg:hg.mozilla.org/mozilla-central:xpcom/threads/nsThread.cpp:7b75416fb54c|1037|0x15
0|44|libxul.so|NS_ProcessNextEvent|hg:hg.mozilla.org/mozilla-central:xpcom/threads/nsThreadUtils.cpp:7b75416fb54c|512|0x11
0|45|libxul.so|mozilla::ipc::MessagePump::Run|hg:hg.mozilla.org/mozilla-central:ipc/glue/MessagePump.cpp:7b75416fb54c|97|0xa
0|46|libxul.so|MessageLoop::RunInternal|hg:hg.mozilla.org/mozilla-central:ipc/chromium/src/base/message_loop.cc:7b75416fb54c|326|0x17
0|47|libxul.so|MessageLoop::Run|hg:hg.mozilla.org/mozilla-central:ipc/chromium/src/base/message_loop.cc:7b75416fb54c|319|0x8
0|48|libxul.so|nsBaseAppShell::Run|hg:hg.mozilla.org/mozilla-central:widget/nsBaseAppShell.cpp:7b75416fb54c|158|0xd
0|49|libxul.so|nsAppStartup::Run|hg:hg.mozilla.org/mozilla-central:toolkit/components/startup/nsAppStartup.cpp:7b75416fb54c|288|0xe
0|50|libxul.so|XREMain::XRE_mainRun|hg:hg.mozilla.org/mozilla-central:toolkit/xre/nsAppRunner.cpp:7b75416fb54c|4694|0x11
0|51|libxul.so|XREMain::XRE_main|hg:hg.mozilla.org/mozilla-central:toolkit/xre/nsAppRunner.cpp:7b75416fb54c|4856|0x8
0|52|libxul.so|XRE_main|hg:hg.mozilla.org/mozilla-central:toolkit/xre/nsAppRunner.cpp:7b75416fb54c|4951|0x5
0|53|firefox|do_main|hg:hg.mozilla.org/mozilla-central:browser/app/nsBrowserApp.cpp:7b75416fb54c|231|0x22
0|54|firefox|main|hg:hg.mozilla.org/mozilla-central:browser/app/nsBrowserApp.cpp:7b75416fb54c|304|0xd
0|55|libc-2.23.so||||0x20830
0|56|firefox|MOZ_ReportAssertionFailure|hg:hg.mozilla.org/mozilla-central:mfbt/Assertions.h:7b75416fb54c|165|0x5
Flags: in-testsuite?
It looks like a Custom Elements related bug, I will take a look, ni? me for tracking.
Flags: needinfo?(echen)
Flags: needinfo?(echen)
Priority: -- → P1
Flags: needinfo?(echen)
The important part is what comes _before_ this particular assertion, not the stack to the assertion (which is just asserting that some earlier code somewhere screwed up).  That part before is:

  PREEXISTING EXCEPTION OBJECT: 'HierarchyRequestError: Node cannot be inserted at the specified point in the hierarchy'
  https://bug1415761.bmoattachments.org/attachment.cgi?id=8926677:12
  o2.onload@https://bug1415761.bmoattachments.org/attachment.cgi?id=8926677:12:44
  EventHandlerNonNull*@https://bug1415761.bmoattachments.org/attachment.cgi?id=8926677:12:13

The problem is that CharacterDataBinding::before looks like this:


  Maybe<AutoCEReaction> ceReaction;
  if (CustomElementRegistry::IsCustomElementEnabled()) {
    CustomElementReactionsStack* reactionsStack = GetCustomElementReactionsStack(obj);
    if (reactionsStack) {
      ceReaction.emplace(reactionsStack);
    }
  }
  binding_detail::FastErrorResult rv;
  self->Before(Constify(arg0), rv);
  if (MOZ_UNLIKELY(rv.MaybeSetPendingException(cx))) {
    return false;
  }

OK, so at the point when we are doing "return false", ~AutoCEReaction happens and we start invoking custom element reactions while we're in the middle of throwing an exception.

There's a spec issue here, actually.  I filed https://github.com/whatwg/html/issues/3217 on that.  But I expect that the only sane answers are (1) pop the thing off the element queue but do _not_ invoke those custom element reactions and (2) catch the exception, invoke the custom element reactions, then rethrow the exception...
We're not shipping Custom Elements yet so setting some versions to unaffected.
OK. Spec is being updated to make it clear that we need to clear the exception off the JSContext while running CEReactions.

We should probably just give AutoCEReaction's ctor a JSContext* argument, allowed to be null (for the use in nsHtml5TreeOperation.cpp).  If non-null, then ~AutoCEReaction saves and restores the exception on the JSContext around calling PopAndInvokeElementQueue().  Make sense?
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #4)
> OK. Spec is being updated to make it clear that we need to clear the
> exception off the JSContext while running CEReactions.
> 
> We should probably just give AutoCEReaction's ctor a JSContext* argument,
> allowed to be null (for the use in nsHtml5TreeOperation.cpp).  If non-null,
> then ~AutoCEReaction saves and restores the exception on the JSContext
> around calling PopAndInvokeElementQueue().  Make sense?

Yes, it make sense. Thank you.
Assignee: nobody → echen
Flags: needinfo?(echen)
JS::AutoSaveExceptionState will probably do what you want.
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #6)
> JS::AutoSaveExceptionState will probably do what you want.

Great to learn this. Thank you.
Summary: Assertion failure: false (We had an exception; we should not have), at /builds/worker/workspace/build/src/dom/script/ScriptSettings.cpp:446 → Assertion failure while we are invoking custom element reactions in the middle of throwing an exception
Comment on attachment 8929965 [details]
Bug 1415761 - Catch the exception and rethrow it after invoking custom elements reactions;

https://reviewboard.mozilla.org/r/201128/#review206640

::: dom/base/CustomElementRegistry.h:483
(Diff revision 1)
>    already_AddRefed<Promise> WhenDefined(const nsAString& aName, ErrorResult& aRv);
>  };
>  
>  class MOZ_RAII AutoCEReaction final {
>    public:
> -    explicit AutoCEReaction(CustomElementReactionsStack* aReactionsStack)
> +    AutoCEReaction(CustomElementReactionsStack* aReactionsStack, JSContext* aCx)

Please document clearly that aCx is allowed to be null if we're guaranteed that we're not doing something that might throw but not finish reporting a JS exception during the lifetime of the AutoCEReaction.

::: dom/tests/mochitest/webcomponents/test_custom_element_reactions_with_exception.html:1
(Diff revision 1)
> +<!DOCTYPE HTML>

Can this be a web platform test?  That would be vastly preferable.
Attachment #8929965 - Flags: review?(bzbarsky) → review+
Comment on attachment 8929965 [details]
Bug 1415761 - Catch the exception and rethrow it after invoking custom elements reactions;

https://reviewboard.mozilla.org/r/201128/#review206640

> Can this be a web platform test?  That would be vastly preferable.

Dominic has submitted a pull request in GitHub, https://github.com/w3c/web-platform-tests/pull/8290, but not yet merged.
I still add a mochitest test here since I think it will be good to have a test when code landed.
You could just land the submitted wpt PR, then let the normal sync stuff deal with conflicts if any arise....
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #13)
> You could just land the submitted wpt PR, then let the normal sync stuff
> deal with conflicts if any arise....

I see. I will land the submitted wpt PR. Thank you.
Pushed by echen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/df3695afb47e
Catch the exception and rethrow it after invoking custom elements reactions; r=bz
https://hg.mozilla.org/mozilla-central/rev/df3695afb47e
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Flags: in-testsuite? → in-testsuite+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.