Closed Bug 1204156 Opened 9 years ago Closed 8 years ago

Allow updated system add-ons to install without a restart

Categories

(Toolkit :: Add-ons Manager, defect)

Product:
Toolkit
Component:
Add-ons Manager
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox43 --- affected
firefox52 --- verified

People

(Reporter: mossop, Assigned: rhelmer)

References

(Blocks 2 open bugs)

Details

(Keywords: feature)

Attachments

(4 files)

Bug 1204156 - allow system add-ons to install and update without restart
58 bytes, text/x-review-board-request
aswan
: review+
Details
Add-ons for testing system add-on delayed update.
15.00 KB, application/x-tar
jason
: feedback+
Details
test_system_addons_bug1204156.signed.tgz
34.27 KB, application/x-gzip
Details
unsigned-qa-system-addons.tar
23.00 KB, application/octet-stream
Details 
To reduce complexity the first pass of bug 1192924 will require a restart for new system add-ons to activate.
Blocks: 1183866
No longer blocks: 1192924
Blocks: 1209264
No longer blocks: 1183866
Depends on: 1231172
Blocks: 1229352
Assignee: nobody → rhelmer
Status: NEW → ASSIGNED
Depends on: 1273709
No longer blocks: 1229352
Depends on: 557710
I have been doing some investigatory work and am thinking of implementing it this way:

Continue using the existing mechanism:

 1) check for server updates and download+verify XPI files to a new UUID directory.
 2) point the directory in the extensions.systemAddonSet pref directory at this new directory.

The above is implemented the installAddonSet method of the system addon install location class. 
The current implementation stops here, and waits for processFileChanges in XPIProviderUtils, which runs on restart and activates the new directory.

To enable restartless installs+upgrades, add a new method: AddonManager.getInstallForSystemAddon(addonID) - this returns a promise which resolves to an initialized AddonInstall.

The above mechanism will now proceed:

 3) getInstallForSystemAddon for each downloaded+verified XPI and call .install()

If any updates are blocking upgrade, then all upgrades in the set will be delayed as well - either until restart or until all addons unblock.

If any installation fails, the whole update set will be rolled back and the previous update set will be used. If there was no previous update, use the built-in system add-ons.
Comment on attachment 8803112 [details]
Bug 1204156 - allow system add-ons to install and update without restart

I think this is almost done, still tracking down a test failure (looks like it's just due to the fact that we leave a directory around a little longer than we used to).

Looking more for feedback on the general approach. I managed to reuse a lot more of the existing code than I thought I would be able to, I suspect that SystemAddonInstallLocation could subclass MutableDirectoryInstallLocation and we could share more there but I think that should be left to a followup bug.

One (fairly minor) concern I have is that now it's possible to uninstall system addon updates via the AddonManager API, since the directory location is not locked. This shouldn't really be a big deal, but we had to remove the ability to userDisable via the API due to other features like about:addons unexpectedly offering users a different way to disable/uninstall/etc.

However it's of course still not possible to uninstall the built-in system addons, just updates, and the updates would come back without requiring restart so it's not the worst thing in the world...
Attachment #8803112 - Flags: feedback?(aswan)
Comment on attachment 8803112 [details]
Bug 1204156 - allow system add-ons to install and update without restart

https://reviewboard.mozilla.org/r/87336/#review86376

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:6658
(Diff revision 1)
>      makeSafe(aCallback)(null);
>    }
>  };
>  
>  /**
> + * Creates a new AddonInstall to install a system add-on from a local file. Installs

The only difference between this and `AddonInstall.createInstall` is the install location, so it might make sense to just make that an optional arg on `.createInstall`...

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:8596
(Diff revision 1)
>  }
>  
>  SystemAddonInstallLocation.prototype = Object.create(DirectoryInstallLocation.prototype);
>  Object.assign(SystemAddonInstallLocation.prototype, {
>    /**
> +   * Removes the specified files or directories in the staging directory and

These staging dir methods are mostly what I was alluding to wrt subclassing MutableDirectoryInstallLocation.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:9001
(Diff revision 1)
> +      trashDir.create(Ci.nsIFile.DIRECTORY_TYPE, FileUtils.PERMS_DIRECTORY);
> +
> +    return trashDir;
> +  },
> +
> +  /**

This is another method which might be shared with a super class.
Comment on attachment 8803112 [details]
Bug 1204156 - allow system add-ons to install and update without restart

Ha, I looked at the patch before reading some of your comments above and came up with pretty much the same issues (finding a way to share the repeated DirecoryInstallLocation code and ramifications of making the system add-ons directory unlocked).
As we discussed briefly on IRC, I think I'll be wanting something similar for webextensions -- when we prompt for permissions we'll want to download the udpate, then keep it somewhere non-temporary until the user has accepted any new permissions.  The notion of having a place to keep the xpi that we intend to install at some point (as opposed to staging/ which has a different meaning) seems like the shared bit here.  But that work is still a few weeks out, I think you should go ahead with the patches here and I'll keep my eyes open for opportunities to remove duplication when I eventually do the permissions work.
Attachment #8803112 - Flags: feedback?(aswan) → feedback+
This is passing try now, however there's one case I realized isn't covered sufficiently:

Any system add-on going into postponed state should cause all add-ons in that upgrade set to do so as well, and they should be resumed at the same time.

There should be an xpcshell test for this as well.
Comment on attachment 8803112 [details]
Bug 1204156 - allow system add-ons to install and update without restart

I've pushed the postpone set/resume set patch, but I want to do some more cleanup so will re-request review.

We should also have a test for fallback - I simplified this somewhat, if an upgraded set fails to install we fall back to the built-in rather than the previous update set.

I think this failure case is unlikely enough that this is an easier and safer route to take.
Attachment #8803112 - Flags: review?(aswan)
Blocks: 1314177
(In reply to Robert Helmer [:rhelmer] from comment #5)
> These staging dir methods are mostly what I was alluding to wrt subclassing
> MutableDirectoryInstallLocation.
> 
> ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:9001
> (Diff revision 1)
> > +      trashDir.create(Ci.nsIFile.DIRECTORY_TYPE, FileUtils.PERMS_DIRECTORY);
> > +
> > +    return trashDir;
> > +  },
> > +
> > +  /**
> 
> This is another method which might be shared with a super class.

I filed bug 1314177 to cover this refactoring.
Comment on attachment 8803112 [details]
Bug 1204156 - allow system add-ons to install and update without restart

https://reviewboard.mozilla.org/r/87336/#review89906

Sorry for taking so long to get to this.  I think I've absorbed the XPIProvider.jsm changes, I haven't had a chance to look at the test yet.
My high-level feedback is that I'm concerned about the duplication here and would like to see if we can find a way to have a little less repetition of existing code?

::: toolkit/mozapps/extensions/AddonManager.jsm
(Diff revision 5)
>  
>    addUpgradeListener: function(aInstanceID, aCallback) {
>      AddonManagerInternal.addUpgradeListener(aInstanceID, aCallback);
>    },
>  
> -  removeUpgradeListener: function(aInstanceID) {

This appears to be used from toolkit/components/extensions/ext-runtime.js, but not actually covered by any tests...

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:5447
(Diff revision 5)
>    /**
>     * Initialises this install to be a staged install waiting to be applied
>     *
>     * @param  aManifest
>     *         The cached manifest for the staged install
> +   * @param  aCallback

Why can't this just return the install?  (or in this case since it's just "this", have the caller handle propagating the install object as necessary)

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:5466
(Diff revision 5)
> -    this.file = null;
> +    this.file = this.sourceURI;
> +    this._sourceBundle = this.sourceURI;
>      this.addon = aManifest;
> +    this.addon.sourceURI = this.sourceURI;
>  
> +    this.alreadyStaged = true;

I don't see anything that ever modifies this?
But I also don't get why it would be true by default?

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:6302
(Diff revision 5)
>        }
>  
>        if (AddonManagerPrivate.callInstallListeners("onDownloadEnded",
>                                                     this.listeners,
>                                                     this.wrapper)) {
> -        // If a listener changed our state then do not proceed with the install
> +

why remove this?

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:6409
(Diff revision 5)
>  
>        yield this.installLocation.requestStagingDir();
>  
>        // remove any previously staged files
> +      if (!this.alreadyStaged) {
> -      yield this.unstageInstall(stagedAddon);
> +        yield this.unstageInstall(stagedAddon);

It looks like `unstageInstall()` is only ever called with the return value of `this.installLocation.getStagingDir()` as its argument and it immediately clones that.  How about simplifying this by just putting the call to `getStagingDir()` inside `unstageInstall()`?

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:8794
(Diff revision 5)
> +        let addon = new Promise(resolve => AddonManager.getAddonByID(id, resolve));
> +        if (addon && "uninstall" in addon) {

This isn't right, addon is a Promise, not the addon, you need to chain another handler on the promise.  And now you probably need to make resetAddonSet() async which is going to propagate all over the place :(

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:8873
(Diff revision 5)
> +    let oldAddons = new Set(Object.keys(addonSet.addons));
> +    let newAddons = new Set(aAddons);
> +    var difference = new Set([...oldAddons].filter(x => !newAddons.has(x)));

I don't think you need Sets here, you can operate directly on arrays here (with includes instead of has etc)

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:8878
(Diff revision 5)
> +      for (let addonID of difference.values()) {
> +        let addon = yield new Promise(resolve => AddonManager.getAddonByID(addonID, resolve));
> +        if (addon) {
> +          addon.uninstall();
> +        }
> +      }

What are the exact semantics you're going for here?  If some of the new set either fail to upgrade or postpone their upgrade, do you still want old addons to be removed?  You don't really have a way to roll back from that here...

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:8905
(Diff revision 5)
> -        // Directory already exists, pick another
> +        logger.debug("Could not create new system add-on updates dir, retrying", e);
>        }
>      }
>  
> -    let copyAddon = Task.async(function*(addon) {
> -      let target = OS.Path.join(newDir.path, addon.id + ".xpi");
> +    // Record the new upgrade directory.
> +    let state = { schema: 1, directory: newDir.leafName, addons: {} };

Does the schema version need to be revved here?
Also what happens if somebody runs this code then downgrades to a build that doesn't have this patch?

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:8910
(Diff revision 5)
> -      let target = OS.Path.join(newDir.path, addon.id + ".xpi");
> -      logger.info(`Copying ${addon.id} from ${addon._sourceBundle.path} to ${target}.`);
> -      try {
> -        yield OS.File.copy(addon._sourceBundle.path, target);
> +    let state = { schema: 1, directory: newDir.leafName, addons: {} };
> +    this._saveAddonSet(state);
> +
> +    this._nextDir = newDir;
> +
> +    let checkPostponed = Task.async(function*(addon) {

why the async task here?

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:8920
(Diff revision 5)
> -      catch (e) {
> -        logger.error(`Failed to copy ${addon.id} from ${addon._sourceBundle.path} to ${target}.`, e);
> -        throw e;
> +      return result;
> +    });
> +
> +    let installAddon = Task.async(function*(addon) {
> +      let install = yield new Promise(resolve => {
> +        let location = XPIProvider.installLocationsByName[KEY_APP_SYSTEM_ADDONS];

when would location != this ?

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:8936
(Diff revision 5)
> -      addon._sourceBundle = new nsIFile(target);
>      });
>  
>      try {
> -      yield waitForAllPromises(aAddons.map(copyAddon));
> +      // All add-ons in position, create the new state and store it in prefs
> +      let state = { schema: 1, directory: newDir.leafName, addons: {} };

You already have a variable set in this scope, you don't need to declare a new one (and if we ever turn on eslint shadow warnings, this will start tripping that)

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:8945
(Diff revision 5)
> +        }
> +      }
> +
> +      this._saveAddonSet(state);
> +
> +      let blockers = yield waitForAllPromises(aAddons.map(checkPostponed));

No need for making this async I think?

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:8953
(Diff revision 5)
> +      } else {
> +        yield waitForAllPromises(aAddons.map(installAddon));
> +      }
>      }
>      catch (e) {
> +      // roll back to built-in set.

Oy, so if we have some set of updates and we fail applying a second set of updates, we fall back to the original versions (prior to the first update)?

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:9001
(Diff revision 5)
> +      yield install.stageInstall(true, stagedAddon, true);
> +
> +      AddonManagerPrivate.callInstallListeners("onInstallPostponed",
> +                                               install.listeners, install.wrapper)
> +
> +      if (AddonManagerPrivate.hasUpgradeListener(addon.id)) {

Hm, have you thought about trying to share code with the existint postpone code?
Comment on attachment 8803112 [details]
Bug 1204156 - allow system add-ons to install and update without restart

https://reviewboard.mozilla.org/r/87336/#review89906

> This appears to be used from toolkit/components/extensions/ext-runtime.js, but not actually covered by any tests...

Hm. Actually I think changing this is not the right thing to do, on second thought... we can probably just leave it alone and get the symbol and use that.

> Why can't this just return the install?  (or in this case since it's just "this", have the caller handle propagating the install object as necessary)

I think the only reason it works this way is to be consistent with `AddonInstall.createInstall`. Just returning the install would be fine too.

> I don't see anything that ever modifies this?
> But I also don't get why it would be true by default?

This is in `initStagedInstall`, which (before now) staged the install in such a way that it would be applied on next restart but could not be resumed. In order to resume, `startInstall` needs to not remove and re-stage.

Any ideas how we could make this clearer?

> why remove this?

Oops that looks like it was just mistakenly removed, sorry.

> This isn't right, addon is a Promise, not the addon, you need to chain another handler on the promise.  And now you probably need to make resetAddonSet() async which is going to propagate all over the place :(

Hrm yeah I think I had that written as a callback and changed it without thinking about it... I can just put it back the way it was.

> What are the exact semantics you're going for here?  If some of the new set either fail to upgrade or postpone their upgrade, do you still want old addons to be removed?  You don't really have a way to roll back from that here...

I think for now we should just remove all upgrades if updates fail, and fall back to the built-in. We can try again later.

We may want to make that more clever (fall back to previous update set should be possible) but I think this patch is complicated enough :/

> Does the schema version need to be revved here?
> Also what happens if somebody runs this code then downgrades to a build that doesn't have this patch?

Hm I can't think why we need to rev it... downgrading to an older build should be fine. The only exception is that the staging dir will be ignored, but it'll go away as addon updates are applied.

I could be wrong though, we could do it just to be safe if you'd prefer.

> when would location != this ?

Fair enough :)

> Oy, so if we have some set of updates and we fail applying a second set of updates, we fall back to the original versions (prior to the first update)?

Originally I tried falling back to the previous update but felt it was getting messy. We can try to do that now if you'd prefer?

> Hm, have you thought about trying to share code with the existint postpone code?

Yeah, it's all in the post-download bit right now and is slightly different, but I could probably move it out to its own function and not be so lazy :)
Comment on attachment 8803112 [details]
Bug 1204156 - allow system add-ons to install and update without restart

https://reviewboard.mozilla.org/r/87336/#review89906

> Fair enough :)

Hrm actually `this` seems to be undefined here... re-opening this issue so I remember to look into this.
Attachment #8803112 - Flags: review?(aswan)
Comment on attachment 8803112 [details]
Bug 1204156 - allow system add-ons to install and update without restart

https://reviewboard.mozilla.org/r/87336/#review89906

> This is in `initStagedInstall`, which (before now) staged the install in such a way that it would be applied on next restart but could not be resumed. In order to resume, `startInstall` needs to not remove and re-stage.
> 
> Any ideas how we could make this clearer?

Sorry I'm just not understanding how it is supposed to work.  It looks to me like you've changed what happens inside `startInstall()` since code that was formerly run unconditionally is now run if `!alreadyStaged` but in fact you initialize `alreadyStaged` here to true but never set it to false.

> I think for now we should just remove all upgrades if updates fail, and fall back to the built-in. We can try again later.
> 
> We may want to make that more clever (fall back to previous update set should be possible) but I think this patch is complicated enough :/

Yeah I'm all for the simplest thing we can get away with, just asking to make sure we're on the same page about what the desired behavior is.  Which suggests that maybe there should be a comment near the declaration of SystemAddonInstallLocation describing how updates are intended to work.

> Hm I can't think why we need to rev it... downgrading to an older build should be fine. The only exception is that the staging dir will be ignored, but it'll go away as addon updates are applied.
> 
> I could be wrong though, we could do it just to be safe if you'd prefer.

Oh, on first read I had thought you were adding new fields but upon closer reading I see that you're not, sorry for the confusion.

> Hrm actually `this` seems to be undefined here... re-opening this issue so I remember to look into this.

Right `this` isn't bound inside the anonymous generator, but looking up the location by name still seems unnecessary here

> Originally I tried falling back to the previous update but felt it was getting messy. We can try to do that now if you'd prefer?

I don't have a preference, just trying to understand the desired outcome.
Comment on attachment 8803112 [details]
Bug 1204156 - allow system add-ons to install and update without restart

https://reviewboard.mozilla.org/r/87336/#review89906

> Sorry I'm just not understanding how it is supposed to work.  It looks to me like you've changed what happens inside `startInstall()` since code that was formerly run unconditionally is now run if `!alreadyStaged` but in fact you initialize `alreadyStaged` here to true but never set it to false.

So the complication here is, `startInstall` isn't really intended to be able to start an already-staged install at runtime - we can do it with delayed updates because we have a reference to the `AddonInstall`. The removing and re-staging the add-on is causing a problem.

Although thinking about it, I should just figure out what is going wrong here and fix it to make the `AddonInstall` created by `AddonInstall.createStagedInstall` look more like a normal `AddonInstall`...

So I think you're right, we should just leave `startInstall` alone and let it run unconditionally.

> Yeah I'm all for the simplest thing we can get away with, just asking to make sure we're on the same page about what the desired behavior is.  Which suggests that maybe there should be a comment near the declaration of SystemAddonInstallLocation describing how updates are intended to work.

Oh that reminds me, we have a spec checked in - https://gecko.readthedocs.io/en/latest/toolkit/mozapps/extensions/addon-manager/SystemAddons.html - that needs to be updated too. Do you think that's sufficient or should we have a comment in the code as well?

> I don't have a preference, just trying to understand the desired outcome.

The more I think about it, if an upgrade set fails to install the safest course of action right now would be to set `PREF_SYSTEM_ADDON_SET` back to the previous upgrade directory + addons (if present), otherwise reset the pref (so built-in will be used), and wait for restart.

I think we can and should do better here, but my reasoning is:

1) this is a very unlikely failure mode, since we QA each set with each targeted version of Firefox (although there could certainly be environmental factors specific to a user's machine etc)

2) our system addon upgrades right now are almost exclusively hotfix-style, which aren't overriding a built-in feature so the danger of e.g. killing Pocket until restart is low

3) this is a tricky enough scenario that I think it deserves a followup.
Try is looking good with the current patch:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=75108e8983ca

Not sure what's up with that ESLint error but I suspect it'll go away when I rebase.
Comment on attachment 8803112 [details]
Bug 1204156 - allow system add-ons to install and update without restart

https://reviewboard.mozilla.org/r/87336/#review91020

Looks promising but I think the postpone/resume logic still needs some work.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:5451
(Diff revision 11)
>     *         The cached manifest for the staged install
> +   * @param  aCallback
> +   *         A callback to call with the resulting install.
> +   * @returns {Object} The AddonInstall for this staged install.
>     */
> -  initStagedInstall: function(aManifest) {
> +  initStagedInstall: function(aManifest, aCallback) {

aCallback isn't actually used

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:6309
(Diff revision 11)
>            return;
>  
>          // If an upgrade listener is registered for this add-on, pass control
>          // over the upgrade to the add-on.
>          if (AddonManagerPrivate.hasUpgradeListener(this.addon.id)) {
> -          logger.info(`${this.addon.id} has an upgrade listener, postponing until restart`);
> +          logger.info(`add-on ${this.addon.id} has an upgrade listener, postponing upgrade set until restart`);

nit: why the word "set" here, this is about an individual add-on upgrade.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:6313
(Diff revision 11)
> -                    logger.warn(`${this.addon.id} tried to resume postponed upgrade, but it's already installed`);
> -                    break;
> -                  case AddonManager.STATE_POSTPONED:
> -                    logger.info(`${this.addon.id} has resumed a previously postponed upgrade`);
> +            logger.info(`${this.addon.id} has resumed a previously postponed upgrade`);
> -                    this.state = AddonManager.STATE_DOWNLOADED;
> +            this.state = AddonManager.STATE_DOWNLOADED;
> -                    this.installLocation.releaseStagingDir();
> +            this.installLocation.releaseStagingDir();

This doesn't feel right, `postpone()` is the thing that called `requestStagingDir()`, it should also be handling releasing it when the install is resumed.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:8594
(Diff revision 11)
>    else {
>      logger.info("SystemAddonInstallLocation directory is missing");
>    }
>  
>    DirectoryInstallLocation.call(this, aName, this._directory, aScope);
> -  this.locked = true;
> +  this.locked = false;

Thinking more about this, I think a consequence of this is that we'll also apply the regular background update logic to system addons.
I assume we don't set updateURLs for system addons, but it seems like we shouldn't just rely on that and should guard against the background update process getting accidentally invoked.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:8914
(Diff revision 11)
> -    let copyAddon = Task.async(function*(addon) {
> -      let target = OS.Path.join(newDir.path, addon.id + ".xpi");
> -      logger.info(`Copying ${addon.id} from ${addon._sourceBundle.path} to ${target}.`);
> -      try {
> -        yield OS.File.copy(addon._sourceBundle.path, target);
> +    // Record the new upgrade directory.
> +    let state = { schema: 1, directory: newDir.leafName, addons: {} };
> +    this._saveAddonSet(state);
> +
> +    this._nextDir = newDir;
> +    let location = this;

The alternative to this is to explicitly bind the generators that you pass to Task.async() below to `this`.  This will all be so much nicer when we can just use async...

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:8927
(Diff revision 11)
> +      let install = yield new Promise(resolve => {
> +        AddonInstall.createInstall(resolve, addon._sourceBundle, location);
> +      });

This bit is the same between `installAddon` and `postponeAddon`, you could create these in common code and then have the switch be whether you call `.install()` or `.postpone()` on all the installs you have.
That would also let you pass the list of installs to `resumseAddonSet()` and you could just re-use them there rather than doing everything that's currently done to re-create usable install objects.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:8939
(Diff revision 11)
> -      }
> +        }
> -      catch (e) {
> -        logger.error(`Failed to copy ${addon.id} from ${addon._sourceBundle.path} to ${target}.`, e);
> -        throw e;
>        }
> -      addon._sourceBundle = new nsIFile(target);
> +      yield install.postpone(resumeFunction);

I think this isn't quite matching up right, if there are blockers for multiple system add-ons, `postpone()` will case `requestStagingDir()` for each of them, but `releaseStagingDir()` will only get called once for the whole set, so it won't get properly cleaned.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:8997
(Diff revision 11)
> +      let location = XPIProvider.installLocationsByName[KEY_APP_SYSTEM_ADDONS];
> +      let stagingDir = location.getStagingDir()
> +      let stagedAddon = stagingDir.clone();
> +      stagedAddon.append(`${id}.xpi`);
> +
> +      let addon = syncLoadManifestFromFile(stagedAddon, location);

you're in a task here, can you `yield loadManifestFromFile(...)` instead

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:9024
(Diff revision 11)
> +
> +    let blockers = addonIDs.filter(
> +      addonID => AddonManagerPrivate.hasUpgradeListener(addonID)
> +    );
> +
> +    if (blockers.length > 1) {

Does this actually work?  I thought the listener remains visible here even if has resumed...

::: toolkit/mozapps/extensions/test/xpcshell/head_addons.js:1349
(Diff revision 11)
>    let file = aDir.clone();
>    file.append(aId);
>    return file
>  }
> +
> +function* serve_system_update(xml, perform_update) {

nit: the convention throughout the rest of this code is camelCase

::: toolkit/mozapps/extensions/test/xpcshell/head_addons.js:1368
(Diff revision 11)
> +// call to the update function so we get rejections on failure.
> +function* install_system_addons(xml) {
> +  do_print("Triggering system add-on update check.");
> +
> +  yield serve_system_update(xml, function*() {
> +    let { XPIProvider } = Components.utils.import("resource://gre/modules/addons/XPIProvider.jsm");

I think you want a second `{}` argument here to avoid importing this globally.  Or, importing it globally wouldn't be so terrible, this could just be a lazy module getter at the top level.

::: toolkit/mozapps/extensions/test/xpcshell/head_addons.js:1393
(Diff revision 11)
> +    });
> +  });
> +}
> +
> +// Builds an update.xml file for an update check based on the data passed.
> +function* build_system_xml(addons) {

I normally hate overly long names, but this one is vague enough to be confusing, could we call is buildSystemAddonUpdates or something?

::: toolkit/mozapps/extensions/test/xpcshell/test_system_delay_update.js:7
(Diff revision 11)
> + * http://creativecommons.org/publicdomain/zero/1.0/
> + */
> +
> +// This verifies that delaying a system add-on update works.
> +
> +// FIXME sign the test addons, or this will fail on beta/release

Yeah and don't forget to remove this before landing

::: toolkit/mozapps/extensions/test/xpcshell/test_system_delay_update.js:32
(Diff revision 11)
> +registerDirectory("XREAppFeat", distroDir);
> +
> +let testserver = new HttpServer();
> +testserver.registerDirectory("/data/", do_get_file("data/system_addons"));
> +testserver.start();
> +let root = testserver.identity.primaryScheme + "://" +

template string?

::: toolkit/mozapps/extensions/test/xpcshell/test_system_delay_update.js:181
(Diff revision 11)
> +  do_check_true(addon_upgraded.isCompatible);
> +  do_check_false(addon_upgraded.appDisabled);
> +  do_check_true(addon_upgraded.isActive);
> +  do_check_eq(addon_upgraded.type, "extension");
> +
> +  yield shutdownManager();

These tests look good but they don't apepar to actually check the contents of the filesystem to make sure all staging dirs have been cleaned up by the time this sequence is done?

::: toolkit/mozapps/extensions/test/xpcshell/test_system_reset.js
(Diff revision 11)
> -      do_check_false(hasFlag(addon.permissions, AddonManager.PERM_CAN_UPGRADE));
> -      do_check_false(hasFlag(addon.permissions, AddonManager.PERM_CAN_UNINSTALL));

This is a side-effect of making the InstallLocation unlocked right?  Should we be explicitly testing for true now?
Attachment #8803112 - Flags: review?(aswan)
Attachment #8803112 - Flags: review?(aswan)
Comment on attachment 8803112 [details]
Bug 1204156 - allow system add-ons to install and update without restart

https://reviewboard.mozilla.org/r/87336/#review91048
Comment on attachment 8803112 [details]
Bug 1204156 - allow system add-ons to install and update without restart

https://reviewboard.mozilla.org/r/87336/#review91020

> This doesn't feel right, `postpone()` is the thing that called `requestStagingDir()`, it should also be handling releasing it when the install is resumed.

OK moved it, I remember discussing this before - it's a weird situation because we want the staging dir to stick around. I guess I should look at how restartless addons do this and make sure it's consistent...

> Thinking more about this, I think a consequence of this is that we'll also apply the regular background update logic to system addons.
> I assume we don't set updateURLs for system addons, but it seems like we shouldn't just rely on that and should guard against the background update process getting accidentally invoked.

Hm, is the locked install location the only thing stopping us from doing this now? Yeah, we definitely want to be more explicit...

> I think this isn't quite matching up right, if there are blockers for multiple system add-ons, `postpone()` will case `requestStagingDir()` for each of them, but `releaseStagingDir()` will only get called once for the whole set, so it won't get properly cleaned.

I think moving `releaseStagingDir()` into the `postpone` function (above) should fix this too?

> Does this actually work?  I thought the listener remains visible here even if has resumed...

Hm I think I tested this... one thing I realized between this and the last review is that we really depend on the add-on removing the listener at the right time :/ But I don't think there's much we can do about that.

We have more control over when WebExtensions remove the listener so that's really only an issue for bootstrap.js-type.

> nit: the convention throughout the rest of this code is camelCase

Yeah this was pulled out of existing test code (same as below), I'll fix these all up.

> template string?

Hm I think it's only more readable if we do a template string and don't split it (so it runs over 80 chars but not by too much)
Comment on attachment 8803112 [details]
Bug 1204156 - allow system add-ons to install and update without restart

https://reviewboard.mozilla.org/r/87336/#review91020

> Hm, is the locked install location the only thing stopping us from doing this now? Yeah, we definitely want to be more explicit...

I think so if you consider these two fragments:
http://searchfox.org/mozilla-central/rev/f5c9e9a249637c9abd88754c8963ecb3838475cb/toolkit/mozapps/extensions/AddonManager.jsm#1476-1477
http://searchfox.org/mozilla-central/rev/f5c9e9a249637c9abd88754c8963ecb3838475cb/toolkit/mozapps/extensions/internal/XPIProvider.jsm#7349-7354
Comment on attachment 8803112 [details]
Bug 1204156 - allow system add-ons to install and update without restart

https://reviewboard.mozilla.org/r/87336/#review91020

> I think so if you consider these two fragments:
> http://searchfox.org/mozilla-central/rev/f5c9e9a249637c9abd88754c8963ecb3838475cb/toolkit/mozapps/extensions/AddonManager.jsm#1476-1477
> http://searchfox.org/mozilla-central/rev/f5c9e9a249637c9abd88754c8963ecb3838475cb/toolkit/mozapps/extensions/internal/XPIProvider.jsm#7349-7354

Thanks for catching that - I guess we can check `isSystem` in http://searchfox.org/mozilla-central/rev/f5c9e9a249637c9abd88754c8963ecb3838475cb/toolkit/mozapps/extensions/internal/XPIProvider.jsm#7352 and re-enable the test that ensures that system add-ons don't have `PERM_CAN_UPGRADE`.

I will check all the other uses of `PERM_CAN_UPGRADE` but I don't think any of them should have an impact on system add-on updates, since they don't show up in the UI and have a totally different update path, but will need to check.
Attachment #8803112 - Flags: review?(aswan)
Comment on attachment 8803112 [details]
Bug 1204156 - allow system add-ons to install and update without restart

https://reviewboard.mozilla.org/r/87336/#review91020

> Hm I think I tested this... one thing I realized between this and the last review is that we really depend on the add-on removing the listener at the right time :/ But I don't think there's much we can do about that.
> 
> We have more control over when WebExtensions remove the listener so that's really only an issue for bootstrap.js-type.

I've addd a test, I think this is working.
Comment on attachment 8803112 [details]
Bug 1204156 - allow system add-ons to install and update without restart

https://reviewboard.mozilla.org/r/87336/#review91682

::: toolkit/mozapps/extensions/internal/AddonTestUtils.jsm:314
(Diff revision 16)
> -      var testDir = this.profileDir.clone();
> +      // ensure no leftover files in the system addon upgrade location
> +      let featuresDir = this.profileDir.clone();
> +      featuresDir.append("features");
> +      // upgrade directories will be in UUID folders under features/
> +      let systemAddonDirs = [];
> +      let featuresDirEntries = featuresDir.directoryEntries

shouldn't this check that the features dir exists before reading it?

::: toolkit/mozapps/extensions/internal/AddonTestUtils.jsm:323
(Diff revision 16)
> +        entry.QueryInterface(Components.interfaces.nsIFile);
> +        systemAddonDirs.push(entry);
> +      }
> +
> +      systemAddonDirs.map(dir => {
> +        dump(`rhelmer debug ${dir}`);

don't forget to remove this

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:5461
(Diff revision 16)
>                             NetUtil.newURI(aManifest.releaseNotesURI) :
> -                           null
> +                           null;
>      this.sourceURI = aManifest.sourceURI ?
>                       NetUtil.newURI(aManifest.sourceURI) :
>                       null;
> -    this.file = null;
> +    this.file = this.sourceURI;

in other isntances of AddonInstall, i think this is an nsIFile, not an nsIURI

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:5462
(Diff revision 16)
> -                           null
> +                           null;
>      this.sourceURI = aManifest.sourceURI ?
>                       NetUtil.newURI(aManifest.sourceURI) :
>                       null;
> -    this.file = null;
> +    this.file = this.sourceURI;
> +    this._sourceBundle = this.sourceURI;

this attribute is usually on AddonInternal instances, not AddonInstall

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:6314
(Diff revision 16)
> -                    break;
> -                  default:
> -                    logger.warn(`${this.addon.id} cannot resume postponed upgrade from state (${this.state})`);
> -                    break;
> -                }
> +          }
> -              },
> +          Task.spawn((function*() {

This confused me momentarily, I think it would be more consistent with the rest of the code if `postpone()` did the `Task.spawn()` itself, then you can just call it here and ignore the promise that it returns.
Comment on attachment 8803112 [details]
Bug 1204156 - allow system add-ons to install and update without restart

https://reviewboard.mozilla.org/r/87336/#review91706
Attachment #8803112 - Flags: review?(aswan) → review+
Jason, would you mind signing the test add-ons in this archive? I intend to land these in mozilla-central, to test a new system add-on upgrade feature.
Attachment #8809162 - Flags: feedback?(jthomas)
Attachment #8809162 - Flags: feedback?(jthomas) → feedback+
Wei could you please sign this test system add-on?
Flags: needinfo?(wezhou)
All files are signed and tgz'd in the attachment.

Thanks and let me know if there are questions.
Flags: needinfo?(wezhou) → needinfo?(rhelmer)
(In reply to :wezhou from comment #45)
> Created attachment 8809186 [details]
> test_system_addons_bug1204156.signed.tgz
> 
> All files are signed and tgz'd in the attachment.
> 
> Thanks and let me know if there are questions.

This is perfect, thanks!
Flags: needinfo?(rhelmer)
Signed test XPIs related FIXME comment have been removed, try looks good:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=092b3bbecc6f58aad015644635782ea307057b9e
(In reply to Robert Helmer [:rhelmer] from comment #49)
> Signed test XPIs related FIXME comment have been removed, try looks good:

Let me try that again: unsigned test add-ons have been have been removed, signed versions added, and related FIXME comment+signature pref flip has been removed, so these tests should work when this hits beta/release.
Pushed by rhelmer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c77ef1e8dc98
allow system add-ons to install and update without restart r=aswan
Blocks: 1316508
https://hg.mozilla.org/mozilla-central/rev/c77ef1e8dc98
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
We should do some testing on this - the basic idea is to host some system add-ons on a test server and point to it with the `extensions.systemAddon.update.url` pref.

I think the most important scenarios are:

1) have a built-in system add-on, restartlessly install an update to it
2) restartlessly install a new system add-on

We should also test upgrading and downgrading from these states.
Keywords: qawanted
After speaking with QA, I've set up people.mozilla.org to use as a test server per comment 54.

All of the needed files are in https://people.mozilla.org/~rhelmer/Bug1204156/

To test, you need to change the following prefs:

extensions.logging.enabled = true
xpinstall.signatures.required = false
extensions.systemAddon.update.url = https://people.mozilla.org/~rhelmer/Bug1204156/update-empty.xml

NOTE - the xpinstall.signatures.required pref disables the add-on signing requirement, and will not work on Beta/Release. We'd need to sign the test add-ons used below if this sort of testing is needed (since the patch hasn't landed there yet I guess it's not an issue currently)

To trigger a system add-on update, run this in the Browser Console (under Menu->Developer):

Components.classes["@mozilla.org/addons/integration;1"].getService(Components.interfaces.nsITimerCallback).notify(null);

You should see debugging output related to the GMP and System Add-on update check in the Browser Console, concluding with:

addons.manager	DEBUG	Background update check complete

I've set up some different scenarios, which you can test by changing the "update-empty.xml" at the end of the extensions.systemAddon.update.url setting:

update-v1.xml
  This should cause a new "fox-candy" system add-on to appear in the toolbar.
  Clicking this will show you the version (1.0)

update-v2.xml
  This will update "fox-candy" to 2.0, and will install a new "crashme" add-on to a new icon in
  the toolbar (clicking the "crashme" icon does nothing currently)

update-v3.xml
  This will update "fox-candy" to 3.0, and will uninstall "crashme" if present

update-remove.xml
  This will remove any system add-ons installed through this mechanism

I've tested that everything works for me when run in the above order, but feel free to try these in different combinations. If you think of any other scenarios you'd like to test let me know and I can help to set that up.

Each time you modify the URL, make sure to trigger the timer in the Browser Console as noted above.
This is currently blocking the Pre-Release sign off, could you please give us an alternative for the beta testing? 

Thank you!
Flags: needinfo?(rhelmer)
Can you please sign these system add-ons?

These are for QA to use on Beta and different than attachment 8809186 [details] which landed in the true for unit tests.

Thanks!
Flags: needinfo?(rhelmer) → needinfo?(jthomas)
Can we use guids that are *@tests.mozilla.org instead of *@rhelmer.org? I don't think this is a standard but we have signed test add-ons in the past and I think it makes sense to continue doing it that way (see bug 1169744).
We found a workaround. The pref is not available on the beta channel, however using an unbranded version from tinderbox (https://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-beta-win64-add-on-devel/1487905138/ , build ID: 20170223185858, in about:support the version is displayed as 52.0b9) the pref is working accordingly. We will continue testing with this version for now.
Please let me know if there are any concerns.
Flags: needinfo?(jthomas)
(In reply to Jason Thomas [:jason] from comment #58)
> Can we use guids that are *@tests.mozilla.org instead of *@rhelmer.org? I
> don't think this is a standard but we have signed test add-ons in the past
> and I think it makes sense to continue doing it that way (see bug 1169744).

Good point, I'll use @tests.mozilla.org in the future to avoid confusion.

It sounds like QA is ok now though with comment 59 so let's not worry about the current batch.
This issue is VERIFIED FIXED on Mac OS X 10.11.6, Ubuntu 16.04 x64, Windows 7 x86 and Windows 10 x64.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: