Fix request clients for prefetch + service workers #378
Conversation
Fixes #346. In particular: * Fixed prefetch activation to create a reserved client properly. Previously, the prefetch activation parts of the specification (in "create a navigation params from a prefetch record") would assume that the request they were being passed had a reserved client, when in reality they did not, because "create a navigation request" did not set a reserved client. We create a new reserved client before calling into "create navigation params from a prefetch record". This new reserved client exists for a short time before being cloned. * Properly creates a new reserved environment during prefetch activation, by cloning the reserved client, instead of reusing the same reserved client each time. This prevents prefetch record reuse from causing the creation of multiple clients with the same ID. * Makes the resultingClientId on the FetchEvent that occurs during prefetch be the empty string. The client ID is not known at the time of the prefetch, so this is the only possibility that makes sense.
| Preview: |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Some early brainstorming... Currently there is an invariant of "client's active service worker (if any) controls the client" in the spec. This proposal separates these two concepts, so I'm not sure whether this breaks (possibly implicit) assumptions/invariants in the spec and impl. @yoshisatoyanagisawa WDYT? |
This reverts commit d3f9152. We'll include this in the future.
| 1. Set |finalSandboxFlags| to the [=set/union=] of |targetSnapshotParams|'s [=target snapshot params/sandboxing flags=] and |responsePolicyContainer|'s [=policy container/CSP list=]'s [=CSP-derived sandboxing flags=]. | ||
| 1. Set |responseOrigin| to the result of [=determining the origin=] given |response|'s [=response/URL=], |finalSandboxFlags|, |documentState|'s [=document state/initiator origin=], and null. | ||
| 1. Set |responseOrigin| to the result of [=determining the origin=] given |redirectChainResponse|'s [=response/URL=], |finalSandboxFlags|, |documentState|'s [=document state/initiator origin=], and null. | ||
| 1. Set |activeServiceWorker| to |redirectChainRequest|'s [=request/reserved client=]'s [=environment/active service worker=]. |
There was a problem hiding this comment.
Previously, I was thinking of copying the reserved client creation process in
https://wicg.github.io/nav-speculation/prefetch.html#create-navigation-params-by-fetching
i.e. copying the Step 17.1 etc:
If request’s reserved client is not null and currentURL’s origin is not the same as request’s reserved client’s creation URL’s origin, then: ...
to make the reserved creation process consistent between navigation and prefetch.
But on the second thought, I'm starting feeling that we can do like (at Line 381):
Set |request|'s [=request/reserved client=]'s [=environment/active service worker=] to the last element of exchangeRecord's redirect chain's last element's request's reserved client's active service worker.
Because the result of the reserved creation process at prefetch (create-navigation-params-by-fetching) is already stored as the last request's reserved client and we don't re-run/emulate it.
Also
- This is behaviorally equivalent
- perhaps easier to read (not sure)
- This avoids exchange record's request points to the same request object across redirects #385.
There was a problem hiding this comment.
I like the current text of setting it each time through the loop as it treats activeServiceWorker the same as responsePolicyContainer, coopEnforcementResult, and finalSandboxFlags.
I don't understand how this avoids #385 since it's behaviorally equivalent...
So, let me keep this as-is for now.
| 1. Set |request|'s [=request/URL list=] to |urlList|. | ||
| 1. <span id="prefetch-activation-client-creation"></span>Set |request|'s [=request/reserved client=] to the result of [=creating a reserved client=] given |navigable| and |request|'s [=request/URL=]. |
There was a problem hiding this comment.
|request|'s [=request/URL=] here after Set |request|'s [=request/URL list=] to |urlList|. just above points to the last redirect leg's URL (in this PR last redirect leg's response URL per #386), and thus doesn't fix #350 -- it's just because the |request|'s [=request/URL=] reference timing is moved here in the recent commit.
But, if we reversing the order (i.e. doing this line BEFORE URL list update), we'll use the initial request URL, even it was redirected.
Maybe it's better to fix #350 separately.
There was a problem hiding this comment.
Ugh, you're right. That's frustrating. OK, let's fix it separately.
| 1. Set |request|'s [=request/URL list=] to |urlList|. | ||
| 1. <span id="prefetch-activation-client-creation"></span>Set |request|'s [=request/reserved client=] to the result of [=creating a reserved client=] given |navigable| and |request|'s [=request/URL=]. | ||
|
|
There was a problem hiding this comment.
Memo: also, the creation URL of the reserved client and the last request URL are:
- Not always the same in the spec (the creation URL is not updated on same-origin redirects?)
- Same in Chromium impl
And thus the creation URL can be mismatched between navigation and prefetch, even without #350 #386.
Anyway I'll investigate a little more and file spec issues, and let's fix that later.
|
Basically looks good. I found a couple of existing or related issues affecting this PR, but probably it's better to land this PR ignoring the other issues, and then fix the other issues if needed later. |
Fixes #346. In particular:
Fixed prefetch activation to use the correct reserved clients. Previously, the prefetch activation parts of the specification (in "create a navigation params from a prefetch record") would assume that the request they were being passed had a reserved client, when in reality they did not, because "create a navigation request" did not set a reserved client. We change them to consult the reserved clients from the redirect chain, which is more correct anyway.
Properly creates a new reserved environment during each prefetch activation, by creating a new environment with correct values, including the active service worker. This is necessary to avoid prefetch record reuse from causing the creation of multiple clients with the same ID.
Makes the resultingClientId on the FetchEvent that occurs during prefetch be the empty string. The client ID is not known at the time of the prefetch, so this is the only possibility that makes sense.
@hiroshige-g PTAL!