1437921 - Run document_idle after both DOMContentLoaded _and_ layout has had a chance to start

Status: VERIFIED FIXED

(WebExtensions :: General, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

See bug 1437644 comment 9.

Comment 1

[Kris Maglione [:kmag]]

This is pretty easy to accomplish from the WebExtension side. From the DOM side, though, I think we'd need a new internal event that fires when stylesheet loads have finished, and possibly a new internal ready state property that we can check.

For reference, the relevant extension framework code is here:

https://searchfox.org/mozilla-central/rev/9d47b5fb14152865ad1375eb3ee2e571d81ecdb9/toolkit/components/extensions/ExtensionContent.jsm#360-370
https://searchfox.org/mozilla-central/rev/9d47b5fb14152865ad1375eb3ee2e571d81ecdb9/toolkit/components/extensions/ExtensionUtils.jsm#295-308

Comment 2

[Boris Zbarsky [:bzbarsky]]

Yep, thank you.  I found it.  I'll have proposed patches up soon.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Add an attribute on document that is a Promise that resolves when the document has fired DOMContentLoaded _and_ maybe started layout

MozReview-Commit-ID: LpPFPKlHTXe

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Switch document_idle to use the documentReadyForIdle promise

MozReview-Commit-ID: 9cJ1cGuMzBJ

Comment 5

[Kris Maglione [:kmag]]

Comment on attachment 8950739 [details] [diff] [review]
part 2.  Switch document_idle to use the documentReadyForIdle promise

Review of attachment 8950739 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/extensions/ExtensionContent.jsm
@@ +354,5 @@
>      try {
>        if (this.runAt === "document_end") {
>          await promiseDocumentReady(window.document);
>        } else if (this.runAt === "document_idle") {
> +        let readyThenIdle = promiseDocumentIdle(window);

Nit: Probably no need for the `readyThenIdle` variable at this point.

::: toolkit/components/extensions/ExtensionUtils.jsm
@@ +317,5 @@
> +  * idle time.
> +  *
> +  * @param {Window} window The window whose document we will await
> +                           the readiness of.
> +  * @returns {Promise<Document>}

This returns a Promise<IdleDeadline> now.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> Nit: Probably no need for the `readyThenIdle` variable at this point.
> This returns a Promise<IdleDeadline> now.

Both fixed.

Comment 7

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8950738 [details] [diff] [review]
part 1.  Add an attribute on document that is a Promise that resolves when the document has fired DOMContentLoaded _and_ maybe started layout

Review of attachment 8950738 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsDocument.cpp
@@ +6782,5 @@
> +{
> +  mMayStartLayout = aMayStartLayout;
> +  if (MayStartLayout()) {
> +    ReadyState state = GetReadyStateEnum();
> +    if (state == READYSTATE_INTERACTIVE || state == READYSTATE_COMPLETE) {

nit: I think it might be clearer if you write it as something like state >= READYSTATE_INTERACTIVE.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce6f309f8dd8
part 1.  Add an attribute on document that is a Promise that resolves when the document has fired DOMContentLoaded _and_ maybe started layout.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/5cfd2a4b70cc
part 2.  Switch document_idle to use the documentReadyForIdle promise.  r=kmag

Comment 9

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/ce6f309f8dd8
https://hg.mozilla.org/mozilla-central/rev/5cfd2a4b70cc

Comment 10

[marius.santa]

Is manual testing required on this bug? If Yes, please provide some STR and the proper webextension(if required), if No set the “qe-verify-“ flag.

Comment 11

[Boris Zbarsky [:bzbarsky]]

I actually have no idea how one would go about testing this manually.  One might be able to test this with some sort of custom extension, maybe, which checks that all <head> sheets are loaded by document_idle, but I'm not sure how to go about writing that.

Comment 12

[Kris Maglione [:kmag]]

Based on the bug this was trying to fix, I'd suggest creating an extension with a content script that runs on <all_urls> and touches `document.body.clientWidth`, and then checking that it does not trigger a flash of unstyled content.

This may require flushing the cache and visiting the site multiple times. I've heard that Github tends to experience this problem, so that might be a good testcase if you can reproduce it there without the patch.

Comment 13

[marius.santa]

Based on https://bugzilla.mozilla.org/show_bug.cgi?id=1437921#c12 if you think this scenario is valid, can you please provide an extension to verify this issue?

Comment 14

[Boris Zbarsky [:bzbarsky]]

I cannot, sorry.  I have way too many other things on my plate right now.

Comment 15

[marius.santa]

I tried testing the FOUC scenario described in https://bugzilla.mozilla.org/show_bug.cgi?id=1437644#c0 but was unable to reproduce the issue with (https://addons.mozilla.org/en-US/firefox/addon/imagus/versions/?page=1#version-0.9.8.59) before they added the hotfix on Firefox 60.0a1 (20180213220104) but it does not reproduce so I think it is not what was fixed by this bug.
I also looked in the .js files of the extension and they did not seem to contain `document.body.clientWidth`, does this need to be a specific scenario or can I look for any flash that might be caused by let's say an add blocker or any page? in order to try and reproduce the issue.

Comment 16

[Boris Zbarsky [:bzbarsky]]

> and they did not seem to contain `document.body.clientWidth`

Sure.  The actual code in Imagus that does the layout flush is described in bug 1437644 comment 1.  It's doing:

        d = d.compatMode === "BackCompat" && d.body || d.documentElement;
        var w = d.clientWidth;
        var h = d.clientHeight;

in client.js.

Also, the Imagus addon's problem would not be fixed by this bug, because it uses document_end, not document_idle.

> does this need to be a specific scenario

The simplest way to reproduce is:

1)  Have an addon that flushes layout on document_idle (e.g. by doing document.body.clientWidth).  Have this addon run the script on all URLs.

2)  Create a test page like so:

  <!DOCTYPE html>
  <head>
    <style>body { font-size: 100px; color: red; }</style>
    <link rel="stylesheet" href="http://software.hixie.ch/utilities/cgi/test-tools/delayed-file?pause=5&mime=text%2Fcss&text=body+%7B+color%3A+green+!important+%3B+%7D">
  </head>
  <body>There should be no red.</body>

and see whether you see red before it goes green after 5 seconds.

> or can I look for any flash that might be caused by let's say an add blocker or any page

Only if that's caused by a flush on document_idle.

Comment 17

[Kris Maglione [:kmag]]

Attachment: test-idle-layout-flush.xpi

Comment 18

[marius.santa]

Attachment: flush working.gif

I was able to reproduce the issue in Firefox 60.0a1 (20180213220104) on windows 10 64Bit.
Retested and verified in Firefox 60.0a1 (20180312100129) on Windows 10 64Bit, Mac OS 10.13.2.

Comment 19

[Kris Maglione [:kmag]]

(In reply to marius.santa from comment #18)
> I was able to reproduce the issue in Firefox 60.0a1 (20180213220104) on
> windows 10 64Bit.
> Retested and verified in Firefox 60.0a1 (20180312100129) on Windows 10
> 64Bit, Mac OS 10.13.2.

Thanks.