Integration with preload #1342

noamr · 2021-10-26T09:19:06Z

Before any particular fetch steps are performed, see if there is a
matching request already in the preload store and consume it.

This is called from the main fetch to avoid race conditions.

Depends on whatwg/html#7260, and together they fix #590.

At least two implementers are interested (and none opposed):
- …
- …
[x ] Tests are written and can be reviewed and commented upon at:
- Add tests to verify preload caching behavior web-platform-tests/wpt#31539
Implementation bugs are filed:
- Chrome: …
- Firefox: https://bugzilla.mozilla.org/show_bug.cgi?id=1743764
- Safari: https://bugs.webkit.org/show_bug.cgi?id=233854

(See WHATWG Working Mode: Changes for more details.)

Preview | Diff

fetch.bs

noamr · 2021-11-04T09:27:42Z

@annevk I've revised this, whatwg/html#7260 would also need some changes to match, but I wanted to see that this is on the right track first.

The idea is that preloads are matched before the fetch becomes parallel and overrides all the HTTP specific check as it's assumed they were done for the preloaded request.

An alternative to doing things this way is to have the preload list inside fetch, and have the HTML spec add to it / clear it. This might make things easier in terms of future potential things like supporting Link headers in workers, but I'm not sure if that should be a concern right now (also doing that is not difficult inside the HTML spec).

annevk

Generally I think this can work, but it will depend a bit on the other details.

I'm also not sure where the cache itself should be defined. I can see arguments either way.

fetch.bs

annevk · 2021-11-04T09:47:53Z

Actually, I'm not sure. Doing a cache lookup synchronously seems bad, but perhaps that is what implementations do? That probably warrants a bit more investigation. One way to do it later in "main fetch" would be to check the value of recursive I suppose, although that might still run into problems with HTTP authentication requests.

noamr · 2021-11-04T10:15:43Z

Actually, I'm not sure. Doing a cache lookup synchronously seems bad, but perhaps that is what implementations do? That probably warrants a bit more investigation. One way to do it later in "main fetch" would be to check the value of recursive I suppose, although that might still run into problems with HTTP authentication requests.

Cache lookup should be synchronous IMO as it relies on something controlled by the main thread, making it parallel would create either locks or unnecessary races.

Maybe keeping it where I put it is correct?

noamr · 2021-11-07T12:15:54Z

See whatwg/html#7260 (comment)

Check that preloaded resources are reused/ignored based on the parameters available to a preload link (href, crossorigin, as), and do not rely on other parameters (e.g. whether a script is a module). See whatwg/html#7260 and whatwg/fetch#1342

fetch.bs

Check that preloaded resources are reused/ignored based on the parameters available to a preload link (href, crossorigin, as), and do not rely on other parameters (e.g. whether a script is a module). See whatwg/html#7260 and whatwg/fetch#1342

* Add tests to verify preload caching behavior Check that preloaded resources are reused/ignored based on the parameters available to a preload link (href, crossorigin, as), and do not rely on other parameters (e.g. whether a script is a module). See whatwg/html#7260 and whatwg/fetch#1342 * Add missing font * whitespace * Cleanup * PR nits * PR nits * More nits

annevk

Where does this end up invoking finalize response?

annevk · 2021-12-03T08:30:11Z

fetch.bs

+ <a for=request>header list</a>, if <var>header</var>'s <a for=header>name</a> is
+ not `<code>If-Modified-Since</code>`, `<code>If-None-Match</code>`, `<code>If-Unmodified-Since</code>`,
+ `<code>If-Match</code>`, `<code>User-Agent</code>`, or `<code>Cache-Control</code>`, set
+ <var>hasCustomHeaders</var> to true.


This would always end up with hasCustomHeaders being true.

How come?
Anyway, maybe we should avoid hasCustomHeaders at all, and Let fetch() set allow preloads to false if there are user-defined headers.

I had misread it, but it also seems the requirement is different now. Any author-defined headers triggers it now. But doesn't that also apply to XMLHttpRequest?

annevk · 2021-12-03T08:31:06Z

fetch.bs

+   <li><p><var>request</var>'s <a for=request>allow preloads</a> is true
+
+   <li><p><var>request</var>'s <a for=request>origin</a> is <var>request</var>'s
+   <a for=request>client</a>'s <a for="environment settings object">origin</a>


When would this not be true?

It's theoretically possible to call fetch with a different origin, the only place where I found this to happen is Process a navigate fetch which is not relevant for preloads. Maybe this can be an assert?

annevk · 2021-12-03T08:32:14Z

fetch.bs

+  <ol>
+   <li><p>Let <var>onPreloadedResponseAvailable</var> be to an algorithm that runs the following
+   step <a>in parallel</a> given <a for=/>response</a> <var>response</var>:
+   Run <a>fetch finale</a> given <var>response</var> and <var>fetchParams</var>.


Wrapping needs to be adjusted here. Run should be lowercase.

noamr · 2021-12-03T12:29:40Z

Where does this end up invoking finalize response?

Here. When the response is available, fetch finale is called and should run finalize response (once #1311 is in that part would be a lot cleaner)

annevk · 2021-12-07T12:13:22Z

fetch.bs

+   <a>fetch finale</a> given <var>response</var> and <var>fetchParams</var>.
+
+   <li><p>Let <var>foundPreloadedResource</var> be the result of invoking
+   <span>consume a preloaded resource</span> for <var>req</var>'s <a for=request>window</a>,


This should use <a>.

annevk · 2021-12-07T12:15:18Z

fetch.bs

+ <a for=request>header list</a>, if <var>header</var>'s <a for=header>name</a> is
+ not `<code>If-Modified-Since</code>`, `<code>If-None-Match</code>`, `<code>If-Unmodified-Since</code>`,
+ `<code>If-Match</code>`, `<code>User-Agent</code>`, or `<code>Cache-Control</code>`, set
+ <var>hasCustomHeaders</var> to true.


I had misread it, but it also seems the requirement is different now. Any author-defined headers triggers it now. But doesn't that also apply to XMLHttpRequest?

annevk

When would we expect to use "allow preloads"? If it's just XHR and fetch() I think we have enough state on request to make that determination already.

noamr · 2021-12-07T12:19:32Z

When would we expect to use "allow preloads"? If it's just XHR and fetch() I think we have enough state on request to make that determination already.

Custom headers currently should disallow preloads, but also things like "synchronous XHR", which I don't know if we have the knowledge for at this point.

annevk

To clarify, this depends on the other PR that merges everything into "fetch finale", right?

annevk · 2021-12-13T11:31:11Z

fetch.bs

+  <p>If all of the following conditions are true:</p>
+
+  <ul class=brief>
+   <li><p><var>request</var>'s <a for=request>destination</a> is not "<code>document</code>"


Should this use https://fetch.spec.whatwg.org/#navigation-request instead? The only overlap I can see with mode is for embed/object. Otherwise this would be redundant.

Actually this line is wrong. It's ok to preload a document.

fetch.bs

Co-authored-by: Anne van Kesteren <annevk@annevk.nl>

noamr · 2021-12-13T11:44:18Z

To clarify, this depends on the other PR that merges everything into "fetch finale", right?

I don't think it needs to be dependent. Fetch finale clarifies things but doesn't change the behavior of this.
Because the response is already fully loaded into a byte stream by the time it's returned from the "consume" algorithm (marking the response "done"), the current version of fetch finale would work just the same.

noamr · 2021-12-16T06:38:14Z

@annevk: another review?

…vior, a=testonly Automatic update from web-platform-tests Add tests to verify preload caching behavior (#31539) * Add tests to verify preload caching behavior Check that preloaded resources are reused/ignored based on the parameters available to a preload link (href, crossorigin, as), and do not rely on other parameters (e.g. whether a script is a module). See whatwg/html#7260 and whatwg/fetch#1342 * Add missing font * whitespace * Cleanup * PR nits * PR nits * More nits -- wpt-commits: ef427f550f2a2a803fb2fa0886951e559fc83d9c wpt-pr: 31539

noamr · 2022-01-17T10:19:53Z

@annevk: another review?

fetch.bs

yoavweiss

Non-authoritative LGTM

annevk

Do the changes I made look okay to you?

noamr · 2022-02-11T13:05:30Z

Do the changes I made look okay to you?

Sure!

annevk · 2022-02-11T13:10:04Z

@noamr where is the work tracked on the "memory cache"? To some extent it was tracked as part of the preload issue before.

noamr · 2022-02-11T13:17:37Z

@noamr where is the work tracked on the "memory cache"? To some extent it was tracked as part of the preload issue before.

What was tracked before is the relationship between preload and the memory cache. The way it is spec'ed now, the relationship with requests and preloads is simple - if the CORS-mode and URL match then the preloaded resource should be consumed, otherwise ignored (re-fetched).

On WebKit and Gecko this is not the case (because of the "memory cache" the CORS mode is ignored) and I submitted a WPT and opened relevant bugs.

I do believe the memory cache should be better defined, but it's not currently tracked.

annevk · 2022-02-11T13:41:44Z

Filed #1400.

annevk · 2022-02-11T13:42:36Z

@noamr could you link the bugs from OP?

noamr · 2022-02-11T13:45:57Z

@noamr could you link the bugs from OP?

Not sure what OP is.
https://bugzilla.mozilla.org/show_bug.cgi?id=1743764
https://bugs.webkit.org/show_bug.cgi?id=233854

annevk · 2022-02-11T13:52:24Z

It stands for original post(er) in a thread.

noamr changed the title ~~Attempt to reuse preloaded responses.~~ Integration with preload Oct 26, 2021

noamr mentioned this pull request Oct 26, 2021

Define the "Preload Cache" w3c/preload#97

Closed

annevk reviewed Nov 3, 2021

View reviewed changes

fetch.bs Outdated Show resolved Hide resolved

fetch.bs Outdated Show resolved Hide resolved

noamr force-pushed the preload branch from d6f7478 to 5c19df6 Compare November 4, 2021 09:23

annevk reviewed Nov 4, 2021

View reviewed changes

fetch.bs Outdated Show resolved Hide resolved

noamr force-pushed the preload branch from 46b1a53 to 1927245 Compare November 7, 2021 12:06

noamr mentioned this pull request Nov 7, 2021

Define behavior of <link rel=preload> in detail whatwg/html#7260

Merged

3 tasks

noamr mentioned this pull request Nov 8, 2021

Add tests to verify preload caching behavior web-platform-tests/wpt#31539

Merged

yoavweiss reviewed Nov 8, 2021

View reviewed changes

fetch.bs Outdated Show resolved Hide resolved

noamr force-pushed the preload branch from 1927245 to d345b8d Compare November 8, 2021 10:18

yoavweiss reviewed Nov 8, 2021

View reviewed changes

fetch.bs Outdated Show resolved Hide resolved

fetch.bs Outdated Show resolved Hide resolved

annevk reviewed Dec 3, 2021

View reviewed changes

annevk reviewed Dec 7, 2021

View reviewed changes

noamr added 5 commits December 7, 2021 15:05

Revise integrity

79fea95

nit

5ee9a1f

nit

22cd255

Algo wording

9f9fcd5

PR comments

899ea7f

noamr added 3 commits December 7, 2021 15:05

Fix link

89f941c

Use 'unsafe fetch'

fff4f40

Avoid preloads for navigation

ea87702

noamr force-pushed the preload branch from 9c73511 to ea87702 Compare December 7, 2021 13:10

Fix ref

50c4049

annevk reviewed Dec 13, 2021

View reviewed changes

Update fetch.bs

7e8ef35

Co-authored-by: Anne van Kesteren <annevk@annevk.nl>

Update fetch.bs

4c66e58

yoavweiss reviewed Feb 9, 2022

View reviewed changes

fetch.bs Show resolved Hide resolved

yoavweiss reviewed Feb 9, 2022

View reviewed changes

nits

77aa0d9

annevk approved these changes Feb 11, 2022

View reviewed changes

annevk merged commit 8ebf2fd into whatwg:main Feb 11, 2022

noamr deleted the preload branch February 11, 2022 13:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integration with preload #1342

Integration with preload #1342

noamr commented Oct 26, 2021 •

edited

Loading

noamr commented Nov 4, 2021

annevk left a comment

annevk commented Nov 4, 2021

noamr commented Nov 4, 2021

noamr commented Nov 7, 2021

annevk left a comment

annevk Dec 3, 2021

noamr Dec 3, 2021

annevk Dec 7, 2021

annevk Dec 3, 2021

noamr Dec 3, 2021

annevk Dec 3, 2021

noamr Dec 3, 2021

noamr commented Dec 3, 2021

annevk Dec 7, 2021

annevk Dec 7, 2021

annevk left a comment

noamr commented Dec 7, 2021

annevk left a comment

annevk Dec 13, 2021

noamr Dec 13, 2021

noamr commented Dec 13, 2021

noamr commented Dec 16, 2021

noamr commented Jan 17, 2022

yoavweiss left a comment

annevk left a comment

noamr commented Feb 11, 2022

annevk commented Feb 11, 2022

noamr commented Feb 11, 2022 •

edited

Loading

annevk commented Feb 11, 2022

annevk commented Feb 11, 2022

noamr commented Feb 11, 2022

annevk commented Feb 11, 2022

Integration with preload #1342

Integration with preload #1342

Conversation

noamr commented Oct 26, 2021 • edited Loading

noamr commented Nov 4, 2021

annevk left a comment

Choose a reason for hiding this comment

annevk commented Nov 4, 2021

noamr commented Nov 4, 2021

noamr commented Nov 7, 2021

annevk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noamr commented Dec 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annevk left a comment

Choose a reason for hiding this comment

noamr commented Dec 7, 2021

annevk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noamr commented Dec 13, 2021

noamr commented Dec 16, 2021

noamr commented Jan 17, 2022

yoavweiss left a comment

Choose a reason for hiding this comment

annevk left a comment

Choose a reason for hiding this comment

noamr commented Feb 11, 2022

annevk commented Feb 11, 2022

noamr commented Feb 11, 2022 • edited Loading

annevk commented Feb 11, 2022

annevk commented Feb 11, 2022

noamr commented Feb 11, 2022

annevk commented Feb 11, 2022

noamr commented Oct 26, 2021 •

edited

Loading

noamr commented Feb 11, 2022 •

edited

Loading