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


(Firefox :: General, defect)

Windows 10
Not set



Firefox 63
Tracking Status
firefox63 --- fixed


(Reporter: Paolo, Assigned: mconley)




(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:

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

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:

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:

Here's the backout patch:

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:

If so, r=me to use the updated revision <>.
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 revision of the backout:

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.

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
Ignore the gfx sanitytest window when recording start-up paints. r=florian
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
> revision of the backout:

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.