1473034 - Investigate failure in "browser_startup_flicker.js" when "browser.xul" loads stylesheets in a certain order

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[:Paolo Amadini]

The "browser/base/content/test/performance/browser_startup_flicker.js" can fail unexpectedly on Windows 10 when stylesheets are loaded directly from "browser.xul" in a certain order. The failure does not occur if the styles are loaded in the same order using "@import" from an external file.

This is a tryserver build with the failure:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1537c306164597344f8d7d2993e4a825c6149958

This build with the same stylesheets using "@import" doesn't fail:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9a1b5f74475c053a027ad49b7d2583a1f096f6a0

This is the order stylesheets are loaded:

 <?xml-stylesheet href="chrome://global/skin/" type="text/css"?>
 <?xml-stylesheet href="chrome://browser/content/browser.css" type="text/css"?>
 <?xml-stylesheet href="chrome://browser/content/downloads/downloads.css"?>
 <?xml-stylesheet href="chrome://browser/content/places/places.css" type="text/css"?>
 <?xml-stylesheet href="chrome://browser/content/usercontext/usercontext.css" type="text/css"?>
 <?xml-stylesheet href="chrome://browser/skin/" type="text/css"?>
 <?xml-stylesheet href="chrome://browser/skin/controlcenter/panel.css" type="text/css"?>
 <?xml-stylesheet href="chrome://browser/skin/customizableui/panelUI.css" type="text/css"?>
 <?xml-stylesheet href="chrome://browser/skin/downloads/downloads.css"?>
 <?xml-stylesheet href="chrome://browser/skin/searchbar.css"?>
 <?xml-stylesheet href="chrome://browser/skin/places/places.css"?>
 <?xml-stylesheet href="chrome://browser/skin/places/editBookmark.css"?>
 <?xml-stylesheet href="chrome://browser/content/tabbrowser.css" type="text/css"?>
 <?xml-stylesheet href="chrome://browser/skin/compacttheme.css" type="text/css" alternate="yes" title="Light/Dark"?>

Comment 1

[Mike Conley (:mconley) (:⚙️)]

The good news is that I can reproduce this by reducing my screen resolution to 1280x1024 (which is what the test machines are using).

The (maybe) less good news is that I can reproduce this with or without paolo's patch. I guess that patch was just the last straw on the camel's back in automation.

At any rate, digging in.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

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

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1473034 - Ignore the gfx sanitytest window when recording start-up paints.

Review commit: https://reviewboard.mozilla.org/r/255120/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/255120/

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1473034 - Remove "browser-window.css" since the workaround isn't needed anymore.

Review commit: https://reviewboard.mozilla.org/r/255122/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/255122/

Comment 5

[Mike Conley (:mconley) (:⚙️)]

I'm 90% sure I understand what happened.

I think this was a timing issue. Our startup recorder waits until the first window reports that it has painted, and then grabs a reference to the first browser window and tells graphics to draw it (again) to be recorded with our frame recording stuff.

On Windows, the first window is actually the graphics sanity test window. So this window was painting before the main browser window had appeared, and it was forcing our graphics code to do a paint of the window even though it hadn't finished being laid out or sized yet (which is why the first frame was the wrong size and looked all wonky).

I've modified the startup recorder to ignore anything except the browser window. I've got that cooking on try here, along with a backout of paolo's workaround:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0c8a3f5d308b0b506daf1dad907c247f5851d26c

If that comes back green, I'll request review.

Comment 6

[Mike Conley (:mconley) (:⚙️)]

So try run doesn't show failures for the browser_startup_flicker test, but it's still plenty of orange. I don't think I backed out correctly.

Hey paolo, this is what the UI looks like with the backout patch: https://taskcluster-artifacts.net/QU53voHfSYKQpa4TDOepXw/0/public/test_info/mozilla-test-fail-screenshot_vynioe.png

Here's the backout patch: https://hg.mozilla.org/try/rev/98fcb6ccbb771b8e3b7b0cbde9a4ad2078085c98

What did I do wrong?

Comment 7

[:Paolo Amadini]

Thanks for figuring this out! The backout just needed to take into account bug 1421433 and bug 1472153.

Let's see if this passes try now:

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

If so, r=me to use the updated revision <https://hg.mozilla.org/try/rev/05a2c620f8e34fcfab99d95cdcd439fc08810381>.

Comment 8

[:Paolo Amadini]

Comment on attachment 8990119 [details]
Bug 1473034 - Ignore the gfx sanitytest window when recording start-up paints.

Requesting review on behalf of Mike, assuming the test in the first try build passed because of the fix and not because of the missing stylesheet. There's a new tryserver build ongoing.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8990119 [details]
Bug 1473034 - Ignore the gfx sanitytest window when recording start-up paints.

Hold up - these needs a slight modification. The check I added at the start of the observer isn't going to work for the xul-window-visible notification. New revision coming up.

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8990119 [details]
Bug 1473034 - Ignore the gfx sanitytest window when recording start-up paints.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255120/diff/1-2/

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8990120 [details]
Bug 1473034 - Remove "browser-window.css" since the workaround isn't needed anymore.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255122/diff/1-2/

Comment 12

[Mike Conley (:mconley) (:⚙️)]

Huh. In a local build, Windows looks like this with the https://hg.mozilla.org/try/rev/05a2c620f8e34fcfab99d95cdcd439fc08810381 revision of the backout:

https://i.imgur.com/0etBhDI.png

Did something else land in the meantime that might have invalidated this backout?

Maybe once the patch gets reviewed by florian, you could do the backout paolo? I feel like you've probably got a better sense of how to do it properly.

Comment 13

[:Paolo Amadini]

Yeah, I can do the backout separately in bug 1473598.

Comment 14

[Florian Quèze [:florian]]

Comment on attachment 8990119 [details]
Bug 1473034 - Ignore the gfx sanitytest window when recording start-up paints.

https://reviewboard.mozilla.org/r/255120/#review262236

Thanks for debugging this Mike!

Comment 15

[Mike Conley (:mconley) (:⚙️)]

np, thanks for the review.

I've autolanded this. I guess once this merges, followers should keep their eyes on bug 1473598 for the backout of the @import workaround.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7056315a51c8
Ignore the gfx sanitytest window when recording start-up paints. r=florian

Comment 17

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/7056315a51c8

Comment 18

[:Paolo Amadini]

(In reply to Mike Conley (:mconley) (:⚙️) (Catching up on needinfos / reviews) from comment #12)
> Huh. In a local build, Windows looks like this with the
> https://hg.mozilla.org/try/rev/05a2c620f8e34fcfab99d95cdcd439fc08810381
> revision of the backout:
> 
> https://i.imgur.com/0etBhDI.png

This looks fine for me, it may just have been a local build issue. I'm running screenshots in bug 1473598 and will land the patch there if everything still looks right.