1231172 - provide API for add-ons to delay restartless updates

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Robert Helmer [:rhelmer]]

Some add-ons want to get restartless updates, but be able to delay them if an important operation is happening.

An example of this is that the Hello add-on might be in the middle of a call.

There should be a way for add-ons to delay the upgrade.

Comment 1

[Robert Helmer [:rhelmer]]

(In reply to Robert Helmer [:rhelmer] from comment #0)
> Some add-ons want to get restartless updates, but be able to delay them if
> an important operation is happening.
> 
> An example of this is that the Hello add-on might be in the middle of a call.
> 
> There should be a way for add-ons to delay the upgrade.

Any thoughts on this? One way would be to have a new function in bootstrap.js ( shouldRestart() ?) that returns a boolean, if it returns `false` then the add-ons manager could set a timer and wait some predetermined amount of time before retrying. `true` is assumed if the method does not exist, for backwards compat.

Comment 2

[Robert Helmer [:rhelmer]]

Also - should this only be available for system add-ons, or any add-on? I don't see any reason why it should not be exposed to all, but I guess it depends on the implementation.

Comment 3

[Mike de Boer [:mikedeboer]]

I'd like to loop-in Yoric, as he's been involved with async shutdown and cancelling shutdown, I *think*.

David, how would you go about to implement deferred add-on unload?

Comment 4

[David Teller [:Yoric] - still alive but not very active]

We have AsyncShutdown, which is dedicated to delaying shutdown long enough to finishing work. However, this is not the scenario highlighted in Comment 0, as we crash (by design) if shutdown takes more than 60s.

There is entirely separate support for cancelling shutdown, through notification quit-application-requested, see https://developer.mozilla.org/en-US/docs/Observer_Notifications#Application%20shutdown.

Does this cover your needs?

Comment 5

[Robert Helmer [:rhelmer]]

(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #4)
> We have AsyncShutdown, which is dedicated to delaying shutdown long enough
> to finishing work. However, this is not the scenario highlighted in Comment
> 0, as we crash (by design) if shutdown takes more than 60s.
> 
> There is entirely separate support for cancelling shutdown, through
> notification quit-application-requested, see
> https://developer.mozilla.org/en-US/docs/
> Observer_Notifications#Application%20shutdown.
> 
> Does this cover your needs?

We just discussed this IRL - what we actually need here is for the add-ons manager to delay upgrading the add-on, we don't want to have to restart the application so unfortunately not but thank you!

Comment 6

[Dave Townsend [:mossop]]

This seems like something useful for all add-ons but I think we should be careful about how this works. We don't want it to be possible to block a user disabling or uninstallin an add-on they don't want. We also don't want an add-on refusing to ever update. A particularly faulty add-on could accidentally make it impossible to fix itself with this. Here is a proposal:

We provide an API to allow an add-on to say it shouldn't be updated. I think it makes most sense for an add-on to be able to signal "busy" then "not busy" when done rather than listening for events and blocking them. Add-ons are always "not busy" at startup.

If we find an update available and after download the add-on is in the busy state we should just queue up the update as if the add-on was non-restartless. This way if an add-on ever forgets the flag "not busy" he update still happens the next time Firefox restarts. Presumably if the add-on flags "not busy" and there is a pending upgrade then we can just apply it then.

System add-ons are slightly more complicated because we've been assuming that system add-ons update all at once to ensure dependencies are satisfied. So one add-on being "busy" would block all updates but again we would schedule the update to happen at the next restart, though since that is what happens now the harder part is to take this into account when we make restartless updates possible.

In terms of how to expose this, I have been wanting for some time for a way for APIs to know what add-on is calling them. So add-on A cannot mark add-on B as "busy" for example. So we should extend the data structure passed to bootstrap.js scripts with a "secret" property which is a unique object that the add-ons manager retains and can be mapped to the add-on. Then we just have:

AddonManager.blockUpdates(secret);
AddonManager.unblockUpdates(secret);

Comment 7

[Tony Mechelynck [:tonymec]]

Possible workaround:

One possible way to delay upgrades is to "Update Add-ons Manually" then if necessary (but it probably won't be) "Reset All Add-ons to Update Manually" before you start your critical operation, whatever it may be (on a desktop computer, it might be running some high-fidelity movie with sound maybe? Or even making a new clone of mozilla-central, which could otherwise suffer transfer timeouts?). Once the critical operation is finished, you may either "Update Add-ons Automatically", or else, remain on manual operation and trigger the "Check for Updates" function of the Add-ons Manager manually when you're ready for it, not doing anything time-critical, and, let's say, at least once every 24 hours.

This comment is from the point of view of a desktop application, which is all I know, and is within the range of the Toolkit product. I am conscious that comment #0 seems to be from a mobile user; in that case I hope that the Add-ons Manager UI in use on mobile devices does not remove too much of the functionality present on desktop applications.

Comment 8

[Robert Helmer [:rhelmer]]

(In reply to Tony Mechelynck [:tonymec] from comment #7)
> Possible workaround:
> 
> One possible way to delay upgrades is to "Update Add-ons Manually" then if
> necessary (but it probably won't be) "Reset All Add-ons to Update Manually"
> before you start your critical operation, whatever it may be (on a desktop
> computer, it might be running some high-fidelity movie with sound maybe? Or
> even making a new clone of mozilla-central, which could otherwise suffer
> transfer timeouts?). Once the critical operation is finished, you may either
> "Update Add-ons Automatically", or else, remain on manual operation and
> trigger the "Check for Updates" function of the Add-ons Manager manually
> when you're ready for it, not doing anything time-critical, and, let's say,
> at least once every 24 hours.
> 
> This comment is from the point of view of a desktop application, which is
> all I know, and is within the range of the Toolkit product. I am conscious
> that comment #0 seems to be from a mobile user; in that case I hope that the
> Add-ons Manager UI in use on mobile devices does not remove too much of the
> functionality present on desktop applications.

Oh, comment #0 was written with desktop in mind - the specific use case in mind is to be able to do restartless update of the "system" add-ons that now ship with Firefox on Nightly right now (Hello and Pocket), without interrupting what the user is doing.

There's no reason to restrict this feature to only system add-ons though, any add-on should be able to temporarily delay update if it would interrupt what the user is trying to do with it.

Comment 9

[Robert Helmer [:rhelmer]]

(In reply to Dave Townsend [:mossop] from comment #6)
> This seems like something useful for all add-ons but I think we should be
> careful about how this works. We don't want it to be possible to block a
> user disabling or uninstallin an add-on they don't want. We also don't want
> an add-on refusing to ever update. A particularly faulty add-on could
> accidentally make it impossible to fix itself with this. Here is a proposal:
> 
> We provide an API to allow an add-on to say it shouldn't be updated. I think
> it makes most sense for an add-on to be able to signal "busy" then "not
> busy" when done rather than listening for events and blocking them. Add-ons
> are always "not busy" at startup.
> 
> If we find an update available and after download the add-on is in the busy
> state we should just queue up the update as if the add-on was
> non-restartless. This way if an add-on ever forgets the flag "not busy" he
> update still happens the next time Firefox restarts. Presumably if the
> add-on flags "not busy" and there is a pending upgrade then we can just
> apply it then.
> 
> System add-ons are slightly more complicated because we've been assuming
> that system add-ons update all at once to ensure dependencies are satisfied.
> So one add-on being "busy" would block all updates but again we would
> schedule the update to happen at the next restart, though since that is what
> happens now the harder part is to take this into account when we make
> restartless updates possible.
> 
> In terms of how to expose this, I have been wanting for some time for a way
> for APIs to know what add-on is calling them. So add-on A cannot mark add-on
> B as "busy" for example. So we should extend the data structure passed to
> bootstrap.js scripts with a "secret" property which is a unique object that
> the add-ons manager retains and can be mapped to the add-on. Then we just
> have:
> 
> AddonManager.blockUpdates(secret);
> AddonManager.unblockUpdates(secret);


This all sounds good to me. Just to make sure I am clear:

1) AddonManager passes secret (e.g. uuid) as new optional arg to bootstrap.js startup()
2) addon calls AddonManager.blockUpdates(secret);
3) update is available but blocked, so queued for next restart (in case addon forgets/is unable to unblock)

If the addon calls AddonManager.unblockUpdates(secret) at this point, is the pending update immediately applied?

Also - I spoke with billm briefly about supporting WebExtensions, maybe it would make sense to add support for accepting the secret to WebExtensionBootstrap.js and leave coming up with a user-facing API call for later?

Comment 10

[Robert Helmer [:rhelmer]]

(In reply to Robert Helmer [:rhelmer] from comment #9)
> Also - I spoke with billm briefly about supporting WebExtensions, maybe it
> would make sense to add support for accepting the secret to
> WebExtensionBootstrap.js and leave coming up with a user-facing API call for
> later?

Actually it looks like there's already an API for this in Chrome's implementation, I think we could support this with the current proposed implementation:
https://developer.chrome.com/extensions/runtime#event-onUpdateAvailable

Comment 11

[Dave Townsend [:mossop]]

(In reply to Robert Helmer [:rhelmer] from comment #9)
> (In reply to Dave Townsend [:mossop] from comment #6)
> > This seems like something useful for all add-ons but I think we should be
> > careful about how this works. We don't want it to be possible to block a
> > user disabling or uninstallin an add-on they don't want. We also don't want
> > an add-on refusing to ever update. A particularly faulty add-on could
> > accidentally make it impossible to fix itself with this. Here is a proposal:
> > 
> > We provide an API to allow an add-on to say it shouldn't be updated. I think
> > it makes most sense for an add-on to be able to signal "busy" then "not
> > busy" when done rather than listening for events and blocking them. Add-ons
> > are always "not busy" at startup.
> > 
> > If we find an update available and after download the add-on is in the busy
> > state we should just queue up the update as if the add-on was
> > non-restartless. This way if an add-on ever forgets the flag "not busy" he
> > update still happens the next time Firefox restarts. Presumably if the
> > add-on flags "not busy" and there is a pending upgrade then we can just
> > apply it then.
> > 
> > System add-ons are slightly more complicated because we've been assuming
> > that system add-ons update all at once to ensure dependencies are satisfied.
> > So one add-on being "busy" would block all updates but again we would
> > schedule the update to happen at the next restart, though since that is what
> > happens now the harder part is to take this into account when we make
> > restartless updates possible.
> > 
> > In terms of how to expose this, I have been wanting for some time for a way
> > for APIs to know what add-on is calling them. So add-on A cannot mark add-on
> > B as "busy" for example. So we should extend the data structure passed to
> > bootstrap.js scripts with a "secret" property which is a unique object that
> > the add-ons manager retains and can be mapped to the add-on. Then we just
> > have:
> > 
> > AddonManager.blockUpdates(secret);
> > AddonManager.unblockUpdates(secret);
> 
> 
> This all sounds good to me. Just to make sure I am clear:
> 
> 1) AddonManager passes secret (e.g. uuid) as new optional arg to
> bootstrap.js startup()

I would make it a property on the data object already passed to all the bootstrap methods. Identifier is a bit overloaded, maybe instanceID is a good name for it rather than secret.

I'd encourage this being an immutable essentially opaque blob so there isn't a trivial way to spoof it. Previously I'd have just suggested a new {} for each add-on but we have ES6 now so how about Symbol(addon.id) which will give you something unique and still retains info about what add-on it is for for logging purposes.

> 2) addon calls AddonManager.blockUpdates(secret);
> 3) update is available but blocked, so queued for next restart (in case
> addon forgets/is unable to unblock)
> 
> If the addon calls AddonManager.unblockUpdates(secret) at this point, is the
> pending update immediately applied?

Yes, but happy for that to be a follow-up bug if that makes sense.

> Also - I spoke with billm briefly about supporting WebExtensions, maybe it
> would make sense to add support for accepting the secret to
> WebExtensionBootstrap.js and leave coming up with a user-facing API call for
> later?

Yeah this is opt-in so we don't have to support different add-on types immediately, we can do that work in other bugs.

Comment 12

[Robert Helmer [:rhelmer]]

Broke out the secret Symbol() passing to bootstrap.js startup() out to bug 1249689.

Comment 13

[Robert Helmer [:rhelmer]]

Mossop and I were discussing this in IRC, and instead of using explicit blockUpdates() and unblockUpdates() methods, we could be closer to the Chrome API (https://developer.chrome.com/extensions/runtime#event-onUpdateAvailable) and do this:

1) AddonManager passes Symbol(addon.id) as a property on the data object passed to bootstrap.js startup()
2) extension chooses to registers listener AddonManager.onUpdateAvailable.addListener(Symbol(addon.id), function callback), otherwise updates are immediately applied
3) when onUpdateAvailable fires, extension can choose to ignore or apply update

If the extension ignores the update, it will be queued for next application restart the same as existing non-restartless extensions.

Comment 14

[Robert Helmer [:rhelmer]]

Attachment: Bug 1231172 - provide API for add-ons to delay restartless updates

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

Comment 15

[Robert Helmer [:rhelmer]]

Comment on attachment 8744083 [details]
Bug 1231172 - provide API for add-ons to delay restartless updates

The API for this is inspired by the https://developer.chrome.com/extensions/runtime#event-onUpdateAvailable and is intended to be easy to implement the web extension API on top of. The approach of delaying upgrade if a listener is present is the same, but in our case the add-on gets a callback to initiate upgrade when it is ready (web extension impl would need to do this part differently).

The add-on must hold a valid instanceID symbol in order to add the upgrade listener.

This patch add a new state to the AddonInstall state machine, STATE_POSTPONED, which reuses the existing restartless install code when the upgradeListener is added. It's possible to leave an add-on in this state (in which case it upgrades upon restart normally) or for the add-on to initiate upgrade when it is ready, in which case it re-uses the already downloaded XPI.

I have existing and new tests passing now, although I think I found a little bug in the instanceID passing that I need to fix (instanceIDs are not passed to addons if they are present on startup it seems, I think the way it's checking to see if it's a restartless add-on is wrong).

So, not ready to land, but the above instanceID issue would be a separate commit so just looking for feedback on this approach. Thanks!

Comment 16

[Robert Helmer [:rhelmer]]

https://reviewboard.mozilla.org/r/48263/#review45021

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:6017
(Diff revision 1)
> +        } else {
> +          // no upgrade listener present, so proceed with normal install
> +          this.install();
> +        }
>  
> +        // TODO do we need to support these?

Oh, glad I added a `TODO` - `linkedInstalls` here refers to https://developer.mozilla.org/en-US/docs/Multiple_Item_Packaging which I suppose we do want to support, although I don't think this type of extension is very common.

::: toolkit/mozapps/extensions/test/xpcshell/data/test_delay_updates.json:1
(Diff revision 1)
> +{

this file is unused and should be removed.

::: toolkit/mozapps/extensions/test/xpcshell/data/test_delay_updates.rdf:1
(Diff revision 1)
> +<?xml version="1.0" encoding="UTF-8"?>

this file is unused and should be removed.

::: toolkit/mozapps/extensions/test/xpcshell/data/test_delay_updates_forget.rdf:1
(Diff revision 1)
> +<?xml version="1.0" encoding="UTF-8"?>

this file is unused and should be removed.

Comment 17

[Andrew Swan [:aswan]]

https://reviewboard.mozilla.org/r/48263/#review44993

Before getting into detailed feedback, I'm having trouble following the expected flow of control.
First, is this right: when a callback has been registered and the download of an upgrade finishes, we call the callback, set `addon.postponed` to true, then enter STATE_POSTPONED.  But then we call `install()` to continue the install state machine which calls `startInstall()` which moves us briefly into STATE_INSTALLING but then since `addon.postponed` is true, it moves us back to STATE_POSTPONED and pauses.  If that's right, that brief pass through STATE_INSTALLING seems quirky.  It also means that if the install is indeed postponed, then when it is resumed we could execute the code at the beginning of `startInstall()` twice (which would for instance trigger onInstallStarted twice).
Also, the way the add-on unblocks the upgrade isn't clear to me, the install callback that is given to the add-on just installs a listener for onInstallPostponed.  (Naming nitpick, that name is misleading since it doesn't mean "the install has been postponed", but rather it means "the install has reached the point where it needs to know whether it is okay to proceed or not"?)  Anyhow, if the add-on calls install synchronously from its upgrade listener, I see how that listener gets invoked, but if the add-on waits some time and then calls install, what gets us back into the AddonInstall state machine to trigger that onInstallPostponed event and move things along?

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:4453
(Diff revision 1)
>        return false;
>  
>      if (this.e10sBlocksEnabling(aAddon))
>        return true;
>  
> +    // Add-ons which have declined upgrade are scheduled for upgrade on restart.

Something seems wrong about this, I would think we would only ever check whether installation requires a restart on an addon that isn't already installed.  And likewise shouldn't we be forcing `uninstallRequiresRestart()` to be true for when `.postponed` is true?

Comment 18

[Robert Helmer [:rhelmer]]

(In reply to Andrew Swan [:aswan] from comment #17)
> https://reviewboard.mozilla.org/r/48263/#review44993
> 
> Before getting into detailed feedback, I'm having trouble following the
> expected flow of control.
> First, is this right: when a callback has been registered and the download
> of an upgrade finishes, we call the callback, set `addon.postponed` to true,
> then enter STATE_POSTPONED.  But then we call `install()` to continue the
> install state machine which calls `startInstall()` which moves us briefly
> into STATE_INSTALLING but then since `addon.postponed` is true, it moves us
> back to STATE_POSTPONED and pauses.  If that's right, that brief pass
> through STATE_INSTALLING seems quirky.
>  It also means that if the install is
> indeed postponed, then when it is resumed we could execute the code at the
> beginning of `startInstall()` twice (which would for instance trigger
> onInstallStarted twice).

Yes I think that's correct, if in STATE_POSTPONED and the add-on has unblocked,  `addon.postponed` causes `requiresRestart` to be true
in startInstall(), which stages the add-on for install on restart, so the update
happens at some point regardless if the add-on forgets or is unable to unblock.

The way add-on manager processes these staged add-ons on startup is here in `XPIProvider.processPendingFileChanges`:
https://dxr.mozilla.org/mozilla-central/rev/1152d99d8c53ac9dae371a6e6d9fab03d3f98697/toolkit/mozapps/extensions/internal/XPIProvider.jsm#3164

However that code is not at all async-friendly so would need some refactoring to extract the shared bits.
I think that would make the flow more clear, what do you think?

> Also, the way the add-on unblocks the upgrade isn't clear to me, the install
> callback that is given to the add-on just installs a listener for
> onInstallPostponed.  (Naming nitpick, that name is misleading since it
> doesn't mean "the install has been postponed", but rather it means "the
> install has reached the point where it needs to know whether it is okay to
> proceed or not"?)

Hm I don't see the distinction - the upgrade has been staged in exactly the same way that a non-restartless add-on would be, so it feels like it is now "postponed" until restart, unless the add-on takes some explicit action. Happy to change it though if it can be made more succinct, do you have any suggestions? :)

> Anyhow, if the add-on calls install synchronously from
> its upgrade listener, I see how that listener gets invoked, but if the
> add-on waits some time and then calls install, what gets us back into the
> AddonInstall state machine to trigger that onInstallPostponed event and move
> things along?


Hm yes I think you're right, I need to rethink that. I'll make the tests cover this too. Thanks!


> ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:4453
> (Diff revision 1)
> >        return false;
> >  
> >      if (this.e10sBlocksEnabling(aAddon))
> >        return true;
> >  
> > +    // Add-ons which have declined upgrade are scheduled for upgrade on restart.
> 
> Something seems wrong about this, I would think we would only ever check
> whether installation requires a restart on an addon that isn't already
> installed.

I see what you mean, do you think using something more direct like `XPIProvider.processPendingFileChanges`
instead of trying to re-use the existing install code would alleviate the wrongness? :)

> And likewise shouldn't we be forcing
> `uninstallRequiresRestart()` to be true for when `.postponed` is true?


I'm not sure. The use case here is that an add-on is busy (e.g. in the middle of a video chat), and the user doesn't want to be unexpectedly interrupted by a silent upgrade taking the add-on down unexpectedly.

However if the user is in the middle of a video call and opens the add-ons manager UI and uninstalls the add-on, then it seems appropriate to tear it down immediately.

Maybe I am overthinking this though, I do see the logic of what you're saying.

Comment 19

[Andrew Swan [:aswan]]

(In reply to Robert Helmer [:rhelmer] from comment #18)
> (In reply to Andrew Swan [:aswan] from comment #17)
> > https://reviewboard.mozilla.org/r/48263/#review44993
> > 
> > Before getting into detailed feedback, I'm having trouble following the
> > expected flow of control.
> > First, is this right: when a callback has been registered and the download
> > of an upgrade finishes, we call the callback, set `addon.postponed` to true,
> > then enter STATE_POSTPONED.  But then we call `install()` to continue the
> > install state machine which calls `startInstall()` which moves us briefly
> > into STATE_INSTALLING but then since `addon.postponed` is true, it moves us
> > back to STATE_POSTPONED and pauses.  If that's right, that brief pass
> > through STATE_INSTALLING seems quirky.
> >  It also means that if the install is
> > indeed postponed, then when it is resumed we could execute the code at the
> > beginning of `startInstall()` twice (which would for instance trigger
> > onInstallStarted twice).
> 
> Yes I think that's correct, if in STATE_POSTPONED and the add-on has
> unblocked,  `addon.postponed` causes `requiresRestart` to be true
> in startInstall(), which stages the add-on for install on restart, so the
> update
> happens at some point regardless if the add-on forgets or is unable to
> unblock.

But this is my confusion from the other line-item comment: I thought postponed is set on the currently installed addon and requiresRestart comes from calling `installRequiresRestart()` on the new addon?

> The way add-on manager processes these staged add-ons on startup is here in
> `XPIProvider.processPendingFileChanges`:
> https://dxr.mozilla.org/mozilla-central/rev/
> 1152d99d8c53ac9dae371a6e6d9fab03d3f98697/toolkit/mozapps/extensions/internal/
> XPIProvider.jsm#3164
> 
> However that code is not at all async-friendly so would need some
> refactoring to extract the shared bits.
> I think that would make the flow more clear, what do you think?

Sorry I didn't follow this, I get that the install needs to be staged so that it will happen at restart if the addon never unblocks.  But are you saying that if/when the addon does unblock that then you want to use the logic from processPendingFileChanges?  I would have thought that clearing the postponed flag and resuming the AddonInstall state machine would be the simpler technique (not least since I think processPendingFileChanges is, as you say, written to be synchronous since it runs at startup time). 

> > Also, the way the add-on unblocks the upgrade isn't clear to me, the install
> > callback that is given to the add-on just installs a listener for
> > onInstallPostponed.  (Naming nitpick, that name is misleading since it
> > doesn't mean "the install has been postponed", but rather it means "the
> > install has reached the point where it needs to know whether it is okay to
> > proceed or not"?)
> 
> Hm I don't see the distinction - the upgrade has been staged in exactly the
> same way that a non-restartless add-on would be, so it feels like it is now
> "postponed" until restart, unless the add-on takes some explicit action.
> Happy to change it though if it can be made more succinct, do you have any
> suggestions? :)

I think that what threw me about this is related to the next comment below: the event of the install being postponed happens once but the install method that the addon uses to unblocks listens for that event and so I was perplexed about how that event would ever occur some significant time after the initial callback to the addon.  Or to put it in other words, I think that to address the comment below you'll end up removing or reworking that event...

> > Anyhow, if the add-on calls install synchronously from
> > its upgrade listener, I see how that listener gets invoked, but if the
> > add-on waits some time and then calls install, what gets us back into the
> > AddonInstall state machine to trigger that onInstallPostponed event and move
> > things along?
> 
> 
> Hm yes I think you're right, I need to rethink that. I'll make the tests
> cover this too. Thanks!
> 
> 
> > ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:4453
> > (Diff revision 1)
> > >        return false;
> > >  
> > >      if (this.e10sBlocksEnabling(aAddon))
> > >        return true;
> > >  
> > > +    // Add-ons which have declined upgrade are scheduled for upgrade on restart.
> > 
> > Something seems wrong about this, I would think we would only ever check
> > whether installation requires a restart on an addon that isn't already
> > installed.
> 
> I see what you mean, do you think using something more direct like
> `XPIProvider.processPendingFileChanges`
> instead of trying to re-use the existing install code would alleviate the
> wrongness? :)

I'm not sure, but I think this is basically the same issue from the very first remarks above.

> > And likewise shouldn't we be forcing
> > `uninstallRequiresRestart()` to be true for when `.postponed` is true?
> 
> 
> I'm not sure. The use case here is that an add-on is busy (e.g. in the
> middle of a video chat), and the user doesn't want to be unexpectedly
> interrupted by a silent upgrade taking the add-on down unexpectedly.
> 
> However if the user is in the middle of a video call and opens the add-ons
> manager UI and uninstalls the add-on, then it seems appropriate to tear it
> down immediately.
> 
> Maybe I am overthinking this though, I do see the logic of what you're
> saying.

Yeah, this is getting kind of clunky.  Its not really true that uninstalling the old addon requires a restart.  Perhaps the more targeted change would be to add `|| existingAddon.blocked` here?
https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#4444

Comment 20

[Robert Helmer [:rhelmer]]

Andrew and I chatted about this over IRC, which concluded:

<aswan> if the addon has an update callback then when the download finishes, we always stage the install, notify the addon, and go to STATE_POSTPONED
<aswan> then when the addon unblocks, we unstage the install and go to STATE_INSTALLING
<aswan> and onInstallStarted always fires right when we enter STATE_INSTALLING
<aswan> which is immediately after the download finishes if there's no callback, or what's described above if there is
<aswan> i think that could be implemented with modest refactoring, just teasing out the existing stage and unstage bits

This should resolve the concerns above and make the control flow easier to follow too.

Comment 21

[Robert Helmer [:rhelmer]]

Comment on attachment 8744083 [details]
Bug 1231172 - provide API for add-ons to delay restartless updates

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

Comment 22

[Robert Helmer [:rhelmer]]

Comment on attachment 8744083 [details]
Bug 1231172 - provide API for add-ons to delay restartless updates

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

Comment 23

[Robert Helmer [:rhelmer]]

Comment on attachment 8744083 [details]
Bug 1231172 - provide API for add-ons to delay restartless updates

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48263/diff/3-4/

Comment 24

[Robert Helmer [:rhelmer]]

This is getting close but a few more things before getting review:

* add a test case to make sure add-ons can resume after a delay (right now it unblocks immediately)
* handle linkedInstall case (https://developer.mozilla.org/en-US/docs/Multiple_Item_Packaging)
* cleanup stage/unstageInstall() refactoring

I have rebased and pushed the latest up to mozreview, doing a full try run now to make sure this doesn't break any existing tests before doing these last bits.

Comment 25

[Robert Helmer [:rhelmer]]

Comment on attachment 8744083 [details]
Bug 1231172 - provide API for add-ons to delay restartless updates

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48263/diff/4-5/

Comment 26

[Robert Helmer [:rhelmer]]

Attachment: Bug 1231172 - instanceID should be passed to add-ons present at startup too

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

Comment 27

[Robert Helmer [:rhelmer]]

Comment on attachment 8744083 [details]
Bug 1231172 - provide API for add-ons to delay restartless updates

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48263/diff/5-6/

Comment 28

[Robert Helmer [:rhelmer]]

Comment on attachment 8744083 [details]
Bug 1231172 - provide API for add-ons to delay restartless updates

Mind taking another look? I think I addressed everything that we've both identified so far.

Comment 29

[Robert Helmer [:rhelmer]]

Comment on attachment 8746833 [details]
Bug 1231172 - instanceID should be passed to add-ons present at startup too

I don't think we want the `aAddon.bootstrap` check here, this isn't set when an addon is present at startup, and this is in the `callBootstrapMethod()` function so it must be using the bootstrap method right?

Comment 30

[Robert Helmer [:rhelmer]]

Sorry the interdiff probably won't be that useful since I've been pushing lots of little changes, the overall patch is pretty small though (mostly test code).

Comment 31

[Andrew Swan [:aswan]]

https://reviewboard.mozilla.org/r/48263/#review47083

Looking good!

::: toolkit/mozapps/extensions/AddonManager.jsm:2275
(Diff revision 6)
> +   this.getAddonByInstanceID(aInstanceID).then(wrapper => {
> +     if (!wrapper) {
> +       throw Error("No addon matching instanceID:", aInstanceID.toString());
> +     }
> +     let addonId = wrapper.addonId();
> +     logger.info(`Registering upgrade listener for ${addonId}`)

I assume this gets downgraded to debug (or removed) before eventually landing?

::: toolkit/mozapps/extensions/AddonManager.jsm:3150
(Diff revision 6)
>      ["STATE_INSTALLED", 6],
>      // The install failed.
>      ["STATE_INSTALL_FAILED", 7],
>      // The install has been cancelled.
>      ["STATE_CANCELLED", 8],
> +    // The install has been postponed.

Nitpick: I think it would be good to be more explicit about the fact that if an install is in this state, that implies that it is fully downloaded and ready to install.  So logically it comes between STATE_DOWNLOADED and STATE_INSTALLING.
If these values never get persisted anywhere, you could even give this the value 5 and renumber everything else, but if that's not practical then just some documentation would be helpful.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:6055
(Diff revision 6)
> +                    this.state = AddonManager.STATE_DOWNLOADED;
> +                    this.install();
> +                    if (this.linkedInstalls) {
> +                      for (let install of this.linkedInstalls) {
> +                        install.state = AddonManager.STATE_DOWNLOADED;
> +                        install.install();

if you move this loop above `this.install()`, I think you don't actually need to call `install()` on all the linked installs?  (since resuming the parent install should do it?)

actually on further thought, this whole linked installs thing is a hairball:
- how do you know its safe to move the linked install to the downloaded state?
- what if one of the linked installs has a registered upgrade listener and it hasn't yet unblocked?

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:6252
(Diff revision 6)
>    },
>  
> +  /**
> +   * Stages an upgrade for next application restart.
> +   */
> +  stageInstall: function*(restartRequired, stagedAddon, isUpgrade) {

I think `restartRequired` is a little misleading, since you sometimes call this on restartless addons?  (ie if it is a postponed upgrade).  I guess in a sense restart is required to do the upgrade unless/until the addon unblocks the upgrade but I still think the name is confusing...

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:6298
(Diff revision 6)
> +        stream.close();
> +      }
> +
> +      logger.debug("Staged install of " + this.addon.id + " from " + this.sourceURI.spec + " ready; waiting for restart.");
> +      if (isUpgrade) {
> +        delete this.existingAddon.pendingUpgrade;

I don't think you need this line since you overwrite pendingUpgrade on the next line

::: toolkit/mozapps/extensions/test/addons/test_delay_update_complete/bootstrap.js:16
(Diff revision 6)
> +function startup(data, reason) {
> +  // apply update immediately
> +  if (data.hasOwnProperty("instanceID") && data.instanceID) {
> +    AddonManager.addUpgradeListener(data.instanceID, (upgrade) => {
> +
> +      // spin the event loop a few times to make sure that applying

I don't know much about the underlying implementation here, is it possible that there are enough other events queued up that after a few calls to processNextEvent() the code in XPIProvider that leaves the install in STATE_POSTPONED hasn't actually run yet?

::: toolkit/mozapps/extensions/test/xpcshell/test_delay_update.js:29
(Diff revision 6)
> +
> +  // Create and configure the HTTP server.
> +  testserver = new HttpServer();
> +  testserver.registerDirectory("/data/", do_get_file("data"));
> +  testserver.registerDirectory("/addons/", do_get_file("addons"));
> +  testserver.start(4444);

nitpick: i've seen other tests let the server bind to an available port and then build up their urls at runtime.  that would prevent intermittent failures in case something else happens to have grabbed port 4444 when your test runs.  for example: https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/tests/data/xpcshellUtilsAUS.js#3307-3314

::: toolkit/mozapps/extensions/test/xpcshell/test_delay_update.js:180
(Diff revision 6)
> +            do_execute_soon(check_test_complete_install);
> +          },
> +          onInstallFailed: function(aInstall) {
> +            do_throw("onInstallFailed should not fire");
> +          },
> +          onInstallPostponed: function(aInstall) {},

This could set a flag that you could test below, just to ensure that it actually happened?

Comment 32

[Andrew Swan [:aswan]]

https://reviewboard.mozilla.org/r/49577/#review47097

This looks good, though Mossop has in the past urged me to write new tests in the promise / add_task() style instead of the callback style here.
I also find that style easier to read...

::: toolkit/mozapps/extensions/test/xpcshell/test_delay_update.js:56
(Diff revision 1)
> +  restartManager();
> +
>    AddonManager.getInstallForFile(do_get_addon("test_delay_update_ignore"), function(install) {
>      install.addListener({
>        onInstallEnded: function(aInstall) {
> -        do_execute_soon(check_test_ignore_postponed);
> +        check_test_ignore_postponed(check_test_ignore_postponed_still);

don't you still need a do_execute_soon() around this?

Comment 33

[Robert Helmer [:rhelmer]]

https://reviewboard.mozilla.org/r/49577/#review47097

We discussed this in IRC, for posterity: I did originally write the tests in the newer promise / add_task() style, but was having problems with the test existing before the upgrade totally finished so I ended up looking at how the older existing update tests worked, and did something similar.

I'll try converting them over to the newer style and see if I can figure out what I was doing wrong before.

> don't you still need a do_execute_soon() around this?

I don't think there's anything else that needs to happen after `onInstallEnded` fires before the next test can begin.

I remembered that `do_execute_soon()` spins the event loop in some way, I just read it over and it adds the callback to the event queue after the current call chain returns https://dxr.mozilla.org/mozilla-central/source/testing/xpcshell/head.js#683 explained further in https://developer.mozilla.org/en-US/Add-ons/Code_snippets/Threads

Maybe the issue here is that I don't need to be using `do_execute_soon()` anywhere in here :) Do you think there's a reason we need it?

Comment 34

[Andrew Swan [:aswan]]

https://reviewboard.mozilla.org/r/49577/#review47097

> I don't think there's anything else that needs to happen after `onInstallEnded` fires before the next test can begin.
> 
> I remembered that `do_execute_soon()` spins the event loop in some way, I just read it over and it adds the callback to the event queue after the current call chain returns https://dxr.mozilla.org/mozilla-central/source/testing/xpcshell/head.js#683 explained further in https://developer.mozilla.org/en-US/Add-ons/Code_snippets/Threads
> 
> Maybe the issue here is that I don't need to be using `do_execute_soon()` anywhere in here :) Do you think there's a reason we need it?

I think its to guard against a situation where the next test case immediately does something like restarting the add-ons manager, which could go badly if it happens from within a callback from the add-ons manager.  It doesn't look like this particular test does that, but I think the practice was just to be consistent and do it everywhere rather than decide case-by-case where it is and isn't needed.

Comment 35

[Robert Helmer [:rhelmer]]

Comment on attachment 8744083 [details]
Bug 1231172 - provide API for add-ons to delay restartless updates

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48263/diff/6-7/

Comment 36

[Robert Helmer [:rhelmer]]

Comment on attachment 8746833 [details]
Bug 1231172 - instanceID should be passed to add-ons present at startup too

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

Comment 37

[Robert Helmer [:rhelmer]]

https://reviewboard.mozilla.org/r/48263/#review47083

> I assume this gets downgraded to debug (or removed) before eventually landing?

Hm, I thought this might be useful to add-on developers, most of which I don't think know to flip the logging pref. Although I think tools like `web-ext` and `jpm` do flip that pref so maybe debug is fine.

> if you move this loop above `this.install()`, I think you don't actually need to call `install()` on all the linked installs?  (since resuming the parent install should do it?)
> 
> actually on further thought, this whole linked installs thing is a hairball:
> - how do you know its safe to move the linked install to the downloaded state?
> - what if one of the linked installs has a registered upgrade listener and it hasn't yet unblocked?

Hm you're right :/ This needs to be a lot more involved if we want it to really work. I wonder if we shouldn't just do this in a followup.

Actually on further investigation - I think that linked installs only matter for the "install" case, not the "upgrade" case (they are installed together initially, but upgrades happen independently).

So I think we can just remove this completely.

> I think `restartRequired` is a little misleading, since you sometimes call this on restartless addons?  (ie if it is a postponed upgrade).  I guess in a sense restart is required to do the upgrade unless/until the addon unblocks the upgrade but I still think the name is confusing...

This was the variable name before I refactored the function. I think there are other reasons besides non-restartless add-ons that restart can be required... I don't recall offhand but will look it up.

> I don't think you need this line since you overwrite pendingUpgrade on the next line

That was in the original code I refactored into this function, if the `delete` is removed then this is thrown:

`TypeError: setting a property that has only a getter`

`pendingUpgrade` is a getter on `AddonWrapper`, and it either returns `null` or a wrapper.

Not sure what the advantage is to doing it this way, but it's done in at least one other place in XPIProvider so should probably be tackled in a separate bug if we want to change it.

> I don't know much about the underlying implementation here, is it possible that there are enough other events queued up that after a few calls to processNextEvent() the code in XPIProvider that leaves the install in STATE_POSTPONED hasn't actually run yet?

I picked that number arbitrarily, I'll see if there's a better way to do this.

> nitpick: i've seen other tests let the server bind to an available port and then build up their urls at runtime.  that would prevent intermittent failures in case something else happens to have grabbed port 4444 when your test runs.  for example: https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/tests/data/xpcshellUtilsAUS.js#3307-3314

Yes this is lazy of me, I'll make it more like the newer tests.

The thing that makes this tricky is that the port number is hardcoded in the `update.rdf` files, I should probably just switch to writing the files from the code instead of checking them in...

I'll do this as part of rewriting the test to the newer style.

Comment 38

[Robert Helmer [:rhelmer]]

Addressed review comments, going to try rewriting the unit test to the new style.

Comment 39

[Robert Helmer [:rhelmer]]

Comment on attachment 8744083 [details]
Bug 1231172 - provide API for add-ons to delay restartless updates

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48263/diff/7-8/

Comment 40

[Robert Helmer [:rhelmer]]

Comment on attachment 8746833 [details]
Bug 1231172 - instanceID should be passed to add-ons present at startup too

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

Comment 41

[Robert Helmer [:rhelmer]]

Comment on attachment 8746833 [details]
Bug 1231172 - instanceID should be passed to add-ons present at startup too

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49577/diff/3-4/

Comment 42

[Robert Helmer [:rhelmer]]

Comment on attachment 8744083 [details]
Bug 1231172 - provide API for add-ons to delay restartless updates

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48263/diff/8-9/

Comment 43

[Robert Helmer [:rhelmer]]

Comment on attachment 8746833 [details]
Bug 1231172 - instanceID should be passed to add-ons present at startup too

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49577/diff/4-5/

Comment 44

[Robert Helmer [:rhelmer]]

Comment on attachment 8744083 [details]
Bug 1231172 - provide API for add-ons to delay restartless updates

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48263/diff/9-10/

Comment 45

[Robert Helmer [:rhelmer]]

Comment on attachment 8744083 [details]
Bug 1231172 - provide API for add-ons to delay restartless updates

Just FYI the reason I keep r?ing myself is because of bug 1277849 in mozreview :)

Comment 46

[Robert Helmer [:rhelmer]]

Comment on attachment 8746833 [details]
Bug 1231172 - instanceID should be passed to add-ons present at startup too

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49577/diff/5-6/

Comment 47

[Robert Helmer [:rhelmer]]

Comment on attachment 8744083 [details]
Bug 1231172 - provide API for add-ons to delay restartless updates

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48263/diff/10-11/

Comment 48

[Robert Helmer [:rhelmer]]

Comment on attachment 8744083 [details]
Bug 1231172 - provide API for add-ons to delay restartless updates

try run revealed that I had pulled a bad cset from fx-team and there were some underlying test failures that were not the fault of these new commits.

I've rebased to latest fx-team and things are looking good locally, I just pushed to try to confirm.

Comment 49

[Andrew Swan [:aswan]]

Comment on attachment 8746833 [details]
Bug 1231172 - instanceID should be passed to add-ons present at startup too

https://reviewboard.mozilla.org/r/49577/#review54872

Comment 50

[Andrew Swan [:aswan]]

Comment on attachment 8744083 [details]
Bug 1231172 - provide API for add-ons to delay restartless updates

https://reviewboard.mozilla.org/r/48263/#review54898

Overall, this looks great, a few minor comments below.  Also a previous version of the patch had a test in which the install listener waited some time (I think by spinning the vent loop a few times) before allowing the install to resume.  What happened to that test?

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:5427
(Diff revision 11)
> +      logger.debug(`Cancelling postponed install of ${this.addon.id}`);
> +      this.state = AddonManager.STATE_CANCELLED;
> +      XPIProvider.removeActiveInstall(this);
> +      AddonManagerPrivate.callInstallListeners("onInstallCancelled",
> +                                               this.listeners, this.wrapper);
> +      this.removeTemporaryFile();

doesn't the staged install also need to get removed here?

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:6048
(Diff revision 11)
> +            let callback = AddonManagerPrivate.getUpgradeListener(this.addon.id);
> +            callback({
> +              install: () => {
> +                switch (this.state) {
> +                  case AddonManager.STATE_INSTALLED:
> +                    // this addon has already been installed, nothing to do

mostly for my curiosity, how would this ever happen?

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:6062
(Diff revision 11)
> +                    logger.warn(`${this.addon.id} cannot resume postponed upgrade from state (${this.state})`);
> +                    break;
> +                }
> +              },
> +            });
> +          }).bind(this));

since you're using arrow functions i think you don't need this?

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:6116
(Diff revision 11)
>                                             this.addon.wrapper,
>                                             requiresRestart);
>  
>      let stagingDir = this.installLocation.getStagingDir();
>      let stagedAddon = stagingDir.clone();
> +    let installedUnpacked = 0;

why did this move?

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:6311
(Diff revision 11)
> +  unstageInstall: function*(stagedAddon) {
> +    let stagedJSON = stagedAddon.clone();
> +    stagedJSON.append(this.addon.id + ".json");
> +
> +    if (stagedJSON.exists()) {
> +      stagedJSON.remove(true);

if this succeeds, then we can just return here right?  ie, the branch below only applies if there isn't a .json file?

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:6314
(Diff revision 11)
> +
> +    if (stagedJSON.exists()) {
> +      stagedJSON.remove(true);
> +    }
> +
> +    stagedAddon.append(this.addon.id);

shouldn't this make a copy of stagedAddon for modifications?

::: toolkit/mozapps/extensions/test/xpcshell/data/test_delay_update_ignore/bootstrap.js:13
(Diff revision 11)
> +// normally we would use BootstrapMonitor here, but we need a reference to
> +// the symbol inside `XPIProvider.jsm`.
> +function startup(data, reason) {
> +  // explicitly ignore update, will be queued for next restart
> +  if (data.hasOwnProperty("instanceID") && data.instanceID) {
> +    AddonManager.addUpgradeListener(data.instanceID, (upgrade) => {});

could this set a pref or broadcast to an observer or something that you could check from the main test, just to know that the listener function did get called?

::: toolkit/mozapps/extensions/test/xpcshell/test_delay_update.js:93
(Diff revision 11)
> +
> +  let update = yield promiseFindAddonUpdates(addon);
> +  let install = update.updateAvailable;
> +
> +  yield promiseCompleteAllInstalls([install]);
> +

Check that install.state is STATE_POSTPONED?

Comment 51

[Robert Helmer [:rhelmer]]

Comment on attachment 8746833 [details]
Bug 1231172 - instanceID should be passed to add-ons present at startup too

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49577/diff/6-7/

Comment 52

[Robert Helmer [:rhelmer]]

Comment on attachment 8744083 [details]
Bug 1231172 - provide API for add-ons to delay restartless updates

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48263/diff/11-12/

Comment 53

[Robert Helmer [:rhelmer]]

https://reviewboard.mozilla.org/r/48263/#review54898

Sorry I should have called that out when I removed the test. I can't think how this would make a difference - the `AddonInstall` should stay in `STATE_POSTPONED` until something happens, right? I don't think doing work inside the callback should make any difference at all, but there is a third case I thought of (below):

I think the normal case is going to be that the add-on just checks some state inside the upgrade listener callback and either confirms or ignores the update (in which case it'll happen on restart). 

The add-on *could* hang on to the reference that is passed in the callback and invoke it later, which should be fine (as the `AddonInstall` will be in `STATE_POSTPONED`) - why don't we test this case explicitly? Do you think that'd satisfy this concern?

> mostly for my curiosity, how would this ever happen?

I don't think this should happen - it shouldn't be harmful so I don't want to assert, but I'd be interested in looking into this if this warning was every logged.

> since you're using arrow functions i think you don't need this?

This is for the `Task.spawn` not the `callback`.

> if this succeeds, then we can just return here right?  ie, the branch below only applies if there isn't a .json file?

Hm, no I think we want to do both, right? 1) remove the staged `json` if it exists, 2) remove the addon file itself if it exists. This was existing code I refactored FWIW.

> shouldn't this make a copy of stagedAddon for modifications?

Yes it should I'll change that.

Comment 54

[Robert Helmer [:rhelmer]]

(In reply to Robert Helmer [:rhelmer] from comment #53)
> https://reviewboard.mozilla.org/r/48263/#review54898
> 
> Sorry I should have called that out when I removed the test. I can't think
> how this would make a difference - the `AddonInstall` should stay in
> `STATE_POSTPONED` until something happens, right? I don't think doing work
> inside the callback should make any difference at all, but there is a third
> case I thought of (below):
> 
> I think the normal case is going to be that the add-on just checks some
> state inside the upgrade listener callback and either confirms or ignores
> the update (in which case it'll happen on restart). 
> 
> The add-on *could* hang on to the reference that is passed in the callback
> and invoke it later, which should be fine (as the `AddonInstall` will be in
> `STATE_POSTPONED`) - why don't we test this case explicitly? Do you think
> that'd satisfy this concern?


Just making sure you see this in the noise of the mozreview comment above - I am not sure it proves anything to do additional work this inside the callback, it sounds more like what you want is for the add-on to hold on to the reference passed to the callback and for the add-on to invoke it later when control gets passed to the add-on, is that correct?

For instance I am thinking the add-on could do something like:

1) register upgrade listener, which assigns the reference from the callback to a global in bootstrap.js
2) register some other listener, and when that gets called then try invoking upgrade.install()

(#2 could be any way in which the app passes control to the add-on, but our options are more limited since there isn't really a UI in the "xpcshell" app - there might be a better test here that is escaping me at the moment).

This seems like a legitimate use case, I suspect some (most?) add-ons just want to delay upgrade until next restart, but some would like to be more clever and apply the update when they are ready.

I think this is how we're going to need to use this API in order to implement this for web extensions:
https://developer.chrome.com/extensions/runtime#event-onUpdateAvailable

Comment 55

[Andrew Swan [:aswan]]

(In reply to Robert Helmer [:rhelmer] from comment #54)
> Just making sure you see this in the noise of the mozreview comment above -
> I am not sure it proves anything to do additional work this inside the
> callback, it sounds more like what you want is for the add-on to hold on to
> the reference passed to the callback and for the add-on to invoke it later
> when control gets passed to the add-on, is that correct?

Right, somebody (maybe it was you?  I can't remember...) mentioned a scenario of Hello deferring an upgrade if there's a call in progress, but then letting the upgrade proceed when the call ends.  I think that basically boils down to what you described (just that nothing happens immediately in the callback but the callback is invoked some time later after we've returned to the event loop).

I think that case is worth covering, as you describe next:

> For instance I am thinking the add-on could do something like:
> 
> 1) register upgrade listener, which assigns the reference from the callback
> to a global in bootstrap.js
> 2) register some other listener, and when that gets called then try invoking
> upgrade.install()

-Andrew

Comment 56

[Robert Helmer [:rhelmer]]

Comment on attachment 8744083 [details]
Bug 1231172 - provide API for add-ons to delay restartless updates

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48263/diff/12-13/

Comment 57

[Robert Helmer [:rhelmer]]

Comment on attachment 8744083 [details]
Bug 1231172 - provide API for add-ons to delay restartless updates

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48263/diff/13-14/

Comment 58

[Robert Helmer [:rhelmer]]

Try run lgtm, I ran all tests including perf tests (some tests install add-ons etc so didn't want to miss any) and the handful of failures don't look related to this patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d6ca01e7c4e0

Comment 59

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Florin, this is something that seems will most likely ride the 50 train to release. It's a mechanism to silently update system add-ons. Just an FYI.

Comment 60

[Andrew Swan [:aswan]]

Comment on attachment 8744083 [details]
Bug 1231172 - provide API for add-ons to delay restartless updates

https://reviewboard.mozilla.org/r/48263/#review55176

nice!

Comment 61

[Robert Helmer [:rhelmer]]

(In reply to Ritu Kothari (:ritu) from comment #59)
> Hi Florin, this is something that seems will most likely ride the 50 train
> to release. It's a mechanism to silently update system add-ons. Just an FYI.

We're unfortunately not quite ready to enable this for system add-ons quite yet - this enables it for all normal add-ons, and will be the basis for system add-ons and also web extension support for this feature.

I've filed a bug for this (bug 1278692) and cc'd you both, sorry for the confusion and thanks for you help!

Comment 62

[Robert Helmer [:rhelmer]]

Attachment: Bug 1231172 - instanceID should be passed to add-ons present at startup too

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

Comment 63

[Robert Helmer [:rhelmer]]

Attachment: Bug 1231172 - provide API for add-ons to delay restartless updates

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

Comment 64

[Andrew Swan [:aswan]]

Comment on attachment 8760988 [details]
Bug 1231172 - instanceID should be passed to add-ons present at startup too

https://reviewboard.mozilla.org/r/58362/#review55222

Comment 65

[Andrew Swan [:aswan]]

Comment on attachment 8760989 [details]
Bug 1231172 - provide API for add-ons to delay restartless updates

https://reviewboard.mozilla.org/r/58364/#review55224

Comment 66

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1878ebd93d8b
instanceID should be passed to add-ons present at startup too r=aswan
https://hg.mozilla.org/integration/mozilla-inbound/rev/482236eb497a
provide API for add-ons to delay restartless updates r=aswan

Comment 67

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

(In reply to Robert Helmer [:rhelmer] from comment #61)
> (In reply to Ritu Kothari (:ritu) from comment #59)
> > Hi Florin, this is something that seems will most likely ride the 50 train
> > to release. It's a mechanism to silently update system add-ons. Just an FYI.
> 
> We're unfortunately not quite ready to enable this for system add-ons quite
> yet - this enables it for all normal add-ons, and will be the basis for
> system add-ons and also web extension support for this feature.
> 
> I've filed a bug for this (bug 1278692) and cc'd you both, sorry for the
> confusion and thanks for you help!

I've CC'ed myself, Andrei Vaida (TL on the Release QA Desktop team), and Rares Bologa (PM on the QA Engineering team) on bug 1204156 (bug 1278692 was marked as the duplicate of this). This will allow us to track the work done on this feature.

@Robert - what's the testing need here for normal add-ons? I'll also add Krupa Raj into the mix since this maybe something of interest for her QA team.

Comment 68

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1878ebd93d8b
https://hg.mozilla.org/mozilla-central/rev/482236eb497a

Comment 69

[Robert Helmer [:rhelmer]]

(In reply to Florin Mezei, QA (:FlorinMezei) from comment #67)
> (In reply to Robert Helmer [:rhelmer] from comment #61)
> > (In reply to Ritu Kothari (:ritu) from comment #59)
> > > Hi Florin, this is something that seems will most likely ride the 50 train
> > > to release. It's a mechanism to silently update system add-ons. Just an FYI.
> > 
> > We're unfortunately not quite ready to enable this for system add-ons quite
> > yet - this enables it for all normal add-ons, and will be the basis for
> > system add-ons and also web extension support for this feature.
> > 
> > I've filed a bug for this (bug 1278692) and cc'd you both, sorry for the
> > confusion and thanks for you help!
> 
> I've CC'ed myself, Andrei Vaida (TL on the Release QA Desktop team), and
> Rares Bologa (PM on the QA Engineering team) on bug 1204156 (bug 1278692 was
> marked as the duplicate of this). This will allow us to track the work done
> on this feature.
> 
> @Robert - what's the testing need here for normal add-ons? I'll also add
> Krupa Raj into the mix since this maybe something of interest for her QA
> team.

Hi! For normal add-ons which do not use the new `AddonManager.addUpgradeListener()` API, they should remain unchanged - they should apply available updates immediately.

For manually testing this change, I found this little test add-on helpful:

https://gist.github.com/rhelmer/550ff10edb782c19160756676adc1737

This add-on will catch any available updates, and add a toolbar button that will allow the upgrade to proceed.

I suggest checking the following scenarios:

1) normal add-ons are updated as usual, no change (you could e.g. download an older version of any restartless add-on from AMO and then use https://addons.mozilla.org/en-US/firefox/addon/timer-fire/ to trigger an Add-on Update Check in "Tools->Fire Timer" and ensure that the upgrade works as usual)

2) add-ons which delay upgrade and never call .install() (e.g. using the test add-on I link to in the gist above and never clicking the button) should have upgrades applied upon restart of Firefox.

3) add-ons which delay upgrade and then *do* call .install() (e.g. using the test add-on in the gist and clicking the button when an update is pending) should have upgrade applied immediately. Restarting Firefox should not make any changes, the latest version should still be installed.

If it is helpful, I can package up my test add-on from the gist into a signed add-on that you can install, let me know.

Comment 70

[Robert Helmer [:rhelmer]]

(In reply to Robert Helmer [:rhelmer] from comment #69)
> 1) normal add-ons are updated as usual, no change (you could e.g. download
> an older version of any restartless add-on from AMO and then use
> https://addons.mozilla.org/en-US/firefox/addon/timer-fire/ to trigger an
> Add-on Update Check in "Tools->Fire Timer" and ensure that the upgrade works
> as usual)

Oops, minor correction here - you don't need the "Timer Fire" add-on at all to test this, there is a "Check for Updates" entry in the gear menu on about:addons that does the same thing.

Comment 71

[Robert Helmer [:rhelmer]]

(In reply to Robert Helmer [:rhelmer] from comment #69)
> If it is helpful, I can package up my test add-on from the gist into a
> signed add-on that you can install, let me know.

BTW I went ahead and did this - there is a signed version of the test add-on and the needed upgrade files hosted here:

http://people.mozilla.org/~rhelmer/Bug1231172/

You should be able to install that, then:

1) load about:addons
2) under gear icon, "Check for Updates"

The add-on should then be waiting for restart to install update.
If you click the "Delay Upgrade" toolbar icon, the update will be applied immediately.

If you see anything confusing/inconsistent in the UI during this process I'd really like to know, thanks!

Comment 72

[Vasilica Tamas, QA [:vtamas]]

Tested all scenarios suggested in Comment 69 on Firefox 50.0a1 (2016-07-26) under Windows 10 64-bit and noticed the following:

 1) a normal add-on is updated as usual

 2) add-ons which delay upgrade and never call .install() is successfully updated after a browser restart

 3) add-ons which delay upgrade and then *do* call .install() is upgraded immediately after clicking the button, but the “Restart now to complete installation” message is still displayed near Tools button http://screencast.com/t/FTAS1I6x8M . Clicking on “Restart now ...” link will restart the browser without making any changes because the add-on was already updated. 

Robert, any thoughts about “Restart now to complete installation” message?

Comment 73

[Robert Helmer [:rhelmer]]

(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #72)
> Tested all scenarios suggested in Comment 69 on Firefox 50.0a1 (2016-07-26)
> under Windows 10 64-bit and noticed the following:
> 
>  1) a normal add-on is updated as usual
> 
>  2) add-ons which delay upgrade and never call .install() is successfully
> updated after a browser restart
> 
>  3) add-ons which delay upgrade and then *do* call .install() is upgraded
> immediately after clicking the button, but the “Restart now to complete
> installation” message is still displayed near Tools button
> http://screencast.com/t/FTAS1I6x8M . Clicking on “Restart now ...” link will
> restart the browser without making any changes because the add-on was
> already updated. 
> 
> Robert, any thoughts about “Restart now to complete installation” message?

Thanks for testing!

I'm having trouble reproducing this, does the "restart" message go away if you reload the about:addons page?

Does this only happen on the "details" page in your screenshot?

Comment 74

[Vasilica Tamas, QA [:vtamas]]

(In reply to Robert Helmer [:rhelmer] from comment #73)
 
> I'm having trouble reproducing this, does the "restart" message go away if you reload 
> the about:addons page?

Yes. The “Restart now to complete installation” message disappears after about:addons page is reloaded.


> Does this only happen on the "details" page in your screenshot?

No. It also appears in Extensions Tab: http://screencast.com/t/SdY3waLqNm3


Another observation: 
 - after the new version of the test add-on is completely downloaded no message is displayed: http://screencast.com/t/tUL8jxvEDA. 
 - the “Restart now/Undo” links appears only after the page is reloaded http://screencast.com/t/c0I8HdiN1mo
 - If possible, it would be useful to guide the user to click on the button in order to complete the upgrade process.

Comment 75

[Vasilica Tamas, QA [:vtamas]]

I am still able to reproduce the above mentioned issue about the “Restart now to complete installation” message that is displayed even the add-on which delays upgrade and then *do* call .install() was already updated on Firefox 50.0b6 (20161010144024), Firefox 51.0a1 (2016-10-12) and Firefox 52.0a2 (2016-10-11) under Windows 10 64-bit.

See the following screencast: http://screencast.com/t/KUTKiD6yqzaO

What do you think Robert? Should I file a new bug for this issue?

Comment 76

[Robert Helmer [:rhelmer]]

(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #75)
> I am still able to reproduce the above mentioned issue about the “Restart
> now to complete installation” message that is displayed even the add-on
> which delays upgrade and then *do* call .install() was already updated on
> Firefox 50.0b6 (20161010144024), Firefox 51.0a1 (2016-10-12) and Firefox
> 52.0a2 (2016-10-11) under Windows 10 64-bit.
> 
> See the following screencast: http://screencast.com/t/KUTKiD6yqzaO
> 
> What do you think Robert? Should I file a new bug for this issue?

Yes I think this is a UI issue, can you please file a new bug?

Comment 77

[Vasilica Tamas, QA [:vtamas]]

(In reply to Robert Helmer [:rhelmer] from comment #76)

> Yes I think this is a UI issue, can you please file a new bug?

Filed Bug 1309850.

I’ve also noticed that “Delay update” button is no longer functional after clicking on “x” that correspond to “Downloaded” button: http://screencast.com/t/1O88KQSX. 
I assume that this issue will be fixed together with Bug 1309850 if the page would be autoreloaded automatically after the download is completed.
But we may encounter another scenario: to click on “x” button while the download is still in progress. 

What’s your opinion? Could this be a valid scenario that needs to be tracked in a separate bug?

Comment 78

[Robert Helmer [:rhelmer]]

(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #77)
> (In reply to Robert Helmer [:rhelmer] from comment #76)
> 
> > Yes I think this is a UI issue, can you please file a new bug?
> 
> Filed Bug 1309850.
> 
> I’ve also noticed that “Delay update” button is no longer functional after
> clicking on “x” that correspond to “Downloaded” button:
> http://screencast.com/t/1O88KQSX. 


This is normal - it only does something when there is a delayed upgrade queued up (there should be a console log in Browser Console when this is the case)

> I assume that this issue will be fixed together with Bug 1309850 if the page
> would be autoreloaded automatically after the download is completed.
> But we may encounter another scenario: to click on “x” button while the
> download is still in progress. 
> 
> What’s your opinion? Could this be a valid scenario that needs to be tracked
> in a separate bug?

Comment 79

[Vasilica Tamas, QA [:vtamas]]

Thanks Robert! 

Based on the above comments and considering that the other issue is tracked separately I am marking this bug as Verified.