Hi Jon,

Would you mind helping me understand what's the problem here? The assertion was added in bug 1483487.

UA Widgets runs in the same compartment as the frame script when the page it initialized on has system principal:

https://searchfox.org/mozilla-central/rev/dac799c9f4e9f5f05c1071cba94f2522aa31f7eb/toolkit/actors/UAWidgetsChild.jsm#85-86,89,92

It would hit an assertion when we call into its destructor method later.

https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=222589422&revision=9011319f94786ad3cd1976ea108975088863b81a

(For background, see

https://firefox-source-docs.mozilla.org/toolkit/content/toolkit_widgets/ua_widget.html )

Stack from linux build:

Assertion failure: cache->PreservingWrapper(), at /builds/worker/workspace/build/src/dom/bindings/DOMJSProxyHandler.cpp:82
#01: mozilla::dom::CheckExpandoObject(JSObject*, JS::Value const&) [mfbt/Assertions.h:38]
#02: mozilla::dom::DOMProxyHandler::GetExpandoObject(JSObject*) [js/public/Value.h:902]
#03: mozilla::dom::HTMLDocument_Binding::DOMProxyHandler::get(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<JS::PropertyKey>, JS::MutableHandle<JS::Value>) const [s3:gecko-generated-sources:0229e2e09c2e1c4633e789333eca6fa972f378052404403e5bac500fce6cc02d4ae44c5f8363b24465934cce22d71bc29a82079c1163e5cb2a164f0a561c7054/dom/bindings/HTMLDocumentBinding.cpp::2033]
#04: js::Proxy::getInternal(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<JS::PropertyKey>, JS::MutableHandle<JS::Value>) [js/src/proxy/Proxy.cpp:373]
#05: js::ProxyGetProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::PropertyKey>, JS::MutableHandle<JS::Value>) [js/src/proxy/Proxy.cpp:387]
#06: ??? (???:???)
#07: ??? (???:???)
#08: ??? (???:???)
#09: js::jit::EnterBaselineAtBranch(JSContext*, js::InterpreterFrame*, unsigned char*) [js/src/jit/BaselineJIT.cpp:128]

More of the same stack shows that this is happening during cycle collector unlinking:

#35: mozilla::DOMEventTargetHelper::DispatchEvent(mozilla::dom::Event&, mozilla::dom::CallerType, mozilla::ErrorResult&) [dom/events/DOMEventTargetHelper.cpp:166]
#36: nsContentUtils::DispatchChromeEvent(mozilla::dom::Document*, nsISupports*, nsTSubstring<char16_t> const&, mozilla::CanBubble, mozilla::Cancelable, bool*) [dom/base/nsContentUtils.cpp:4246]
#37: mozilla::detail::RunnableFunction<mozilla::dom::Element::NotifyUAWidgetTeardown(mozilla::dom::Element::UnattachShadowRoot)::$_10>::Run() [dom/base/Element.cpp:1258]
#38: nsContentUtils::RemoveScriptBlocker() [dom/base/nsContentUtils.cpp:5217]
#39: mozilla::dom::Document::cycleCollection::Unlink(void*) [dom/base/nsContentUtils.h:3552]
#40: nsHTMLDocument::cycleCollection::Unlink(void*) [mfbt/RefPtr.h:61]
#41: nsCycleCollector::CollectWhite() [xpcom/base/nsCycleCollector.cpp:3086]

So what's happening is that JS is manipulating an HHTMLDocument that is in the process of being unlinked. This seems bad to me, but maybe this is allowed?

Of this assertion, added in bug 1483487, peterv wrote:

Hmm, this needs a comment, because I don't think this check holds in general. Unlinking clears the flag signaling that we're
preserving the wrapper but doesn't clear the expando object. I think we can do this check in the cases below because nothing
should call those functions on a binding for an object that was unlinked.

So this is a situation where these functions are being called for an object that was unlinked.

I tried rearranging the order of operations in unlinking so that this code ran before nsINode::Unlink, but that caused a ton of assertion failures. I tried also clearing the wrapper when we release it, but that didn't fixing the problem and also caused others.

I don't know how to fix this. Peter, do you have any ideas here? I guess it depends on whether JS is supposed to be able to see such objects. Either we need to stop this from happening, or make it safe to do so. The problem I'm concerned about is that nothing traces the mExpandoAndGeneration.expando after we release the wrapper, so if we GC at this point I think we could end up with this being a dangling pointer.

Alternatively, we could workaround this in the front-end, if the code could inspect the state of the object without triggering an assertion.

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #9)

Alternatively, we could workaround this in the front-end, if the code could inspect the state of the object without triggering an assertion.

It doesn't really have to be observable in JS — if there is something Element::NotifyUAWidgetTeardown() can probe and skip UA Widget destruction it would work too.

I have this in a recording. The element that the UA widget is attached to is being collected by the CC (and so is its document). I don't think it makes sense to run the destructor function for the UA widget at that point. If it's ok to skip the destructor, then I think NotifyUAWidgetTeardown should just check if the onwer document's GetScriptHandlingObject returns null and sets aHasHadScriptHandlingObject == true, and skip posting the script runner. If it's not ok to skip it we need to find a better spot to run it.

Thank you. Let's see if this works & let me know if this is correct.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4866f1f3969b411ad29bed44cc5e4917f6e44672

https://hg.mozilla.org/mozilla-central/rev/4d21e1285e2a

Won some perf improvements! \0/

== Change summary for alert #19162 (as of Tue, 05 Feb 2019 08:34:43 GMT) ==

Improvements:

14% raptor-tp6-imgur-firefox osx-10-10 opt 1,057.27 -> 912.17

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=19162