Open Bug 1836707 Opened 1 year ago Updated 8 days ago

Add helper `nsIServiceWorkerManager::CreateBrowsingContextFilter` to create a filter to help devtools determine what console messages and/or network channels related to ServiceWorkers are relevant to devtools

Categories

(Core :: DOM: Workers, task)

Product:
Core
Component:
DOM: Workers
Type:
task

Tracking

()

Status:
ASSIGNED

People

(Reporter: jdescottes, Assigned: asuth)

References

(Blocks 3 open bugs)

Details

Attachments

(1 file)

Bug 1836707 - [devtools] Test highlighting nsIServiceWorkerManager new API requirements.
48 bytes, text/x-phabricator-request
Details | Review

(From to Andrew Sutherland [:asuth] (he/him) from Bug 1267119 comment #16)

We could certainly add a helper to nsIServiceWorkerManager that could resolve a BrowsingContextID to the nsIServiceWorkerRegistrationInfo that is controlling a window.

Such an API would be really useful in devtools in order to match scripts or requests related to service workers when the toolbox is only debugging a specific tab.

We would probably also want to handle the situation where a (window belonging to a) BrowsingContextID has called navigator.serviceWorker.register() but the window is not controlled. Since this can happen dynamically on the fly, this is a little more complicated. It's not immediately obvious to me how to best structure this.

Moving to the appropriate component.

(In reply to Andrew Sutherland [:asuth] (he/him) from comment #1)

We would probably also want to handle the situation where a (window belonging to a) BrowsingContextID has called navigator.serviceWorker.register() but the window is not controlled. Since this can happen dynamically on the fly, this is a little more complicated. It's not immediately obvious to me how to best structure this.

Right, it sounds useful especially for the requests performed to download the script. For those scenarios it might make more sense to have something on the request itself in order to link it to the browsing context which called register? For regular workers using PFetch, I think Eden made it so that it could work, but maybe this is different? A nsIServiceWorkerManager API that would also handle this use case sounds harder to design.

Component: Netmonitor → DOM: Workers
Product: DevTools → Core

edit note: I have changed previous use of "browsing context id" to BrowserId for clarity.

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

Right, it sounds useful especially for the requests performed to download the script. For those scenarios it might make more sense to have something on the request itself in order to link it to the browsing context which called register? For regular workers using PFetch, I think Eden made it so that it could work, but maybe this is different? A nsIServiceWorkerManager API that would also handle this use case sounds harder to design.

An additional complication is that because service workers has a job queue with coalescing semantics in step 6 of schedule job we end up with a situation where potentially multiple browsing contexts could be associated with a single ServiceWorker register() job and the resulting new registration. Like if you open 5 tabs in parallel and they all call register, they would all end up with promises waiting on the same single register job.

And for completeness, some related edge cases:

  • A ServiceWorker registered for a scope that doesn't actually exist for interception purposes (ex: "/this-url-does-not-exist"), but does exist for push notification purposes or some other rationale where a page would call getRegistrations() to locate the registration whose scope does not match the page, and then postMessage() the SW.
  • A page "/foo/bar" is controlled by the SW with scope "/". Another tab registers a new SW with scope "/foo" that claims the pre-existing "/foo/bar" page (which did not call register). This is extreme (and probably a bad idea for the site), but possible.
  • The update check in step 7 of the update algorithm is not performed by the script loader (which would re-do any loads if the update check finds changes, although those might then be cached).

Maybe a sketch for an API that could handle the various permutations would be:

  • Add nsIServiceWorkerManager::CreateBrowsingContextFilter method that takes a (edited:) BrowserId and some flags to scope level of interest and the method returns a new class/interface nsIServiceWorkerFilter.
  • nsIServiceWorkerFilter::FilterLoadInfo(nsILoadInfo) is the key method that can be used to efficiently check if the fetch is coming from an involved ServiceWorker, and it returns an enum or bitmask describing the relationship between the browsing context and the SW. I propose integer enum values below in order of decreasing precedence where 0 would be that the fetch isn't interesting or related:
    • Controlling: The SW is controlling the page. This could perhaps be further broken down into:
      • ControllingLoaded: The page has been controlled by the SW since its load. (Note that a SW that does not handle "fetch" events is still notionally controlling a page, so this does not distinguish if the page load actually went to the network or not.)
      • ControllingClaimed: The page was claimed by the SW (and may or may not have been previously claimed by a different SW).
    • PageRegistered: The page issued a register call to this SW.
    • RegistrationUpdating: The page didn't call register() but an update check for the registration resulting from a functional event or handle fetch algorithm ran and dediced we need to update the SW and the SW is somewhere in the evaluating/installing/waiting lifecycle, with "active" corresponding to the ControllingClaimed value above.
    • RegistrationUpdateCheck: There isn't actually a SW yet, we're just in the update-check as part of the update algorithm, so there's no script loader active yet. This is the predecessor state of RegistrationUpdating above.
    • ScopeMatches: The page's URL will match the scope of the SW but the page didn't call register, but this inherently would be the result of another page issuing a fresh call to register().
    • PageMessaged: The page used ServiceWorker.postMessage to talk to the SW. (A page can find all SWs via getRegistrations().)
    • ServiceWorkerMessaged: The ServiceWorker messaged the page by using the Clients API. (Both this and the above could be true, which might be an argument for a bit-mask or separate flags.)
  • We could have a bunch of (less performant) helpers that could be used after the (faster) filter check above to return the ServiceWorkerID and/or the nsIServiceWorkerInfo for the ServiceWorker (although there would be neither of these for an update check fetch).
  • Medium-term future: The filter could let devtools indicate that it wants all the ServiceWorkers to effectively have setDebuggerReady(false) called on them as they're created and allow for a listener callback that gets invoked with the corresponding (new) nsIWorkerDebugger. I guess this would also implicitly call attachDebugger on the nsIServiceWorkerInfo.
    • The context is that, as part of our attempt to improve ServiceWorker performance by not having them be owned by the main thread, we will likely move to having their exposed nsIWorkerDebuggers be on the parent process main thread rather than on the main thread of the content process where they live. This would also end up happening for SharedWorkers too. But it means we can have the association between nsIServiceWorkerInfo and nsIWorkerDebugger be much more direct because they will be exposed in the same process.

This would represent a shift of more decision-making into ServiceWorker C++ logic, but this is desirable because a number of the notable changes for a SW becoming relevant to a browsing context already happen on the PBackground thread, and in the future it's quite possible we could move even more of them to a background thread and we could update the state more atomically with this API so that fetches don't get lost, whereas if we had to message devtools for it to update its own predicates first, it might miss a bunch of fetches, especially if/as we shard PBackground into multiple threads/task queues and start doing more toplevel IPC endpoint stuff.

Hello Andrew.

Thanks for detailed comments.

We are currently looking into fixing the various Service Worker issues in devtools, one which is Bug 1432311 (to display fetch requests triggered from within the service worker).
From the discussions in Bug 1267119 and the comments here, this looks like it'll be a blocker for us.

Not sure of the requirement to get this done, but was wondering if there's a short term solution which would help unblock our work?

Many thanks!!

Thanks

Blocks: 1432311
Flags: needinfo?(bugmail)

(In reply to Hubert Boma Manilla (:bomsy) from comment #4)

Not sure of the requirement to get this done, but was wondering if there's a short term solution which would help unblock our work?

Unfortunately, from https://bugzilla.mozilla.org/show_bug.cgi?id=1267119#c17 it sounded like the short term solution wouldn't be of any use for devtools.

Flags: needinfo?(bugmail)

Hi Andrew,
Just to mention, there is a patch in review for Bug 1865784 to add tests on the devtools side which asserts all the scenarios to be fixed. In our discussions it was mentioned that having some tests would be helpful in supporting the platform work.
Pls let me know if there are other ways i can help push this along thanks.

Thanks

Flags: needinfo?(bugmail)
See Also: → 1865784

To followup the meeting, the following function may be the one being called on each ServiceWorker being registered:
https://searchfox.org/mozilla-central/rev/16526d690cccc9c60cacf09cc08e2c3041ef884e/dom/serviceworkers/ServiceWorkerManager.cpp#834-836

RefPtr<ServiceWorkerRegistrationPromise> ServiceWorkerManager::Register(
    const ClientInfo& aClientInfo, const nsACString& aScopeURL,
    const nsACString& aScriptURL, ServiceWorkerUpdateViaCache aUpdateViaCache) {

And this other function may be the place where to communicate IDs onto the client class:
https://searchfox.org/mozilla-central/rev/16526d690cccc9c60cacf09cc08e2c3041ef884e/dom/clients/manager/ClientSource.cpp#59-71

void ClientSource::ExecutionReady(const ClientSourceExecutionReadyArgs& aArgs) {

If we pass the browserId on ClientInfo (the browserId is a better ID for devtools than the browsingContextId),
we could then add this browserId on the ServiceWorkerRegistrationInfo?
Then DevTools could used that new attribute to identify the relevant service workers?

Does that sounds somewhat correct?

Assignee: nobody → jmarshall
Flags: needinfo?(bugmail)
Summary: Add helper in nsIServiceWorkerManager to resolve BrowsingContextID to the controlling nsIServiceWorkerRegistrationInfo → Add helper `nsIServiceWorkerManager::CreateBrowsingContextFilter` to create a filter to help devtools determine what console messages and/or network channels related to ServiceWorkers are relevant to devtools

(In reply to Andrew Sutherland [:asuth] (he/him) from comment #3)

Maybe a sketch for an API that could handle the various permutations would be:

  • Add nsIServiceWorkerManager::CreateBrowsingContextFilter method that takes a browsing context id and some flags to scope level of interest and the method returns a new class/interface nsIServiceWorkerFilter.

A quick comment given that there is movement toward this particular option:

I was rather envisioning having a browserId or BrowsingContext ID (array?) on nsIServiceWorkerInfo or nsIServiceWorkerRegistrationInfo.
Comment 7 contains questions in order to investigate doing such API. Then we could expose Service Worker ID on the channel/loadInfo to do the matching. We could maintain, in the parent process, the list of active Service Worker IDs debugged by the DevTools.

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

(In reply to Andrew Sutherland [:asuth] (he/him) from comment #3)
A quick comment given that there is movement toward this particular option:

  • Ultimately DevTools, when debugging a given tab, tracks a browserId rather than one BrowsingContext ID.
    This allows us to easily handle navigations moving the tab to a distinct BrowsingContext.

Yes, BrowserId (objects replaced by navigation docs also relevant) seems like the right type/identifier to use, thanks for pointing that out. I've edited comment 3 (and annotated the edit) to help clarify that.

This won't help yet, but can help soon as we implement bug 1672491 and bug 1672493. As briefly discussed at the meeting, the rough idea would be that we would expose the capabilities of nsIWorkerDebugger related to talking to the worker via the nsIServiceWorkerInfo in the parent process and not expose it at all in the content process.

One way we might do this is by having nsIServiceWorkerInfo::attachDebugger return the nsIWorkerDebugger/new-interface in a more RAII fashion so that if the returned interface gets GC'ed, we can automatically detach from the debugger. We would also have a detach/dispose/disconnect/close method on that returned interface too so it could be more eagerly detached without waiting for GC.

In terms of this interacting with the filter, since I think devtools wants to have at most 1 ServiceWorkerRegistrationActor per nsIServiceWorkerRegistrationInfo and ServiceWorkerActor per nsIServiceWorkerInfo and these are I think owned by ServiceWorkerRegistrationActorList... it probably makes sense for the filter to have something like FilterServiceWorkerRegistration(nsIServiceWorkerRegistrationInfo) and maybe FilterServiceWorker(nsIServiceWorkerInfo).

Would that make sense? Another possible style could be that the filter could expose a method like AddFilteredRegistrationListener(...) which would be a variation of the nsIServiceWorkerManagerListener notification methods onRegister/onRegister that would be invoked when a registration becomes relevant or irrelevant to a browserId.

I was looking forward this new API for network events, to also help this other part of DevTools.

Yes, I think this is a large area of potential improvement.

I was rather envisioning having a browserId or BrowsingContext ID (array?) on nsIServiceWorkerInfo or nsIServiceWorkerRegistrationInfo.
Comment 7 contains questions in order to investigate doing such API. Then we could expose Service Worker ID on the channel/loadInfo to do the matching. We could maintain, in the parent process, the list of active Service Worker IDs debugged by the DevTools.

I think it could be reasonable to expose a list of BrowserIds on them, yes.

That said, I would very much like to move in the direction of using the filter approach as much as possible because the filter can work off the main thread (OMT) and otherwise let us avoid exposing objects to JS that don't need to be and thereby avoid extra GC/etc. For example, we should soon be able to move the ServiceWorker script loads of an installed ServiceWorker so that they don't touch the main thread of any process. I believe devtools would like to know about those, though, and the filter could let us do that without having to pay that cost when devtools[1] is not interested in a tab.

1: Disclaimer: there is also the WebExtensions webRequest use-case that potentially interacts, but that has a different set of complicated issues and potential optimizations.

attachment 9377604 [details] helps seeing the first usage of this new suggested API for network requests.

(In reply to Andrew Sutherland [:asuth] (he/him) from comment #9)

This won't help yet, but can help soon as we implement bug 1672491 and bug 1672493. As briefly discussed at the meeting, the rough idea would be that we would expose the capabilities of nsIWorkerDebugger related to talking to the worker via the nsIServiceWorkerInfo in the parent process and not expose it at all in the content process.

I understand the willingness to avoid having JS-exposed objects to avoid GC challenges.
But I have some concerns about our ability to get there?
I'm looking forward having a solution sooner than later.
Bug 1672491 is 3 years old. Is there an active plan to fix the two mentioned bug ?

One way we might do this is by having nsIServiceWorkerInfo::attachDebugger return the nsIWorkerDebugger/new-interface in a more RAII fashion so that if the returned interface gets GC'ed, we can automatically detach from the debugger. We would also have a detach/dispose/disconnect/close method on that returned interface too so it could be more eagerly detached without waiting for GC.

Sounds like an appealing API. Once we get closer to implement such API I can investigate more to confirm it fits all DevTools needs.

In terms of this interacting with the filter, since I think devtools wants to have at most 1 ServiceWorkerRegistrationActor per nsIServiceWorkerRegistrationInfo and ServiceWorkerActor per nsIServiceWorkerInfo and these are I think owned by ServiceWorkerRegistrationActorList... it probably makes sense for the filter to have something like FilterServiceWorkerRegistration(nsIServiceWorkerRegistrationInfo) and maybe FilterServiceWorker(nsIServiceWorkerInfo).

This code is actually not so relevant. Today, it is only used by about:debugging to retrieve the list of all service worker registrations.
In the context of about:debugging, we are listing all the service workers of all the pages and all the domains. There is no need to filter anything.

Thanks to the work which recently landed in bug 1651522, we are no longer involving registrations when debugging service workers.
Service workers can now be debugged from regular devtools, when debugging a page, you will see the service workers right in the debugger.
This is still preffed-off behind devtools.debugger.features.windowless-service-workers, but this should soon be the default way to debug service workers.
This new feature no longer relies on the registration objects. Instead, we have a JSProcessActor in all the content processes which will observe workers in all the processes. If a Service Worker matches the current page's domain, then we attach to it via the WorkerDebugger interface.
So today, we have one filtering, it is done via nsIWorkerDebugger.principal.host over there in DevToolsServiceWorkerChild.sys.mjs.

Moving this logic to the parent process isn't a requirement for DevTools. It wouldn't simplify things significantly. It would mostly be saving some cycles in content processes. Now, if moving nsIWorkerDebugger to the parent process simplifies the C++ implementation, DevTools can move to the parent process. It wouldn't complexify things either.

Would that make sense? Another possible style could be that the filter could expose a method like AddFilteredRegistrationListener(...) which would be a variation of the nsIServiceWorkerManagerListener notification methods onRegister/onRegister that would be invoked when a registration becomes relevant or irrelevant to a browserId.

Yes this makes sense, as soon as we also have the attachDebugger/setDebuggerReady/initialize APIs working from the main process.
It is not impossible that we could simplify these three methods into only one or two method(s).

I was looking forward this new API for network events, to also help this other part of DevTools.

Yes, I think this is a large area of potential improvement.

I was rather envisioning having a browserId or BrowsingContext ID (array?) on nsIServiceWorkerInfo or nsIServiceWorkerRegistrationInfo.
Comment 7 contains questions in order to investigate doing such API. Then we could expose Service Worker ID on the channel/loadInfo to do the matching. We could maintain, in the parent process, the list of active Service Worker IDs debugged by the DevTools.

I think it could be reasonable to expose a list of BrowserIds on them, yes.

That said, I would very much like to move in the direction of using the filter approach as much as possible because the filter can work off the main thread (OMT) and otherwise let us avoid exposing objects to JS that don't need to be and thereby avoid extra GC/etc. For example, we should soon be able to move the ServiceWorker script loads of an installed ServiceWorker so that they don't touch the main thread of any process. I believe devtools would like to know about those, though, and the filter could let us do that without having to pay that cost when devtools[1] is not interested in a tab.

I'm fine with moving to main-process filter API, but I'm getting back to the timeline of all this.

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

I understand the willingness to avoid having JS-exposed objects to avoid GC challenges.
But I have some concerns about our ability to get there?
I'm looking forward having a solution sooner than later.
Bug 1672491 is 3 years old. Is there an active plan to fix the two mentioned bug ?

There are patches on bug 1672493 which are a significant step in this direction; they are currently waiting on a change to using endpoints. The bug relationship is somewhat backwards now as bug 1672491 will become significantly more feasible once bug 1672493 lands, allowing us to move the debugger communication to IPC.

This new feature no longer relies on the registration objects. Instead, we have a JSProcessActor in all the content processes which will observe workers in all the processes. If a Service Worker matches the current page's domain, then we attach to it via the WorkerDebugger interface.
So today, we have one filtering, it is done via nsIWorkerDebugger.principal.host over there in DevToolsServiceWorkerChild.sys.mjs.

When we address the above, we will no longer be able to expose the nsIWorkerDebugger for remote workers (ServiceWorker and SharedWorker) in the content process where they live, but instead in the parent process where they are owned. But we can make it so you can run the same logic in the parent process without much in the way of changes; the nsIWorkerDebuggerManager would expose all of the dedicated workers in the parent process and the running service workers in any process and the running shared workers in any process. An upside of this is that we could also directly expose a link from the nsIWorkerDebugger to its nsIServiceWorkerManager-exposed data/metadata so it would not be necessary to correlate based on serviceWorkerID.

The semantics of setDebuggerReady would be maintained. I would still want to have an RAII-style attachDebugger mechanism somewhere but where exactly doesn't matter.

The one logistical complication right now is that SharedWorker state and RemoteWorker state is authoritative on PBackground, not on the main thread, but this is a known problem with a bunch of hacks that we want to address by moving more bookkeeping to the main thread and this should not be too bad because it's mainly a question of moving things to the main thread and removing hacks. (This may involve endpoints a little here.)

This is something where we can potentially help with any migration as long as you already have operating test coverage or have provided tentative tests like you've provided here. (Thank you so much for providing those here and on other bugs!)

Moving this logic to the parent process isn't a requirement for DevTools. It wouldn't simplify things significantly. It would mostly be saving some cycles in content processes. Now, if moving nsIWorkerDebugger to the parent process simplifies the C++ implementation, DevTools can move to the parent process. It wouldn't complexify things either.

It's more than a simplification issue; this will help us clean up a source of security bugs, reduce ServiceWorker latencies, and let us get closer to eliminating the need for dedicate ServiceWorker processes.

Yes this makes sense, as soon as we also have the attachDebugger/setDebuggerReady/initialize APIs working from the main process.
It is not impossible that we could simplify these three methods into only one or two method(s).

Sounds good.

I'm fine with moving to main-process filter API, but I'm getting back to the timeline of all this.

My availability and Eden's availability has been fairly unpredictable and spotty due to regressions/sec-bugs, but I think we are converging on a specific plan here which should make it more clear what Joshua should implement and allow me to provide faster review cycles, etc. and where we have good synergies for things Eden already has WIPs for and/or the groundwork has been laid for faster next steps.

I'm going to take this because I think I need this for bug 1131324 to do the right thing for ServiceWorkerContainer::Register on workers, or at least it will help make the situation better in general rather than just move hacks around.

Assignee: jmarshall → bugmail
Blocks: 1131324
Status: NEW → ASSIGNED

Hm, so one complication here is that Eden's cleanups on bug 1672493 have not landed yet; its https://phabricator.services.mozilla.com/D197563 creates PRemoteWorkerNonLifeCycleOpController which has the ErrorPropagation of ErrorValue instances on it which potentially could help resolve some error plumbing issues. I don't think that blocks implementation of this bug, but it does potentially limit how much can be cleaned up on this bug.

See Also: → 1672493
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: