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

Status: RESOLVED FIXED

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

Description

[Jason Kratzer [:jkratzer]]

Attachment: 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

Comment 1

[Edgar Chen [:edgar]]

It looks like a Custom Elements related bug, I will take a look, ni? me for tracking.

Comment 2

[Boris Zbarsky [:bzbarsky]]

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...

Comment 3

[Andrew Overholt [:overholt]]

We're not shipping Custom Elements yet so setting some versions to unaffected.

Comment 4

[Boris Zbarsky [:bzbarsky]]

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?

Comment 5

[Edgar Chen [:edgar]]

(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.

Comment 6

[Boris Zbarsky [:bzbarsky]]

JS::AutoSaveExceptionState will probably do what you want.

Comment 7

[Edgar Chen [:edgar]]

(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.

Comment 8

[Edgar Chen [:edgar]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a7506388935c881683bdf558677e73788f370a4&group_state=expanded&filter-tier=1

Comment 9

[Edgar Chen [:edgar]]

Attachment: Bug 1415761 - Catch the exception and rethrow it after invoking custom elements reactions;

The spec was unclear on how CEReactions interact with thrown exceptions; see https://github.com/whatwg/html/issues/3217.
The spec is now being clarified in https://github.com/whatwg/html/pull/3235.

Review commit: https://reviewboard.mozilla.org/r/201128/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/201128/

Comment 10

[Boris Zbarsky [:bzbarsky]]

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.

Comment 11

[Edgar Chen [:edgar]]

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.

Comment 12

[Edgar Chen [:edgar]]

Comment on attachment 8929965 [details]
Bug 1415761 - Catch the exception and rethrow it after invoking custom elements reactions;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/201128/diff/1-2/

Comment 13

[Boris Zbarsky [:bzbarsky]]

You could just land the submitted wpt PR, then let the normal sync stuff deal with conflicts if any arise....

Comment 14

[Edgar Chen [:edgar]]

(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.

Comment 15

[Edgar Chen [:edgar]]

Comment on attachment 8929965 [details]
Bug 1415761 - Catch the exception and rethrow it after invoking custom elements reactions;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/201128/diff/2-3/

Comment 16

[Edgar Chen [:edgar]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f893ce778557ff7802a6746f84063dc11eb3b736&filter-tier=1&group_state=expanded

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/df3695afb47e
Catch the exception and rethrow it after invoking custom elements reactions; r=bz

Comment 18

[Cristina Coroiu [:ccoroiu]]

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