1267119 - request to download a serviceworker script is missing in the network monitor

Status: NEW

(DevTools :: Netmonitor, defect, P2)

Description

[(no longer active)]

STR:

Go to https://serviceworke.rs/json-cache_demo.html. A "service-worker.js" entry doesn't show up in the network monitor.

I'm not sure why this might be, but my hunch is that the information we use to associate the network connections made in tab A versus tab B may be incorrect somehow...

Comment 1

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Thanks for the report!

I am setting P2 since nobody is having this on the TODO list atm, but sounds important to me to be fixed.

Honza

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

This is kind of that same as bug 1246289, but maybe a bit different.

Note, we also don't show any network requests for fetches initiated from within the service worker.

We probably need to attach the service worker ID to the nsIChannel somehow and then devtools can use nsIServiceWorkerManager:ShouldReportToWindow() to determine if the toolbox should show that channel.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Ben Kelly [:bkelly] from comment #2)
> Note, we also don't show any network requests for fetches initiated from
> within the service worker.

This particular part has been fixed.  We still don't show the service worker script load itself, though.  (And probably not importScripts() either.)

Comment 4

[Ben Hsu [:HoPang]]

Attachment: Experiment

Comment 6

[Ben Hsu [:HoPang]]

Attachment: P1: Use aLoadGroup instead of creating a new one

Comment 7

[Ben Hsu [:HoPang]]

Attachment: P2: Add a chrome mochitest

Comment 8

[Ben Hsu [:HoPang]]

Thanks

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8852022 [details] [diff] [review]
P1: Use aLoadGroup instead of creating a new one

Review of attachment 8852022 [details] [diff] [review]:
-----------------------------------------------------------------

r=me because this is an improvement, but I think we may want to some follow-on fixes.  For example, I don't think this will show importScripts() or background update requests in the network monitor.  It will only show requests triggered from a .register() call.

We may need some server worker specific code in devtools to understand a SW loadgroup and call ServiceWorkerManager::ShouldReportToWindow() to figure out if it should be displayed in a particular network monitor:

https://dxr.mozilla.org/mozilla-central/source/dom/interfaces/base/nsIServiceWorkerManager.idl#228

::: dom/workers/ServiceWorkerScriptCache.cpp
@@ +655,4 @@
>                       uri, aPrincipal,
>                       nsILoadInfo::SEC_REQUIRE_SAME_ORIGIN_DATA_IS_BLOCKED,
>                       nsIContentPolicy::TYPE_INTERNAL_SERVICE_WORKER,
> +                     aLoadGroup,

Just for my own clarification, this is safe to do because we are not actually passing the document's load group.  Instead we actually have a separate loadgroup that also happens to point at the document's loadgroup's notification callbacks:

https://dxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerManager.cpp#671

So if the window is closed the registration load won't be canceled.

I don't think this will help report service worker script loads for update() calls or automatic updates.  In those cases we pass nullptr for the load group:

https://dxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerManager.cpp#2864

Also, I don't think this change will report network channels for importScripts() loads.

Comment 10

[Hsin-Yi Tsai (she/her) [:hsinyi]]

This seems depends on SW redesign clientId support as well ...

Comment 11

[Jan Honza Odvarko [:Honza] (always need-info? me)]

I've been looking at this again using the following STR:

1. Load https://firefox-devtools-waiting-sw.glitch.me/
2. Open DevTools and select the Network panel
3. Reload the page
4. Look for sw.js request (the service worker file). It isn't in the list => BUG

Comments:

• Should DevTools expect "service-worker-synthesized-response" notification to be fired for the sw.js file (the service worker itself)?
  I am not seeing any.

DevTools are installing a listener for "service-worker-synthesized-response" here
https://searchfox.org/mozilla-central/rev/dc4560dcaafd79375b9411fdbbaaebb0a59a93ac/devtools/server/actors/network-monitor/network-observer.js#255-258
The comment sounds confusing, but the listener is installed in both parent & child process.

• I am seeing some notifications related to sw.js request coming from nsIHttpActivityDistributor, but they are all filtered out by this method.
  https://searchfox.org/mozilla-central/rev/dc4560dcaafd79375b9411fdbbaaebb0a59a93ac/devtools/server/actors/network-monitor/network-observer.js#65

The matchRequest is supposed to filter out all request that are not executed by the current page. The logic is based on using outerWindowID coming from request's top level frame (we get the frame through nsILoadContext) or channel.loadInfo.topOuterWindowId.

The request for the SW file itself doesn't pass the filter and is filtered out.

You can also see these helper methods.
https://searchfox.org/mozilla-central/rev/dc4560dcaafd79375b9411fdbbaaebb0a59a93ac/devtools/shared/webconsole/network-helper.js#217-264

The patch attached to his bug is playing with a loadGroup, which sounds close to what the issue could be.

Andrew, do you know more form the platform perspective? Does my analysis make sense?

Honza

Comment 12

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

(In reply to Jan Honza Odvarko [:Honza] (always need-info? me) from comment #11)

• Should DevTools expect "service-worker-synthesized-response" notification to be fired for the sw.js file (the service worker itself)?
  I am not seeing any.

No. This is only notified when a ServiceWorker controlling a page intercepts a "fetch" request and invokes respondWith on the FetchEvent, generating a synthesized response. The ServiceWorker script loads:

• First go to the Cache API storage assigned to the given ServiceWorker instance.
• Fail over to the network during initial "install" phase; if we're past install, the loads fails.

The Cache API loads aren't going to the network once the SW is installed, so will not be observable without us doing something new. Cache API hits resolve here.

Currently the script loading does involve the content process main thread (including the use of the Cache API which itself happens in PBackground and QM and a dedicated I/O on the parent, never the main thread), but it's our hope to move this to the worker thread in the near future. This might end up introducing a new channel type, though, which could be made to look more like the rest of the network loads except we would want the parent part of the channel to live in PBackground, so... that could be tricky.

Perhaps this wants to use a mechanism similar to console logging for ServiceWorkers. I understand our goal there is to expose messages logged by the ServiceWorker to all windows controlled by a SW's registration as well as any windows which called register() and are thereby associated with the given registration. (This might be expanded in the future for any action taken to potentially interact with the registration, like using postMessage().)

More discussion after the next point.

• I am seeing some notifications related to sw.js request coming from nsIHttpActivityDistributor, but they are all filtered out by this method.
  https://searchfox.org/mozilla-central/rev/dc4560dcaafd79375b9411fdbbaaebb0a59a93ac/devtools/server/actors/network-monitor/network-observer.js#65

The matchRequest is supposed to filter out all request that are not executed by the current page. The logic is based on using outerWindowID coming from request's top level frame (we get the frame through nsILoadContext) or channel.loadInfo.topOuterWindowId.

The request for the SW file itself doesn't pass the filter and is filtered out.

This makes sense. The ServiceWorker doesn't get explicitly associated with any window because, like a SharedWorker, there are potentially multiple windows that should be associated with the SW. (Also, unlike a SharedWorker, the ServiceWorker can be spawned with no windows involved due to push notifications.)

The P1 patch was messing with the loadGroup because under child intercept the load group of the ServiceWorker was potentially derived from that of the thing triggering the spawn of the ServiceWorker. That's no longer relevant under parent intercept and wasn't reliable under child-intercept. (Also, I think we may have changed things at one point? Or never changed in the first place? There's been a bit of churn about the load group over time as I think we were accidentally stopping the ServiceWorker's loads when the window's loads got stopped?)

There are somewhat more options here since this is an actual network request, but since the steady-state involves the Cache API loads, maybe we want to make sure the approach handles both the initial script network fetches that get stored to the Cache API as well as the Cache API loads? (Although the initial script network fetches are arguably uniquely interesting since debugging the initial register() of a SW or update() of its scope can potentially fail in interesting ways.)

Andrew, do you know more form the platform perspective? Does my analysis make sense?

Makes sense, yes.

In terms of implementing things, I guess the big question is whether we expect a debugger to be attached to the potentially relevant ServiceWorker instances in all cases. I'm a little unclear on where we ended up on the long term game plan for the console messages.

I think the biggest gap right now is that the ServiceWorkerManager and RemoteWorker infrastructure know about workers interacting with windows and other workers that could be exposed to devtools that are potentially complex. This has historically been boiled down to the outerWindowID, but that was always a hack that didn't quite scale.

Given that the network filter is really just a predicate based on knowing that you're debugging a window, maybe we could generalize things by adding a method to nsIWorkerDebuggerManager like startDebuggingWindow(PWindowGlobal or WindowId or something) which would return an nsIWorkerDebuggingSession which could then have methods like:

• stopDebugging()
• matchesWorkerNetworkLoad(nsIChannel) which would consult the channel's loadInfo and from there the ClientInfo and then check if the client corresponds to a ServiceWorker on the registration controlling the given client or a registration that has become associated with the given window.

A bit of the rationale here is:

• The ClientInfo on the loadinfo is noscript because ClientInfo is not XPCOM. (But does have WebIDL bindings for other purposes.)
• ServiceWorkerManager wants to move off of the main thread to PBackground, and it's a lot easier to keep the predicate up-to-date across threads if we just know the debugger's interested in a specific global/window rather than attempting to replicate all state across to the main thread and then potentially expose them via XPCOM and have to event them, etc.
• This could also be altered to change from a predicate to directly invoking a callback when relevant channels have things happening on them, which could avoid observers needing to watch everything and allow optimizing things.
  • We might also need/want a callback for the serviceworker script loads.
  • The callback might be able to use more appropriate types that would make it easier for the network panel to request the body payloads more efficiently than holding onto nsIInputStreams forever or how it currently works.

Comment 13

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Thanks Andrew for the feedback!

matchesWorkerNetworkLoad(nsIChannel) which would consult the channel's loadInfo and from there the ClientInfo and then check if the client
corresponds to a ServiceWorker on the registration controlling the given client or a registration that has become associated with the given window.

Yes, this makes sense and sounds like a key part to solve this bug.

Should I file another bug - perhaps in Core :: DOM: Service Workers for the actual platform work?

Honza

Comment 14

[Julian Descottes [:jdescottes]]

Renaming to clarify the scope of this bug, and avoid mixing it with Bug 1432311.

This bug is only about the initial request to download the service worker script (eg sw.js)
Bug 1432311 is about any potential request initiated by a service worker.

Although I think comment #12 covers a bit of both.

Comment 15

[Julian Descottes [:jdescottes]]

Hi Andrew,

(this is both a question for this bug and for bug 1432311)

This is a follow-up question to your comment #12. We want to resume the effort of showing service worker requests in the netmonitor, both the initial request for the service worker script, as well as the fetch requests initiated by a service worker).

We can already see fetch() requests from service worker scripts in the Browser Toolbox, which we catch in the parent process.
I can see that the channel for such requests don't contain much information which can be used to identify which serviceworker it came from, or which tab it might relate to.

As I said, we observe this from the parent process, so is there any other process/thread where we should try to monitor service worker requests and where we could get more details about the request. I imagine the answer might be different if we talk about the initial serviceworker script request or about fetch() requests initiated by the worker itself.

Sorry for asking again even though you already provided lots of insights in comment #12. Since that was 3 years ago, things might have changed.

Comment 16

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

Currently all worker script loading happens from the process hosting the worker; in the case of a ServiceWorker this will (currently) be from a dedicated ServiceWorker process as opposed to the process hosting the intercepted page load. In bug 1797639 we plan to change this so that we use the PFetch infrastructure to issue the requests from the parent process in order to avoid the main thread of the process hosting the ServiceWorker. When we do this, we may change it so that ServiceWorkers no longer have their own process type. (This change will also affect dedicated workers!)

fetch() requests initiated by the worker now already go through PFetch. Assuming we want to generalize from Eden's work in bug 1819570 (which you reviewed :), we likely want to expose additional metadata or helpers on the nsILoadInfo for the benefit of devtools. I think right now the canonical identifier for service workers is the nsIServiceWorker.id. We could expose this on nsILoadInfo when the fetch comes from a ServiceWorker. I forget if devtools already knows that id when it's looking at a controlled page or not. We could certainly add a helper to nsIServiceWorkerManager that could resolve a BrowsingContextID to the nsIServiceWorkerRegistrationInfo that is controlling a window.

Comment 17

[Julian Descottes [:jdescottes]]

Thanks for the info! In that case it might be even worth blocking on bug 1797639.

I forget if devtools already knows that id when it's looking at a controlled page or not. We could certainly add a helper to nsIServiceWorkerManager that could resolve a BrowsingContextID to the nsIServiceWorkerRegistrationInfo that is controlling a window.

I don't think we have this information, because in all places where we need to check if a page is controller by a worker, we do hacky workarounds based on the url of the page vs urls controlled by service workers. I will file a bug for this.

Comment 18

[Hubert Boma Manilla (:bomsy)]

Attachment: WIP: Bug 1267119 - [devtools] Show the request for the service worker file in the netmonitor