985084 - Experiment add-ons should be disabled at startup

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Benjamin Smedberg]

Currently Experiments.jsm installs addons after checking conditions, but on startup we don't recheck conditions before starting the addon.

Also we need to make sure that experiment addons don't participate in the normal update cycle.

To do this Blair has suggested that we add a small amount of experiment-aware glue code to XPIProvider to delay starting the experiment addon at startup until we're sure that its conditions have been met. While we're there, we can also make sure that the addon manager understanding of the current experiment matches what the experiment system believes.

Comment 1

[Georg Fritzsche [:gfritzsche]]

In my understanding, the AddonManager can't depend on browser/.
Do we have a pattern for that "upward" coupling? try-catch on Cu.import(Experiments.jsm, localScope)?

Comment 2

[Gregory Szorc [:gps]]

I think Blair should propose a technical solution here.

Given my knowledge of the Addons Manager and XPIProvider, it might make sense to assign this to me (depending on what Blair says).

Comment 3

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Hmm, this is a good reason why we shouldn't have overloaded userDisabled=true to result in uninstall. Rather than shoehorning in a new API that would typically be required for this (and likely only ever be used for this), we should be using the tools we have available already - which is the ability for code to to enable/disable add-ons via setting Addon.userDisabled. That means changing how Experiments.jsm currently works, such that disabling no longer triggers an uninstall. And the permissions in the patch in bug 975000 will need adjusted.


So:

XPIProvider won't auto-start add-ons that are type=experiment. *IMPORTANT:* This should include add-ons that are installed while Firefox is running, to cover the case where an install doesn't originate from Experiments.jsm. And not auto-starting as opposed to setting userDisabled=true on shutdown covers the case of an unclean shutdown (crash). So add-ons with type=experiment default to userDisabled=true on startup and on install  - key part of this is setting userDisabled=true on install, and then just making sure we never persist userDisabled in the DB in updateAddonDisabledState(). Also need to ensure that experiments are never stored in the extensions.bootstrappedAddons pref.

Comment 4

[Georg Fritzsche [:gfritzsche]]

Per bug 973992 / bug 975000, the AddonManager UI will not set |userDisabled| for user clicks.

Comment 5

[Gregory Szorc [:gps]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #4)
> Per bug 973992 / bug 975000, the AddonManager UI will not set |userDisabled|
> for user clicks.

I didn't implement that :)

Anyway, this bug overlaps drastically with 975000. I'm tempted to merge them so Blair has the full context when he does review.

Blair: Would that help?

Comment 6

[Gregory Szorc [:gps]]

I'm going to take this bug. Will implement as a separate patch set (for now). It's easy to squash commits. Let me know, Blair.

Comment 7

[Gregory Szorc [:gps]]

An unfortunate side-effect of experiments disabled at startup is that it may limit the ability for experiments to change behavior related to or near startup. There will be a window between Add-on Manager initialization and Experiments Manager initialization. I'm not sure how long that will be in practice. A few seconds after profile startup?

I reckon we could close that window later if it became an issue. (Have a dedicated add-on property or a preference containing the active experiment or something.)

Comment 8

[Gregory Szorc [:gps]]

Attachment: Experiment add-ons should be disabled by default

Very simple patch so far. Still need to restructure Experiments.jsm to
handle events properly.

The fact the Telex unit tests pass with this patch applied seems
to imply we have a gap in test coverage around the enabled/disabled
flow. I'll get on that.

Comment 9

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Gregory Szorc [:gps] from comment #5)
> (In reply to Georg Fritzsche [:gfritzsche] from comment #4)
> > Per bug 973992 / bug 975000, the AddonManager UI will not set |userDisabled|
> > for user clicks.
> 
> I didn't implement that :)

I added an issue to the review for 975000 so you would :) The UI shows the Disable button (which sets userDisabled=true) if Addon.permissions includes PERM_CAN_DISABLE). So we just stop including that and start including PERM_CAN_UNINSTALL instead. And presumably we'll want a separate bug to tweak how the Uninstall button is presented in the UI.


> Anyway, this bug overlaps drastically with 975000. I'm tempted to merge them
> so Blair has the full context when he does review.

Not that much overlap, IMO - just the permissions changes. Doesn't matter much to me either way.

Comment 10

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8395106 [details] [diff] [review]
Experiment add-ons should be disabled by default

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

::: browser/experiments/Experiments.jsm
@@ +1300,2 @@
>            gLogger.trace("ExperimentEntry::start() - onInstallStarted for " + this.id);
>            // TODO: this check still needs changes in the addon manager

Should fix this while you're at it.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm
@@ +4241,5 @@
>      let appDisabledChanged = aAddon.appDisabled != appDisabled;
>  
> +    // Update the properties in the database.
> +    // We never persist this for experiments because the disabled flags
> +    // are controlled by the Experiments Manager.

Also need to ensure we don't persist these to the extensions.bootstrappedAddons pref - this is the quick-access cache we use on startup to see what bootstrap extensions to load.

::: toolkit/mozapps/extensions/test/xpcshell/test_experiment.js
@@ +55,5 @@
> +
> +    let listener = {
> +      onEnabled: (addon2) => {
> +        Assert.equal(addon2.id, addon.id, "Changed add-on matches expected.");
> +        Assert.equal(addon2.appDisabled, false, "Add-on is no longer disabled.");

Setting userDisabled never affects appDisabled. I think you're not quite understanding how these properties work for add-ons:

The state (enabled or disabled) of an add-on is governed by the isActive property. This is read-only, and is computed from 4 other properties of the add-on. The relevant two here are userDisabled and appDisabled. userDisabled can be set via outside code, while appDisabled can only be set by the add-on's provider. (The other two are softDisabled for blocklisting, and visible for when there are two copies of an add-on installed in different locations.)

So, you want to be checking isActive here.

Comment 11

[Georg Fritzsche [:gfritzsche]]

(In reply to Blair McBride [:Unfocused] from comment #10)
> ::: browser/experiments/Experiments.jsm
> @@ +1300,2 @@
> >            gLogger.trace("ExperimentEntry::start() - onInstallStarted for " + this.id);
> >            // TODO: this check still needs changes in the addon manager

Hm, just seeing this - i put up a patch over on bug 973992 (adding support for experiment addons in the experiments manager) that also covers this.

Comment 12

[Georg Fritzsche [:gfritzsche]]

Do you think the XPIProvider can sensibly guarantee that we only have one addon of type "experiment" active at a time?
If yes, would that be better done as a follow-up?
(If yes, it would obsolete bug 986040, which asserts this from the experiments manager.)

Comment 13

[Gregory Szorc [:gps]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #12)
> Do you think the XPIProvider can sensibly guarantee that we only have one
> addon of type "experiment" active at a time?
> If yes, would that be better done as a follow-up?
> (If yes, it would obsolete bug 986040, which asserts this from the
> experiments manager.)

The XPIProvider could potentially provide this guarantee. But given that experiments will be disabled on start and enabling/installing can only be performed via API (via Experiments.jsm), I think it's reasonable for Experiments.jsm to perform the management today. If XPIProvider truly needs to perform that management, we can move the code there. I'm assuming the effort either way is roughly equal. If XPIProvider is simpler, my vote would be to do that.

I think this is really a question for Blair.

Comment 14

[Gregory Szorc [:gps]]

Attachment: Experiment add-ons should be disabled by default

Experiment add-ons are now disabled by default on application load. It
is up to the Experiments Manager to enable them.

This means that experiments may not be able to reliably collect data or
modify behavior close to application startup. (There is a window between
when the Addon Manager initializes and when the Experiments Manager
initializes.) This window is acceptable for the initial version of the
experiments feature.

The Experiments Manager doesn't currently enable experiments on startup.
This will be addressed in a subsequent patch. Its tests do not regress
(indicating a lack of test coverage), so no harm no foul.

Comment 15

[Gregory Szorc [:gps]]

Attachment: Experiment add-ons should be disabled by default

Fixed a test failure and expanded test coverage slightly.

Comment 16

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8397473 [details] [diff] [review]
Experiment add-ons should be disabled by default

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

As mentioned on IRC, in persistBootstrappedAddons you need to filter out experiment add-ons so they don't get started on startup (see review comment 10).

Comment 17

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Gregory Szorc [:gps] from comment #13)
> (In reply to Georg Fritzsche [:gfritzsche] from comment #12)
> > Do you think the XPIProvider can sensibly guarantee that we only have one
> > addon of type "experiment" active at a time?
> > If yes, would that be better done as a follow-up?
> > (If yes, it would obsolete bug 986040, which asserts this from the
> > experiments manager.)
> 
> The XPIProvider could potentially provide this guarantee. But given that
> experiments will be disabled on start and enabling/installing can only be
> performed via API (via Experiments.jsm), I think it's reasonable for
> Experiments.jsm to perform the management today. If XPIProvider truly needs
> to perform that management, we can move the code there. I'm assuming the
> effort either way is roughly equal. If XPIProvider is simpler, my vote would
> be to do that.
> 
> I think this is really a question for Blair.

XPIProvider could potentially do this. In fact, it has a system already so it can do this for themes... the trouble is, the current way of doing it *for themes* is horribly complicated and I've been trying to rip it out for ages (it's blocking other work). That's why I hadn't suggested this.

But, re-evaluating that, I don't think it would be a drain to continue supporting just that base mechanism within XPIProvider - it's a relatively simple API at it's heart. Look for calls to notifyAddonChanged() - currently XPIProvider does this only for themes. And then just you'll need to disable other experiment add-ons in addonChanged(). Can we please do that in a separate bug though, this one is getting confusing :\

It will be more reliable doing it within XPIProvider. And it should be a relatively small patch too. It's just going to be fun for me down the line, but I'll deal with that later :) At the very least, I can imagine other add-on types potentially having some use for this functionality.

Comment 18

[Gregory Szorc [:gps]]

Attachment: Experiment add-ons should be disabled by default

Now with filtered bootstrappedAddons pref.

Comment 19

[Gregory Szorc [:gps]]

Blocked on bug 975000 for landing.

I'm not sure if I'll open a new bug for the follow-up work or do it here. Likely depends on when this lands (I don't like keeping partially landed bugs open if it can be avoided). Either way, working on the follow-up will be my primary task for Thursday.

Comment 20

[Gregory Szorc [:gps]]

https://hg.mozilla.org/integration/fx-team/rev/4834a3833639

Comment 21

[Gregory Szorc [:gps]]

Backed out because bug 975000 didn't work after all.

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

Comment 22

[Gregory Szorc [:gps]]

https://hg.mozilla.org/integration/fx-team/rev/0633949f1000

Comment 23

[Gregory Szorc [:gps]]

As part of working on bug 989137, I discovered that this patch is buggy.

Specifically, if you set userDisabled -> false and then call getAddonByID(), userDisabled is reported as true in the callback. This causes all kind of state confusion.

I'll work on a more creative patch in bug 989137 to unbust this. I may put it up here depending on the timing.

Comment 24

[Gregory Szorc [:gps]]

I'll do the followup here.

Comment 25

[Gregory Szorc [:gps]]

Attachment: Part 2: Properly report userDisabled in the API

The change in 0633949f1000 was buggy. If a new Addon instance were
created, it would always return the value stored in the database. This
meant that if you set userDisabled = false and called getAddonByID(),
userDisabled would likely be reported as true.

Comment 26

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/0633949f1000

Comment 27

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8399159 [details] [diff] [review]
Part 2: Properly report userDisabled in the API

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

*facepalm* Part of my main is still in the old Sqlite DB days, before we refactored how some of this worked.

Comment 28

[Gregory Szorc [:gps]]

I'm still running into issues with bug 989137. I'm going to hold off landing this until I'm more confident we're done with XPIProvider changes.

Comment 29

[Gregory Szorc [:gps]]

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

I got the issues in the other bug sorted out, so landing. There may still be a slight issue with these patches. But I think things can be deferred to a follow-up.

Comment 30

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

https://hg.mozilla.org/mozilla-central/rev/aa3898f1f4df

Comment 31

[Marco Mucci [:MarcoM]]

Hi Kamil, will verification of this bug be able to be completed before the end of our iteration on Monday April 14?

Comment 32

[Kamil Jozwiak [:kjozwiak]]

I went through the entire ticket and tested the following so far using the latest Nightly build:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-04-10-03-02-00-mozilla-central/

- Ensured that currently installed experiment isn't appearing under the "extensions.bootstrappedAddons" preference in about:config
- Downloaded three different add-ons and ensured that they're correctly being listed under the "extensions.bootstrappedAddons" preference
- Using the staging servers listed in Bug #973998 comment #18, I installed an experiment and ensured that it wasn't being listed with the add-on information in "extensions.bootstrappedAddons"
- Restarted Night several times and made sure that the installed experiment wasn't being added into the "extensions.bootstrappedAddons" preference
- Disabled the experiment and ensured that the add-on information wasn't being removed from "extensions.bootstrappedAddons" preference
- Uninstalled the experiment and removed experiments.json, ensured that the add-on information wasn't being removed from the "extensions.bootstrappedAddons" preference

I've renamed the experiments.xpi into a .zip file and extracted the contents, trying to figure out where to add something a long the lines of "Cu.reportError("addon started")" in the bootstrap.js code so I can make sure that the experiment is disabled during start up. Hopefully I can make some progress tomorrow and finish this up for Monday. I'll try hacking on this on the weekend also if I don't figure out tomorrow :)

Comment 33

[Kamil Jozwiak [:kjozwiak]]

Went through verification using the following Nightly build:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-04-12-03-02-02-mozilla-central/ (20140412030202)
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-04-14-03-02-03-mozilla-central/ (20140414030203)

I renamed "experiment.xpi" to "experiment.zip" and extracted the all the files. At that point, I added the following line into bootstrap.js right under the startup function:
- Cu.reportError("TEST TEST TEST addon started TEST TEST TEST");

Once I rezipped all the files and renamed the file to "experiment.xpi", I added the following option inside the "firefox-manifest.json":
- "buildIDs": ["20140412030202"] (dictates which build's are valid)

- Launched the experiment while Build # 20140412030202 was installed and confirmed that the above message appeared in the Browser Console
- Ensured that "extensions.bootstrappedAddons" didn't have the experiment listed
- Ensured that the currently installed addon-ons are appearing under "extensions.bootstrappedAddons"
- Ensured that the "Experiments" category appeared under about:addons
- Ensured that the experiment appeared enabled under about:support
- While the experiment was enabled and installed, updated to Build # 20140414030203 (which is NOT listed as a valid ID in the "buildIDs" option under firefox-manifest.json
- Restarted the browser after the update was completed and ensured that the "TEST" message didn't appear in the Browser Console
- Ensured that the "Experiments" category wasn't appearing under about:addons after the restart
- Ensured that the current experiment appeared as disabled under about:support after the restart
- Ensured that all the add-ons still appeared under "extensions.bootstrappedAddons" once the experiment wasn't launched after the reboot

Added everything I learned from the following ticket into the wiki (still in-progress):
- https://wiki.mozilla.org/QA/Desktop_Firefox/Telemetry_Experiments