1405543 - stylo: media element controls are sometimes displayed unstyled

Status: VERIFIED FIXED

(Toolkit :: Video/Audio Controls, defect, P2)

Description

[beldirai]

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20171002181526

Steps to reproduce:

I was listening to mp3 files in my browser with the embedded player (legal files, from a Japanese game developer's site). The player used to look perfectly normal, but somehow updating to Firefox Quantum has changed it to the point it's unusable.


Actual results:

For example, I'm trying to listen to a CD sample from Angelique Retour. https://www.gamecity.ne.jp/ange_retour/sound/cd/c05.mp3 This one.

The embedded player used to look like this: https://imgur.com/7M17XsH
But after the Quantum update, it looks like this, no matter which file I play: https://imgur.com/nW5oVup

There aren't any buttons or links to click, so it's impossible to pause the file or navigate to a certain minute/second.


Expected results:

The embedded player should still look the same as it did before, right? I haven't even made any changes to my Firefox settings that would explain the player's current appearance - the Inspector Module still shows the same settings it always has. ...It just seems like a bug caused by something in the Quantum update (specifically 57.0b5).

Comment 1

[YF (Yang)]

WFM in 57b5 and Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:58.0) Gecko/20100101 Firefox/58.0 ID:20171004220309.

Comment 3

[randomreporter]

I think I came across this bug a couple of times. Here is the screenshot of the interface after I opened .mp3 in new tab: https://i.imgur.com/owERYTt.png
Unfortunately, I couldn't find reliable STR, I took that screenshot after I clicked direct link to .mp3 around 20 times with ctrl-click, only one of those tabs had broken interface.

Comment 4

[randomreporter]

Possible STR: hold Ctrl and click this link from the first post around 20 times very quickly, imagine you're trying to kill someone in Battlefield using a pistol:
https://www.gamecity.ne.jp/ange_retour/sound/cd/c05.mp3
~5 tabs out of those 20 had broken interface in my case. Nightly 04-10-17.

Comment 5

[Peja Stija]

Using Mozregression, I could pinpoint it down to https://hg.mozilla.org/integration/autoland/rev/44053b9ef955
Sorry that I'm not too knowledged about using Mozregression, I'll keep it open in case someone needs more (informative) info.

Comment 6

[Peja Stija]

Please disregard above, trying multiple times gives different results, the problem is that the issue is not permanent but occurs occasionally.

Comment 7

[Jim Mathies [:jimm]]

Can you please try disabling stylo and retesting after a restart?

layout.css.servo.enabled = false

Comment 8

[Peja Stija]

Jim, bisecting with Mozregression with layout.css.servo.enabled = false ...
All tests turn out good, couldn't find any issue, so this looks to be related.

Comment 9

[Ray Lin[:ralin]]

Hi Cameron,

According to Peja's observation and the screenshot, it seems that sometimes the XBL <stylesheets> aren't loaded and cause the media controls unstyled(see https://imgur.com/nW5oVup) when servo enabled, do you know what might be the cause or who's right person to this? Thanks :D

Comment 10

[Cameron McCormack (:heycam)]

Unfortunately I haven't managed to reproduce this so far, on Linux or macOS, with current Nightly.  It does seem plausible that the XBL style sheets either aren't loading or we're not restyling when we should.  Emilio/TY, can either of your reproduce this?

Comment 11

[Cameron McCormack (:heycam)]

After restarting the browser I did manage to reproduce.  Every fourth tab has unstyled media controls, and they are all in the same content process.

Comment 12

[Cameron McCormack (:heycam)]

So far I can't reproduce in my locally built Firefox, only with my currently running Nightly.  It must be something to do with matching or loading of the XBL sheet.  For example, the <xul:videocontrols class="mediaControlsFrame"> has this binding:

  -moz-binding: url("chrome://global/content/bindings/videocontrols.xml#videoControls")

But it doesn't match the .mediaControlsFrame rule from the style sheet the binding specifies, chrome://global/content/bindings/videocontrols.css.  Toggling the <video> display:none and back again yields the same, unstyled controls.

Comment 13

[Emilio Cobos Álvarez (:emilio)]

(In reply to Peja Stija from comment #5)
> Using Mozregression, I could pinpoint it down to
> https://hg.mozilla.org/integration/autoland/rev/44053b9ef955
> Sorry that I'm not too knowledged about using Mozregression, I'll keep it
> open in case someone needs more (informative) info.

So, I haven't been able to repro this, but if my commit is to blame, then it changes three things that may be relevant to this:

 * It makes use the bloom filter optimization. There may be the case that with all the XBL flattened tree weirdness we're messing it up with some rule that matters?
 * The nth-index cache. Not sure how this would have any effect here tbh.
 * The slow selector flags. Not sure either how it could affect this.
 * It technically changes the MatchingMode when matching pseudos, but that's the fix in bug 1403115.

I think it's unlikely that my patch is really to blame, but if it was, it'd be because of that.

If anyone can reproduce on a locally built Firefox, what I would try to pinpoint this down is toggling those things in `context` while executing this block:

  https://github.com/servo/servo/blob/8dece5e74e32223d968648080bb371f3be7fbd7c/components/style/stylist.rs#L1275

And see which one is to blame.

Comment 14

[Xidorn Quan [:xidorn] UTC+11]

I failed to reproduce this issue as well, in either Nightly or local debug build :/

Comment 15

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

I cannot reproduce this either on latest Nightly or my local debug build ...

Comment 16

[Hiroyuki Ikezoe (:hiro)]

I could reproduce it on local builds.  I think https://www.w3schools.com/html/html5_video.asp is easier to reproduce than  https://www.gamecity.ne.jp/ange_retour/sound/cd/c05.mp3.  And I guess it's more easier to reproduce if both sites open in several tabs.

Comment 17

[Astley Chen (inactive)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #16)
> I could reproduce it on local builds.  I think
> https://www.w3schools.com/html/html5_video.asp is easier to reproduce than 
> https://www.gamecity.ne.jp/ange_retour/sound/cd/c05.mp3.  And I guess it's
> more easier to reproduce if both sites open in several tabs.

Thanks, Hiro. I could easily reproduce on Nightly 58.0a1 (2017-10-08).

Comment 19

[Emilio Cobos Álvarez (:emilio)]

I managed to repro right-clicking and then "view video".

Comment 20

[Astley Chen (inactive)]

mozregression on macOS told me the same story as comment 5:
https://hg.mozilla.org/integration/autoland/rev/44053b9ef955

Comment 21

[Emilio Cobos Álvarez (:emilio)]

So, I filed bug 1406875 with the underlying problem, which was also pre-existing (we just didn't rely on it by luck).

This bug happened because the quirks mode of the document holding the binding and the quirks mode of the binding's styleset got out of sync because a binding with a different doctype got reused.

Since we always used the same `MatchingContext` and the top-level styleset's quirks-mode, under the assumption it was the same, we ended up looking for rules using the lower-cased string, which means that we didn't find rules for a bunch of stuff.

The easiest way to repro is, as described in bug 1405526, to just go to https://www.w3schools.com/html/html5_video.asp, and right-click -> view video, since the video viewer is in quirks mode.

The patch for this is https://github.com/servo/servo/pull/18794. That should be uplifted to beta, probably.

Comment 22

[Emilio Cobos Álvarez (:emilio)]

Attachment: Patch

Comment 23

[Hiroyuki Ikezoe (:hiro)]

https://hg.mozilla.org/mozilla-central/rev/ca72a7721ef7

Comment 24

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8916548 [details] [review]
Patch

Patch: https://hg.mozilla.org/mozilla-central/rev/ca72a7721ef7

Approval Request Comment
[Feature/Bug causing the regression]: bug 1403115
[User impact if declined]: unstyled video controls
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: see comment 21 
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: not much
[Why is the change risky/not risky?]: It's a pretty self-contained change that reverts a specific part of bug 1403115 because some invariant doesn't hold.
[String changes made/needed]: none

Comment 25

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Emilio, I test Nightly (2017-10-09). With the steps in Comment 21, in the view video page, the video's control doesn't show up when the mouse hover on it.

Comment 26

[Emilio Cobos Álvarez (:emilio)]

Which build ID? I'm on 20171009220104 and I can't repro the bug anymore. Now we have two nightlies a day the date may not be enough...

Comment 27

[Emilio Cobos Álvarez (:emilio)]

Oh, but I think I see what you mean, yeah, and I bet I know what happens... Sigh. It's related, but not a recent regression either. I'll write something.

Comment 28

[Emilio Cobos Álvarez (:emilio)]

This is either due to the bloom filter or the invalidation code being confused about different quirk modes... Let's just fix that in bug 1406875 instead of playing whack-a-mole.

Comment 29

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Jared, the patch has not been r+'d. I am confused about the beta uplift request. Could you please help? Thanks!

Comment 30

[Jared Wein [:jaws] (please needinfo? me)]

heycam r+'d the patch on github at https://github.com/servo/servo/pull/18794

This patch should be uplifted to beta if possible.

Comment 31

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Thanks, will take it.

Comment 32

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8916548 [details] [review]
Patch

Recent regression, Beta57+

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f8a4d5043ad7

Comment 34

[Oana Botisan, Desktop QA]

I managed to reproduce the bug with an older version of Nightly (2017-10-09) on Windows 10x64 and Ubuntu 16.04x64.  The way I tested was using the steps from comment 21. 
I retested everything using beta 57.0b8 and latest Nightly on the same platforms, but the bug is not reproducing anymore.

Comment 35

[beldirai]

I have completely forgotten to mention the player has been fixed for me. Sorry about that!

Comment 36

[aditya]

I could reproduce it on local builds. I think https://www.welookups.com/html/html5_video.html

is easier to reproduce than https://www.gamecity.ne.jp/ange_retour/sound/cd/c05.mp3. And I guess it's more easier to reproduce if both sites open in several tabs.

Comment 37

[Emilio Cobos Álvarez (:emilio)]

If you can reproduce this problem in something later than Firefox 57, can you file another bug with steps to reproduce it?

Though I suspect this is just spam given it's exactly the same comment with one link changed as one above.