1566004 - ServiceWorker intercepting requests don't need to pass through proxy service

Status: RESOLVED WONTFIX

(Core :: Networking: HTTP, task)

Description

[Andrea Marchesini [:baku]]

When a request is handled by a service worker, doesn't matter what onRequest callback returns. Probably we should filter out these requests completely.

Comment 1

[Shane Caraveo (:mixedpuppy)]

(In reply to Andrea Marchesini [:baku] from comment #0)

When a request is handled by a service worker, doesn't matter what onRequest callback returns.

I don't understand the issue...are you saying that serviceworker requests are explicitly never proxied? If that's the case, it sounds wrong and that it would be a serviceworker bug to fix that.

Comment 2

[Andrea Marchesini [:baku]]

moving the NI to asuth

Comment 3

[Andrew Sutherland [:asuth] (he/him)]

When talking about network requests that potentially involve ServiceWorkers, there's really 2 phases of operation:

1. When a ServiceWorker may be involved. Step 3 of HTTP fetch (https://fetch.spec.whatwg.org/#http-fetch) calls out to the ServiceWorker spec at https://w3c.github.io/ServiceWorker/#handle-fetch to potentially intercept the request and never go to the network.
2. When the request is definitely going to the network which is at the subsequent step 4 of HTTP fetch (https://fetch.spec.whatwg.org/#http-fetch).

State-wise, we use the load flag of nsIChannel::LOAD_BYPASS_SERVICE_WORKER when originating a request and per spec the load should never be intercepted by a ServiceWorker, or after we've decided that a ServiceWorker should potentially be involved because nsINetworkInterceptController.shouldPrepareForIntercept[1] has returned true but we reset interception because the ServiceWorker didn't handle the fetch, and so we redirect to a network channel and need to make sure we don't try and involve the SW a 2nd time.

While it is appropriate to make sure WebExtensions are able to intercept requests to ServiceWorkers for phase 1, I think it makes sense that only the webRequest API or similar handle that case. It seems like the network proxy API at https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/proxy/onRequest only wants to be involved during phase 2.

I think the fundamental problem here is that we're in phase 1 until nsHttpChannel::Connect() is called, which is the code that calls ShouldIntercept()[2]. However, the proxy code is invoked during nsHttpChannel::AsyncOpenFinal()[3] which happens strictly before that. Specifically, AsyncOpenFinal is succeeded by BeginConnect(), BeginConnectActual(), ContinueBeginConnectWithResult(), PrepareToConnect(), OnBeforeConnect(), ContinueOnBeforeConnect(), and only then Connect().

I wonder if the nsHttpChannel state machine should perhaps be calling ShouldPrepareForIntercept earlier? (needinfo for :mayhemer) It's not clear that many of the steps preceding Connect() make sense if a ServiceWorker will be consulted, and it's not clear that there's a way for the proxy logic to filter out the request without knowing the result of ShouldPrepareForIntercept.

1: https://searchfox.org/mozilla-central/rev/07f7390618692fa4f2a674a96b9b677df3a13450/netwerk/base/nsINetworkInterceptController.idl#255
2: https://searchfox.org/mozilla-central/rev/07f7390618692fa4f2a674a96b9b677df3a13450/netwerk/protocol/http/nsHttpChannel.cpp#697
3: https://searchfox.org/mozilla-central/rev/07f7390618692fa4f2a674a96b9b677df3a13450/netwerk/protocol/http/nsHttpChannel.cpp#6520

Comment 4

[Shane Caraveo (:mixedpuppy)]

What is confusing me about this: if it is an http/s request, why would you not proxy it? Am I missing something basic here?

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

The idea is that with a ServiceWorker you can run a webpage / webapp entirely offline. The ServiceWorker decides whether to go to network or use cached data in the Cache API or IndexedDB, etc. Baku is saying that the proxy webextension is getting to see the requests before they hit the ServiceWorker, which effectively takes the ServiceWorker out of the equation. It does make sense to proxy the requests that the ServiceWorker decides should go to the network.

Comment 6

[Shane Caraveo (:mixedpuppy)]

Ah! So HttpChannel is preferring proxy over serviceworker, it perhaps should be the other way around. This would happen with any proxy config, not just extensions.

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

The product::component has been changed since the backlog priority was decided, so we're resetting it.
For more information, please visit auto_nag documentation.

Comment 8

[Honza Bambas (:mayhemer)]

Guys, the concerns here doesn't make absolutely any sense to me. The assertion that "the proxy webextension is getting to see the requests before they hit the ServiceWorker, which effectively takes the ServiceWorker out of the equation" is totally out.

If we assign a proxyinfo on a channel (which is there for two things: 1/ possible pre-connect and if to do it at all, 2/ network connection creation, when we decide to do it eventually), why would we be ever not calling service worker interception for proxyinfo != null? Can you show me that exact code?

Comment 9

[Honza Bambas (:mayhemer)]

Note that ResolveProxy (which we always call and is always async for default settings) continues at BeginConnect() from [1].

[1] https://searchfox.org/mozilla-central/rev/b9041f813de0a05bf6de95a145d4e25004499517/netwerk/protocol/http/nsHttpChannel.cpp#7085

Comment 10

[Andrew Sutherland [:asuth] (he/him)]

Baku, any chance you can add a test case to the existing WebExt tests[1] to help create a reproduction for what you're seeing?

1: https://searchfox.org/mozilla-central/search?q=&case=false&regexp=false&path=test_ext_proxy should find them under toolkit/components/extensions/test/xpcshell/. I believe they use the ExtensionTestUtils[2] mechanism that can also be used in mochitests.
2: https://searchfox.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/ExtensionTestUtils.js with some docs at https://wiki.mozilla.org/WebExtensions/Contribution_Onramp#Testing

Comment 11

[Shane Caraveo (:mixedpuppy)]

asuth: we cleared this up on slack earlier. This bug is about optimizing the requests that would be intercepted by serviceworker, so that we don't bother passing it through the proxyservice. IOW nothing is broken, we currently do the proxy resolve when it's unnecessary.

Comment 12

[Honza Bambas (:mayhemer)]

And I vote for WONTFIXING this. Proxy resolution must happen very soon to be able to decide on preconnections and dns prefetches. Racing is necessary here.

Note that I want to start a project to clean the opening phase of nsHttpChannel up and make more obvious what can race and what must be sequenced and when.

Comment 13

[Andrea Marchesini [:baku]]

wontfix is OK for me.