Closed Bug 1295732 Opened 3 years ago Closed 3 years ago

allow system add-on upgrade to override defaults per-addon-ID

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox49 --- fixed
firefox50 --- fixed
firefox51 --- fixed

People

(Reporter: rhelmer, Assigned: rhelmer)

References

Details

User Story

As an add-on developer wanting to deploy or update my add-on as a system add-on, I should only have to sign, package and upload my add-on, and not be responsible for resigning, repacking or reuploading other add-ons (built-in or otherwise).

Attachments

(3 files, 1 obsolete file)

Right now, system add-ons switch install locations when an update is available - that is, the "default" built-in location is used unless there are *any* updates installed, in which case the "default" location is disabled and the "updates" location is used.

This is problematic for folks that want to ship updates, because they must package, sign and upload *all* system add-ons that should continue to be enabled as updates.

AddonManager has a concept of per-ID precedence, for example this is used to allow normal add-ons to install "over" system add-ons, and temporary add-ons to install "over" both of these. We should use this to allow "updates" to install over "defaults".

See also bug 1273709 comment 18.
Status: NEW → ASSIGNED
Component: Add-on Manager → Add-ons Manager
Product: Firefox for Android → Toolkit
Blocks: 1292029
No longer depends on: 1292029
Since we're going to do still require restart in the meantime, we don't need to block on bug 557710.
No longer depends on: 557710
Duplicate of this bug: 1273709
Blocks: 1281347
Comment on attachment 8781908 [details]
Bug 1295732 - update system add-on spec to reflect override behavior of updates

https://reviewboard.mozilla.org/r/72228/#review69924
Attachment #8781908 - Flags: review?(mkelly) → review+
User Story: (updated)
Comment on attachment 8781892 [details]
Bug 1295732 - allow system add-ons updates to override defaults per addon ID

https://reviewboard.mozilla.org/r/72220/#review69982

The same line item comment on the system_reset test applies to system_update.  This is my first time looking at this code and I'm having a hard time following it.
Related but probably out of scope for this bug: the heavy overloading of the name "system addons" to mean both addons that ship with Firefox and addons installed in shared system-wide directories is unfortunate, but I suspect it is probably impractical to change either one without creating a ton of unnecessary change.  One incremental change that would help me though would be to explicitly put "updates" into the install location that is currently just called SYSTEM_ADDONS.  Again though, that's probably out scope for this bug, just recording my impressions here on my first time through any system add-ons related code.

::: toolkit/mozapps/extensions/test/xpcshell/test_system_reset.js:33
(Diff revision 2)
>    let uuidGen = AM_Cc["@mozilla.org/uuid-generator;1"].
>                  getService(AM_Ci.nsIUUIDGenerator);
>    return uuidGen.generateUUID().toString();
>  }
>  
> -function* check_installed(inProfile, ...versions) {
> +function* check_installed(inProfiles, ...versions) {

It would be more verbose but I think also more readable if instead this took an array of objects, each of which has a `verion` and `inProfile` property rather than the parallel array and spread args.
Attachment #8781908 - Attachment is obsolete: true
Comment on attachment 8781892 [details]
Bug 1295732 - allow system add-ons updates to override defaults per addon ID

https://reviewboard.mozilla.org/r/72220/#review69982

Totally agree, we discussed in IRC and will file followup bugs to do some renaming.

> It would be more verbose but I think also more readable if instead this took an array of objects, each of which has a `verion` and `inProfile` property rather than the parallel array and spread args.

Yes that was lazy of me, thanks.
(In reply to Robert Helmer [:rhelmer] from comment #10)
> Comment on attachment 8781892 [details]
> Bug 1295732 - allow system add-ons updates to override defaults per addon ID
> 
> https://reviewboard.mozilla.org/r/72220/#review69982
> 
> Totally agree, we discussed in IRC and will file followup bugs to do some
> renaming.

Bug 1296398
Comment on attachment 8782722 [details]
Bug 1295732 - update system add-on spec to reflect override behavior of updates

https://reviewboard.mozilla.org/r/72770/#review70526
Attachment #8782722 - Flags: review?(mkelly) → review+
Comment on attachment 8781892 [details]
Bug 1295732 - allow system add-ons updates to override defaults per addon ID

https://reviewboard.mozilla.org/r/72220/#review70554

Sorry for the naming nitpicking but I had a hard time getting started understanding the existing tests and the naming was a big part of that.
Feel free to leave potential cleanup of naming for a follow-up though.

::: toolkit/mozapps/extensions/test/xpcshell/test_system_reset.js:7
(Diff revision 6)
>  // application versions
>  const PREF_SYSTEM_ADDON_SET = "extensions.systemAddonSet";
>  
>  BootstrapMonitor.init();
>  
>  const featureDir = FileUtils.getDir("ProfD", ["features"]);

Sorry to keep on this but I'm having a very hard time following this test with the current naming.  This is actually the updates directory right?  Can we rename it to reflect that?

::: toolkit/mozapps/extensions/test/xpcshell/test_system_reset.js:42
(Diff revision 6)
>      let addon = yield promiseAddonByID(id);
>  
> -    if (versions[i]) {
> +    if (!("inProfile" in condition) || !("version" in condition)) {
> +      throw Error("condition must contain inProfile and version");
> +    }
> +    let inProfile = conditions[i].inProfile;

While I'm nitpicking names, this flag is really about whether we expect to see the addon in the updates directory (as opposed to the origina/default directory) right?

::: toolkit/mozapps/extensions/test/xpcshell/test_system_reset.js:188
(Diff revision 6)
>    let addonSet = {
>      schema: 1,
>      directory: featureDir.leafName,
>      addons: {
>        "system2@tests.mozilla.org": {
>          version: "1.0"

wouldn't this test be more clear if these get a new version (i.e., 2.0)

::: toolkit/mozapps/extensions/test/xpcshell/test_system_update.js:551
(Diff revision 6)
>    if (yield OS.File.exists(featureDir.path)) {
>      let iterator = new OS.File.DirectoryIterator(featureDir.path);
>      yield iterator.forEach(entry => {
>        if (entry.isDir) {
> +
> +        do_print(`get_directories adding entry for ${entry.path}`);

did you mean to keep this in?

::: toolkit/mozapps/extensions/test/xpcshell/test_system_update.js:584
(Diff revision 6)
>    let expectedDirs = 0;
>  
>    // If the initial state was using the profile set then that directory will
>    // still exist.
> -  if (initialState[0])
> +
> +  if (initialState.filter(a => a.inProfile).some(a => a)) {

I think you could just write this as `initialState.some(a => a.inProfile)` ?
(and likewise a few lines below)
Attachment #8781892 - Flags: review?(aswan) → review+
Comment on attachment 8781892 [details]
Bug 1295732 - allow system add-ons updates to override defaults per addon ID

https://reviewboard.mozilla.org/r/72220/#review70554

> Sorry to keep on this but I'm having a very hard time following this test with the current naming.  This is actually the updates directory right?  Can we rename it to reflect that?

Right. There's a `features` dir in the app ("defaults"), and one in the profile ("updates").

I'll rename the `const` here to make this clear.

> While I'm nitpicking names, this flag is really about whether we expect to see the addon in the updates directory (as opposed to the origina/default directory) right?

Right. Should we change this too?
Comment on attachment 8781892 [details]
Bug 1295732 - allow system add-ons updates to override defaults per addon ID

https://reviewboard.mozilla.org/r/72220/#review70554

> Right. Should we change this too?

I went ahead and changed this from `inProfile` to `isUpgrade`.
Attachment #8781892 - Flags: review?(dtownsend)
No longer blocks: 1292029
Pushed by rhelmer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d0fcf3b07253
allow system add-ons updates to override defaults per addon ID r=aswan
https://hg.mozilla.org/integration/autoland/rev/5e90d3459e8c
update system add-on spec to reflect override behavior of updates r=mkelly
Comment on attachment 8781892 [details]
Bug 1295732 - allow system add-ons updates to override defaults per addon ID

Approval Request Comment
[Feature/regressing bug #]: 1295732
[User impact if declined]: Potential for delay and mistakes in pushing "system add-on" updates to users
[Describe test coverage new/current, TreeHerder]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c8552a31829f - also have done local manual testing of expected use cases.
[Risks and why]: The Firefox "system add-on" implementation currently requires that we re-package/sign/upload all built-in add-ons and provide them as part of each update. This means e.g. an important hotfix needs to block on re-packaging Pocket, even if Pocket has no changes. This change allows updates to override defaults on a per-addon-ID basis so only updates need to be provided. The code change itself is very small and uses the existing install location precedence feature, and has automated test coverage.
[String/UUID change made/needed]: None
Attachment #8781892 - Flags: approval-mozilla-beta?
Attachment #8781892 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/d0fcf3b07253
https://hg.mozilla.org/mozilla-central/rev/5e90d3459e8c
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Hello Rob, Felipe mentioned that they might be planning to do an e10s staged rollout in ~10 days. Do you plan to uplift this to moz-release by then? Or in other words is this something we should try to fix before that? 

Hi Sylvestre, just fyi, in case this needs to be included in a 48.0.2 release.
Flags: needinfo?(sledru)
Flags: needinfo?(rhelmer)
Comment on attachment 8781892 [details]
Bug 1295732 - allow system add-ons updates to override defaults per addon ID

Even though this is a pretty big change, this is a huge pain point we need to fix asap to ensure system add-on updates go smoothly (especially if only one add-on is being updated). I think there is some risk associated with this change and hopefully we will be able to test it in the coming week. Aurora50+
Attachment #8781892 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to Ritu Kothari (:ritu) from comment #28)
> Hello Rob, Felipe mentioned that they might be planning to do an e10s staged
> rollout in ~10 days. Do you plan to uplift this to moz-release by then? Or
> in other words is this something we should try to fix before that? 
> 
> Hi Sylvestre, just fyi, in case this needs to be included in a 48.0.2
> release.

Discussed in IRC but for posterity - the patch is all client-side so would need to ship to any version of Firefox that we wish to use it in first.
Flags: needinfo?(rhelmer)
Comment on attachment 8781892 [details]
Bug 1295732 - allow system add-ons updates to override defaults per addon ID

I agree with Ritu, we want to bring this to beta so system updates will not have to ship together every time they change.
Attachment #8781892 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Needs rebasing for Aurora. Might as well check Beta while you're at it.
Flags: needinfo?(rhelmer)
(In reply to Ryan VanderMeulen [:RyanVM] from comment #32)
> Needs rebasing for Aurora. Might as well check Beta while you're at it.

Ah, there are some small changes from bug 1291569 (scoping bug affecting one of these tests) and bug 1294621 (eslint formatting fix).

I've made a patch which includes this and will attach momentarily - let me know if this should be done differently.
Flags: needinfo?(rhelmer) → needinfo?(ryanvm)
This patch includes relevant changes from bug 1291569 and bug 1294621.
Flags: needinfo?(ryanvm)
Attachment #8784906 - Flags: feedback?(ryanvm)
Comment on attachment 8784906 [details] [diff] [review]
patch for d0fcf3b07253 which applies cleanly to aurora and beta

Looks good, thanks.
Attachment #8784906 - Flags: feedback?(ryanvm) → feedback+
ok, thanks Ritu
Flags: needinfo?(sledru)
You need to log in before you can comment on or make changes to this bug.