Closed Bug 1471045 Opened Last year Closed Last year

Vox Two stage rendering

Categories

(Core :: Layout, defect)

62 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla63
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- unaffected
firefox61 --- unaffected
firefox62 --- verified
firefox63 --- verified

People

(Reporter: ekr, Assigned: emilio)

References

()

Details

(Keywords: regression)

Attachments

(1 file)

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.
:jwatt does not accept ni now.
:heycam, can you look into this?
Flags: needinfo?(cam)
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?
Flags: needinfo?(ekr)
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)
On my linux environment, setting layout.css.parsing.parallel false solves this.
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.
Blocks: 1466004
Flags: needinfo?(emilio)
Assignee: nobody → emilio
Flags: needinfo?(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 ;)
Flags: needinfo?(cam)
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
Attachment #8987811 - Flags: review+
(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.
Flags: needinfo?(ekr)
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/mozilla-inbound/rev/85d78bc8bcd3
Don't flush layout if the ready promise is not resolved yet. r=heycam
https://hg.mozilla.org/mozilla-central/rev/85d78bc8bcd3
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Has Regression Range: --- → yes
Emilio, should we verify this on Nightly and uplift to beta?
Flags: needinfo?(emilio)
(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.
Flags: needinfo?(emilio) → needinfo?(pascalc)
Flags: needinfo?(pascalc) → qe-verify+
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
(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?
Flags: needinfo?(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.
Flags: needinfo?(emilio)
Did you want to request uplift to beta?
Flags: needinfo?(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
Flags: needinfo?(emilio)
Attachment #8987811 - Flags: approval-mozilla-beta?
Status: RESOLVED → VERIFIED
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.
Attachment #8987811 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.