1386840 - CSS is renderblocking regardless of link media attribute

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P3)

Description

[Dominic Farolino [:domfarolino]]

This bug is the result of a small conversation with bz in #content after I first brought this up in #whatwg (freenode).

Some people teach that to minimize the amount of CSS that is in the critical rendering path (amt of renderblocking CSS) a technique is to use the `media` attribute on `<link rel=stylesheet ...>` to let the browser be smart about which CSS files it should block rendering on. As it stands, Chrome Canary and Safari only block rendering on the loading/processing of stylesheets loaded via link elements whose media attr value matches that of the current UA environment. In Firefox we block rendering regardless of this value when it appears this is not necessary.

An example of this can be seen by opening a new tab, viewing the network pane in the devtools and visiting `https://shocking-territory.glitch.me/` to see how many CSS resources (each taking longer to load than the previous) must be loaded and processed before the initial page render. As a result of Firefox's behavior, the initial page render is much slower than that of Canary + Safari when it probably shouldn't be.

As bz mentioned the HTML spec should probably be changed to indicate this (bug: https://github.com/whatwg/html/issues/2886) BUT this can also be valid behavior (probably, boris?) without a spec change given some hand-wavy wording etc.

Comment 1

[Jet Villegas (inactive)]

Sounds like a good thing to do, though I don't know if we have access to the media list early enough during stylesheet URI loading to do this. heycam: wdyt?

Comment 2

[Cameron McCormack (:heycam)]

Seems reasonable to me.  We do have access to the media list before we kick off the network load.  I'm not sure what the exact mechanism is that we use to make style sheet loads block rendering.  (I thought they only affected document load event dispatch, and I didn't think that could delay painting by this long.)  I'll take a look when I get a moment.

Comment 3

[Boris Zbarsky [:bzbarsky]]

I can describe that mechanism.

What stylesheets block is calls to PresShell::Initialize, and hence initial frame construction and hence layout.  That happens due to nsContentSink::StartLayout calling nsContentSink::WaitForPendingSheets, which checks nsContentSink::mPendingSheetCount.

mPendingSheetCount is manipulated by three things:

1) Calls nsStyleLinkElement::UpdateStyleSheet that set *aIsAlternate to false (implying that the sheet should block layout).

2) Calls to css::Loader::LoadStyleLink directly from nsContentSink::ProcessStyleLink that set *aIsAlternate to false.

3) nsContentSink::StyleSheetLoaded, which is a notification the CSS loader sends for things that it claimed were not alternate sheets.

So in practice, you will probably want to rename UpdateStyleSheet's aIsAlternate member to have the "is alternate or for an irrelevant medium" semantics, double-check that all the callers are OK with those semantics, then modify the CSS loader to set that boolean accordingly based on media.  Whether you also change these different-media sheets to have the lower-priority loading alternate sheets have is a bit less clear.  If you do, you'll just want to double-check that Loader::SetPreferredSheet handles them reasonably.

We really need to simplify the CSS loader.  :(

Comment 4

[Anne (:annevk)]

(Note that the HTML Standard since changed to explicitly allow this, thanks to OP.)

Comment 5

[Dominic Farolino [:domfarolino]]

Thanks for the description and all, bz. I should probably mention that I might no longer be able to get to this. I won't have enough time for probably a while if someone wanted to take it instead, else it'll be a while for me to commit the time to this. Thanks.

Comment 6

[Dominic Farolino [:domfarolino]]

Ah, I just realized this was not assigned to me; whoops. Looks like I just assumed it was. Sorry!

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1386840: Defer loading and don't block rendering for non-matching stylesheets.

Review commit: https://reviewboard.mozilla.org/r/239384/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/239384/

Comment 8

[Emilio Cobos Álvarez (:emilio)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d117cec05c8cbb0ab0e52592b177ce75af49198d

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Suggestions for testing this would be appreciated. I couldn't think of a way that wasn't racy...

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Also, note that this is a best-effort thing, not flushing layout at the point of the load, per the perf issues highlighted in bug 861870. I don't think it's a huge issue to have layout up-to-date at the point of the load, since you can conceivably change it while the load is going on, and you can't suddenly start blocking again, for example... But let me know if you think otherwise.

FWIW, the test-case I've used to verify this is working as intended is a simple slow.php:

  <?php header('Content-Type: text/css'); sleep(5); ?>

And a test.html page that looks like:

  <!doctype html>
  <script>let start = Date.now();</script>
  <link rel="stylesheet" href="slow.php" media="print">
  <div>Hey! Hopefully seeing me before 5 seconds</div>
  <script>document.write(Date.now() - start);</script>

Which you can test locally with:

  php -S localhost:8000

And browsing to localhost:8000/test.html

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8970640 [details]
Bug 1386840: Defer loading and don't block rendering for non-matching stylesheets.

https://reviewboard.mozilla.org/r/239384/#review245578

r=me with the "change all the constructors" issue fixed.

Thank you for picking this up!

::: layout/style/Loader.cpp:167
(Diff revision 1)
>    , mIsLoading(false)
>    , mIsBeingParsed(false)
>    , mIsCancelled(false)
>    , mMustNotify(false)
>    , mWasAlternate(aIsAlternate)
> +  , mMediaMatched(aMediaMatches)

You need to set mMediaMatched in the other two constructors too, right?

For that matter, should the @import case get deferred as needed too?  That one is complicated because it will keep the parent sheet from applying...

::: layout/style/Loader.cpp:2104
(Diff revision 1)
>                                            aObserver, principal, requestingNode);
>    NS_ADDREF(data);
>  
> -  // If we have to parse and it's an alternate non-inline, defer it
> +  auto result = LoadSheetResult { Completed::No, isAlternate, matched };
> +
> +  MOZ_ASSERT(!result.ShouldBlock() == data->ShouldDefer(),

For some reason I find "result.ShouldBlock() == !data->ShouldDefer()" more readable... either way, though.

::: layout/style/Loader.cpp:2112
(Diff revision 1)
> +  // If we have to parse and it's a non-blocking non-inline sheet, defer it.
>    if (aURL &&
>        state == eSheetNeedsParser &&
>        mSheets->mLoadingDatas.Count() != 0 &&
> -      isAlternate == IsAlternate::Yes) {
> +      !result.ShouldBlock()) {
>      LOG(("  Deferring alternate sheet load"));

Fix the log message?

Comment 12

[Boris Zbarsky [:bzbarsky]]

As far as testing goes, you could try something like this:

  <link id="test" rel="stylesheet" href="something" media="non-matching"></link>
  <script>
    var link = document.getElementById("test");
    // now either link.sheet should be null or link.sheet.cssRules should throw.
  </script>

This can still be racy in that the sheet load could complete before the </script> bit is reached...  If you really want to avoid that, you might be able to have an inline script document.write() the above snippet; pretty sure that should be reliable in Gecko's current setup.

Basically, the idea is to test whether script execution is blocking on the sheet load, which it shouldn't with these changes.

Comment 13

[Cameron McCormack (:heycam)]

It's a bit more work, but you could also write a httpd.js-based test, if you want more control of when the linked style sheet finishes loading:

https://developer.mozilla.org/en-US/docs/Mozilla/httpd.js/HTTP_server_for_unit_tests

e.g. like test_load_events_on_stylesheets.html uses slow_ok_sheet.sjs.

Comment 14

[Cameron McCormack (:heycam)]

Comment on attachment 8970640 [details]
Bug 1386840: Defer loading and don't block rendering for non-matching stylesheets.

https://reviewboard.mozilla.org/r/239384/#review245652

r=me but please add tests.

::: layout/style/Loader.cpp:148
(Diff revision 1)
>                               const nsAString& aTitle,
>                               nsIURI* aURI,
>                               StyleSheet* aSheet,
>                               nsIStyleSheetLinkingElement* aOwningElement,
>                               bool aIsAlternate,
> +                             bool aMediaMatches,

As in the previous bug, might be good to use the MediaMatches type here.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0741467f091
Defer loading and don't block rendering for non-matching stylesheets. r=bz,heycam

Comment 16

[Stefan Hindli [:stefan_hindli]]

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