Closed Bug 1514098 Opened 6 years ago Closed 6 years ago

UA Widgets destructor and change methods does not run if the widget is on system principal page

Categories

(Toolkit :: UI Widgets, defect, P3)

Product:
Toolkit
Component:
UI Widgets
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla67
Tracking Flags:
Tracking Status
firefox67 --- fixed

People

(Reporter: timdream, Assigned: timdream)

References

Details

Attachments

(1 file, 2 obsolete files)

Bug 1514098 - Don't call into UA Widget distructor if the element is being CC'd
47 bytes, text/x-phabricator-request
Details | Review 
While working on bug 1511130 it was found that UAWidgetsChild is incorrectly probing into wrappedJSObject property to access these methods. Since the property itself does not exist for system principal object, the methods to call would not exist either.

A tiny fix, follow up to bug 1511130 will be able to fix this, but it is currently blocked by a11y test failures. The tests could hit an assertion

Assertion failure: cache->PreservingWrapper(), at /builds/worker/workspace/build/src/dom/bindings/DOMJSProxyHandler.cpp:84

When the UA Widget try to access the reference to document saved in the destructor call.

22:20:11     INFO - GECKO(1986) | videocontrols.js:729 fullscreenchange
22:20:11     INFO - GECKO(1986) | 0 terminate() ["chrome://global/content/elements/videocontrols.js":729:12]
22:20:11     INFO - GECKO(1986) |     this = [object Object]
22:20:11     INFO - GECKO(1986) | 1 destructor() ["chrome://global/content/elements/videocontrols.js":2274:4]
22:20:11     INFO - GECKO(1986) |     this = [object Object]
22:20:11     INFO - GECKO(1986) | 2 destructor() ["chrome://global/content/elements/videocontrols.js":72:4]
22:20:11     INFO - GECKO(1986) |     this = [object Object]
22:20:11     INFO - GECKO(1986) | 3 teardownWidget(aElement = [cross-compartment wrapper]) ["resource://gre/actors/UAWidgetsChild.jsm":112:8]
22:20:11     INFO - GECKO(1986) |     this = [object Object]
22:20:11     INFO - GECKO(1986) | 4 handleEvent(aEvent = [cross-compartment wrapper]) ["resource://gre/actors/UAWidgetsChild.jsm":26:8]
22:20:11     INFO - GECKO(1986) |     this = [object Object]
22:20:11     INFO - GECKO(1986) | 5 handleActorEvent(actor = "UAWidgetsChild", event = [cross-compartment wrapper]) ["resource://gre/modules/ActorManagerChild.jsm":134:25]
22:20:11     INFO - GECKO(1986) |     this = [object Object]
22:20:11     INFO - GECKO(1986) | 6 handleActorEvent([cross-compartment wrapper]) ["self-hosted":1018:16]
22:20:11     INFO - GECKO(1986) |     this = [object ContentFrameMessageManager]

(These are based on the findings from try

https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=216744610&repo=try&lineNumber=3130

)
I don't recall if there is any usage of the UA Widget-backed HTML elements (marquee/embed/object/video/input type=date) on system principal pages in production, though. It is still necessary to get this fix to ensure correctness.
When the widget is initialized on a system principal page, UAWidgetsChild should not be accessing its method via wrappedJSObject.
Priority: -- → P3
Attachment #9031316 - Attachment description: Bug 1514098 - Allow UAWidgetsChild to call onchange() and destructor() correctly → Bug 1514098 - Allow UAWidgetsChild to call onchange() and destructor() correctly on chrome window
Attachment #9031316 - Attachment description: Bug 1514098 - Allow UAWidgetsChild to call onchange() and destructor() correctly on chrome window → Bug 1514098 - Allow UAWidgetsChild to call widget methods correctly on chrome window
This addresses the comment from ExE-Boss, and use the window object of the chrome document in place of the sandbox object for widgets on chrome window. Previously, a plain |Object.create({})| was used.

Let's see if this gets rid of the assertion in JS.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a0a1a3b48a882809d519939f188b74df04f0b308
... and it does not fix the issue. 😞

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 )

Flags: needinfo?(jcoppeard)

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.

Flags: needinfo?(jcoppeard) → needinfo?(peterv)

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.

Flags: needinfo?(peterv)
Attachment #9031316 - Attachment is obsolete: true
Attachment #9040221 - Attachment is obsolete: true
Pushed by timdream@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/4d21e1285e2a
Don't call into UA Widget distructor if the element is being CC'd r=peterv,bgrins

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

Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox67: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla67

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

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: