Closed Bug 1329021 Opened 6 years ago Closed 6 years ago

Embedded WebExtensions need shutdown method to shut down properly

Categories

(WebExtensions :: General, defect, P1)

defect

Tracking

(firefox53 fixed)

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: billm, Assigned: rpl)

Details

(Whiteboard: triaged)

Attachments

(1 file)

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.
That test defines a shutdown method.
(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.
Assignee: nobody → lgreco
Status: NEW → ASSIGNED
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 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).
Priority: -- → P1
Whiteboard: triaged
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
Attachment #8824347 - Flags: review?(aswan) → review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b81a0ab1e4ee
Stop an embedded webextension when a shutdown bootstrap method is not defined. r=aswan
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b81a0ab1e4ee
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.