Fix the wrong service-worker mode.

[qingxinwu]

Our implementation uses default "all" for service worker mode, not "none". However, given the fact that all of these requests have a null client, one of the side-effects of this is neutering all service worker interception (effectively "none" although set to default "all), despite not having to set the service workers mode. The service worker interception algorithm returns at step 15.2 (our request has the default kEmpty destination, so it's a subresource request), and nothing happened before this step.

There is an issue filed for this.

---

Preview | Diff

[domfarolino]

I mean the spec PR looks fine, but I'm not super comfortable landing this without at least some knowledge of what the implementation does. I think this PR would make the request interceptable by service workers (but not sure if the client has to be non-null for that), and I certainly don't know what the implementation does.

For example, if we determine that even with a null client, per spec this request would be intercepted by service workers, but we also determine that our implementation doesn't match this due to it being a browser-initiated request, and it would be very hard to change that, would we want to spend a ton of time hacking the implementation to match the spec here, or would we just want to revert this PR and use none service workers mode?

Knowing what our implementation does here seems important given the somewhat anomalous implementation (not being sent via a renderer, and not being a navigation request), so landing this PR without that knowledge seems premature.

[qingxinwu]

I mean the spec PR looks fine, but I'm not super comfortable landing this without at least some knowledge of what the implementation does. I think this PR would make the request interceptable by service workers (but not sure if the client has to be non-null for that), and I certainly don't know what the implementation does.

For example, if we determine that even with a null client, per spec this request would be intercepted by service workers, but we also determine that our implementation doesn't match this due to it being a browser-initiated request, and it would be very hard to change that, would we want to spend a ton of time hacking the implementation to match the spec here, or would we just want to revert this PR and use none service workers mode?

Knowing what our implementation does here seems important given the somewhat anomalous implementation (not being sent via a renderer, and not being a navigation request), so landing this PR without that knowledge seems premature.

Matt commented under the other github issue:
"The scripts don't go through SW because they're sent directly to the network stack, just like internal Chrome network requests. There's no SW loader stuck in front of the network stack for them, so the value of skip_service_worker has no effect. So despite not explicitly setting it to none currently, it is effectively none, since no service worker loader factory is in front of the real network loader factory".

[domfarolino]

Layering-wise in the spec, service worker interception is done in Fetch, so it's possible you still need to set the service workers mode to "none" for the spec. That is, the reason Chrome skips service workers might be implementation-specific. I believe the service worker interception algorithm that gets initially invoked by Fetch is https://w3c.github.io/ServiceWorker/#on-fetch-request-algorithm. Could you check that algorithm out and see if it would intercept these kinds of requests? If not, then maybe Chrome and the spec are aligned, but I'm just trying to protect against the case where the spec needs the service worker mode to be "none" even though the implementation technically doesn't.

[qingxinwu]

Layering-wise in the spec, service worker interception is done in Fetch, so it's possible you still need to set the service workers mode to "none" for the spec. That is, the reason Chrome skips service workers might be implementation-specific. I believe the service worker interception algorithm that gets initially invoked by Fetch is https://w3c.github.io/ServiceWorker/#on-fetch-request-algorithm. Could you check that algorithm out and see if it would intercept these kinds of requests? If not, then maybe Chrome and the spec are aligned, but I'm just trying to protect against the case where the spec needs the service worker mode to be "none" even though the implementation technically doesn't.

I think it does not intercept these kinds of requests. If I understand it correctly, it returns at step 15.2 (our request has the default kEmpty destination, so it's a subresource request), and nothing happened before this step.

[domfarolino]

That makes sense to me, thanks. Given the fact that all of these requests have a null client, could you add a non-normative note somewhere saying that one of the side-effects of this is neutering all service worker interception, despite not having to set the service workers mode? I think that'd be good to clarify somewhere. I'm not sure if it would be too noisy to do it for each request or not, so I'll leave that to you.

Also if you could update the OP with your rationale for why this causes no spec behavior change (because of step 15.2 etc.) that would be great, just so we can easily find the logic here if we ever have to do any git blame archaeology in the future.

[qingxinwu]

That makes sense to me, thanks. Given the fact that all of these requests have a null client, could you add a non-normative note somewhere saying that one of the side-effects of this is neutering all service worker interception, despite not having to set the service workers mode? I think that'd be good to clarify somewhere. I'm not sure if it would be too noisy to do it for each request or not, so I'll leave that to you.

Also if you could update the OP with your rationale for why this causes no spec behavior change (because of step 15.2 etc.) that would be great, just so we can easily find the logic here if we ever have to do any git blame archaeology in the future.

Done.