Clients.get: block on reserved clients. #1315
Conversation
| @@ -1148,6 +1148,11 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe | |||
| 1. Run these substeps <a>in parallel</a>: | |||
| 1. For each [=/service worker client=] |client| whose [=service worker client/origin=] is the <a lt="same origin">same</a> as the associated [=ServiceWorkerGlobalScope/service worker=]'s [=environment settings object/origin=]: | |||
| 1. If |client|'s [=environment/id=] is not |id|, continue to the next iteration of the loop. | |||
| 1. If |client|'s [=environment/execution ready flag=] is unset, then: | |||
| 1. Wait for the [=/HTTP fetch=] steps using the [=/request=] that |client| is the associated [=request/reserved client=] of to finish. | |||
There was a problem hiding this comment.
This seems a bit hand-wavey, since there isn't an explicit connection between a client & it's fetch.
Adding this connection would likely be really hard, since fetch doesn't really return an instance (this has been a problem elsewhere).
Feels like it would be easier to turn the execution ready flag into an enum, eg execution state, which could be "pending", "ready" or "failed". Then, this part of the spec would be explicit, as you'd be waiting for that enum to change to "failed" (in which case you'd reject and abort) or "ready" (in which case you'd continue).
There was a problem hiding this comment.
I agree that waiting for a state change is probably better. At least, it matches what I've anticipated implementing.
Note, I'm not sure we need a tri-state enum, though. I think we can just get by with an execution ready boolean. If the browser fails to mark a client as execution ready I believe it should simply destroy the client fairly quickly. So we can just listen for "client goes away before being marked execution ready".
There was a problem hiding this comment.
Ohh, if we have a standard way of describing "goes away", I agree that would fit nicely.
There was a problem hiding this comment.
Its probably not clearly defined for windows in the html spec. I think defining when to set this error state would be equivalent to defining discarding the client on failure, though.
There was a problem hiding this comment.
Note, we also already depend on client discarding in other places in the service worker spec. We activate waiting workers when the last client "goes away". If that last controlled client is a reserved client being navigated, then we have this same situation.
Not saying we shouldn't spec it better, but just highlighting that we already depend on this concept pretty heavily.
There was a problem hiding this comment.
Strangely, "terminating" links to "service worker termination", which doesn't really make sense... service workers are not service worker clients.
I didn't know about reserved clients going away on redirect. I thought the reserved client gets set one time in the HTML spec here https://html.spec.whatwg.org/#process-a-navigate-fetch before continuing on to the Fetch spec. The Fetch spec follows the redirects here https://fetch.spec.whatwg.org/#http-redirect-fetch. So I thought there's just one time the reserved client gets set, at the start of the navigation.
There was a problem hiding this comment.
Oh, it probably means to link to worker termination in the HTML spec.
There was a problem hiding this comment.
I think probably here "process a navigate response" https://html.spec.whatwg.org/multipage/browsing-the-web.html#process-a-navigate-response we can add something like "discard reservedEnvironment" and have the service worker spec wait for that.
There was a problem hiding this comment.
To clarify, the reserved client is not discarded on all redirects. Redirecting cross-origin, though, must result in a new client being reserved with the new origin to avoid violating same-origin restrictions. So at that point the old reserved client can be discarded.
There was a problem hiding this comment.
@wanderview Hum, that's not what I thought. I filed #1316
FYI: created whatwg/html#3723
This will allow specifications to do things like "wait for the environment to be destroyed" as needed in w3c/ServiceWorker#1315.
These are run when navigation or worker start fails, and will be used by w3c/ServiceWorker#1315.
|
I'm having trouble rebasing. I will make a new PR. |
|
Did we decide that we wanted to reject if the reserved client is discarded? The original PR has "NetworkError" there, but not sure that is quite right. We could also just resolve Edit: Nevermind. I see the new PR resolves |
|
Closing this in favor of #1323 (sorry about my git troubles) |
These are run when navigation or worker start fails, and will be used by w3c/ServiceWorker#1315.
@jakearchibald @jungkees Can you review this? The index.html looks huge because it includes the spec recompile commit.
Preview | Diff