Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 12 commits into from
Nov 30, 2021
Merged

Conversation

noamr
Copy link
Contributor

@noamr noamr commented Oct 26, 2021

A document keeps a list of preloaded resources, with a request and
response for each. A preloaded resource is a result of

When consumed (from the FETCH algorithm), the response is reused if the
request matches all relevant parameters, and removed from the store.

When the document is fully loaded ("load" event) the store is cleared.

See whatwg/fetch#590

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


/infrastructure.html ( diff )
/links.html ( diff )
/semantics.html ( diff )

noamr added a commit to noamr/fetch that referenced this pull request Oct 26, 2021
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
@noamr noamr changed the title Define storage of preloaded resources Define behavior of <link rel=preload> in detail Oct 26, 2021
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this!

One thing I'm unsure about is whether we should have separate preload/image/speculative parse caches or a single cache with more involved lookup semantics. Especially as we also need to deal with their interaction and we probably do not want to clone responses (and read their body) too many times.

source Show resolved Hide resolved
source Outdated

<p>There is no default type for resources given by the <code data-x="rel-preload">preload</code>
keyword.</p>

<p>A <code>Document</code> has a <dfn>list of preloaded resources</dfn> (a list of
<span>tuple</span>s, each consisting of a <span data-x="concept-request">request</span> and a
<span data-x="concept-response">response</span>), which is initially empty.</p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

xref list

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be helpful to spell out the implicit key somewhere as request is a bit larger than what you need, perhaps in a note.

This also doesn't match implementations, for which the key depends on the resource type. E.g., @emilio tells me Firefox currently uses (url, as, cors, origin, {sheet origin, script kind}) as per https://searchfox.org/mozilla-central/source/uriloader/preload/PreloadHashKey.h.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@annevk The script-kind/sheet-origin is different behavior from other browsers. Can you share the thought behind it?

Copy link
Contributor Author

@noamr noamr Oct 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From browsing the gecko code seems like this comes from the fact that the preload key is used for speculative HTML parsing - as currently there is no way to declare that a preload link is e.g. a module vs. classic script.

So I think this shouldn't go in the spec, and maybe this is a minor "bug", causing observable behavior that exposes browser-specific implementation detail.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed xreflist, and fixed to use an explicit set of parameters from the request that are definable in a link rel=preload element

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The style origin is needed because we don't want to share stylesheets that are loaded as user agent / user sheets (as those parse differently as if they weren't). This shouldn't be observable for preload's purposes since all <link rel=preload> are effectively author sheets.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@emilio how about script-kind? Doesn't that mean that when preloading scripts they would only be consumable by classic scripts and not by module imports or <script type=module>?

source Show resolved Hide resolved
source Show resolved Hide resolved
@noamr
Copy link
Contributor Author

noamr commented Oct 27, 2021

Thanks for working on this!

One thing I'm unsure about is whether we should have separate preload/image/speculative parse caches or a single cache with more involved lookup semantics. Especially as we also need to deal with their interaction and we probably do not want to clone responses (and read their body) too many times.

I wonder if this is observable behavior? Couldn't think of a desirable scenario where it is. Maybe a non-normative note?

I see the following observable and thus testable boundaries of preload:

  • preloaded responses should be reused and not cause extra fetches, even in case of errors
  • preloads should not break cache semantics beyond the first qualifying request

Implementations are free to optimize beyond that as long as it's not observable ...

@noamr
Copy link
Contributor Author

noamr commented Oct 27, 2021

Actually one intricate way where this is currently observable is the onload of the link element. It's unclear when it's supposed to fire if we're using a resource-type-specific cache. For images should it be fired only after the image is decoded? For scripts only after the script is executed? Hence the response cloning, which could be implemented in other more optimized ways of course (but guarantees the observable behavior of the link's load event)
.

Copy link
Contributor

@yoavweiss yoavweiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this!! One comment, and adding a few more folks I'd like to take a look at this.

source Show resolved Hide resolved
@noamr
Copy link
Contributor Author

noamr commented Oct 31, 2021

Updated a new version, following implementations more closely in terms of integrity and referrer handling, with some informative notes to explain some of the less-obvious decisions.

noamr added a commit to noamr/fetch that referenced this pull request Nov 7, 2021
At the very beginning of the fetch, while still in the main thread,
make a call to the HTML spec to see if the active document has a
preloaded entry that matches this fetch's request.

If such entry exists, we exit the regular fetch process and continue
consuming the response once it is available.

Together with whatwg/html#7260
@noamr
Copy link
Contributor Author

noamr commented Nov 7, 2021

New revision, together with whatwg/fetch#1342, addresses a lot of the issues after extensive research:

  • The map key for the preload entry is URL, destination (aka as), CORS mode and credentials mode.
  • Handling sync XHR etc. is done by having an allow preloads flag on a request, which would be set to false in those cases.
  • The preload map is populated synchronously when the link element is processed, and evicted synchronously when it matches an actual fetch. This makes explicit what happens when a consumer tries to access a preload entry that it not fully loaded yet.
  • Integrity matching is done both for the preload and for the consumer - this covers all the corner cases like having matching integrity in both but with different SHA algorithms. User-agents can make further optimizations without changing this observable behavior.
  • Behavior in case of removing a <link rel=preload element is not handled yet.

noamr added a commit to web-platform-tests/wpt that referenced this pull request Nov 8, 2021
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
noamr added a commit to web-platform-tests/wpt that referenced this pull request Nov 8, 2021
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
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
Copy link
Contributor

@yoavweiss yoavweiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-authoritative LGTM

@noamr
Copy link
Contributor Author

noamr commented Nov 20, 2021

@annevk, @domenic, this is ready for a round of reviews I believe.
It covers the interoperable parts of current preload behavior.

source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
A document keeps a list of preloaded resources, each with relevant
parameters from the request, and the response once available.

Once a <link rel=preload> element starts fetching a resource, that
entry is added, and once the response is fully loaded, the fetch
consuming the resource receives the response.

See whatwg/fetch#590
source Show resolved Hide resolved
@@ -14067,6 +14047,40 @@ interface <dfn interface>HTMLLinkElement</dfn> : <span>HTMLElement</span> {
</li>
</ol>

<p>The steps to <dfn>create a link element request</dfn> given <code>link</code> element
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"To create a link element request given a link element el:"

Also, <code> around link

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still should be "To" instead of "The steps to:"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only one remaining normative thing, about finalizing and reporting timing for error cases.

@@ -14067,6 +14047,40 @@ interface <dfn interface>HTMLLinkElement</dfn> : <span>HTMLElement</span> {
</li>
</ol>

<p>The steps to <dfn>create a link element request</dfn> given <code>link</code> element
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still should be "To" instead of "The steps to:"

source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
Copy link
Contributor Author

@noamr noamr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New revision following comments from @domenic.

source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
@@ -14067,6 +14047,40 @@ interface <dfn interface>HTMLLinkElement</dfn> : <span>HTMLElement</span> {
</li>
</ol>

<p>The steps to <dfn>create a link element request</dfn> given <code>link</code> element
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
@domenic
Copy link
Member

domenic commented Nov 30, 2021

I'll merge now, but it looks like web-platform-tests/wpt#31539 does not pass in all browsers. Can you be sure to file bugs on all of them and then edit the OP with links appropriately?

@domenic
Copy link
Member

domenic commented Nov 30, 2021

Also I noticed

When the document is fully loaded ("load" event) the store is cleared.

in the OP but that behavior doesn't appear to be in the spec. I assume this is because you did testing and found out it wasn't actually desired? If so, did you write tests to confirm?

@domenic domenic merged commit cfb20f5 into whatwg:main Nov 30, 2021
@noamr
Copy link
Contributor Author

noamr commented Dec 1, 2021

Also I noticed

When the document is fully loaded ("load" event) the store is cleared.

in the OP but that behavior doesn't appear to be in the spec. I assume this is because you did testing and found out it wasn't actually desired? If so, did you write tests to confirm?

Yes, it wasn't desired. I'll update the tests now that the spec is updated.

@noamr
Copy link
Contributor Author

noamr commented Dec 1, 2021

I'll merge now, but it looks like web-platform-tests/wpt#31539 does not pass in all browsers. Can you be sure to file bugs on all of them and then edit the OP with links appropriately?

Absolutely.

noamr added a commit to web-platform-tests/wpt that referenced this pull request Dec 1, 2021
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
noamr added a commit to web-platform-tests/wpt that referenced this pull request Dec 1, 2021
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
noamr added a commit to web-platform-tests/wpt that referenced this pull request Dec 1, 2021
* 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
Copy link
Member

annevk commented Dec 2, 2021

@noamr if you filed bugs, can you update OP with the links? Thanks!

@noamr
Copy link
Contributor Author

noamr commented Dec 2, 2021

@noamr if you filed bugs, can you update OP with the links? Thanks!

Done

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Dec 20, 2021
…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
annevk pushed a commit to whatwg/fetch that referenced this pull request Feb 11, 2022
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.

Tests: web-platform-tests/wpt#31539.
ericorth pushed a commit to ericorth/fetch that referenced this pull request Feb 18, 2022
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 whatwg#590.

Tests: web-platform-tests/wpt#31539.
@domfarolino
Copy link
Member

Also I noticed

When the document is fully loaded ("load" event) the store is cleared.

in the OP but that behavior doesn't appear to be in the spec. I assume this is because you did testing and found out it wasn't actually desired? If so, did you write tests to confirm?

Yes, it wasn't desired. I'll update the tests now that the spec is updated.

Can we also perhaps remove this line from the OP? I know that is pretty minor, but when I was perusing this I read that in the OP, and it took scrolling down here to realize that was obsolete.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

6 participants