1609920 - Allow WebExtensions Framework to register a moz-extension:// script as a service worker

Status: RESOLVED FIXED

(WebExtensions :: General, enhancement, P1)

Description

[Luca Greco [:rpl] [:luca] [:lgreco]]

This issue is part of the blockers for Bug 1573659 (tracker for the manifest v3 background.service_worker implementation).

Part of this issue goals:

• allow the WebExtensions Framework to ensure that the extension background.service_worker is registered (possibly without creating a webextension page to register it using the WebAPI exposed to the webpages, window.navigator.serviceWorker.register)

• add the background.service_worker property definition to the manifest's JSON schema, and lock it behind a preference (and set it to be disallowed by default)

• ensure that the moz-extension:// service worker are being spawned only in child processes with the "extension" remoteType

• allow the WebExtensions Framework to be sure that all the moz-extensions:// workers are being terminated while the related extension is shutting down

non-goals for this issue:

• allow an extension to register its own moz-extension:// service workers (tracked by Bug 1344561, will be evaluated separately, in the meantime this issue should lock that ability behind a separate about:config preference which disallow it by default)

• provide WebExtension APIs bindings to the background service worker (which is going to be tracked by a separate bugzilla issue, Bug 1609923)

Comment 1

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1609920 - part 1: Allow the WebExtension Framework to register a moz-extension service worker.

• Adds the new about:config pref "extensions.backgroundServiceWorker.enabled" (currently defaults to false).
• Adds the background.service_worker property to the manifest JSON schema definition
• Locks background.service_worker manifest property behind the new preference
• Adds a new BackgroundWorker class to ext-backgroundPage.js (responsible for managing the background
  service worker for the extension, e.g. make sure that the expected worker script is registered
  as expected when the extension is starting up)
• Adds to the ServiceWorkerManager a new method to allow the WebExtension Framework to register the
  background service worker without an existing extension page
• Allows the "moz-extension" schema in the dom/serviceworkers and dom/cache internals

Comment 2

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1609920 - part 2: Guard ServiceWorkerContainer::Register to allow/disallow moz-extension scheme based on prefs.

• Adds a new "extensions.serviceWorkerRegister.allowed" pref (defaults to false)
• Makes ServiceWorkerContainer::Register to throw NS_ERROR_DOM_SECURITY_ERR if the
  script url is a moz-extension url and the caller is a non-system caller
  (but do not throw NS_ERROR_DOM_SECURITY_ERR if the caller is a moz-extension
  and the new pref is set to true)

Depends on D60244

Comment 3

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1609920 - part 3: Terminate extension background workers on shutdown and unregister on uninstall.

• Include a remoteType property to the RemoteWorkerData
• Changes to RemoteWorkerManager::LaunchNewContentProcess to launch a new process with remoteType "extension"
  when the service worker is from an extension
• Changes to RemoteWorkerManager::SelectTargetActorForServiceWorker to select an actor spawner in a child
  process with remoteType "extension" when the service worker is from an extension
• Changes to RemoteWorkerManager::RegisterActor does not launch an extension service worker
  in a non-"extension" remoteType" child process

Depends on D60245

Comment 4

[Stuart Colville [:muffinresearch]]

:asuth :perry hi, do you have any idea on when you would have time to move forward with the review?

Comment 5

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

We're currently focused on a parent-intercept ServiceWorker regression on trunk (Bug 1622451) but when that's addressed there should be some bandwidth.

Comment 6

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1609920 - part 4: Add new mochitests and marionette test related to the new background service worker prefs.

Comment 7

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

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:rpl, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 8

[Luca Greco [:rpl] [:luca] [:lgreco]]

The patches attached to this issue depend on the changes applied in Bug 1642676, which I pushed to autoland yesterday and they did reach mozilla-central today.
I'm going to land these on Nightly 80 as well.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0e68db4ad6af
part 1: Allow the WebExtension Framework to register a moz-extension service worker. r=dom-workers-and-storage-reviewers,asuth,mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/fd1e4db7cf78
part 2: Guard ServiceWorkerContainer::Register to allow/disallow moz-extension scheme based on prefs. r=dom-workers-and-storage-reviewers,asuth,webidl,smaug
https://hg.mozilla.org/integration/autoland/rev/eaa0bb2cf36b
part 3: Terminate extension background workers on shutdown and unregister on uninstall. r=dom-workers-and-storage-reviewers,asuth,mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/1c8faab05606
part 4: Add new mochitests and marionette test related to the new background service worker prefs. r=marionette-reviewers,asuth,whimboo,mixedpuppy

Comment 10

[Dorel Luca [:dluca]]

Backed out 4 changesets (bug 1609920) for leaks in browser-chrome. CLOSED TREE

Log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=308197231&repo=autoland&lineNumber=27005

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&revision=1c8faab05606ec8314fa008cfff15df65f304865

Backout:
https://hg.mozilla.org/integration/autoland/rev/728bf9ad480a59d7dfeacff4265c86e0990938af

Comment 11

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Dorel Luca [:dluca] from comment #10)

Backed out 4 changesets (bug 1609920) for leaks in browser-chrome. CLOSED TREE

Log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=308197231&repo=autoland&lineNumber=27005

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&revision=1c8faab05606ec8314fa008cfff15df65f304865

Backout:
https://hg.mozilla.org/integration/autoland/rev/728bf9ad480a59d7dfeacff4265c86e0990938af

I did spent some time on looking into the failures triggered by LeakSanitizer in the asan build, but I'm not sure that they are really introduced or triggered by this set of patches.
As an example of something that suggest the opposite (the failure may be unrelated), the same leaks seems to be detected even without these patches by running those tests on the mozilla-central tip here:

• https://treeherder.mozilla.org/#/jobs?repo=try&revision=746c0bead63021f04707b6da2733d7d4af3c1e40&selectedTaskRun=LNhykVtXS_a1dJ2DFFPxMg.1

Besides that, by looking to the tests that are running as part of that job, I'm surprised that they would be able to trigger anything new that the attached patches may do (because most of the changes are actually locked behind a pref, which would keep most of the changes inactive in those set of tests, only a very small fragment of the changes may be able to actually be used while these tests are running)

would it be possible to double-check if I'm right (as the push to try linked above seems to suggest) and those failure are not really triggered by this patches?

Comment 12

[Luca Greco [:rpl] [:luca] [:lgreco]]

Hi :mccr8 and :sfoster
The patches attached to this issue have been backed out for some LeakSanitizer failures in browser/components/downloads/test/browser/ tests (See link to the failures in comment 10).

As I did mention in comment 11, I don't see (at least not yet) how the patches backed out are able to trigger that failure (or make it more frequent), and it looks that I can trigger the same (or very similar) failures without any of those patches on a mozilla-central tip (see link to the try push in comment 11).

Could you take a look to the failure in comment 10 and see if you can spot anything that could give us some pointers to get it fixed (or how to investigate it further, or if you think that it is unrelated to the patches attached to this bugzilla issue)?

Thanks in advance for your help!

Comment 13

[Sam Foster [:sfoster] (he/him)]

So it looks like it encountered problems running browser/components/downloads/tests/browser/browser_pdfjs_preview.js, which loads a minimal PDF document into a new tab using pdf.js, and that seems to be where at least some of the leaks are coming from. For context, that test opens "downloaded" PDF files into tabs by interacting with the various download lists - the downloads panel, the library -> all downloads window, the about:downloads view, in both non-private and private windows.

I have no idea how this service worker / moz-extension:// script work would have any impact on that.
I did see some asan test failures in try runs while I've been working on these tests over the last couple of weeks. It might be the most expedient step for now is just to disable this test for asan builds.

Comment 14

[Sam Foster [:sfoster] (he/him)]

Brendan, do the logs from comment 10 mean anything more to you? The browser_pdfjs_preview.js test logs start around line 1740 I'm afraid most of the leak sanitizer output is gobbledy-gook to me.

Comment 15

[Andrew McCreight [:mccr8]]

The leak sanitizer output basically means that we're leaking lots of DOM stuff. This particular leak has been happening frequently but only intermittently for a few weeks now.

It does seem unambiguous that it became a permanent failure with your push, and it also seems to have gone away when your patch got backed out: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedTaskRun=ejWvA3HoRWaFEap_i_tMMA.0&searchStr=bc6%2Clinux&revision=728bf9ad480a59d7dfeacff4265c86e0990938af

....but then about 2 hours later it turned into a permafailure again, which seems to have made it all the way to mozilla-central. I'm not sure what is going on here.

Comment 16

[Andrew McCreight [:mccr8]]

I think I've found the second regressor for this leak. I'll ask sheriffs to back it out once I've got a bit more confirmation.

Comment 17

[Andrew McCreight [:mccr8]]

That was bug 1645538. I wonder if changes to prefs are somehow causing issues. I don't know how that would happen, exactly.

Comment 18

[Andrew McCreight [:mccr8]]

Ok, that wasn't actually a regression. It just added a test and caused the test failure to move from bc12 to bc6. Maybe your patch did something similar. You'd want to look for another ASan bc hitting that same failure on the changeset before you landed, and then look at the raw logs to double check that it contains a reference to browser/components/downloads/test/browser/browser_pdfjs_preview.js

Comment 19

[Andrew McCreight [:mccr8]]

Yeah, it looks like it was failing in bc12 before your patch landed, so it should be okay to reland your patch (assuming this was the only failure it was backed out for). I'm still not sure what was causing it, but it appears it wasn't your patch. Thanks to Aryx for reminding me about tests hopping chunks...

Comment 20

[Sam Foster [:sfoster] (he/him)]

Note that the browser_pdfjs_preview.js test is relatively new: https://hg.mozilla.org/mozilla-central/log/tip/browser/components/downloads/test/browser/browser_pdfjs_preview.js - it only landed this May.

That test does set and unset a pref while it runs using the pushPrefEnv/popPrefEnv helpers, but it would be odd for bug 1645538 to have an impact on this one test that wasn't seen in the many 100s of other test that use them? Unless there is something particular about this test (which isn't impossible as it employs private & not-private windows and other details that combined could make it unique I guess?)

Comment 21

[Andrew McCreight [:mccr8]]

The failure doesn't have anything to do with the patch in this bug. It just added or removed a test, which caused the failure to move from bc12 to bc6, so it looked like a regression, but it was just shifting it.

Comment 22

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1609920 - Skip browser_pdfjs_preview.js in asan builds due to frequent failures. r?mak

Depends on D73326

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f567e48e5957
part 1: Allow the WebExtension Framework to register a moz-extension service worker. r=dom-workers-and-storage-reviewers,asuth,mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/3e35dbe7e66f
part 2: Guard ServiceWorkerContainer::Register to allow/disallow moz-extension scheme based on prefs. r=dom-workers-and-storage-reviewers,asuth,webidl,smaug
https://hg.mozilla.org/integration/autoland/rev/8ca84442f85d
part 3: Terminate extension background workers on shutdown and unregister on uninstall. r=dom-workers-and-storage-reviewers,asuth,mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/fbcb0ee2d17a
part 4: Add new mochitests and marionette test related to the new background service worker prefs. r=marionette-reviewers,asuth,whimboo,mixedpuppy

Comment 24

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/f567e48e5957
https://hg.mozilla.org/mozilla-central/rev/3e35dbe7e66f
https://hg.mozilla.org/mozilla-central/rev/8ca84442f85d
https://hg.mozilla.org/mozilla-central/rev/fbcb0ee2d17a