986040 - Telemetry experiments: Assure that no experiment addon is running yet before starting an experiment.

Status: RESOLVED FIXED

(Firefox Health Report Graveyard :: Client: Desktop, defect, P3)

Description

[Georg Fritzsche [:gfritzsche]]

In ExperimentsEntry.start() we should make sure that no experiment is running yet (e.g. by getting all addons of the "experiments" type from the addon manager).

Comment 1

[Georg Fritzsche [:gfritzsche]]

> From running tests on this it looks like when trying to install an addon via
> getInstallForURL(), it just succeeds when the addon is already installed.
> Can i differentiate this case (without knowing the addon id)?

Comment 2

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Georg Fritzsche [:gfritzsche] from comment #0)
> In ExperimentsEntry.start() we should make sure that no experiment is
> running yet (e.g. by getting all addons of the "experiments" type from the
> addon manager).

Then you want AddonsManager.getAddonsByTypes()

(In reply to Georg Fritzsche [:gfritzsche] from comment #1)
> > From running tests on this it looks like when trying to install an addon via
> > getInstallForURL(), it just succeeds when the addon is already installed.
> > Can i differentiate this case (without knowing the addon id)?

Once you get at least the onDownloadEnded event, you can access the install.addon property - which has not only the install.addon.id property, but also the install.existingAddon property. If existinAddon is non-null, then an add-on with the same ID is already installed.

Comment 3

[Georg Fritzsche [:gfritzsche]]

Attachment: Assure no experiment addons are installed yet.

Add checks for already installed experiment addons, refactor things a bit, fixup tests.

Part of this try push:
https://tbpl.mozilla.org/?tree=Try&rev=4ffee5ea7661

Comment 4

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8395387 [details] [diff] [review]
Assure no experiment addons are installed yet.

Review of attachment 8395387 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/experiments/Experiments.jsm
@@ +156,5 @@
>           gPrefsTelemetry.get(PREF_TELEMETRY_PRERELEASE, false);
>  }
>  
> +// Returns a promise that is resolved with the AddonInstall for that URL.
> +function addonInstallForURL(url, hash) {

Bug 987512 would make this so much easier... (don't wait for it though)

@@ +1303,5 @@
> +      let addons = yield installedExperimentAddons();
> +      if (addons.length > 0) {
> +        let message = "there are already " + addons.length + " experiment addons installed";
> +        gLogger.error("ExperimentEntry::start() - " + message);
> +        throw new Error(message);

So, if this does happen, currently it seems like the Experiments system can get into a state where it would never recover. That would be a Bad Thing.

@@ +1311,5 @@
> +    }.bind(this));
> +  },
> +
> +  // Async install of the addon for this experiment.
> +  _installAddon: function () {

Nit: I find it bemusing that you split this out to it's own function, instead of the part that has little to do with starting to run the experiment (ie, checking other experiments).

@@ +1333,5 @@
> +          gLogger.trace("ExperimentEntry::_installAddon() - onDownloadEnded for " + this.id);
> +
> +          if (install.existingAddon) {
> +            gLogger.error("ExperimentEntry::_installAddon() - onDownloadEnded, addon already installed");
> +            failureHandler({state: -1, error: -1}, "onDownloadEnded");

What exactly are you trying to protect against here? ie, why won't you want to allow a pave-over install? At the very least, we need some documentation here explaining this; but I'm struggling to coming up with a scenario where we need to avoid it.

@@ +1346,5 @@
>              failureHandler({state: -1, error: -1}, "onInstallStarted");
>              return;
>            }
>  
> +          if (install.existingAddon) {

Don't need this check twice - it's set right before onDownloadEnded, it'll never change after that.

Comment 5

[Georg Fritzsche [:gfritzsche]]

(In reply to Blair McBride [:Unfocused] from comment #4)
> @@ +1303,5 @@
> > +      let addons = yield installedExperimentAddons();
> > +      if (addons.length > 0) {
> > +        let message = "there are already " + addons.length + " experiment addons installed";
> > +        gLogger.error("ExperimentEntry::start() - " + message);
> > +        throw new Error(message);
> 
> So, if this does happen, currently it seems like the Experiments system can
> get into a state where it would never recover. That would be a Bad Thing.

I'm not sure if there is a good way to recover from this, although i suppose we could just blindly uninstall the offending addon(s).

> > +  // Async install of the addon for this experiment.
> > +  _installAddon: function () {
> 
> Nit: I find it bemusing that you split this out to it's own function,
> instead of the part that has little to do with starting to run the
> experiment (ie, checking other experiments).

I mainly wanted to separate out the addon installation part for having start()s logic clearer.

> @@ +1333,5 @@
> > +          gLogger.trace("ExperimentEntry::_installAddon() - onDownloadEnded for " + this.id);
> > +
> > +          if (install.existingAddon) {
> > +            gLogger.error("ExperimentEntry::_installAddon() - onDownloadEnded, addon already installed");
> > +            failureHandler({state: -1, error: -1}, "onDownloadEnded");
> 
> What exactly are you trying to protect against here? ie, why won't you want
> to allow a pave-over install? At the very least, we need some documentation
> here explaining this; but I'm struggling to coming up with a scenario where
> we need to avoid it.

The experiments system does upgrades of running experiments via stop/uninstall & subsequent start/install of the new xpi. I don't think we should try to support other mechanisms if we have a working one.

> @@ +1346,5 @@
> >              failureHandler({state: -1, error: -1}, "onInstallStarted");
> >              return;
> >            }
> >  
> > +          if (install.existingAddon) {
> 
> Don't need this check twice - it's set right before onDownloadEnded, it'll
> never change after that.

Is onDownloadEnded always called, e.g. for already cached xpis too?

Comment 6

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Georg Fritzsche [:gfritzsche] from comment #5)
> I'm not sure if there is a good way to recover from this, although i suppose
> we could just blindly uninstall the offending addon(s).

If Experiments.jsm doesn't think it's managing that add-on, then I think it should uninstall it, yes.

> > What exactly are you trying to protect against here? ie, why won't you want
> > to allow a pave-over install? At the very least, we need some documentation
> > here explaining this; but I'm struggling to coming up with a scenario where
> > we need to avoid it.
> 
> The experiments system does upgrades of running experiments via
> stop/uninstall & subsequent start/install of the new xpi. I don't think we
> should try to support other mechanisms if we have a working one.

Right. But it's a similar case to the above - if Experiments.jsm doesn't think it's managing that existing add-on, it should just replace it.

> Is onDownloadEnded always called, e.g. for already cached xpis too?

The only time it won't get called is for installs initiated explicitly as a local file install (cached files do not count, they act as though they're coming from the network).

Note, however, that you *cannot* cancel an install from within onInstallStarted. You can however return false, which will revert it back to the STATE_DOWNLOADED state - and it can be cancelled afterwards from then (we have a bug on file somewhere about this).

Comment 7

[Georg Fritzsche [:gfritzsche]]

Attachment: Assure no experiment addons are installed yet.

Take two then, this should cover your concerns.

Comment 8

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8396462 [details] [diff] [review]
Assure no experiment addons are installed yet.

Review of attachment 8396462 [details] [diff] [review]:
-----------------------------------------------------------------

So, re-thinking the event handling here a bit, because I'm not happy with how it is currently...

Also: We should have a bug on file to make it so that Experiments.jsm only allows experiment add-ons with IDs that conform to some standard (ie, ends with @telemetry-experiments.mozilla.org), to ensure we avoid issues around ID conflicts with normal add-ons.

::: browser/experiments/Experiments.jsm
@@ +205,5 @@
> +    }
> +  };
> +
> +  listener.onUninstalled = handler;
> +  listener.onDisabled = handler;

Don't need the onDisabled handler (using this event would actually mean that this gets resolved *before* all add-ons are uninstalled).

Can cleanup this code a little without that too.

@@ +1342,4 @@
>      let deferred = Promise.defer();
>  
> +    addonInstallForURL(this._manifestData.xpiURL, this._manifestData.xpiHash).then(install => {
> +       let failureHandler = (install, handler) => {

Holy 3-space indent, Batman!

@@ +1360,3 @@
>  
> +           if (install.existingAddon) {
> +             gLogger.error("ExperimentEntry::_installAddon() - onDownloadEnded, addon already installed");

Shouldn't be checking this. You're uninstalling all other experiments earlier in the process, so this should only happen due to a race condition - and in which case, we want to pave-over install anyway.

@@ +1369,5 @@
>  
> +           if (install.addon.type !== "experiment") {
> +             gLogger.error("ExperimentEntry::_installAddon() - wrong addon type");
> +             failureHandler({state: -1, error: -1}, "onInstallStarted");
> +             return;

You're not actually stopping the install here. As I mentioned earlier, these checks really need to be in onDownloadEnded to be effective. And reading through the failure handler again, it doesn't look like it is cancelling the install (I was sure it did!). So when any condition fails in there, it should be calling |install.cancel()|. And remove the failure handler from there -  instead it should be called from the onInstallFailed and onInstallCancelled events.

Thinking throught the cases again where checking conditions in onInstallStarted might be useful, it only covers the case where installs are local files. And as I mentioned, you can't *cancel* the install from here (only stall it - there's an alternate route if need be). But, since this is an install the experiments system has started itself, I really don't think it should be worrying about local file installs. So you should just move all condition checks into onDownloadEnded, and cancel the install from there if they fail.

@@ +1381,5 @@
> +           let addon = install.addon;
> +           this._name = addon.name;
> +           this._addonId = addon.id;
> +           this._description = addon.description || "";
> +           this._homepageURL = addon.homepageURL || "";

And these should probably be set in onInstallEnded - that way you know they're only set if the install succeeds.

I don't think you actually have a use for onInstallStarted.

Comment 9

[Georg Fritzsche [:gfritzsche]]

(In reply to Blair McBride [:Unfocused] from comment #8)
> Thinking throught the cases again where checking conditions in
> onInstallStarted might be useful, it only covers the case where installs are
> local files. And as I mentioned, you can't *cancel* the install from here
> (only stall it - there's an alternate route if need be). But, since this is
> an install the experiments system has started itself, I really don't think
> it should be worrying about local file installs.

I'd much prefer not opening up to potential issues here if it's avoidable. Per comment 6, returning false should throw it back to onDownloadEnded() and we can just cancel from there, right?

Comment 10

[Georg Fritzsche [:gfritzsche]]

Attachment: Assure no experiment addons are installed yet.

Address mentioned issues.

Comment 11

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8397014 [details] [diff] [review]
Assure no experiment addons are installed yet.

Review of attachment 8397014 [details] [diff] [review]:
-----------------------------------------------------------------

Ah, this patch feels much cleaner :)


(In reply to Georg Fritzsche [:gfritzsche] from comment #9)
> I'd much prefer not opening up to potential issues here if it's avoidable.
> Per comment 6, returning false should throw it back to onDownloadEnded() and
> we can just cancel from there, right?

No, it won't fire the onDownloadEnded event again.

Re-reading through how XPIProvider handles this case.... if you return false from onInstallStarted, it is *effectively* cancelling the install. Not exactly the same, which is what confused me. But the important part is that it removes it from being an "active install". And it'll then call onInstallCancelled - which is what we want anyway, and this patch of course handles all that. Apologies for the confusion.

Comment 12

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Now, the question is.... do we want to pursue the approach outlined in bug 985084 comment 17 (making 
XPIProvider be the one to ensure only one experiment add-on is active at any given time), which would make parts of this patch redundant?

Comment 13

[Georg Fritzsche [:gfritzsche]]

I opened bug 988795.

Do you think we have a pressing reason to choose that one over the approach here?
Given that we have a patch ready to land here i would prefer to go ahead with this and replace it with bug 988795 if or when we do that.
Does that sound reasonable?

Comment 14

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Georg Fritzsche [:gfritzsche] from comment #13)
> I opened bug 988795.
> 
> Do you think we have a pressing reason to choose that one over the approach
> here?
> Given that we have a patch ready to land here i would prefer to go ahead
> with this and replace it with bug 988795 if or when we do that.
> Does that sound reasonable?

No, I don't think there's any specific need right now. I was wondering if you had anything, since you brought it up :) So yes, my thinking was that we can safely land this, and look at bug 988795 another time.

Comment 15

[Gregory Szorc [:gps]]

https://hg.mozilla.org/integration/fx-team/rev/791222eee8ae

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/791222eee8ae