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

RESOLVED INCOMPLETE

Status

()

defect
P3
normal
RESOLVED INCOMPLETE
2 years ago
5 months ago

People

(Reporter: edgar, Assigned: edgar)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

No description provided.
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.
Assignee: nobody → echen
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.
Olli, does using GetIncumbentGlobal() make sense here?
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
Attachment #8940639 - Flags: review?(bugs) → review+
Pushed by echen@mozilla.com:
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
(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.
Flags: needinfo?(echen)
Set to P3 per comment 12.
Priority: P2 → P3
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.
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → INCOMPLETE
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.