Closed Bug 1473034 Opened 2 years ago Closed 2 years ago

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

Categories

(Firefox :: General, defect)

Unspecified
Windows 10
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 63
Tracking Status
firefox63 --- fixed

People

(Reporter: Paolo, Assigned: mconley)

References

Details

Attachments

(1 file, 1 obsolete file)

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"?>
Flags: needinfo?(mconley)
Assignee: nobody → 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.
Blocks: 1473598
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.
Flags: needinfo?(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?
Flags: needinfo?(paolo.mozmail)
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>.
Flags: needinfo?(paolo.mozmail)
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.
Attachment #8990119 - Flags: review?(florian)
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.
Attachment #8990119 - Flags: review?(florian)
Attachment #8990120 - Flags: review?(paolo.mozmail)
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.
Flags: needinfo?(paolo.mozmail)
Yeah, I can do the backout separately in bug 1473598.
Flags: needinfo?(paolo.mozmail)
Attachment #8990120 - Attachment is obsolete: true
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!
Attachment #8990119 - Flags: review?(florian) → review+
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.
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7056315a51c8
Ignore the gfx sanitytest window when recording start-up paints. r=florian
https://hg.mozilla.org/mozilla-central/rev/7056315a51c8
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
(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.
You need to log in before you can comment on or make changes to this bug.