1471045 - Vox Two stage rendering

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Eric Rescorla (:ekr)]

Go to www.vox.com. In Nightly you get a giant Vox logo in the background and then the site renders. In Beta you get the site immediately.

Comment 1

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=4cc60824e00d29a34537e081a7a150b9d05710da&tochange=11fe54576742b8d0662ca9c040931b312c20764b

@:jwatt
Your bunch of patch seems to cause the regression. Can you please look into this?

Comment 2

[Alice0775 White]

:jwatt does not accept ni now.
:heycam, can you look into this?

Comment 3

[Cameron McCormack (:heycam)]

I narrowed this down to bug 1466004.  But interestingly, I can only reproduce this if I have my ad blocker (uBlock Origin) enabled.  Without the ad block enabled, I don't get the flash of the Vox logo.

Eric, out of interest, do you have an ad blocker, and if so, does the problem go away if you disable it when loading the page?

Comment 4

[Alice0775 White]

I can reproduce the problem with new clean profile. 
It happens when open the page at 2nd time or reload(F5), but not happen when reload override cache Ctrl+F5)

Comment 5

[Hiroyuki Ikezoe (:hiro)]

On my linux environment, setting layout.css.parsing.parallel false solves this.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Bug 1466004 is the likely culprit. Flushing layout from there can cause a FOUC, or this effect, if document.fonts.ready is accessed.

We should not flush layout if there are pending stylesheet loads IMO.

Comment 7

[Phabricator Automation]

Attachment: Bug 1471045: Don't flush layout if the ready promise is not resolved yet. r=heycam

This prevents FOUC, and also matches the chromium logic:

  https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/css/font_face_set_document.cc?l=105&rcl=e70e652d516c7d14d50e547aae2da1690c4ae54c

Comment 8

[Emilio Cobos Álvarez (:emilio)]

(In reply to Cameron McCormack (:heycam) from comment #3)
> I narrowed this down to bug 1466004.  But interestingly, I can only
> reproduce this if I have my ad blocker (uBlock Origin) enabled.  Without the
> ad block enabled, I don't get the flash of the Vox logo.
> 
> Eric, out of interest, do you have an ad blocker, and if so, does the
> problem go away if you disable it when loading the page?

Oh, I had not noticed this comment before writing comment 6, oh well :)

I locally verified this fixes the issue. I have no ad blocker and I can repro on a clean build, fwiw.

I'm cancelling cam's ni? since he has a review request now ;)

Comment 9

[Phabricator Automation]

Comment on attachment 8987811 [details]
Bug 1471045: Don't flush layout if the ready promise is not resolved yet. r=heycam

Cameron McCormack (:heycam) has approved the revision.

https://phabricator.services.mozilla.com/D1827

Comment 10

[Cameron McCormack (:heycam)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #8)
> I locally verified this fixes the issue. I have no ad blocker and I can
> repro on a clean build, fwiw.

Can't test myself right now, but I think it will fix it locally for me too.

Cancelling my ni for ekr.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/85d78bc8bcd3
Don't flush layout if the ready promise is not resolved yet. r=heycam

Comment 12

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/85d78bc8bcd3

Comment 13

[Pascal Chevrel:pascalc (PTO until April 26)]

Emilio, should we verify this on Nightly and uplift to beta?

Comment 14

[Bobby Holley (:bholley)]

(In reply to Pascal Chevrel:pascalc from comment #13)
> Emilio, should we verify this on Nightly and uplift to beta?

Yeah, this was reported shortly before the merge as Nightly-only, but landed afterwards. If QA could do the necessary verification that'd be great. I'm not sure the current preferred way to request that, so bouncing the NI back to Pascal.

Comment 15

[laszlo.bialis]

I verified this issue on Mac OS X 10.13, Windows 10 and Ubuntu 16.04 using FF Nightly 63.0a1(2018-07-16) and I can still reproduce  the issue. 
I was able to reproduce the issue using the link from the description with or without uBlock add-on, simply by refreshing the page with F5  multiple times or by clicking on the Reload page button multiple times. You can see a video here about what happened: https://youtu.be/XOErJDioSTk

Comment 16

[Bobby Holley (:bholley)]

(In reply to laszlo.bialis from comment #15)
> I verified this issue on Mac OS X 10.13, Windows 10 and Ubuntu 16.04 using
> FF Nightly 63.0a1(2018-07-16) and I can still reproduce  the issue. 
> I was able to reproduce the issue using the link from the description with
> or without uBlock add-on, simply by refreshing the page with F5  multiple
> times or by clicking on the Reload page button multiple times. You can see a
> video here about what happened: https://youtu.be/XOErJDioSTk

I see the same, but it only happens when I interrupt an existing load by refreshing. Not sure how much of a problem this is in practice.

Emilio, given what you understand about the pathology of this issue, does it make sense that we'd see this issue with an interrupted load?

Comment 17

[Emilio Cobos Álvarez (:emilio)]

(In reply to Bobby Holley (:bholley) from comment #16)
> (In reply to laszlo.bialis from comment #15)
> > I verified this issue on Mac OS X 10.13, Windows 10 and Ubuntu 16.04 using
> > FF Nightly 63.0a1(2018-07-16) and I can still reproduce  the issue. 
> > I was able to reproduce the issue using the link from the description with
> > or without uBlock add-on, simply by refreshing the page with F5  multiple
> > times or by clicking on the Reload page button multiple times. You can see a
> > video here about what happened: https://youtu.be/XOErJDioSTk
> 
> I see the same, but it only happens when I interrupt an existing load by
> refreshing. Not sure how much of a problem this is in practice.
> 
> Emilio, given what you understand about the pathology of this issue, does it
> make sense that we'd see this issue with an interrupted load?

Yes, as things stand, but I'm ~99% sure that's not a regression. Some force-stopped loads force a layout flush, and the layout flush causes the flash.

I remember having debugged the same issue in the past... I could figure out the stack if needed.

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Did you want to request uplift to beta?

Comment 19

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8987811 [details]
Bug 1471045: Don't flush layout if the ready promise is not resolved yet. r=heycam

Yeah, good point.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1466004
[User impact if declined]: FOUC
[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 0
[List of other uplifts needed for the feature/fix]: no
[Is the change risky?]: not risky
[Why is the change risky/not risky?]: Has been in nightly for a while, plus turns back to the behavior pre-bug 1466004 for the cases when it changes behavior.
[String changes made/needed]:none

Comment 20

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8987811 [details]
Bug 1471045: Don't flush layout if the ready promise is not resolved yet. r=heycam

Fix verified in nightly, for a recent regression in 62. 
Let's uplift for beta 10.

Comment 21

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/96f07d80f013

Comment 22

[ovidiu boca[:Ovidiu]]

I verified this issue on Windows 10, Mac OS X 10.12 and Ubuntu 16.04 with the latest FF beta 62.0b10 and I can' reproduce the issue form description. I will mark this as verified fixed.