1956486 - Teach SharedStyleSheetCache (or so) to share inline styles

Status: RESOLVED FIXED

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

Description

[Emilio Cobos Álvarez [:emilio]]

See bug 1928800 for something that would benefit for this (but in general maybe we can do it regardless).

Tricky thing is that the base URI is different for different pages, so we need to check for that, or for the sheet not having relative URIs...

Comment 1

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1956486 - Teach SharedStyleSheetCache to share inline styles. r=#style,#layout

Comment 2

[Emilio Cobos Álvarez [:emilio]]

https://perf.compare/compare-results?baseRev=ac2723a193794288d5ad5108ee2777b28d283c9a&newRev=5f24f65a742c5c25ef2b10ed6e61db6663f34282&baseRepo=try&newRepo=try&framework=13

Comment 3

[Emilio Cobos Álvarez [:emilio]]

https://perf.compare/compare-results?baseRev=fc6f11416726ce152ae434ff6021a51f25e298bc&newRev=659fe897b615daf3f96b5791d4e3bf73dfdf0d54&baseRepo=try&newRepo=try&framework=13

(These are numbers without the base URI cache, assuming they're roughly comparable)

Comment 4

[Emilio Cobos Álvarez [:emilio]]

So I think we should do this because bug 1928800 and co look a lot better with this.

I found a few legit bugs that needed fixing are are easy to fix. I found one which is just rather annoying to fix tho, and it's (mostly) pre-existing.

The test is security/manager/ssl/tests/mochitest/mixedcontent/test_cssBefore1.html (well, and test_cssContent1.html, but basically the same). It tests that the mixed content flag show before and after navigation.

The issue is that now the sheet is cached, which means that we don't do the second image request on navigation. This is expected, but that doesn't update the UI state. The issue is already present there if you use a <link rel=stylesheet> rather than an inline stylesheet...

Tom, Freddy, how much do we care about mixed-content warnings nowadays? Should I dig into how to potentially fix this? It's.. not easy, because we somewhat aggressively cache these requests. I can of course do something hacky like "if there are http uris, don't put the sheet in the cache", but that's just lame.

Comment 5

[Tom Schuster]

I think with the work on HTTPS-First and Only we care about the mixed content blocker a lot less. If it's already broken for external stylesheets and blocking performance work I wouldn't bother.

Comment 6

[Emilio Cobos Álvarez [:emilio]]

Thanks.

Comment 7

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1956486 - Fix some devtools tests with inline stylesheet caching. r=#devtools-reviewers

Network override needs this already for <link>, the test was using
@import to bypass the usual caching.

browser_resources_css_messages.js needs to clear the cache as well now
that reloading might not reparse the stylesheet.

Comment 8

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1956486 - Annotate some mixed content tests as failing after navigation. r=tschuster,freddyb

As per the comments in the bug, this is already broken for <link
rel=stylesheet>. The issue is that we don't re-trigger the image request
now, as we cache the whole stylesheet.

An alternative would be to use a CSS variable or so to bypass the style
resource caching, lmk if you want me to do that instead.

Comment 9

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1956486 - Make reftest harness clear stylesheet cache on pref changes. r=#layout

Without this tests like layout/reftests/svg/as-image/context-fill-*.html
fail, because they test both pref-on and pref-off on the same
stylesheet.

Comment 10

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1956486 - Tweak a few tests to work regardless of whether inline stylesheets are cached. r=#style,#layout

Two tweaks:

• Use variables for tests that rely on background images triggering a
  load that had otherwise been cached already.
  printpreview_images_sw.html use serviceworkers, and
  visited_image_loading_frame.html uses a server script to watch the
  loads.

• Add a null-check for test_rules_out_of_sheets. Now we hit this early
  return in appendRule1. The no-sheet case was what this test was
  intending to hit anyways, so it was just by chance it didn't before
  (because the sheet was kept alive by Loader::mInlineSheets, but with
  the new cache we shallow-clone it).

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3a5f93fe4f7f
Fix some devtools tests with inline stylesheet caching. r=devtools-reviewers,jdescottes

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5220e96009bb
Make reftest harness clear stylesheet cache on pref changes. r=layout-reviewers,tnikkel

Comment 13

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/3a5f93fe4f7f
https://hg.mozilla.org/mozilla-central/rev/5220e96009bb

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9a66f0a0cfa9
Tweak a few tests to work regardless of whether inline stylesheets are cached. r=jwatt

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2d1fa8be32fd
Teach SharedStyleSheetCache to share inline styles. r=jwatt
https://hg.mozilla.org/integration/autoland/rev/8042121f2dce
Annotate some mixed content tests as failing after navigation. r=tschuster

Comment 16

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/9a66f0a0cfa9
https://hg.mozilla.org/mozilla-central/rev/2d1fa8be32fd
https://hg.mozilla.org/mozilla-central/rev/8042121f2dce

Comment 17

[Mayank Bansal]

1.5%-2% improvement on tp5o suite (much more noticeable on Linux):

17% on ifeng.com
14% on https://www.uol.com.br/
12.2% on homeway.com.cn

Comment 18

[Mayank Bansal]

Lead to a 4KB improvement on a image-memory subtet on AWSY