Closed Bug 1280083 Opened 4 years ago Closed 4 years ago

Support dependencies for bootstrapped add-ons

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: kmag, Assigned: kmag)

Details

(Whiteboard: triaged)

Attachments

(1 file)

This is necessary for WebExtensions Experiments to handle updates correctly.
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/
What happens when the dependency is not installed?
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.
(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.
Attachment #8762716 - Flags: review?(aswan) → review+
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?
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.
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...
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.
Priority: -- → P2
Whiteboard: triaged
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.
Flags: needinfo?(kmaglione+bmo)
(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.
Flags: needinfo?(kmaglione+bmo)
(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.
Flags: needinfo?(kmaglione+bmo)
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")
(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.
Flags: needinfo?(kmaglione+bmo)
https://hg.mozilla.org/mozilla-central/rev/446f5f465b92
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.