1876060 - Local script override doesn't work for SW cached request

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Description

[Alexandre Poirot [:ochameau]]

STR:

• open https://googlechrome.github.io/samples/service-worker/read-through-caching/index.html
• override analytics.js and change the JS file content to something different
• reload the page

AR:
analytics.js isn't overrided and instead the SW cached version is used.
Notice that the purple icon is displayed in the source tree, but the file isn't really overriden.

ER:
the override is used.

Note that if you disable service worker via dom.serviceWorkers.enabled set to false, the override works.

Comment 1

[Alexandre Poirot [:ochameau]]

We actually do try to override the channel's response content... but it doesn't impact the actual content used.
The request do emit service-worker-synthesized-response notification
and we do call NetworkOverride.overrideChannelWithFilePath from there:
https://searchfox.org/mozilla-central/rev/734887cfb193b5697e59a857d394e8d4245996db/devtools/shared/network-observer/NetworkObserver.sys.mjs#371
but it doesn't work for some reason. I suspect that service-worker-synthesized-response is emitted too late.
(I don't see any particular assertion in debug build)

May be the override should be done before the call to mChannel->StartSynthesizedResponse() ?
https://searchfox.org/mozilla-central/rev/734887cfb193b5697e59a857d394e8d4245996db/dom/serviceworkers/ServiceWorkerEvents.cpp#353-365

Comment 2

[Julian Descottes [:jdescottes]]

Valentin, do you know if the service-worker-synthesized-response observer notification is a good spot to override the response?

Comment 3

[Valentin Gosu [:valentin] (he/him)]

(In reply to Julian Descottes [:jdescottes] from comment #2)

Valentin, do you know if the service-worker-synthesized-response observer notification is a good spot to override the response?

I am not very familiar with how service worker channels work, but I think service-worker-synthesized-response might actually be too late.
If I understand correctly how SW work, the page attempts to load URL, nsHttpChannel calls ShouldIntercept and possibly redirects to the InteceptedHttpChannel, which then runs the SW. The SW will either provide the response from cache, or pass it through to the network via another channel.

If we want to override the contents of the JS file, I assume we want to do that before deciding whether to intercept? (please correct me if that's not the case)
If so, the notification we need to use to override the contents is http-on-before-connect.

Eden knows more about SW and might know if there's a better way to do this in SW code.

Comment 4

[Alexandre Poirot [:ochameau]]

(In reply to Valentin Gosu [:valentin] (he/him) from comment #3)

If we want to override the contents of the JS file, I assume we want to do that before deciding whether to intercept? (please correct me if that's not the case)
If so, the notification we need to use to override the contents is http-on-before-connect.

This makes a lot of sense. Sounds worth investigating using http-on-before-connect for all overrides.

Comment 5

[Julian Descottes [:jdescottes]]

(In reply to Alexandre Poirot [:ochameau] from comment #4)

(In reply to Valentin Gosu [:valentin] (he/him) from comment #3)

If we want to override the contents of the JS file, I assume we want to do that before deciding whether to intercept? (please correct me if that's not the case)
If so, the notification we need to use to override the contents is http-on-before-connect.

This makes a lot of sense. Sounds worth investigating using http-on-before-connect for all overrides.

Quick note: we are trying to switch to an observer notification to create the network events a bit earlier in Bug 1849686. We initially tried to use http-on-before-connect, we are now trying with a new notification which :valentin is working on, emitted before dispatching the transaction. Although for blocking/rewriting purposes I guess http-on-before-connect probably works fine.

Comment 6

[Eden Chuang[:edenchuang]]

https://searchfox.org/mozilla-central/rev/cee2c396081d950f9e3401113fb179999e404ab8/dom/serviceworkers/FetchEventOpChild.cpp#528-538

According to codes, InterceptedHttpChannel starts synthesizing response and also start pump to the content, then sends service-worker-synthesized-response notification. So the service-worker-synthesized-response may not be a good point to override the content.

Comment 7

[Julian Descottes [:jdescottes]]

Right now the new notification added in Bug 1849686 is blocked on a devtools service worker test issue. If needed we could just switch to http-on-before-connect only for the local script override, but for now let's try to wait for the new observer notification.

Comment 8

[Julian Descottes [:jdescottes]]

While I was testing migrating devtools' local override feature to use the Channel setResponseOverride API added in Bug 1900375, I also had to move handling the override to http-on-before-connect (because setResponseOverride only works if you call it before the request reaches the network).

It seems to fix this scenario, but only if we make sure that the request is not already in the service worker cache, because in that case we will not even get http-on-before-connect.

So you basically have to do a hard reload to make sure we fetch the override script. Given that setResponseOverride currently messes up with the netmonitor tracking, I think it will be best to only move the handling the overrides to http-on-before-connect for now.

Comment 9

[Julian Descottes [:jdescottes]]

Attachment: Bug 1876060 - [devtools] Handle local script override in http-on-before-connect

Depends on D217026

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d14cbebffa0c
[devtools] Handle local script override in http-on-before-connect r=devtools-reviewers,bomsy

Comment 11

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/d14cbebffa0c