1516292 - Assertion failure: GetHost() (ShadowRoot always has a host, how did we create this ShadowRoot?), at src/obj-firefox/dist/include/mozilla/dom/ShadowRoot.h:65

Status: RESOLVED FIXED

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

Description

[Tyson Smith [:tsmith]]

Attachment: testcase.html

Assertion failure: GetHost() (ShadowRoot always has a host, how did we create this ShadowRoot?), at src/obj-firefox/dist/include/mozilla/dom/ShadowRoot.h:65

#0 mozilla::dom::ShadowRoot::Host() const src/obj-firefox/dist/include/mozilla/dom/ShadowRoot.h:63:5
#1 mozilla::dom::ShadowRoot_Binding::get_host(JSContext*, JS::Handle<JSObject*>, mozilla::dom::ShadowRoot*, JSJitGetterCallArgs) src/obj-firefox/dom/bindings/ShadowRootBinding.cpp:103:59
#2 bool mozilla::dom::binding_detail::GenericGetter<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) src/dom/bindings/BindingUtils.cpp:2956:13
#3 js::jit::CallNativeGetter(JSContext*, JS::Handle<JSFunction*>, JS::Handle<JSObject*>, JS::MutableHandle<JS::Value>) src/js/src/jit/VMFunctions.cpp:1434:8
#4 0x7f023eb307ea  (<unknown module>)



This test case also triggers a crash:

==91534==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000010 (pc 0x7f148fce8d20 bp 0x7fff7dadcff0 sp 0x7fff7dadcf20 T0)
==91534==The signal is caused by a READ memory access.
==91534==Hint: address points to the zero page.
    #0 0x7f148fce8d1f in GetWrapperPreserveColor src/obj-firefox/dist/include/nsWrapperCacheInlines.h:14:13
    #1 0x7f148fce8d1f in nsWrapperCache::GetWrapper() const src/obj-firefox/dist/include/nsWrapperCacheInlines.h:27
    #2 0x7f1495827439 in DoGetOrCreateDOMReflector<mozilla::dom::Element, mozilla::dom::binding_detail::eWrapIntoContextCompartment> src/obj-firefox/dist/include/mozilla/dom/BindingUtils.h:959:26
    #3 0x7f1495827439 in GetOrCreateDOMReflector<mozilla::dom::Element> src/obj-firefox/dist/include/mozilla/dom/BindingUtils.h:1034
    #4 0x7f1495827439 in mozilla::dom::ShadowRoot_Binding::get_host(JSContext*, JS::Handle<JSObject*>, mozilla::dom::ShadowRoot*, JSJitGetterCallArgs) src/obj-firefox/dom/bindings/ShadowRootBinding.cpp:105
    #5 0x7f149727f5b0 in bool mozilla::dom::binding_detail::GenericGetter<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) src/dom/bindings/BindingUtils.cpp:2956:13
    #6 0x7f149ff8f5a0 in js::jit::CallNativeGetter(JSContext*, JS::Handle<JSFunction*>, JS::Handle<JSObject*>, JS::MutableHandle<JS::Value>) src/js/src/jit/VMFunctions.cpp:1434:8
    #7 0x2bf592b2007f  (<unknown module>)

Comment 1

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Edgar, can you please take a look?

Comment 2

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

Is this UAWidgets related issue?

Comment 3

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

Though, there is also svg:use around. Plenty of internal Shadow DOM usage ;)

Comment 4

[Tim Guan-tin Chien [:timdream] (please needinfo)]

JS stack at the time of the crash.

0 get isVideoInFullScreen() ["chrome://global/content/elements/videocontrols.js":1302:12]
    this = [object Object]
1 startFade(element = [object HTMLDivElement], fadeIn = false, immediate = true) ["chrome://global/content/elements/videocontrols.js":1195:0]
    this = [object Object]
2 setupNewLoadState() ["chrome://global/content/elements/videocontrols.js":370:8]
    this = [object Object]
3 init(shadowRoot = [object ShadowRoot]) ["chrome://global/content/elements/videocontrols.js":1983:8]
    this = [object Object]
4 onsetup() ["chrome://global/content/elements/videocontrols.js":2182:4]
    this = [object Object]
5 switchImpl() ["chrome://global/content/elements/videocontrols.js":62:6]
    this = [object Object]
6 onsetup() ["chrome://global/content/elements/videocontrols.js":28:4]
    this = [object Object]
7 setupWidget(aElement = [cross-compartment wrapper]) ["resource://gre/actors/UAWidgetsChild.jsm":96:8]
    this = [object Object]
8 setupOrNotifyWidget(aElement = [cross-compartment wrapper]) ["resource://gre/actors/UAWidgetsChild.jsm":37:6]
    this = [object Object]
9 handleEvent(aEvent = [cross-compartment wrapper]) ["resource://gre/actors/UAWidgetsChild.jsm":22:8]
    this = [object Object]
10 handleActorEvent(actor = "UAWidgetsChild", event = [cross-compartment wrapper]) ["resource://gre/modules/ActorManagerChild.jsm":134:25]
    this = [object Object]
11 handleActorEvent([cross-compartment wrapper]) ["self-hosted":1018:16]
    this = [object ContentFrameMessageManager]
12 go() ["https://bug1516292.bmoattachments.org/attachment.cgi?id=9033209":3:16]
    this = [object Window]
13 onload(event = [object Event]) ["https://bug1516292.bmoattachments.org/attachment.cgi?id=9033209":1:0]
    this = [object Window]

Comment 5

[Tim Guan-tin Chien [:timdream] (please needinfo)]

The assertion-triggering access to shadowRoot.host was added in bug 1505547. I can change that one line to

  this.video.isSameNode(this.video.getRootNode().mozFullScreenElement);

What I don't understand when I looking at the stack is that we accessed shadowRoot.host at the same tick in

https://searchfox.org/mozilla-central/rev/fef7f858efb695a76010b4c624da5277c16e95b3/toolkit/content/widgets/videocontrols.js#1918

What makes it work and this doesn't?

That said, given that UA Widget is in beta already, we should just land this wallpaper...

Comment 6

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Bug 1516292 - Use isSameNode() to compare mozFullScreenElement and the video host element r=edgar

This does what's done in bug 1505547 but in a different way. For some reason, access
shadowRoot.host in the function can trigger an assertion.

this.video is a Proxy so it cannot be compared, but all its methods are proxied.

Comment 7

[Tim Guan-tin Chien [:timdream] (please needinfo)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ffe5a1d33a4a81e813947c6021fa21de7014167

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/42dc31f09859
Use isSameNode() to compare mozFullScreenElement and the video host element r=edgar

Comment 9

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/42dc31f09859

Comment 10

[Tim Guan-tin Chien [:timdream] (please needinfo)]

need uplift.

Comment 11

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 9034506 [details]
Bug 1516292 - Use isSameNode() to compare mozFullScreenElement and the video host element r=edgar

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1505547

User impact if declined: video controls may not behave correctly on some pages

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: No

Needs manual test from QE?: No

If yes, steps to reproduce: The crashtest page should not trigger an assertion failure.

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Just an oneliner.

String changes made/needed: None

Comment 12

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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

User impact if declined: video controls may not behave correctly on some pages

According to finding in bug 1518655, "some pages" probably only limited to <video> inside <svg:use>.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9034506 [details]
Bug 1516292 - Use isSameNode() to compare mozFullScreenElement and the video host element r=edgar

[Triage Comment]
Video controls not working "probably only limited to <video> inside <svg:use>" seems like a pretty edge case scenario, but it's a one-liner with a testcase for a new regression in Fx65. Let's take it to avoid shipping the regression on the off chance someone on the web is actually doing this. Approved for 65.0b10.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2046aa360552

Comment 15

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Noted that bug 1518655 is still valid. Video controls initialized in the test case will fail to init inside <svg:use> with JS thrown somewhere. We should address that in bug 1518655.