1651522 - Create WorkerTargets for service workers

Status: RESOLVED FIXED

(DevTools :: Framework, enhancement)

Description

[Alexandre Poirot [:ochameau]]

Bug 1633712 will introduce the first framework code in order to create WorkerTargetActor's via the Watcher Actor. This will focus on the regular web page's workers, running in the web page's process.
Bug 1651518 will followup in order to also support parent process workers and shared workers.
Then, the last step would be to support service workers. Like bug 1651518, it may also be only about tweaking a few filtering methods. The filtering would be slightly more complex for service workers as we might have to filter based on the tab's current origin.
And who knows, it may be more complex than just tweaking a few filtering methods.

The filtering around tab's current origin is being explained over here:
https://phabricator.services.mozilla.com/D77425#2416452
3 options are mentioned.
I would personaly go for option C. I think that it would allow debugging SW in all cases from the debugger and stop having you to have to go through about:debugging in order to debug SW startup/shutdown, which may occur after the page navigated.

Harald told me:

Solution C does indeed seem useful, even though SWs don't react to navigation in any special way (apart from the usual tear down they have after inactivity). Other tools like Console and Network persist data; so showing a SW from a prior domain seems not that bad.

Finally, I think that this isn't strictly necessary for shipping fission.
For now, I'll flag this in the reserve list. But I may be wrong and we can revisit this triage.

Comment 1

[Chris Peterson [:cpeterson]]

Tracking dt-fission-m2-reserve bugs for Fission Beta milestone (M7).

Comment 2

[Alexandre Poirot [:ochameau]]

We might want to first change the behavior of SW targets lifecycle via bug 1655439 before implementing this?

Comment 3

[Neha Kochar [:neha]]

Bulk move of all dt-fission-m2-reserve bugs to Fission MVP milestone.

Comment 4

[Chris Peterson [:cpeterson]]

Moving "dt-fission-m3-reserve" bugs to "dt-fission-future" because they don't block Fission MVP.

Comment 5

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1651522 - [devtools] Implement server side Service Worker targets.

Comment 6

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1651522 - WIP patch to debug SW test.

Comment 7

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1651522 - [devtools] Enable SW debugging in debugger by default

Comment 8

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1651522 - [devtools] Filter the service workers by domain.

Comment 9

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1651522 - [devtools] Destroy Service Workers on navigation.

I think we should NOT do that as it prevent debugging SW on navigation to another domain.
But we must probably have some helpful information in the frontend.

Comment 10

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1651522 - [devtools] Avoid clearing documents when navigating.

Comment 11

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1651522 - [devtools] Remove the now useless `destroyServiceWorkersOnNavigation` flag on TargetCommand.

Comment 12

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1651522 - [devtools] Fix browser_dbg-windowless-service-workers-reload.js intermittent.

The SW wasn't properly unregistered between tests, probably causing failure in other test using this test page.

Comment 13

[Phabricator Automation]

Comment on attachment 9359392 [details]
Bug 1651522 - [devtools] Avoid clearing documents when navigating.

Revision D191450 was moved to bug 1861941. Setting attachment 9359392 [details] to obsolete.

Comment 14

[Phabricator Automation]

Comment on attachment 9356184 [details]
Bug 1651522 - [devtools] Enable SW debugging in debugger by default

Revision D189825 was moved to bug 1862157. Setting attachment 9356184 [details] to obsolete.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9855542eb321
[devtools] Implement server side Service Worker targets. r=devtools-reviewers,nchevobbe
https://hg.mozilla.org/integration/autoland/rev/f80194bc4d6c
[devtools] Remove the now useless `destroyServiceWorkersOnNavigation` flag on TargetCommand. r=devtools-reviewers,nchevobbe
https://hg.mozilla.org/integration/autoland/rev/7871ba0badea
[devtools] Fix browser_dbg-windowless-service-workers-reload.js intermittent. r=devtools-reviewers,nchevobbe
https://hg.mozilla.org/integration/autoland/rev/81d3ee520ab6
[devtools] Filter the service workers by domain. r=devtools-reviewers,nchevobbe

Comment 17

[Sandor Molnar[:smolnar]]

Backed out for causing dt failures on devtools/shared/commands/target/tests/browser_target_command_service_workers_navigation.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/8d79f4aa58a79c3c19369e71ee6c6a8094c68c2d

Push with failures

Failure log -> TEST-UNEXPECTED-FAIL | devtools/shared/commands/target/tests/browser_target_command_service_workers_navigation.js | Uncaught exception in test bound test_NavigationToPageWithExistingWorker

Comment 18

[Alexandre Poirot [:ochameau]]

I was having a race condition in the new shared helper to unregister a Service worker.
It should be fixed now.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/17dc7ba17c9c
[devtools] Implement server side Service Worker targets. r=devtools-reviewers,nchevobbe
https://hg.mozilla.org/integration/autoland/rev/0ac0cfa3ed67
[devtools] Remove the now useless `destroyServiceWorkersOnNavigation` flag on TargetCommand. r=devtools-reviewers,nchevobbe
https://hg.mozilla.org/integration/autoland/rev/b44436c89af2
[devtools] Fix browser_dbg-windowless-service-workers-reload.js intermittent. r=devtools-reviewers,nchevobbe
https://hg.mozilla.org/integration/autoland/rev/ac5e5139fe3d
[devtools] Filter the service workers by domain. r=devtools-reviewers,nchevobbe

Comment 20

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/17dc7ba17c9c
https://hg.mozilla.org/mozilla-central/rev/0ac0cfa3ed67
https://hg.mozilla.org/mozilla-central/rev/b44436c89af2
https://hg.mozilla.org/mozilla-central/rev/ac5e5139fe3d

Comment 21

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

In bug 1866627 reported against Firefox 122 there are reports of ServiceWorker processes being created/hanging around; is it possible that these changes might lead to a change in the behavior around calls to nsIServiceWorkerInfo::attachDebugger?

Comment 22

[Alexandre Poirot [:ochameau]]

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

In bug 1866627 reported against Firefox 122 there are reports of ServiceWorker processes being created/hanging around; is it possible that these changes might lead to a change in the behavior around calls to nsIServiceWorkerInfo::attachDebugger?

Oh yes, that's a terrible mistake I did here.
These magic calls to {attach,detach}Debugger should only be called for Service Workers which actually relates to the currently debugged context.
At least when opening regular DevTools, debugging a Tab, only Service Workers related to that tab will be impacted.
But the issue would still remain for the Browser Toolbox, which is debugging all the service workers.
For that, we may have to further revisit this.