759678 - Flash of unstyled content in Fennec in this case

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Martijn Wargers (dead)]

This is basically like bug 721523, but filed against Fennec.

See testcase, in Fennec the page looks red for a moment, which does not happen in Firefox desktop.
However, I'm also seeing it on Google Chrome beta on Android.

The stylestheet used in the testcase has to be of a certain size to get the bug reproduced (so I put just a lot of junk in it and the green background rule at the end)

When the meta viewport tag is removed:
    <meta name="viewport" content="width=device-width, user-scalable=no, maximum-scale=1">
I can't reproduce the bug anymore in Fennec (in Google Chrome beta on Android, I'm also seeing the bug there).

I guess this bug might occur because of the interaction with the meta viewport tag?

Comment 1

[Martijn Wargers (dead)]

Bug 721523 (and the duped bug 755865) are blocking-Fennec1.0+, so this bug should too.

Comment 2

[Brad Lassey [:blassey] (use needinfo?)]

AMO says this isn't reproducible in Fx12, can we confirm that? And get a regression range if so.

Comment 3

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Odd. Depends on a slow load whether you see red or green.  I did it with the phone network and it stayed red.  When I tried with wi-fi I got mixed red then turning to green.  If the page is in cache, it doesn't necessarily flash red at all.  (tested on Nightly, 5/31/2012 on Galaxy Nexus)

Comment 4

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Happens in Fennec 12 nightly native, 2012-01-03 http://hg.mozilla.org/mozilla-central/rev/44d992ccc97a

Happens in Fennec 11 aurora native, http://hg.mozilla.org/releases/mozilla-aurora/rev/e44cc659b759

Seems to work fine in Fennec 10 Nightly XUL, 11/02/2011 buildconfig : http://hg.mozilla.org/mozilla-central/rev/978002c0b0ad

Comment 5

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Regression range on Birch:

Works fine for Fennec 11 Birch Native 11/23/2011 ( http://hg.mozilla.org/projects/birch/rev/cd5725c23a13 )

Red shows in Fennec 11 Birch 11/24/2011 (http://hg.mozilla.org/projects/birch/rev/f8505fcb5808 )

See http://hg.mozilla.org/projects/birch/shortlog/f8505fcb5808

Comment 6

[Matt Brubeck (:mbrubeck)]

I suspect this change, which altered the content.sink.* preferences:
http://hg.mozilla.org/projects/birch/rev/98861dbb3277

We could try reverting those prefs in about:config.

Comment 7

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

In the nightly 5/31/2012, I changed the perfs to match the values in: 
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsContentSink.cpp#159

It made the red more visible.... it wasn't just a flash, it was red, pause, green.

Comment 8

[Boris Zbarsky [:bzbarsky]]

If you're seeing a flash, that means something is starting layout on the page before the stylesheet is done loading.

The most likely reason for that is the Fennec UI asking for layout information sometime during the load process before we've even started layout.

If someone who has a debug fennec build can reproduce this, a stack to PresShell::InitialReflow would probably give us at least some idea of what's triggering the layout start.

Comment 9

[Kartikaya Gupta (email:[e-mail])]

Attachment: Backtrace to InitialReflow

Here's the backtrace. The event being dispatched at the bottom of the stack is the onDOMMetaAdded event, and I assume the script that requests scrollWidth is the DOMMetaAdded listener in browser.js

Comment 10

[Boris Zbarsky [:bzbarsky]]

Yeah.  So.... don't do that.  ;)

We can expose a getter for the "layout started" state if that would be useful...

Comment 11

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch

This makes us skip processing DOMMetaAdded events that arrive between the onLocationChange (the start of a page load) and the before-first-paint event (when the page is about to get displayed). Note that the before-first-paint handler runs updateMetadata, so any DOMMetaAdded events during this period are effectively batch-processed once just before the page is displayed.

Comment 12

[Kartikaya Gupta (email:[e-mail])]

Landed on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce4fcd70f3c9

Note that without bug 763592 the "flash of unstyled content" will still be there when *reloading* the page, as that scenario is incorrectly detected as a same-document page load. Navigating to a page for the first time, or from some other page, should be fixed with just this patch.

Comment 13

[Graeme McCutcheon [:graememcc]]

https://hg.mozilla.org/mozilla-central/rev/ce4fcd70f3c9

Comment 14

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 631964 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: on pages with a meta viewport, an extra reflow is done during page load, which could result in rendering some half-baked document (because other information in the <head> may not have been processed yet). the extra reflow could also have a negative performance impact.
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): mobile-only, pretty low risk.
String or UUID changes made by this patch: none

note that this patch depends on the patch from bug 763592 to work properly on page reloads.

Comment 15

[Wil Clouser [:clouserw]]

(In reply to Kartikaya Gupta (:kats) from comment #14)
> Comment on attachment 631964 [details] [diff] [review]
> Patch
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): 
> User impact if declined: on pages with a meta viewport, an extra reflow is
> done during page load, which could result in rendering some half-baked
> document (because other information in the <head> may not have been
> processed yet). the extra reflow could also have a negative performance
> impact.
> Testing completed (on m-c, etc.): on m-c
> Risk to taking this patch (and alternatives if risky): mobile-only, pretty
> low risk.
> String or UUID changes made by this patch: none
> 
> note that this patch depends on the patch from bug 763592 to work properly
> on page reloads.

When you're considering taking this patch, you should know we have a very hacky workaround on AMO to make our pages load without the flicker which I almost r-'d and I'm still considering pulling if it starts using up developer time to deal with.  We definitely need this in ASAP if you want a good mobile add-ons experience.

Comment 16

[Johnathan Nightingale [:johnath]]

Comment on attachment 631964 [details] [diff] [review]
Patch

Discussed in triage with kats - approved for b7. Go go go!

Comment 17

[Kartikaya Gupta (email:[e-mail])]

https://hg.mozilla.org/releases/mozilla-aurora/rev/191465219cd6
https://hg.mozilla.org/releases/mozilla-beta/rev/6366dba32997

and on the b7 relbranch:
https://hg.mozilla.org/releases/mozilla-beta/rev/275b7159bf29

Comment 18

[Kevin Brosnan [Ex-Mozilla]]

verified on Firefox 14 beta 7 build 2

Comment 19

[Adrian Tamas (:AdrianT)]

The red background does not flash when loading/reloading the testcase page.

Verified fixed on:
Aurora 15.0a2 2012-07-10/Nightly 16.0a1 2012-07-10/Firefox Mobile 14.0b11
HTC Desire
Android 2.2.2