1280083 - Support dependencies for bootstrapped add-ons

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect, P2)

Description

[Kris Maglione [:kmag]]

This is necessary for WebExtensions Experiments to handle updates correctly.

Comment 1

[Kris Maglione [:kmag]]

Attachment: Bug 1280083: Support dependencies for bootstrapped add-ons.

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

Comment 2

[Kris Maglione [:kmag]]

Comment on attachment 8762716 [details]
Bug 1280083: Support dependencies for bootstrapped add-ons.

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

Comment 3

[Andy McKay]

What happens when the dependency is not installed?

Comment 4

[Kris Maglione [:kmag]]

The add-on won't be enabled. But that's not going to be an issue in practice, since this will only be used internally.

Comment 5

[Andy McKay]

(In reply to Kris Maglione [:kmag] from comment #4)
> The add-on won't be enabled. But that's not going to be an issue in
> practice, since this will only be used internally.

Sounds good, thanks.

Comment 6

[Andrew Swan [:aswan]]

Comment on attachment 8762716 [details]
Bug 1280083: Support dependencies for bootstrapped add-ons.

https://reviewboard.mozilla.org/r/59230/#review56408

The implementation and test look sound, though I think it would be helpful to document some of the details (e.g., the fact that there's no effort to actually install dependencies, an add-on is simply appDisabled if a dependency is not installed)

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:2437
(Diff revision 1)
>    // Flag to know if ToolboxProcess.jsm has already been loaded by someone or not
>    _toolboxProcessLoaded: false,
>    // Have we started shutting down bootstrap add-ons?
>    _closing: false,
>  
> +  sortBootstrappedAddons: function() {

Adding a comment here would be helpful, e.g. "returns all bootstrapped addons in an order in which every add-on always comes after any of its own dependencies"

::: toolkit/mozapps/extensions/test/xpcshell/test_dependencies.js:73
(Diff revision 1)
> +    let dir = AM_Cc["@mozilla.org/file/local;1"].createInstance(AM_Ci.nsIFile);
> +    dir.initWithPath("/home/kris");
> +    // addonFiles[addonFiles.length - 1].copyTo(dir, null);

remove this?

Comment 7

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/59230/#review56408

> Adding a comment here would be helpful, e.g. "returns all bootstrapped addons in an order in which every add-on always comes after any of its own dependencies"

Yeah, I meant to add a doc comment when I was done, but I forgot.

> remove this?

Well, that's embarrassing.

Comment 8

[Andrew Swan [:aswan]]

https://reviewboard.mozilla.org/r/59230/#review57348

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:4823
(Diff revision 2)
> +      // Extensions are automatically deinitialized in the correct order at shutdown.
> +      if (aMethod == "shutdown" && aReason != BOOTSTRAP_REASONS.APP_SHUTDOWN) {
> +        activeAddon.disable = true;
> +        for (let addon of this.getDependentAddons(aAddon)) {
> +          if (addon.active)
> +            this.updateAddonDisabledState(addon);

Thinking about this further while reviewing 1263011, it occurs to me that if the dependent add-on is not restartless this won't actually disable it.
I realize that's not an issue for the immediate intended application of this capability since it is meant for webextensions which are always restartless.
But it also doesn't play nicely with bug 1231172.  I'm not sure what the right thing to do about that is though...

Comment 9

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/59230/#review57348

> Thinking about this further while reviewing 1263011, it occurs to me that if the dependent add-on is not restartless this won't actually disable it.
> I realize that's not an issue for the immediate intended application of this capability since it is meant for webextensions which are always restartless.
> But it also doesn't play nicely with bug 1231172.  I'm not sure what the right thing to do about that is though...

Yes, this is only intended to work with bootstrapped add-ons. I already talked to rhelmer about bug 1231172. We'll probably have to come up with a better solution, at some point, but as long as this is in the prototype stage, I'm fine with just not allowing add-ons with dependencies to delay restarts when their dependencies have updates.

Comment 10

[Dave Townsend [:mossop]]

Can you give me some more details on why this is needed and what constraints we expect to have on this feature? We had dependencies a long time ago and supporting even the bare essentials became too complicated so I'm wary of re-introducing it.

Comment 11

[Kris Maglione [:kmag]]

(In reply to Dave Townsend [:mossop] from comment #10)
> Can you give me some more details on why this is needed and what constraints
> we expect to have on this feature? We had dependencies a long time ago and
> supporting even the bare essentials became too complicated so I'm wary of
> re-introducing it.

For the moment, this is only for internal use, to allow extension APIs to be
written as extensions. When we have bootstrapped add-ons that depend on APIs
provided by other bootstrapped add-ons, we need to handle updates (and
ideally startup order) carefully.

In particular, if the API extension needs to be updated, then we need to:

1) Shut down any extensions that depend on it.
2) Shut down the extension itself.
3) Update the extension, and restart it.
4) Restart any extensions that rely on it.

If we don't do that, then either any running extensions that depend on the API
extensions will either be broken, or will keep code from the previous version
alive.

There are ways to handle this outside of the add-on manager, but they're much
more complicated, and less reliable.

Comment 12

[Dave Townsend [:mossop]]

(In reply to Kris Maglione [:kmag] from comment #11)
> (In reply to Dave Townsend [:mossop] from comment #10)
> > Can you give me some more details on why this is needed and what constraints
> > we expect to have on this feature? We had dependencies a long time ago and
> > supporting even the bare essentials became too complicated so I'm wary of
> > re-introducing it.
> 
> For the moment, this is only for internal use, to allow extension APIs to be
> written as extensions. When we have bootstrapped add-ons that depend on APIs
> provided by other bootstrapped add-ons, we need to handle updates (and
> ideally startup order) carefully.

Who writes these extension APIs? Is it us and can we enforce that they can never become disabled, have specific forms of IDs and then add-ons can only depends on those forms? That would go a long way to reducing the number of edge cases that exist here.

Comment 13

[Andrew Swan [:aswan]]

I think you could take out the code that reads dependencies from install.rdf and this would continue to work as intended for webextensions where dependencies are computed during manifest parsing.  All your existing tests would break and a new strategy would be needed though...
If extra protection against abuse is desired, the code that actually walks dependencies could refuse to start an addon that depends on anything other than an apiextension (which you could do by checking the type directly or adding a new computed property called something like "isValidDependency")

Comment 14

[Kris Maglione [:kmag]]

(In reply to Dave Townsend [:mossop] from comment #12)
> Who writes these extension APIs? Is it us and can we enforce that they can
> never become disabled, have specific forms of IDs and then add-ons can only
> depends on those forms? That would go a long way to reducing the number of
> edge cases that exist here.

They're meant to be written and maintained by the community, but approved and
distributed by us. The final details aren't decided yet, but until they are,
this is going to be a non-release-only feature.

We can't enforce that they never become disabled, because we need to support
updates, and we don't want them to be loaded until they're needed. But, yes, I
think that limiting this to API add-ons makes sense. I don't intend for this
to ever be used by add-ons in general.

(In reply to Andrew Swan [:aswan] from comment #13)
> I think you could take out the code that reads dependencies from install.rdf
> and this would continue to work as intended for webextensions where
> dependencies are computed during manifest parsing.  All your existing tests
> would break and a new strategy would be needed though...

Before this rides the trains to release builds, I agree that the install.rdf
dependency properties should be removed or limited to test runs.

Comment 15

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/446f5f465b92e93c7389037bb3c8721ed43235ef
Bug 1280083: Support dependencies for bootstrapped add-ons. r=aswan

Comment 16

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/446f5f465b92