- From: Marijn Kruisselbrink <notifications@github.com>
- Date: Fri, 09 Feb 2024 16:14:27 -0800
- To: w3c/ServiceWorker <ServiceWorker@noreply.github.com>
- Cc: Subscribed <subscribed@noreply.github.com>
- Message-ID: <w3c/ServiceWorker/pull/1701/review/1873293827@github.com>
@mkruisselbrink requested changes on this pull request.
> + typedef (RouterSourceDict or RouterSourceEnum) RouterSource;
+
+ dictionary RouterSourceDict {
+ DOMString cacheName;
+ };
+
+ enum RunningStatus { "running", "not-running" };
+ enum RouterSourceEnum { "cache", "fetch-event", "network" };
+ </pre>
+
+ <section>
+ <h4 id="register-router-method">{{InstallEvent/addRoutes(rules)|event.addRoutes(rules)}}</h4>
+
+ {{InstallEvent/addRoutes(rules)}} registers this service worker the rules to offload simple tasks that the fetch handler does.
+
+ The <dfn method for="InstallEvent"><code>addRoutes(|rules|)</code></dfn> method steps are:
The WebIDL says this returns a promise, yet the algorithm here doesn't return anything?
> @@ -207,8 +207,12 @@ spec: storage; urlPrefix: https://storage.spec.whatwg.org/
A [=/service worker=] has an associated <dfn>all fetch listeners are empty flag</dfn>. It is initially unset.
+ A [=/service worker=] has an associated <dfn>list of router rules</dfn>. It is initially an empty [=list=].
Can you specify the type of this? (similar to how the definition of "set of used scripts" above mentions that 1) it is a set, and 2) the items of the set are URLs)
> A [=/service worker=] is said to be <dfn>running</dfn> if its [=event loop=] is running.
+ A [=/service worker=] has an associated <dfn>ServiceWorkerGlobalScope is ready</dfn>. It is initially unset.
I assume this is supposed to be a flag, so call it something like "ServiceWorkerGlobalScope is ready flag"
Although given the usage of this flag seems to be more specific to security checks, I wonder if a name that better reflects that would be possible. As is it isn't very clear how this differs from a `service worker/global object` being set.
> + : Input
+ :: |serviceWorker|, a [=/service worker=]
+ :: |forceBypassCache|, an optional boolean, false by default
+ : Output
+ :: a [=Completion=] or *failure*
+
+ Note: This algorithm blocks until the service worker is [=running=] or fails to start.
+
+ 1. If |serviceWorker| is [=running=], then return |serviceWorker|'s [=start status=].
+ 1. If |serviceWorker|'s [=service worker/state=] is "`redundant`", then return *failure*.
+ 1. Assert: |serviceWorker|'s [=start status=] is null.
+ 1. Let |script| be |serviceWorker|'s [=service worker/script resource=].
+ 1. Assert: |script| is not null.
+ 1. Let |startFailed| be false.
+ 1. If |serviceWorker|'s [=ServiceWorkerGlobalScope is ready=] is not set:
+ 1. If run the [=Setup ServiceWorkerGlobalScope=] algorithm with |serviceWorker| returns false, then return *failure*.
I don't think this algorithm as currently written is correct.
Previously (assuming starting with a running worker) after calling Terminate Service Worker we'd reset "start status" to null and cause `|serviveWorker|` to not be running. At that point calling Run Service Worker again would cause a new global object to be created. But with your changes, as far as I can tell nothing resets the "ServiceWorkerGlobalScope is ready" flag, so the algorithm here would skip calling "Setup ServiceWorkerGlobalScope" and end up trying to reuse the already terminated global object.
>
- Note: This algorithm blocks until the service worker is [=running=] or fails to start.
+ Note: This algorithm returns true if |serviceWorker| has a {{ServiceWorkerGlobalScope}} usable for a CSP check.
Does this algorithm actually has to modify anything on the passed in `|serviceWorker|`, or would it be possible to instead return a ServiceWorkerGlobalScope or "failure"? Since this entire algorithm is merely a way to write spec language what in an implementation won't actually cause a ServiceWorkerGlobalScope to be created, it seems we could avoid storing it in `|serviceWorker|` in the case where we're not actually starting a service worker. That way we don't have to worry about half-initialized service workers in the spec, and don't need the "ServiceWorkerGlobalScope is ready" flag at all.
The fetch algorithm might need some minor tweaks to either get the global object from the running worker or create a new object (or maybe just always create a new object? It's all spec fiction that just has to behave the way we want to behave, but performance as written doesn't matter).
>
Note: From this point, the [=/service worker client=] starts to <a>use</a> its <a>active service worker</a>'s <a>containing service worker registration</a>.
1. Else if |request| is a <a>subresource request</a>, then:
1. If |client|'s <a>active service worker</a> is non-null, set |registration| to |client|'s <a>active service worker</a>'s <a>containing service worker registration</a>.
1. Else, return null.
1. Let |activeWorker| be |registration|'s <a>active worker</a>.
+ 1. If |activeWorker|'s [=service worker/list of router rules=] is [=list/is not empty=]:
+ 1. Let |source| be the result of running [=Get Router Source=] algorithm with |registration|'s <a>active worker</a> and |request|.
nit:
```suggestion
1. Let |source| be the result of running [=Get Router Source=] with |registration|'s <a>active worker</a> and |request|.
```
>
Note: From this point, the [=/service worker client=] starts to <a>use</a> its <a>active service worker</a>'s <a>containing service worker registration</a>.
1. Else if |request| is a <a>subresource request</a>, then:
1. If |client|'s <a>active service worker</a> is non-null, set |registration| to |client|'s <a>active service worker</a>'s <a>containing service worker registration</a>.
1. Else, return null.
1. Let |activeWorker| be |registration|'s <a>active worker</a>.
+ 1. If |activeWorker|'s [=service worker/list of router rules=] is [=list/is not empty=]:
+ 1. Let |source| be the result of running [=Get Router Source=] algorithm with |registration|'s <a>active worker</a> and |request|.
+ 1. If |source| is {{RouterSourceEnum/"network"}}, return null.
+ 1. Else if |source| is {{RouterSourceEnum/"cache"}}, or |source|["{{RouterSourceDict/cacheName}}"] [=map/exists=], then:
+ 1. [=map/For each=] |cacheName| → |cache| of the [=relevant name to cache map=]:
+ 1. If |source|["{{RouterSourceDict/cacheName}}"] [=map/exists=] and |source|["{{RouterSourceDict/cacheName}}"] does not match |cacheName|, [=continue=].
what does "match" mean in this case? Do you mean https://infra.spec.whatwg.org/#string-is? Or some other string equivalency relationship?
>
Note: From this point, the [=/service worker client=] starts to <a>use</a> its <a>active service worker</a>'s <a>containing service worker registration</a>.
1. Else if |request| is a <a>subresource request</a>, then:
1. If |client|'s <a>active service worker</a> is non-null, set |registration| to |client|'s <a>active service worker</a>'s <a>containing service worker registration</a>.
1. Else, return null.
1. Let |activeWorker| be |registration|'s <a>active worker</a>.
+ 1. If |activeWorker|'s [=service worker/list of router rules=] is [=list/is not empty=]:
+ 1. Let |source| be the result of running [=Get Router Source=] algorithm with |registration|'s <a>active worker</a> and |request|.
+ 1. If |source| is {{RouterSourceEnum/"network"}}, return null.
+ 1. Else if |source| is {{RouterSourceEnum/"cache"}}, or |source|["{{RouterSourceDict/cacheName}}"] [=map/exists=], then:
+ 1. [=map/For each=] |cacheName| → |cache| of the [=relevant name to cache map=]:
"relevant name to cache map" is defined in terms of "this's associated global object". In every existing usage "this" is a CacheStorage object so that is well defined. But here there is no "this", so also no "relevant name to cache map". I think you want the name to cache map associated with `registration`s storage key?
> + 1. [=map/For each=] |cacheName| → |cache| of the [=relevant name to cache map=]:
+ 1. If |source|["{{RouterSourceDict/cacheName}}"] [=map/exists=] and |source|["{{RouterSourceDict/cacheName}}"] does not match |cacheName|, [=continue=].
+ 1. Let |requestResponses| be the result of running [=Query Cache=] with |request|, a new {{CacheQueryOptions}}, and |cache|.
+ 1. If |requestResponses| is an empty [=list=], return null.
+ 1. Else:
+ 1. Let |requestResponse| be the first element of |requestResponses|.
+ 1. Let |response| be |requestResponse|'s response.
+ 1. If |activeWorker| is not [=running=] or |activeWorker|'s [=ServiceWorkerGlobalScope is ready=] is not set:
+ 1. If the result of running the [=Setup ServiceWorkerGlobalScope=] algorithm with |activeWorker| is false, then return null.
+
+ Note: If this step succeeds, then |activeWorker|'s [=relevant settings object=] is now ready to use.
+
+ 1. Let |settingsObject| be |activeWorker|'s [=relevant settings object=].
+ 1. If |response|'s [=response/type=] is "`opaque`", and [=cross-origin resource policy check=] with |settingsObject|'s [=environment settings object/origin=], |settingsObject|, "", and |response|'s [=filtered response/internal response=] returns <b>blocked</b>, then return null.
+ 1. Return |response|.
+ 1. Return null.
nit: maybe add an explicit `Assert: |source| is "fetch-event"` in between here
> + 1. If |source|["{{RouterSourceDict/cacheName}}"] [=map/exists=] and |source|["{{RouterSourceDict/cacheName}}"] does not match |cacheName|, [=continue=].
+ 1. Let |requestResponses| be the result of running [=Query Cache=] with |request|, a new {{CacheQueryOptions}}, and |cache|.
+ 1. If |requestResponses| is an empty [=list=], return null.
+ 1. Else:
+ 1. Let |requestResponse| be the first element of |requestResponses|.
+ 1. Let |response| be |requestResponse|'s response.
+ 1. If |activeWorker| is not [=running=] or |activeWorker|'s [=ServiceWorkerGlobalScope is ready=] is not set:
+ 1. If the result of running the [=Setup ServiceWorkerGlobalScope=] algorithm with |activeWorker| is false, then return null.
+
+ Note: If this step succeeds, then |activeWorker|'s [=relevant settings object=] is now ready to use.
+
+ 1. Let |settingsObject| be |activeWorker|'s [=relevant settings object=].
+ 1. If |response|'s [=response/type=] is "`opaque`", and [=cross-origin resource policy check=] with |settingsObject|'s [=environment settings object/origin=], |settingsObject|, "", and |response|'s [=filtered response/internal response=] returns <b>blocked</b>, then return null.
+ 1. Return |response|.
+ 1. Return null.
+ 1. If |request| is a <a>non-subresource request</a>, |request| is a [=navigation request=], |registration|'s [=navigation preload enabled flag=] is set, |request|'s [=request/method=] is \`<code>GET</code>\`, |registration|'s [=active worker=]'s [=set of event types to handle=] [=set/contains=] <code>fetch</code>, and |registration|'s [=active worker=]'s [=all fetch listeners are empty flag=] is not set then:
why the addition of the "non-subresource request" check? "navigation request" is a strict subset of those already, so the extra check doesn't seem like it makes any difference.
> + 1. Let |preloadResponseObject| be a new {{Response}} object associated with a new {{Headers}} object whose [=guard=] is "`immutable`".
+ 1. [=header list/Append=] to |preloadRequestHeaders| a new [=header=] whose [=header/name=] is \`<code>Service-Worker-Navigation-Preload</code>\` and [=header/value=] is |registration|'s [=navigation preload header value=].
+ 1. Set |preloadRequest|'s [=service-workers mode=] to "`none`".
+ 1. Let |preloadFetchController| be null.
+ 1. Run the following substeps [=in parallel=], but [=abort when=] |controller|'s [=fetch controller/state=] is "<code>terminated</code>" or "<code>aborted</code>":
+ 1. Set |preloadFetchController| to the result of [=Fetch|fetching=] |preloadRequest|.
+
+ To [=fetch/processResponse=] for |navigationPreloadResponse|, run these substeps:
+
+ 1. If |navigationPreloadResponse|'s [=response/type=] is "`error`", reject |preloadResponse| with a `TypeError` and terminate these substeps.
+ 1. Associate |preloadResponseObject| with |navigationPreloadResponse|.
+ 1. Resolve |preloadResponse| with |preloadResponseObject|.
+ 1. [=If aborted=], then:
+ 1. Let |deserializedError| be the result of [=deserialize a serialized abort reason=] given null and |workerRealm|.
+ 1. [=fetch controller/Abort=] |preloadFetchController| with |deserializedError|.
+ 1. Else, resolve |preloadResponse| with undefined.
1. Let |shouldSoftUpdate| be true if any of the following are true, and false otherwise:
Should shouldSoftUpdate ever be true when router rules are used? Or do we want to skip triggering Soft Update for stale registrations if router rules are in play? (I believe as currently written you'd return before getting here and thus not call Soft Update)
> + 1. For each |orCondition| of |orConditions|:
+ 1. If running [=Match Router Condition=] algorithm with |orCondition|, |serviceWorker| and |request| returns true, then return true.
+ 1. Return false.
+ 1. Return true.
+ </section>
+
+ <section algorithm>
+ <h3 id="get-router-source-algorithm"><dfn>Get Router Source</dfn></h3>
+ : Input
+ :: |serviceWorker|, a [=/service worker=]
+ :: |request|, a [=/request=]
+ : Output
+ :: {{RouterSource}} or null
+
+ 1. [=list/For each=] |rule| of |serviceWorker|'s [=service worker/list of router rules=]:
+ 1. If running [=Match Router Condition=] with |rule|["{{RouterRule/condition}}"], |serviceWorker| and |request| returns false, [=continue=].
personally I'd swap around the condition, so rather than `if (!condition) { continue; } else { return; }` it would just be `if (condition) { return; }`
> + 1. For each |orCondition| of |orConditions|:
+ 1. If running [=Verify Router Condition=] algorithm with |orCondition| and |serviceWorker| returns false, return false.
+ 1. Set |hasCondition| to true.
+ 1. Return |hasCondition|.
+ </section>
+
+ <section algorithm>
+ <h3 id="match-router-condition-algorithm"><dfn>Match Router Condition</dfn></h3>
+ : Input
+ :: |condition|, a {{RouterCondition}}
+ :: |serviceWorker|, a [=/service worker=]
+ :: |request|, a [=/request=]
+ : Output
+ :: a boolean
+
+ Note: if there are multiple conditions (e.g. `urlPattern`, `runningStatus`, and `requestMethod` are set), all conditions will be matched to return true.
I find this phrasing a bit confusing. Maybe something like:
```suggestion
Note: if there are multiple conditions (e.g. `urlPattern`, `runningStatus`, and `requestMethod` are set), all conditions need to match for true to be returned.
```
> @@ -3175,6 +3283,103 @@ spec: storage; urlPrefix: https://storage.spec.whatwg.org/
1. Return |response|.
</section>
+ <section algorithm>
+ <h3 id="parse-urlpattern-algorithm"><dfn>Parse URL Pattern</dfn></h3>
+ : Input
+ :: |rawPattern|, a {{URLPatternCompatible}}
+ :: |serviceWorker|, a [=/service worker=]
+ : Output
+ :: {{URLPattern}}
+
+ 1. Let |baseURL| be |serviceWorker|'s [=service worker/script url=].
+ 1. Return the result of [=building a URLPattern from a Web IDL value=] |rawPattern| given |baseURL| and |serviceWorker|'s [=service worker/global object=]'s [=relevant realm=].
This isn't always called from a place where service worker's global object is initialized (let alone from a context where it is okay/possible for that algorithm to actually create a URLPattern instance, i.e. from within the Handle Fetch algorithm). I see URL Pattern has "Ideally we wouldn’t need a realm here. If we extricate the URL pattern concept from the [URLPattern](https://urlpattern.spec.whatwg.org/#urlpattern) interface, we won’t anymore." mentioned, so I guess at the moment it is actually impossible to properly spec the service worker behavior. Is there an existing URL Pattern issue you can link to to at least call out these problems?
> +
+ <section algorithm>
+ <h3 id="match-router-condition-algorithm"><dfn>Match Router Condition</dfn></h3>
+ : Input
+ :: |condition|, a {{RouterCondition}}
+ :: |serviceWorker|, a [=/service worker=]
+ :: |request|, a [=/request=]
+ : Output
+ :: a boolean
+
+ Note: if there are multiple conditions (e.g. `urlPattern`, `runningStatus`, and `requestMethod` are set), all conditions will be matched to return true.
+
+ 1. If |condition|["{{RouterCondition/urlPattern}}"] [=map/exists=], then:
+ 1. Let |rawPattern| be |condition|["{{RouterCondition/urlPattern}}"].
+ 1. Let |pattern| be the result of running <a>Parse URL Pattern</a> algorithm passing |rawPattern| and |serviceWorker|.
+ 1. If running [=match=] with |pattern| and |request|'s [=request/URL=] returns null, return false.
request's URL is a URL, but match wants a USVString or a URLPatternInit. You probably need some kind of conversion? (and this might also be another gap in the URL Pattern spec).
> + 1. Let |pattern| be the result of running <a>Parse URL Pattern</a> algorithm passing |rawPattern| and |serviceWorker|.
+ 1. If running [=match=] with |pattern| and |request|'s [=request/URL=] returns null, return false.
+ 1. If |condition|["{{RouterCondition/requestMethod}}"] [=map/exists=], then:
+ 1. Let |method| be |condition|["{{RouterCondition/requestMethod}}"].
+ 1. If |request|'s [=request/method=] is not |method|, return false.
+ 1. If |condition|["{{RouterCondition/requestMode}}"] [=map/exists=], then:
+ 1. Let |mode| be |condition|["{{RouterCondition/requestMode}}"].
+ 1. If |request|'s [=request/mode=] is not |mode|, return false.
+ 1. If |condition|["{{RouterCondition/requestDestination}}"] [=map/exists=], then:
+ 1. Let |destination| be |condition|["{{RouterCondition/requestDestination}}"].
+ 1. If |request|'s [=request/destination=] is not |destination|, return false.
+ 1. If |condition|["{{RouterCondition/runningStatus}}"] [=map/exists=], then:
+ 1. Let |runningStatus| be |condition|["{{RouterCondition/runningStatus}}"].
+ 1. If |runningStatus| is {{RunningStatus/"running"}}, and |serviceWorker| is not [=running=], return false.
+ 1. If |runningStatus| is {{RunningStatus/"not-running"}}, and |serviceWorker| is [=running=], return false.
+ 1. If |condition|["{{RouterCondition/_or}}"] [=map/exists=], then:
nit: (throughout) I think RouterCondition/or is the "proper" way of referring to the field. The _ is only needed in the actual webIDL snippets, but otherwise the identifier is called "or"
> + 1. Let |pattern| be the result of running <a>Parse URL Pattern</a> algorithm passing |rawPattern| and |serviceWorker|.
+ 1. If running [=match=] with |pattern| and |request|'s [=request/URL=] returns null, return false.
+ 1. If |condition|["{{RouterCondition/requestMethod}}"] [=map/exists=], then:
+ 1. Let |method| be |condition|["{{RouterCondition/requestMethod}}"].
+ 1. If |request|'s [=request/method=] is not |method|, return false.
+ 1. If |condition|["{{RouterCondition/requestMode}}"] [=map/exists=], then:
+ 1. Let |mode| be |condition|["{{RouterCondition/requestMode}}"].
+ 1. If |request|'s [=request/mode=] is not |mode|, return false.
+ 1. If |condition|["{{RouterCondition/requestDestination}}"] [=map/exists=], then:
+ 1. Let |destination| be |condition|["{{RouterCondition/requestDestination}}"].
+ 1. If |request|'s [=request/destination=] is not |destination|, return false.
+ 1. If |condition|["{{RouterCondition/runningStatus}}"] [=map/exists=], then:
+ 1. Let |runningStatus| be |condition|["{{RouterCondition/runningStatus}}"].
+ 1. If |runningStatus| is {{RunningStatus/"running"}}, and |serviceWorker| is not [=running=], return false.
+ 1. If |runningStatus| is {{RunningStatus/"not-running"}}, and |serviceWorker| is [=running=], return false.
+ 1. If |condition|["{{RouterCondition/_or}}"] [=map/exists=], then:
I think the algorithm could be clearer (without having to understand the validation logic as well) if this was structured as:
1. If "or" exists:
1. Do or stuff
2. Else:
1. Other condition 1
2. Other condition 2
3. Etc
> + 1. If |request|'s [=request/method=] is not |method|, return false.
+ 1. If |condition|["{{RouterCondition/requestMode}}"] [=map/exists=], then:
+ 1. Let |mode| be |condition|["{{RouterCondition/requestMode}}"].
+ 1. If |request|'s [=request/mode=] is not |mode|, return false.
+ 1. If |condition|["{{RouterCondition/requestDestination}}"] [=map/exists=], then:
+ 1. Let |destination| be |condition|["{{RouterCondition/requestDestination}}"].
+ 1. If |request|'s [=request/destination=] is not |destination|, return false.
+ 1. If |condition|["{{RouterCondition/runningStatus}}"] [=map/exists=], then:
+ 1. Let |runningStatus| be |condition|["{{RouterCondition/runningStatus}}"].
+ 1. If |runningStatus| is {{RunningStatus/"running"}}, and |serviceWorker| is not [=running=], return false.
+ 1. If |runningStatus| is {{RunningStatus/"not-running"}}, and |serviceWorker| is [=running=], return false.
+ 1. If |condition|["{{RouterCondition/_or}}"] [=map/exists=], then:
+ 1. Let |orConditions| be |condition|["{{RouterCondition/_or}}"].
+ 1. For each |orCondition| of |orConditions|:
+ 1. If running [=Match Router Condition=] algorithm with |orCondition|, |serviceWorker| and |request| returns true, then return true.
+ 1. Return false.
since nothing (afaict) requires orConditions to be non-empty you probably want to handle the empty case separately? I assume that case is supposed to return true rather than false?
>
Note: From this point, the [=/service worker client=] starts to <a>use</a> its <a>active service worker</a>'s <a>containing service worker registration</a>.
1. Else if |request| is a <a>subresource request</a>, then:
1. If |client|'s <a>active service worker</a> is non-null, set |registration| to |client|'s <a>active service worker</a>'s <a>containing service worker registration</a>.
1. Else, return null.
1. Let |activeWorker| be |registration|'s <a>active worker</a>.
+ 1. If |activeWorker|'s [=service worker/list of router rules=] is [=list/is not empty=]:
+ 1. Let |source| be the result of running [=Get Router Source=] algorithm with |registration|'s <a>active worker</a> and |request|.
(or "the [=Get Router Source=] algorithm", the spec seems to use both forms somewhat interchangeably when invoking algorithms).
--
Reply to this email directly or view it on GitHub:
https://github.com/w3c/ServiceWorker/pull/1701#pullrequestreview-1873293827
You are receiving this because you are subscribed to this thread.
Message ID: <w3c/ServiceWorker/pull/1701/review/1873293827@github.com>
Received on Saturday, 10 February 2024 00:14:35 UTC