1638099 - Mark extension service worker registrations as disabled when an extension does shutdown

Status: RESOLVED FIXED

(WebExtensions :: General, task, P3)

Description

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

As a follow up to Bug 1609920:

When an extension is shutting down, the service worker registered are going to be terminated, but they should also be marked as "disabled" to be sure that they are not going to be started again until the WebExtensions framework does explicitly mark them as "enabled" again (once the extension is fully started again).

Comment 1

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

Attachment: Bug 1638099 - Ensure ContentPrincipal::AddonPolicy does not return a stale WebExtensionPolicy instance. r?mixedpuppy!,asuth!

While working on the patch that follows, I did trigger an unexpected failure in one of the existing
background service worker tests because:

• the extension was being updated from version 1 to version 2 as part of the test case
• the principal associated to the existing service worker registration was still returning
  (through ContentPrincipal::AddonPolicy) an instance to the WebExtensionPolicy instance
  that was active when ContentPrincipal::AddonPolicy instance was called for the first time
  while version 1 was being started
• as a result ServiceWorkerPrincipal::SpawnWorkerIfNeeded aborts the registration job
  because of the additional check added in the patch that follows.

An alternative option to prevent the particular issue related to the extension service workers
could be to refresh the principal associated to the existing registration in
ServiceWorkerRegisterJob::AsyncExecute with the new principal got from the call to
ServiceWorkerManager::RegisterForAddonPrincipal, but I was a bit concerned that the current behavior
of ContentPrincipal::AddonPolicy could lead to some other subtle issues (e.g. if a stale
WebExtensionPolicy instance is being used for some other kind of checks, like permissions-related ones,
without also checking if the instance is the active and current one) and I decided to attach this
additional patch to evaluate the approach that we prefer.

This patch may likely require an additional blocking reviewer, to be double-checked and added
after an initial feedback round from Shane and Andrew.

Depends on D119530

Comment 2

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

Attachment: Bug 1638099 - Ensure spawning a new service worker for a disabled webextension does fail. r?asuth!

This patch does prevent a service worker previously registered by a webextension to be spawned
if the webextension isn't enabled.

Instead of marking the service worker as disabled (e.g. as part of the registration data that we
store on disk), this patch is currently adding an additional check in
ServiceWorkerPrivateImpl::SpawnIfNeeded to make it early exit with an error if:

• the worker principal has a moz-extension url
• and it does not have a WebExtensionPolicy or the WebExtensionPolicy is not active
  (which would mean that the extension was being uninstalled or disabled)

This additional check along with:

• as part of the extension shutdown the extension background workers are explicitly terminated
  (and so there should be any still running after that)
• as part of the extension uninstall flow all the extension background workers are being unregistered

Should prevent the WebExtension background service worker from being spawned after the extension
has been unable and disabled.

As a side note, "allowing an extension to register other service workers besides the background
service worker defined in the extension manifest.json file" is locked behind a separate pref,
and we are not yet planning to enable that.

Before enabling that, we should also make sure to terminate also all other service workers that
the same extension may have registered (I'll file a separate follow up for that and make it a
blocker for Bug 1344561, which is tracking general service worker API support in extensions).

TODO:

• [x] figure out a reasonable testing strategy to cover this behavior
• [ ] double-check with Andrew assertion crash triggered by manually forcing a SpawnWorkerIfNeeded failure
  (due to ServiceWorkerUpdateJob::Finish being called twice when SpawnWorkerIfNeeded aborts
  the ServiceWorkerUpdateJob, called once from ServiceWorkerUpdateJob::Update and once more
  from ServiceWorkerUpdateJob::ComparisonResult)

Depends on D119531

Comment 3

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

Attachment: Bug 1638099 - Add xpcshell test to explicitly assert that service workers fail to spawn if the addon is disabled. r?asuth!,mixedpuppy!

This patch introduces a new xpcshell test that covers the behavior expected by the changes
applied from D119532.

The current testing strategy is based on the same Firefox DevTools internal API that about:debugging
is using to detect when a worker is being spawned and terminated, and it does also use the same
strategy used by about:addons to force spawn a registered service worker (by calling the
nsIServiceWorkerRegistrationInfo.attachDebugger method).

TODO:

• [x] double-check with ochameau that the test is using the DevTools API correctly,
  without anything not actually necessary (and in a non invasive way, we don't want
  to rely on any part of the DevTools API that is not supposed to be called directly,
  or that may have to be removed or replaced soon)
• [x] after a feedback round from a DevTools engineer, add an additional reviewer from the
  WebExtensions team (which also help to make sure the testing approach is properly
  shared with at least one other engineer in the team).

Depends on D119532

Comment 4

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

Attachment: Bug 1638099 - Unregister all extension service workers on extension shutdown if the app is not also shutting down. r?mixedpuppy!

This patch introduces changes to parent/ext-backgroundPage.js and Extension's shutdown methods to make sure
that all service workers registered by an extension are unregistered when the extension is shutting down,
unless the application is also shutting down (in which case the registration is not unregistered because
for the already installed extenson the previously activeWorker is expected to be still active across browser
restarts).

These changes prevent also to hit the issue that D119532 was triggering when an extension was reloaded
and it does not need any of the changes to ContentPrincipal::AddonPolicy from D119531.

Comment 5

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

Attachment: Bug 1638099 - Cover with an explicit xpcshell test the expected service worker lifecycle events on addon reload and browser restart. r?asuth!,mixedpuppy!

This patch introduce an additional xpcshell test task (built on top of the other one added in D119799)
which cover explicitly the following behaviors:

• the extension service worker registrations are expected to:
  • not be unregistered if the addon is shutting down as part of the application shutdown
  • to be unregistered if the addon is disabled (while the application is not shutting down)
• the extension service worker is expected to:
  • receive the "install" and "activate" lifecycle events when the service worker is registered for the first time
    or if the addon is disabled and then re-enabled while the application is not shutting down
  • not receive the "install" and "activate" lifecycle events when a previously active worker
    is spawned again (in particular after the entire application is restarted)

Ideally these behavior shoud be tested by restarting the browser instance as in a real Browser instance
but asserting all these behavior from a marionette test would be a bit trickier (but I still plan to
take a look into that, at least to cover with a marionette test a subset of the expected behaviors on
browser restart).

As an additional side note, the test case introduced in this test is currently assuming that we
may not proceed with D119531, and so to avoid to hit the issue described in that patch description
this patch is currently introducing an additional test-only method to nsIServiceWorkerManager
(named reloadRegistrationsForTest) which currently does the bare minimum to mock the "Browser restart"
scenario.

If on the contrary, we decide to proceed further with D119531, the new nsIServiceWorkerManager.reloadRegistrationsForTest
helper would not be needed anymore (because we would just use the existing ServiceWorkerRegistrationInfo instance which
is basically in the same state that the new one loaded from the ServiceWorkerRegistar would be, besides the issue with the
stale WebExtensionPolicy instance stored in the principal associated to the previous service worker registration).

Depends on D119799

Comment 6

[Phabricator Automation]

Comment on attachment 9230498 [details]
Bug 1638099 - Ensure ContentPrincipal::AddonPolicy does not return a stale WebExtensionPolicy instance. r?mixedpuppy!,asuth!

Revision D119531 was moved to bug 1722298. Setting attachment 9230498 [details] to obsolete.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8a7bdba4ffa7
Unregister all extension service workers on extension shutdown if the app is not also shutting down. r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/5e1dc6e286ad
Ensure spawning a new service worker for a disabled webextension does fail. r=asuth
https://hg.mozilla.org/integration/autoland/rev/369a6610d10f
Add xpcshell test to explicitly assert that service workers fail to spawn if the addon is disabled. r=asuth,mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/5ffc908676c3
Cover with an explicit xpcshell test the expected service worker lifecycle events on addon reload and browser restart. r=asuth,mixedpuppy

Comment 8

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/8a7bdba4ffa7
https://hg.mozilla.org/mozilla-central/rev/5e1dc6e286ad
https://hg.mozilla.org/mozilla-central/rev/369a6610d10f
https://hg.mozilla.org/mozilla-central/rev/5ffc908676c3

Comment 9

[Geoff Lankow (:darktrojan)]

Thunderbird's failing this new test (example) and I don't understand it enough to work out why. Any ideas?

Comment 10

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

(In reply to Geoff Lankow (:darktrojan) from comment #9)

Thunderbird's failing this new test (example) and I don't understand it enough to work out why. Any ideas?

Follows some background context about what that failure means and what may be triggering it.

I'm going to re-assign a needinfo to me to take a more deeper look after having reproduced it locally (to pin point exactly what would fix the test failure on Thunderbird).

As a side note this test is covering a feature that is still in development and not used in release builds yet, and so it would be ok to skip the test on Thunderbird until we figure this out.

If you decide to do so, feel free to file an issue to disable the test on Thunderbird (and send a patch my way for sign-off from our side) and a follow up to re-enable it (needinfo me on it, and asap I'll comment there with more details and possibly a patch to re-enable the test).

---

I haven't tried yet to reproduce it locally, but based on the failure logs it looks that

• the test is hitting the assertion introduced in D119532 (see the debug warning "Trying to wake up a service worker for a disabled webextension.", which was logged right before the assertion failure).

• the assertion failure looks exactly like the one I did mention at the end of comment 2, in Firefox I couldn't triggered that assertion anymore after introducing in D120351 the test-only helper method nsIServiceWorkerManager.reloadRegistrationsForTest

• but based on the log from the test failures it look that in that run the service worker from the extension service worker registration was being spawned before we get to call that nsIServiceWorkerManager method (which I think it why the test is able to trigger the assertion failure on ServiceWorkerUpdateJob) and so:

  • the application was definitely spawning the old registration (instead of the refreshed one that reloadRegistrationForTest would create)
  • and that triggers the issue described in comment 1
  • and then the ServiceWorkerUpdateJob assertion failure as a side effect of that

Comment 11

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

Adding back needinfo assigned to me, as a reminder to look more into the assertion failure as currently triggered in Thunderbird while running the test added in D120351.

Comment 12

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

Hey Geoff,
I figured out what is different in Thunderbird and make the xpcshell test able to trigger that assertion failure consistently, the test expects the pref "extensions.webextensions.background-delayed-startup" to be set to true, but it is set to false on thunderbird.

I do have a small (few lines) patch that make the xpcshell test to pass consistently on both Firefox and Thunderbird, do you have already filed an issue to fix the test failure for Thunderbird?
If you didn't yet, I can file one and attach the patch to the newly filed bug, otherwise I'm happy to attach to an existing thunderbird issue if we already have one.

Comment 13

[Geoff Lankow (:darktrojan)]

No, I didn't open a new bug for this one. Since you're likely offline I'll do it now.