1329021 - Embedded WebExtensions need shutdown method to shut down properly

Status: RESOLVED FIXED

(WebExtensions :: General, defect, P1)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

If I write an embedded WebExtension and the bootstrap.js file doesn't have a shutdown method, then reloading the extension gives me this exception:

JavaScript error: resource://gre/modules/LegacyExtensionsUtils.jsm, line 134: Error: This embedded extension has already been started

I think the problem is that, before we get here:
http://searchfox.org/mozilla-central/rev/82ebc0c5ea45477ef4a4bfb3bbff35069bd18cee/toolkit/mozapps/extensions/internal/XPIProvider.jsm#4894
we exit here:
http://searchfox.org/mozilla-central/rev/82ebc0c5ea45477ef4a4bfb3bbff35069bd18cee/toolkit/mozapps/extensions/internal/XPIProvider.jsm#4862

Crucially, none of the example code includes shutdown methods.

Comment 1

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

That's pretty strange that we didn't catch it with the reload test case:

https://dxr.mozilla.org/mozilla-central/rev/b548da4e16f067e5b69349376e37b2db97983cf7/toolkit/mozapps/extensions/test/xpcshell/test_webextension_embedded.js#213

I'll look into it.

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

That test defines a shutdown method.

Comment 3

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

(In reply to Bill McCloskey (:billm) from comment #2)
> That test defines a shutdown method.

oh yep, I was reading Comment 0 wrong, the early exit on the missing shutdown method is definitely the reason.

I'm going to prepare a proposed fix and a new test case for it.

Comment 4

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

Attachment: Bug 1329021 - Stop an embedded webextension when a shutdown bootstrap method is not defined.

Review commit: https://reviewboard.mozilla.org/r/102870/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/102870/

Comment 5

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

Pushed to try:
- https://treeherder.mozilla.org/#/jobs?repo=try&revision=bfc9961048ebdcfef3f0524a51025179632bfda2

Comment 6

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

Comment on attachment 8824347 [details]
Bug 1329021 - Stop an embedded webextension when a shutdown bootstrap method is not defined.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102870/diff/1-2/

Comment 7

[Andrew Swan [:aswan]]

Comment on attachment 8824347 [details]
Bug 1329021 - Stop an embedded webextension when a shutdown bootstrap method is not defined.

https://reviewboard.mozilla.org/r/102870/#review103492

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:4873
(Diff revision 2)
>        if (!method) {
>          logger.warn("Add-on " + aAddon.id + " is missing bootstrap method " + aMethod);
> +
> +        if (aAddon.hasEmbeddedWebExtension && aMethod == "shutdown") {
> +          LegacyExtensionsUtils.getEmbeddedExtensionFor(params).shutdown();
> +        }
> +
>          return;
>        }

Instead of having the call to `.shutdown()` on the webextension in two places, can we just move this whole check until after line 4907 below?
The code related to dependent addons should probably also run even if this extension doesn't have a shutdown method -- it doesn't really apply right now since the only extensions that use dependencies are experiments and those use a fixed bootstrap file that does have a shutdown method but still, I think this would be more correct and less prone to future bugs.

Comment 8

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

Comment on attachment 8824347 [details]
Bug 1329021 - Stop an embedded webextension when a shutdown bootstrap method is not defined.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102870/diff/2-3/

Comment 9

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

Comment on attachment 8824347 [details]
Bug 1329021 - Stop an embedded webextension when a shutdown bootstrap method is not defined.

https://reviewboard.mozilla.org/r/102870/#review103492

> Instead of having the call to `.shutdown()` on the webextension in two places, can we just move this whole check until after line 4907 below?
> The code related to dependent addons should probably also run even if this extension doesn't have a shutdown method -- it doesn't really apply right now since the only extensions that use dependencies are experiments and those use a fixed bootstrap file that does have a shutdown method but still, I think this would be more correct and less prone to future bugs.

Yeah, I agree with your points and I think that it makes sense.

In the updated patch I moved the block below, and I removed the early return (it is now one of the branches of the actual bootstrap method execution), so that we do not skip what comes after the bootstrap method call (let's see how the try push goes).

Comment 10

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

Push to try (of the updated patch):

- https://treeherder.mozilla.org/#/jobs?repo=try&revision=580cd167643ca8c789eb4e67bdf73ddf2ab99e1e

Comment 11

[Andrew Swan [:aswan]]

Comment on attachment 8824347 [details]
Bug 1329021 - Stop an embedded webextension when a shutdown bootstrap method is not defined.

https://reviewboard.mozilla.org/r/102870/#review103964

Comment 12

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

Comment on attachment 8824347 [details]
Bug 1329021 - Stop an embedded webextension when a shutdown bootstrap method is not defined.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102870/diff/3-4/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b81a0ab1e4ee
Stop an embedded webextension when a shutdown bootstrap method is not defined. r=aswan

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b81a0ab1e4ee