1761242 - Generalize the EarlyHintPreloader to cover more assets

Status: RESOLVED FIXED

(Core :: Networking: HTTP, enhancement, P2)

Description

[Manuel Bucher [:manuel]]

Currently the EarlyHintPreloader from Bug 1753730 only covers images. This bug is about generalizing it to cover other assets from /dom/html/HTMLLinkElement.cpp#443-448

This will be done in two patches:

1. extract the parsing of HTMLLinkElement::ParseAsValue and expose it in nsNetUtil.h
2. use the ParseAsValue function in the EarlyHintPreloader and configure the channel depending on the asset type

Comment 1

[Manuel Bucher [:manuel]]

Attachment: Bug 1761242 - Make link asset parsing accessible in nsNetUtils.h r=#necko

Comment 2

[Manuel Bucher [:manuel]]

Attachment: Bug 1761242 - Generalize the EarlyHintPreloader to cover all assets intended to preload r=#necko

Depends on D141760

Comment 3

[Manuel Bucher [:manuel]]

Attachment: Bug 1761242 - Test early hint preloads for all implemented asset types r=#necko

Depends on D142201

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d034973e99ae
Make link asset parsing accessible in nsNetUtils.h r=necko-reviewers,dragana

Comment 5

[Manuel Bucher [:manuel]]

Parsing the script type from the Link header is still missing from the generic preloader (notes to self for later):

• module load
• normal load
• NewChannel (called in both modes)
• Difference in security flags

Comment 6

[Narcis Beleuzu [:NarcisB]]

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

Comment 7

[Manuel Bucher [:manuel]]

Attachment: Bug 1761242 - Expose computing security flags for early hint preloader r=#necko

Depends on D139740

Comment 8

[Manuel Bucher [:manuel]]

Question that came up when working on D139740. Anne, can you clear me up there?

Not sure if there is something incorrect here (or I don't understand the code yet):

• nsILoadInfo.idl#117-123: SEC_ALLOW_CROSS_ORIGIN_SEC_CONTEXT_IS_NULL is used to disable CORS checks
• FetchPreloader.cpp#88-91: FetchPreloader sets this flag on aCORSMode == CORS_NONE
• PreloadService.cpp#225, Element.cpp#3389-3392: CORS_NONE is set, when the preload doesn't specify a cors mode
• In my understanding Fetches should always sent CORS (preflight?) requests. So why are fetch preloads without CORS allowed (or what is it I'm not understanding yet)?

Comment 9

[Anne (:annevk)]

It is supposed to depend on whether or not the crossorigin attribute on the Link header is present. As I understand it Early Hints integration with the web platform is detailed in https://html.spec.whatwg.org/#early-hints (and there's a part in https://fetch.spec.whatwg.org/ for returning the 103 responses to the caller). Have you checked with those algorithms?

As I understand them, that can mean the request mode can end up being "no-cors" (in terms of Fetch), which is somewhat okay as the preloaded resource might be for an img element without a crossorigin attribute, which would also end up doing a "no-cors" request. Hope that helps.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4f5ed111093b
Expose computing security flags for early hint preloader r=ckerschb,smaug
https://hg.mozilla.org/integration/autoland/rev/7cda175b833d
Generalize the EarlyHintPreloader to cover all assets intended to preload r=necko-reviewers,ckerschb,dragana,kershaw
https://hg.mozilla.org/integration/autoland/rev/4a5e10110c6a
Test early hint preloads for all implemented asset types r=necko-reviewers,dragana,kershaw

Comment 11

[Sandor Molnar[:smolnar]]

Backed out 5 changesets (bug 1761242, bug 1744822, bug 1761252) for causing browser-chrome failures in netwerk/test/browser/browser_103_assets.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/d8e2d67cb443903c2fd609832b1395597ef843c1

Push with failures

Failure log

INFO - Buffered messages finished
[task 2022-06-08T15:51:36.858Z] 15:51:36     INFO - TEST-UNEXPECTED-FAIL | netwerk/test/browser/browser_103_assets.js | test_103_asset_normal (fetch): Unexpected amount of requests made - {"hinted":0,"normal":0} deepEqual {"hinted":0,"normal":1} - JS frame :: chrome://mochitests/content/browser/netwerk/test/browser/browser_103_assets.js :: test_hint_asset :: line 32
[task 2022-06-08T15:51:36.858Z] 15:51:36     INFO - Stack trace:
[task 2022-06-08T15:51:36.858Z] 15:51:36     INFO - chrome://mochitests/content/browser/netwerk/test/browser/browser_103_assets.js:test_hint_asset:32
[task 2022-06-08T15:51:36.859Z] 15:51:36     INFO - Leaving test bound test_103_asset_fetch

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/37b5dc69ab45
Expose computing security flags for early hint preloader r=ckerschb,smaug
https://hg.mozilla.org/integration/autoland/rev/968740080966
Generalize the EarlyHintPreloader to cover all assets intended to preload r=necko-reviewers,ckerschb,dragana,kershaw
https://hg.mozilla.org/integration/autoland/rev/c81eaa316306
Test early hint preloads for all implemented asset types r=necko-reviewers,dragana,kershaw

Comment 13

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/37b5dc69ab45
https://hg.mozilla.org/mozilla-central/rev/968740080966
https://hg.mozilla.org/mozilla-central/rev/c81eaa316306