1646776 - https://webxr.today fails to reload (because of <link rel=preload> not firing load events when in the stylesheet cache).

Status: RESOLVED FIXED

(Core :: DOM: Networking, defect, P2)

Description

[Randall Barker [:rbarker]]

Attachment: Broken page after reload.

STR:

1. Visit https://webxr.today
2. Reload the page

Expected
Page is reloaded

Actual:
Page is broken

Reloading the page worked in GV Nightly 79.0.20200615092624 and is broken in 79.0.20200616092900

Comment 1

[Randall Barker [:rbarker]]

This is a ship blocker for FxR.

Comment 2

[Mike Taylor [:miketaylr]]

Randall, does it reproduce in Desktop by chance?

Comment 3

[Randall Barker [:rbarker]]

Works -> https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=3ce9af6580b2748bd34837cb3d1aa074c6f76436&tochange=419a897d7e4b9489ad51b51044f154e5bcbb605e
Fails -> https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=ebf050bcdc162b30b45087f11a69c2ab6f8b090c&tochange=419a897d7e4b9489ad51b51044f154e5bcbb605e

Got bogus information from mozregression GUI.

Comment 4

[Mike Taylor [:miketaylr]]

ni? to take a look at the regression

Comment 5

[Lars T Hansen [:lth]]

Not very plausible that this was regressed by my patch, as that pertains to wasm SIMD only, but I'm investigating.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

There's both script and CSS involved, though off-hand it seems a bit more likely to be a regression from my changes. Randal, can you double-check the regression range?

Comment 7

[Emilio Cobos Álvarez (:emilio)]

There's a:

<link rel="preload" href="/css/index.css" as="style" onload="this.onload=null;this.rel='stylesheet'">

whose load event is not firing. That smells a lot like something my change could've plausibly caused :)

Comment 8

[Lars T Hansen [:lth]]

As far as I can tell, the page remains broken with both wasm SIMD disabled and the optimizing compiler disabled that was affected by that patch, and has no wasm content. So this looks increasingly unlikely to me to be the problem.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Moderately sure this is bug 1599160 then. I'll take a look.

Comment 10

[Randall Barker [:rbarker]]

I can run it again. I find the mozregression GUI UI to be confusing.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Attachment: Reduced test-case (should be green).

I think this exposes an existing issue with our preload implementation.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Attachment: Test-case that repros the issue before my patch.

Comment 13

[Randall Barker [:rbarker]]

Switching to cli for mozregression gave the expected regressing: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=3ce9af6580b2748bd34837cb3d1aa074c6f76436&tochange=419a897d7e4b9489ad51b51044f154e5bcbb605e

Sorry for tagging the wrong commit.

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Attachment: Test-case that shows that this is also an issue for images.

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9157741 [details]
Test-case that shows that this is also an issue for images.

Ah, nevermind, this is not true, I was not testing from http, so I was not preloading to begin with. It seems Chrome preloads from `file://` uris. Honza do you know what's up with that? Is that difference tracked anywhere?

Comment 16

[Emilio Cobos Álvarez (:emilio)]

So, the issue is that if we don't ever create a PreloaderBase for a given preload we'll fire an error event for that node... Though when we hit the stylesheet cache we don't create an StreamLoader.

Honza, we could do multiple things to fix this I think...

• Maybe we don't even need the PreloaderBase machinery for stylesheets, and we can just use the regular css::Loader machinery, which already can take care of firing load / error events for <link rel=stylesheet>... Though that changes the event we fire in the case of bogus @imports, to match what <link rel=stylesheet> does... Perhaps that's not bad?

• Otherwise, I think the thing that should inherit from PreloaderBase might want to be SheetLoadData, perhaps?

Comment 17

[Emilio Cobos Álvarez (:emilio)]

(Or even StyleSheet, but not a fan of bloating them even more)

Comment 18

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1646776 - Clean up HTMLLinkElement::CheckPreloadAttrs. r=smaug,mayhemer

This is just cleanup while I was going through this code.

Comment 19

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1646776 - Add some logging for sheet cache misses for the same URI. r=#style

This makes it easy to see why your test is not failing without your
patch, for example ;)

Note that we log only when the URIs differ, which I think is a
reasonable compromise in verbosity.

Depends on D80287

Comment 20

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1646776 - Make sure to fire load rather than error events for preload links that hit the CSS cache. r=mayhemer

When we hit the stylesheet cache for an existing stylesheet in a
different document:

• There's no existing preload in the PreloadService (because the sheet
  was loaded in another document).

• We don't create a PreloaderBase (because we don't trigger a load at
  all).

• And thus we believe the load has just errored, which is wrong...

Fix it by properly passing through the "already complete" status from
PreloadStyle.

Note that there's still another issue, which is that we'd still hit this
broken path if two stylesheets with the same URI are loading in
different documents and the CSS loader coalesces them.

For that, I think we'd have to make SheetLoadData the thing that
actually implements PreloaderBase. This is kind of an obscure case, and
SheetLoadData has a pretty complex lifetime already (keeps alive a whole
lot of stuff), so I'd prefer to do this in a separate bug.

Depends on D80288

Comment 21

[Emilio Cobos Álvarez (:emilio)]

Ok, so... Sent a necessary fix that fixes this. I'll spin off the PreloaderBase change, if Honza agrees, to another bug.

Comment 22

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1646776 - Don't set an URL for pending constructable stylesheet parses. r=nordzilla

This shouldn't have any behavior change, but it makes the code make a
bit more sense. Rather than counting inline stylesheets like a pending
load, we won't (but note that any @import inside it will).

The SheetLoadData::mURL it's supposed to be the url of the stylesheet,
so for StyleSheet::Replace it should be null.

Depends on D80289

Comment 23

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1646776 - Move the preloader for stylesheets to SheetLoadData rather than StreamLoader. r=mayhemer

So as to make sense in a world where we can coalesce loads across
documents. The per-load object is the SheetLoadData, so this way we
guarantee that we fire the right events in presence of the load
coalescing that the SharedStyleSheetCache does.

Depends on D80379

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6bcac6d68384
Clean up HTMLLinkElement::CheckPreloadAttrs. r=smaug
https://hg.mozilla.org/integration/autoland/rev/0be160e4f5ae
Add some logging for sheet cache misses for the same URI. r=heycam

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5855c8067f8d
Make sure to fire load rather than error events for preload links that hit the CSS cache. r=mayhemer

Comment 26

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/24291 for changes under testing/web-platform/tests

Comment 27

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 28

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/6bcac6d68384
https://hg.mozilla.org/mozilla-central/rev/0be160e4f5ae

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e847c2e2c84e
Don't set an URL for pending constructable stylesheet parses. r=heycam
https://hg.mozilla.org/integration/autoland/rev/be056dc9e0f1
Move the preloader for stylesheets to SheetLoadData rather than StreamLoader. r=mayhemer

Comment 30

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/5855c8067f8d

Comment 31

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 32

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/e847c2e2c84e
https://hg.mozilla.org/mozilla-central/rev/be056dc9e0f1