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

Reporter

Description

•

5 years ago

Attached file trigger.html — Details

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?

Edgar Chen [:edgar]

Assignee

Comment 1

•

5 years ago

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

Flags: needinfo?(echen)

Edgar Chen [:edgar]

Assignee

Updated

•

5 years ago

Flags: needinfo?(echen)

Priority: -- → P1

Edgar Chen [:edgar]

Assignee

Updated

•

5 years ago

Flags: needinfo?(echen)

Boris Zbarsky [:bzbarsky]

Comment 2

•

5 years ago

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

Andrew Overholt [:overholt]

Comment 3

•

5 years ago

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

Blocks: custom-elements-initial-release

status-firefox56: --- → unaffected

status-firefox57: --- → unaffected

status-firefox-esr52: --- → unaffected

Boris Zbarsky [:bzbarsky]

Comment 4

•

5 years ago

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?

Edgar Chen [:edgar]

Assignee

Comment 5

•

5 years ago

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

Boris Zbarsky [:bzbarsky]

Comment 6

•

5 years ago

JS::AutoSaveExceptionState will probably do what you want.

Edgar Chen [:edgar]

Assignee

Comment 7

•

5 years ago

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

Edgar Chen [:edgar]

Assignee

Comment 8

•

5 years ago

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

Edgar Chen [:edgar]

Assignee

Updated

•

5 years ago

URL: https://github.com/whatwg/html/issues...

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 hidden (mozreview-request)

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/

Boris Zbarsky [:bzbarsky]

Comment 10

•

5 years ago

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

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

Edgar Chen [:edgar]

Assignee

Comment 11

•

5 years ago

mozreview-review-reply

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 hidden (mozreview-request)

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/

Boris Zbarsky [:bzbarsky]

Comment 13

•

5 years ago

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

Edgar Chen [:edgar]

Assignee

Comment 14

•

5 years ago

(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 hidden (mozreview-request)

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/

Edgar Chen [:edgar]

Assignee

Comment 16

•

5 years ago

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

Pulsebot

Comment 17

•

5 years ago

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

Cristina Coroiu [:ccoroiu]

Comment 18

•

5 years ago

bugherder

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

Status: NEW → RESOLVED

Closed: 5 years ago

status-firefox59: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla59

Ryan VanderMeulen [:RyanVM]

Updated

•

5 years ago

status-firefox58: --- → disabled

Flags: in-testsuite? → in-testsuite+

Nobody; OK to take it and work on it

Updated

•

4 years ago

Component: DOM → DOM: Core & HTML

trigger.html 5 years ago Jason Kratzer [:jkratzer] 747 bytes, text/html		Details
Bug 1415761 - Catch the exception and rethrow it after invoking custom elements reactions; 5 years ago Edgar Chen [:edgar] 59 bytes, text/x-review-board-request	bzbarsky : review+	Details