1420178 - Consider adding assertions to ensure the custom element reactions are not scheduled to BackupQueue if they are from author code

Status: RESOLVED INCOMPLETE

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

Comment 1

[Edgar Chen [:edgar] (PTO until 02/09)]

Currently, if we are missing [CEReation] on the attribute/operation that might trigger custom element reactions, the reactions will be scheduled to BackupQueue and hard to realize that reactions are actually invoked at the unexpected time. Add assertions would help to catch such problem.

Comment 2

[Edgar Chen [:edgar] (PTO until 02/09)]

Attachment: Bug 1420178 - Add assertion to ensure the custom element reactions aren't scheduled to BackupQueue if they are from author code;

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

Comment 3

[Edgar Chen [:edgar] (PTO until 02/09)]

Comment on attachment 8940639 [details]
Bug 1420178 - Add assertion to ensure the custom element reactions aren't scheduled to BackupQueue if they are from author code;

https://reviewboard.mozilla.org/r/210884/#review217032

::: dom/base/CustomElementRegistry.cpp:1060
(Diff revision 1)
>      mReactionsStack.LastElement()->AppendElement(aElement);
>      elementData->mReactionQueue.AppendElement(aReaction);
>      return;
>    }
>  
> +  MOZ_ASSERT(!IsJSAPIActive());

It is more accurate to use GetIncumbentGlobal(), perhaps.

Comment 4

[Edgar Chen [:edgar] (PTO until 02/09)]

Comment on attachment 8940639 [details]
Bug 1420178 - Add assertion to ensure the custom element reactions aren't scheduled to BackupQueue if they are from author code;

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

Comment 5

[Edgar Chen [:edgar] (PTO until 02/09)]

Olli, does using GetIncumbentGlobal() make sense here?

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8940639 [details]
Bug 1420178 - Add assertion to ensure the custom element reactions aren't scheduled to BackupQueue if they are from author code;

https://reviewboard.mozilla.org/r/210884/#review217114

I guess !GetIncumbentGlobal() is fine. It could be also something else, shouldn't matter too much.

Are there any weird cases related to document.write or setting innerHTML when that assertion might fire? Perhaps not, I assume we have tests for those.

::: dom/base/CustomElementRegistry.cpp:1065
(Diff revision 2)
>    // If the custom element reactions stack is empty, then:
>    // Add element to the backup element queue.
>    MOZ_ASSERT(mReactionsStack.IsEmpty(),
>               "custom element reactions stack should be empty");
> +  MOZ_ASSERT(!GetIncumbentGlobal(),
> +             "Custom element reaction should not be scheduled to backup queue if it is from aurthor code");

aurthor
author

Comment 7

[Edgar Chen [:edgar] (PTO until 02/09)]

Comment on attachment 8940639 [details]
Bug 1420178 - Add assertion to ensure the custom element reactions aren't scheduled to BackupQueue if they are from author code;

https://reviewboard.mozilla.org/r/210884/#review217114

Yes, we have web-platform tests for them, like
- https://searchfox.org/mozilla-central/rev/03877052c151a8f062eea177f684a2743cd7b1d5/testing/web-platform/tests/custom-elements/reactions/Document.html
- https://searchfox.org/mozilla-central/rev/03877052c151a8f062eea177f684a2743cd7b1d5/testing/web-platform/tests/custom-elements/reactions/HTMLAnchorElement.html
- ...

Comment 8

[Edgar Chen [:edgar] (PTO until 02/09)]

Comment on attachment 8940639 [details]
Bug 1420178 - Add assertion to ensure the custom element reactions aren't scheduled to BackupQueue if they are from author code;

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

Comment 9

[Edgar Chen [:edgar] (PTO until 02/09)]

Comment on attachment 8940639 [details]
Bug 1420178 - Add assertion to ensure the custom element reactions aren't scheduled to BackupQueue if they are from author code;

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

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/760107333833
Add assertion to ensure the custom element reactions aren't scheduled to BackupQueue if they are from author code; r=smaug

Comment 11

[Natalia Csoregi [:nataliaCs]]

Backed out for assertion failure at dom/base/CustomElementRegistry.cpp:1065 in browser-chrome on Windows

Backout: https://hg.mozilla.org/integration/autoland/rev/d24dd3821124a66cc06fd069534f237372d620ba

Push that triggered the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=760107333833656db6c6b8706da966047e1874f2

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=155266788&repo=autoland&lineNumber=16576

Comment 12

[Edgar Chen [:edgar] (PTO until 02/09)]

(In reply to Natalia Csoregi [:nataliaCs] from comment #11)
> https://treeherder.mozilla.org/logviewer.
> html#?job_id=155266788&repo=autoland&lineNumber=16576

Aha, here is a case that the assertion failure might be a false positive.

Stack is like,
> Assertion failure: !GetIncumbentGlobal() (Custom element reaction should not be scheduled to backup queue if it is from author code), at z:/build/build/src/dom/base/CustomElementRegistry.cpp:1065
> #01: mozilla::dom::CustomElementReactionsStack::EnqueueCallbackReaction(mozilla::dom::Element *,mozilla::UniquePtr<mozilla::dom::CustomElementCallback,mozilla::DefaultDelete<mozilla::dom::CustomElementCallback> >) [dom/base/CustomElementRegistry.cpp:1037]
> #02: mozilla::dom::CustomElementRegistry::EnqueueLifecycleCallback(nsIDocument::ElementCallbackType,mozilla::dom::Element *,mozilla::dom::LifecycleCallbackArgs *,mozilla::dom::LifecycleAdoptedCallbackArgs *,mozilla::dom::CustomElementDefinition *) [dom/base/CustomElementRegistry.cpp:454]
> #03: nsContentUtils::EnqueueLifecycleCallback(nsIDocument::ElementCallbackType,mozilla::dom::Element *,mozilla::dom::LifecycleCallbackArgs *,mozilla::dom::LifecycleAdoptedCallbackArgs *,mozilla::dom::CustomElementDefinition *) [dom/base/nsContentUtils.cpp:10222]
> #04: mozilla::dom::Element::UnbindFromTree(bool,bool) [dom/base/Element.cpp:2060]
> #05: mozilla::dom::Element::UnbindFromTree(bool,bool) [dom/base/Element.cpp:2087]
> #06: mozilla::dom::Element::UnbindFromTree(bool,bool) [dom/base/Element.cpp:2087]
> #07: mozilla::dom::HTMLSharedElement::UnbindFromTree(bool,bool) [dom/html/HTMLSharedElement.cpp:264]
> #08: nsDocument::cycleCollection::Unlink(void *) [dom/base/nsDocument.cpp:2058]
> #09: nsHTMLDocument::cycleCollection::Unlink(void *) [dom/html/nsHTMLDocument.cpp:208]
> #10: nsCycleCollector::CollectWhite() [xpcom/base/nsCycleCollector.cpp:3398]
> #11: nsCycleCollector::Collect(ccType,js::SliceBudget &,nsICycleCollectorListener *,bool) [xpcom/base/nsCycleCollector.cpp:3764]
> #12: nsCycleCollector_collect(nsICycleCollectorListener *) [xpcom/base/nsCycleCollector.cpp:4311]
> #13: nsJSContext::CycleCollectNow(nsICycleCollectorListener *) [dom/base/nsJSEnvironment.cpp:1506]
> #14: nsJSEnvironmentObserver::Observe(nsISupports *,char const *,char16_t const *) [dom/base/nsJSEnvironment.cpp:345]
> #15: nsObserverList::NotifyObservers(nsISupports *,char const *,char16_t const *) [xpcom/ds/nsObserverList.cpp:112]
> #16: nsObserverService::NotifyObservers(nsISupports *,char const *,char16_t const *) [xpcom/ds/nsObserverService.cpp:300]
> #17: XPTC__InvokebyIndex
> #18: CallMethodHelper::Call() [js/xpconnect/src/XPCWrappedNative.cpp:1269]
> #19: XPCWrappedNative::CallMethod(XPCCallContext &,XPCWrappedNative::CallMode) [js/xpconnect/src/XPCWrappedNative.cpp:1234]
> #20: XPC_WN_CallMethod(JSContext *,unsigned int,JS::Value *) [js/xpconnect/src/XPCWrappedNativeJSOps.cpp:929]
> #21: js::CallJSNative(JSContext *,bool (*)(JSContext *,unsigned int,JS::Value *),JS::CallArgs const &) [js/src/jscntxtinlines.h:291]
> #22: js::InternalCallOrConstruct(JSContext *,JS::CallArgs const &,js::MaybeConstruct) [js/src/vm/Interpreter.cpp:473]
> #23: Interpret [js/src/vm/Interpreter.cpp:3096]
> #24: js::RunScript(JSContext *,js::RunState &) [js/src/vm/Interpreter.cpp:423]
> #25: js::InternalCallOrConstruct(JSContext *,JS::CallArgs const &,js::MaybeConstruct) [js/src/vm/Interpreter.cpp:495]
> #26: js::Call(JSContext *,JS::Handle<JS::Value>,JS::Handle<JS::Value>,js::AnyInvokeArgs const &,JS::MutableHandle<JS::Value>) [js/src/vm/Interpreter.cpp:541]
> #27: js::ForwardingProxyHandler::call(JSContext *,JS::Handle<JSObject *>,JS::CallArgs const &) [js/src/proxy/Wrapper.cpp:176]
> #28: js::CrossCompartmentWrapper::call(JSContext *,JS::Handle<JSObject *>,JS::CallArgs const &) [js/src/proxy/CrossCompartmentWrapper.cpp:359]
> #29: js::proxy_Call(JSContext *,unsigned int,JS::Value *) [js/src/proxy/Proxy.cpp:770]
> #30: js::CallJSNative(JSContext *,bool (*)(JSContext *,unsigned int,JS::Value *),JS::CallArgs const &) [js/src/jscntxtinlines.h:291]
> #31: js::InternalCallOrConstruct(JSContext *,JS::CallArgs const &,js::MaybeConstruct) [js/src/vm/Interpreter.cpp:455]
> #32: js::Call(JSContext *,JS::Handle<JS::Value>,JS::Handle<JS::Value>,js::AnyInvokeArgs const &,JS::MutableHandle<JS::Value>) [js/src/vm/Interpreter.cpp:541]
> #33: JS_CallFunctionValue(JSContext *,JS::Handle<JSObject *>,JS::Handle<JS::Value>,JS::HandleValueArray const &,JS::MutableHandle<JS::Value>) [js/src/jsapi.cpp:2970]
> #34: nsFrameMessageManager::ReceiveMessage(nsISupports *,nsIFrameLoader *,bool,nsTSubstring<char16_t> const &,bool,mozilla::dom::ipc::StructuredCloneData *,mozilla::jsipc::CpowHolder *,nsIPrincipal *,nsTArray<mozilla::dom::ipc::StructuredCloneData> *) [dom/base/nsFrameMessageManager.cpp:1098]
> #35: nsFrameMessageManager::ReceiveMessage(nsISupports *,nsIFrameLoader *,nsTSubstring<char16_t> const &,bool,mozilla::dom::ipc::StructuredCloneData *,mozilla::jsipc::CpowHolder *,nsIPrincipal *,nsTArray<mozilla::dom::ipc::StructuredCloneData> *) [dom/base/nsFrameMessageManager.cpp:909]
> #36: mozilla::dom::ContentChild::RecvAsyncMessage(nsTString<char16_t> const &,nsTArray<mozilla::jsipc::CpowEntry> &&,IPC::Principal const &,mozilla::dom::ClonedMessageData const &) [dom/ipc/ContentChild.cpp:2493]
> #37: mozilla::dom::PContentChild::OnMessageReceived(IPC::Message const &) [s3:gecko-generated-sources:ed8d3fe6b88ec31e0a9b4afd59227a2a93d788eeb800120c9bcd972d60f84e3f718e3a916251c4fb1c30f9cbc0f501f8901eefa841a0815e5ee7df2f7e5ca84d/ipc/ipdl/PContentChild.cpp::8538]
> #38: mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const &) [ipc/glue/MessageChannel.cpp:2111]
> #39: mozilla::ipc::MessageChannel::DispatchMessageW(IPC::Message &&) [ipc/glue/MessageChannel.cpp:2042]
> #40: mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask &) [ipc/glue/MessageChannel.cpp:1887]
> #41: mozilla::ipc::MessageChannel::MessageTask::Run() [ipc/glue/MessageChannel.cpp:1920]
> .....

The disconnected callback reaction is from cycle collection, enqueuing to backup queue is an expected behavior.
But since the cycle collection is happened during JS execution (for processing frame message) then we hit this assertion failure.
Look like it is not a good way to detect missing [CEReations]. Don't have other idea yet.

Comment 13

[Edgar Chen [:edgar] (PTO until 02/09)]

Set to P3 per comment 12.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

So far I haven't got any good ideas how to add these assertions in a useful way.
This doesn't need to block shipping as such.