STR: visit https://bug1376004.bmoattachments.org/attachment.cgi?id=8885585 We used to show an error message below the icon (please see the screenshot), and now the message is gone. The narrowed regression window indicates the cause is about enabling stylo by default[0]. *please ignore the background-color difference in the screenshot comparison, the background-color was changed in another bug. [0] https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=5f721a664bf64fed99184a866b60c24a6afcb3a0&tochange=62cebea1d1578461a423a9ca7848706455db9ea5

Hi Astley, Could you help me forward this issue to the right component? Thanks.

So this label is supposed to be shown at: http://searchfox.org/mozilla-central/rev/40e8eb46609dcb8780764774ec550afff1eed3a5/toolkit/content/widgets/videocontrols.xml#781 I don't see anything setting hidden="false" on the overlay, though I could be wrong. In any case, assuming that's not it, which I guess it's true, this looks like either an incremental restyle bug, or bug 1374247 (though I don't see any <children> element around).

Ray, do you know what's the best way to debug the video controls? The browser toolbox doesn't work on them, and it would be really helpful to have some sort of inspector.

(In reply to Emilio Cobos Álvarez [:emilio] from comment #2) > I don't see anything setting hidden="false" on the overlay, though I could > be wrong. In any case, assuming that's not it, which I guess it's true, this > looks like either an incremental restyle bug, or bug 1374247 (though I don't > see any <children> element around). In my build instrumented to investigate bug 1374247 that assertion doesn't trigger, so not the case...

(In reply to Emilio Cobos Álvarez [:emilio] from comment #3) > Ray, do you know what's the best way to debug the video controls? The > browser toolbox doesn't work on them, and it would be really helpful to have > some sort of inspector. I would set pref "browser.tabs.remote.autostart" to false and restart the browser in non-e10s mode, then browser toolbox would be able to inspect inside the anon content. Hope it helps :)

That is indeed helpful. So we're definitely getting the invalidation list right: DEBUG:style::invalidation::element::invalidator: Collected invalidations (self: true): DEBUG:style::invalidation::element::invalidator: > self: [Invalidation([anonid="errorSrcNotSupported"])] DEBUG:style::invalidation::element::invalidator: > descendants: [Invalidation([anonid="errorSrcNotSupported"])] DEBUG:style::invalidation::element::invalidator: > siblings: [] DEBUG:style::invalidation::element::invalidator: StyleTreeInvalidator::invalidate_descendants(<div class="statusOverlay stackItem"> (0x7f10f00fe980)) DEBUG:style::invalidation::element::invalidator: > [Invalidation([anonid="errorSrcNotSupported"])] So not sure where the failure is. It may be either that we don't find the child during the invalidation process, or what not... I'm looking into it.

So we're bailing because we think the status overlay is display: none... Now, how and why that happens is not clear to me.

So that bailout is valid under the assumption that stuff under it has no style data, which from my logging seems to be the invariant that breaks...

After taking a closer look the styles look sane, may be a change hint or frame constructor bug? I'm not sure yet...

Arrrghhh, got it. This is a style sharing bug, of course. Running stylo with `DISABLE_STYLE_SHARING_CACHE=1` makes the problem disappear. And it makes sense, because we don't look into XBL stylesheets for style sharing.

Phew, this one was fun :)

Comment on attachment 8922880 [details] Bug 1412251: Make style sharing look at XBL / Shadow DOM rules. https://reviewboard.mozilla.org/r/194030/#review199106 ::: servo/components/style/stylist.rs:1492 (Diff revision 1) > true > } > ); > } > > + element.each_xbl_stylist(|stylist| { This is ok only if we preserve the invariant that a target and a candidate have the same set of applicable revalidation selectors. That means we must not reach this code in any case when the target and candidate have different sets of XBL stylists. That's important anyway, because if two elements have different sets of XBL stylists they could have different style based on tag name or class or whatnot even if they look absolutely identical. So presumably we need to add some sort of check in test_candidate over in sharing/mod.rs to check that the two elements have the same get_xbl_binding() and the same get_xbl_binding_parent() on self.rule_hash_target(), right? And document it in the big comment at the top of that file. While I was there, I looked at rule_hash_target in components/style/dom.rs and the "NB" comment makes no sense; it's not even coherent English. :( If you can figure out what it's trying to say, please fix it?

Oh, and we should add a test where the target and candidate are otherwise identical but have different relevant bindings and those bindings have different rules...

Comment on attachment 8922880 [details] Bug 1412251: Make style sharing look at XBL / Shadow DOM rules. https://reviewboard.mozilla.org/r/194030/#review199106 > This is ok only if we preserve the invariant that a target and a candidate have the same set of applicable revalidation selectors. > > That means we must not reach this code in any case when the target and candidate have different sets of XBL stylists. > > That's important anyway, because if two elements have different sets of XBL stylists they could have different style based on tag name or class or whatnot even if they look absolutely identical. > > So presumably we need to add some sort of check in test_candidate over in sharing/mod.rs to check that the two elements have the same get_xbl_binding() and the same get_xbl_binding_parent() on self.rule_hash_target(), right? And document it in the big comment at the top of that file. > > While I was there, I looked at rule_hash_target in components/style/dom.rs and the "NB" comment makes no sense; it's not even coherent English. :( If you can figure out what it's trying to say, please fix it? Yeah, as discussed on IRC, I thought this couldn't be the case for XBL because two elements with different bindings also have different style necessarily. But it can happen with shadow DOM. I added a test for it which without the style_scope() check causes the bitvec length assertion to fire.

Comment on attachment 8922880 [details] Bug 1412251: Make style sharing look at XBL / Shadow DOM rules. https://reviewboard.mozilla.org/r/194030/#review199142 ::: servo/components/style/gecko/wrapper.rs:955 (Diff revision 3) > > fn after_pseudo_element(&self) -> Option<Self> { > self.get_before_or_after_pseudo(/* is_before = */ false) > } > > + /// FIXME(emilio): if XBL ever goes away, convert the style scope in either s/in either/to either/? Not sure what this is trying to say. ::: servo/components/style/gecko/wrapper.rs:961 (Diff revision 3) > + /// a document or a ShadowRoot. > + /// > + /// Also, ensure this accurately represents the rules that an element may > + /// match, even in the native anonymous content case. > + fn style_scope(&self) -> Self::ConcreteNode { > + if !self.may_be_in_binding_manager() { Hmm. This seems a bit fishy. It happens to work out right, but for non-obvious reasons. Specifically, say we have an element A, which has an XBL binding attached. That binding has a shadow tree which contains a node B, which has a child C. Now A and B have may_be_in_binding_manager() test true, I think (not 100% sure for B), but I'm pretty sure C does not. So we'll have the "wrong" style scope for B; it really should be A, I would think. As I said, I think it happens to work out, because B won't share styles with nodes outside the binding attached to A, so neither will C. But it's not very obvious. At the very least, this needs clear documentation about why this optimization is OK, but failing that we may want to just remove this optimization...

Comment on attachment 8922880 [details] Bug 1412251: Make style sharing look at XBL / Shadow DOM rules. https://reviewboard.mozilla.org/r/194030/#review199142 > s/in either/to either/? Not sure what this is trying to say. Yeah, what I was trying to say is that hopefuly in the future this can look like: ``` if self.is_in_shadow_tree() { return self.containing_shadow(); } self.owner_doc().as_node() ``` But, probably will take a bit to get to that state... > Hmm. This seems a bit fishy. It happens to work out right, but for non-obvious reasons. > > Specifically, say we have an element A, which has an XBL binding attached. That binding has a shadow tree which contains a node B, which has a child C. > > Now A and B have may_be_in_binding_manager() test true, I think (not 100% sure for B), but I'm pretty sure C does not. So we'll have the "wrong" style scope for B; it really should be A, I would think. > > As I said, I think it happens to work out, because B won't share styles with nodes outside the binding attached to A, so neither will C. But it's not very obvious. > > At the very least, this needs clear documentation about why this optimization is OK, but failing that we may want to just remove this optimization... Yeah, I thought that may_be_in_binding_manager would test true for everything in an XBL binding, but this is obviously not the case, so I'll just ditch that early-out.

Comment on attachment 8922880 [details] Bug 1412251: Make style sharing look at XBL / Shadow DOM rules. https://reviewboard.mozilla.org/r/194030/#review199142 > Yeah, what I was trying to say is that hopefuly in the future this can look like: > > ``` > if self.is_in_shadow_tree() { > return self.containing_shadow(); > } > > self.owner_doc().as_node() > ``` > > But, probably will take a bit to get to that state... (well, probably accounting for the shadow host case too, but you get what I'm saying, probably). Maybe I should just remove the comment.

https://github.com/servo/servo/pull/19045 is the servo bits.

Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/autoland/rev/13dfed0f47dc Make style sharing look at XBL / Shadow DOM rules. r=bz

Comment on attachment 8922880 [details] Bug 1412251: Make style sharing look at XBL / Shadow DOM rules. https://reviewboard.mozilla.org/r/194030/#review199142 > (well, probably accounting for the shadow host case too, but you get what I'm saying, probably). Maybe I should just remove the comment. I think the comment is fine; just say "to either" and it's pretty clear what it's talking about. > Yeah, I thought that may_be_in_binding_manager would test true for everything in an XBL binding, but this is obviously not the case, so I'll just ditch that early-out. Makes sense, thanks!

Backed out for failing reftest layout/reftests/bugs/272646-1.xul on OS X: https://hg.mozilla.org/integration/autoland/rev/39400e032624c7cd7b3c63c62957c5c32aa8b008 https://hg.mozilla.org/integration/autoland/rev/6087df884cb256fce27d97574eedb12e91db73eb Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=13dfed0f47dcc55d82199b0b992391c8b4bc0058&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=140397490&repo=autoland

That was an unexpected pass because this also fixes bug 1410787. Now I need to wait for the Servo bit to get backed out to then reland, but will do that.

Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/autoland/rev/6290bb77c687 Make style sharing look at XBL / Shadow DOM rules. r=bz

Relanded the servo bits too as https://hg.mozilla.org/integration/autoland/rev/80274284243b.

It works now on latest Nightly build 58.0a1 (2017-10-29) (64-bit). Emilio, could you please create an uplift request for 57 beta?

Comment on attachment 8922880 [details] Bug 1412251: Make style sharing look at XBL / Shadow DOM rules. This approval request is for https://hg.mozilla.org/integration/autoland/rev/80274284243b too, which includes the Servo bits. Approval Request Comment [Feature/Bug causing the regression]: stylo [User impact if declined]: wrong style in video controls, causing issues like the one described in comment 0. [Is this code covered by automated tests?]: yes [Has the fix been verified in Nightly?]: yes [Needs manual test from QE? If yes, steps to reproduce]: no [List of other uplifts needed for the feature/fix]: none [Is the change risky?]: slightly, but not much [Why is the change risky/not risky?]: it's not a super-trivial patch, but it is isolated and can only cause us to share less styles (we were doing so incorrectly). There's also the option to land a smaller patch if needed for 57, disabling style sharing for elements under an XBL binding, but I think this patch as is should be ok to uplift. [String changes made/needed]: none

Comment on attachment 8922880 [details] Bug 1412251: Make style sharing look at XBL / Shadow DOM rules. Stylo related, Beta57+

https://hg.mozilla.org/releases/mozilla-beta/rev/f7252be9b5e6 https://hg.mozilla.org/releases/mozilla-beta/rev/11ab96f5bc95

We had to disable the automated tests on Beta because they depend on bug 1409079 being present (Shadow DOM support enabled by default for Stylo). https://hg.mozilla.org/releases/mozilla-beta/rev/150889c6edef Therefore, we definitely need manual QA verification of this fix on Beta. Note that it missed the b12 gtb cut-off, so verification will need to wait until b13 barring any sort of respin.

I have reproduced this issue using Firefox 58.0a1 (2017.10.27) on Win 8.1 x64. I can confirm this issue is fixed, it is show an error message below the icon, I verified using Firefox 58.0a1 (2017.11.01) on Windows 8.1 x64, Ubuntu 14.04 x64 and Mac OS X 10.12. Note that tomorrow when appear the 57.0b14, I hope the fix will be there and I will verify the issue.

I can confirm this issue is fixed on Firefox 57.0b14, it is show an error message below the icon, verified using Windows 8.1 x64, Ubuntu 14.04 x64 and Mac OS X 10.13.