1217282 - Experiments are enabled even when telemetry is disabled

Status: VERIFIED FIXED

(Toolkit :: Telemetry, defect, P1)

Description

[:Felipe Gomes (needinfo for replies!)]

Don't know how we never saw this before, as I was sure I even had problems getting things working properly on my local builds where telemetry was disabled. It could be that the process was blocked somewhere else along the way and with the fhr/telemetry changes this has changed.

See Kamil's report in bug 1193089 comment 42 for more details.

The problem is that, when toolkit.telemetry.enabled is toggled, this pref observer runs:
> http://mxr.mozilla.org/mozilla-central/source/browser/experiments/Experiments.jsm?rev=d6793bb3e45b#583
> gExperimentsEnabled = enabled && telemetryEnabled();

However, after a restart, only this run during init()
> http://mxr.mozilla.org/mozilla-central/source/browser/experiments/Experiments.jsm?rev=d6793bb3e45b#392
> gExperimentsEnabled = gPrefs.get(PREF_ENABLED, false);

which doesn't check the telemetry pref

Comment 1

[:Felipe Gomes (needinfo for replies!)]

Digging more into this, the code in Experiments.jsm is definitely wrong as shown above, but the code in ExperimentsService.js does the right thing:

> XPCOMUtils.defineLazyGetter(
>   this, "gExperimentsEnabled", () => {
>     // We can enable experiments if either unified Telemetry or FHR is on, and the user
>     // has opted into Telemetry.
>     return gPrefs.get(PREF_EXPERIMENTS_ENABLED, false) &&
>            (gPrefs.get(PREF_HEALTHREPORT_ENABLED, false) || IS_UNIFIED_TELEMETRY) &&
>            gPrefs.get(PREF_TELEMETRY_ENABLED, false);
>   });

And if that is false, it won't call Experiments.instance(). So the bug didn't happen before because nothing else was calling Experiments.instance() and initializing it.

But with Kamil's STR, there's nothing forcing that to be called, so something was introduced more recently that does it. I suspect this was triggered by:
http://hg.mozilla.org/mozilla-central/rev/7fecb9b2f002#l1.202

Comment 2

[Georg Fritzsche [:gfritzsche]]

Do we know which versions are affected?

Comment 3

[Georg Fritzsche [:gfritzsche]]

(In reply to :Felipe Gomes from comment #1)
> But with Kamil's STR, there's nothing forcing that to be called, so
> something was introduced more recently that does it. I suspect this was
> triggered by:
> http://hg.mozilla.org/mozilla-central/rev/7fecb9b2f002#l1.202

That sounds reasonable and would make all Firefox >=39 affected.

Comment 4

[Thomas Huelbert]

bumped to p1

Comment 5

[Liz Henry (:lizzard) (relman/hg->git project)]

Is this something we can fix quickly before we deploy the e10s experiment in bug 1193089 and bug 1217613? 

It sounds likely that we won't be collecting data there, but I also don't like the idea that we download and install anything for users who specifically opted out.    I can check back later tonight and see what you all think.  I also just emailed Benjamin and Vladan. If we'd be delaying this just one day, for example, maybe it's better to hold off.  If next week, they may want us to go ahead anyway with the experiment. Thanks!

Comment 6

[:Felipe Gomes (needinfo for replies!)]

There's something we can do in the experiments manifest to avoid it being downloaded to a number of users (namely, we can avoid download for telemetry-off users, but not for fhr-off, so it's not perfect but it's something).
I suggested this (see bug 1193089 comment 44), but the concern was about having to re-test things since the experiment had already been QA'ed.

I'll let Benjamin do the call.

Comment 7

[Liz Henry (:lizzard) (relman/hg->git project)]

I see we would also have to re-sign it. Let's assume we're going ahead with this for tomorrow.

Comment 8

[Georg Fritzsche [:gfritzsche]]

I did some testing today. The short version:
* this affects the current release
* by forcing manifest updates i could get 41 release to install an experiment with Telemetry disabled
* regular (update-timer triggered) manifest updates are not happening if Telemetry is off [0]
* but users could have experiments installed from cached manifests

So if you had Telemetry and FHR off before, you would not load experiment manifests and not install new contents from it.

Details:

The manifest is loaded or updated from Experiments.updateManifest():
https://dxr.mozilla.org/mozilla-central/rev/daa7d98525e859d32a3b3e97101e129a897192a1/browser/experiments/Experiments.jsm#834
This can be triggered when:
(1) the update timer kicks in - this is protected by a proper check [0].
(2) "experiments.enabled" or "experiments.manifest.uri" are flipped - those are not expected to be flipped, they are internal prefs.
(3) "toolkit.telemetry.enabled" is flipped - this can be flipped e.g. through the user-facing preferences.
    However, due to a crude implementation, we won't ever trigger a manifest update there [1].

So, that part happens to work "ok".

However, the following scenario is bad:
* have Telemetry enabled and load a manifest
* the experiments in it don't apply yet (e.g. startTime check)
* we disable Telemetry
* restart the browser
* the experiments could activate now


0: https://dxr.mozilla.org/mozilla-central/rev/daa7d98525e859d32a3b3e97101e129a897192a1/browser/experiments/ExperimentsService.js#60
1: https://dxr.mozilla.org/mozilla-central/rev/daa7d98525e859d32a3b3e97101e129a897192a1/browser/experiments/Experiments.jsm#590

Comment 9

[Georg Fritzsche [:gfritzsche]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #8)
> However, the following scenario is bad:
> * have Telemetry enabled and load a manifest
> * the experiments in it don't apply yet (e.g. startTime check)
> * we disable Telemetry
> * restart the browser
> * the experiments could activate now

A filter function on telemetryEnabled in the manifest would address this, but there may be corner-cases here like enterprise setups disabling FHR ("datareporting.healthreport.service.enabled" to false) but for some reason "toolkit.telemetry.enabled" being true.

Comment 10

[Alessio Placitelli [:Dexter]]

(In reply to :Felipe Gomes from comment #1)
> But with Kamil's STR, there's nothing forcing that to be called, so
> something was introduced more recently that does it. I suspect this was
> triggered by:
> http://hg.mozilla.org/mozilla-central/rev/7fecb9b2f002#l1.202

As you suspected, this is triggered by [0], which is called when building the initial environment cache as soon as Firefox starts.

[0] - https://dxr.mozilla.org/mozilla-central/rev/470f4f8c2b2d6f82e56e161a4b05262c85f55b59/toolkit/components/telemetry/TelemetryEnvironment.jsm#647

Comment 11

[Alessio Placitelli [:Dexter]]

Attachment: bug1217282.patch

This patch fixes [0] to check the status of the Telemetry pref, making sure no experiments run if Telemetry is off.

We will make sure no experiment is initialized before the 60s Telemetry init delay in bug 1229710.

[0] http://mxr.mozilla.org/mozilla-central/source/browser/experiments/Experiments.jsm?rev=d6793bb3e45b#392

Comment 12

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8694657 [details] [diff] [review]
bug1217282.patch

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

Please add test-coverage for this.

::: browser/experiments/Experiments.jsm
@@ +160,5 @@
>    });
>  }
>  
>  function telemetryEnabled() {
> +  return gPrefsTelemetry.get(PREF_TELEMETRY_ENABLED, false) === true;

Just use TelemetryUtils.isTelemetryEnabled and drop this function?

Comment 13

[Alessio Placitelli [:Dexter]]

Attachment: bug1217282.patch

Test coverage coming in a separate patch.

Comment 14

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8695962 [details] [diff] [review]
bug1217282.patch

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

Please take this through some basic manual testing and add automation test-coverage.

Comment 15

[Alessio Placitelli [:Dexter]]

Attachment: bug1217282_tests.patch

Comment 16

[Alessio Placitelli [:Dexter]]

(In reply to Georg Fritzsche [:gfritzsche] [away Dec 19 - Jan 3] from comment #14)
> Please take this through some basic manual testing and add automation
> test-coverage.

This was manually tested using the procedure from 1193089 comment 42.

Comment 17

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8698080 [details] [diff] [review]
bug1217282_tests.patch

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

::: browser/experiments/test/xpcshell/test_telemetry_disabled.js
@@ +15,5 @@
> +  let experiments = Experiments.instance();
> +  Assert.ok(!experiments.enabled, "Experiments must be disabled if Telemetry is disabled.");
> +
> +  Services.prefs.setBoolPref(PREF_TELEMETRY_ENABLED, true);
> +  Assert.ok(!experiments.enabled, "Experiments can run if they are enabled and Telemetry is enabled.");

You are testing for |!experiments.enabled| here but the message says that it should be enabled.

Comment 18

[Alessio Placitelli [:Dexter]]

Attachment: bug1217282_tests.patch

Good catch. I've fixed the test, and I stumbled upon another issue (see bug 1232648).

Comment 19

[Alessio Placitelli [:Dexter]]

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

Comment 20

[Alessio Placitelli [:Dexter]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d2a9b687071

Comment 21

[Alessio Placitelli [:Dexter]]

Attachment: bug1217282_tests.patch

This fixes the fallout from debug builds (they have the Telemetry pref off by default) and a test which wasn't in the browser/experiments directory.

Comment 22

[Alessio Placitelli [:Dexter]]

https://hg.mozilla.org/integration/fx-team/rev/3da21787e06a339e5231d0484637e6d16c473843
Bug 1217282 - Experiments are enabled even when telemetry is disabled. r=gfritzsche

https://hg.mozilla.org/integration/fx-team/rev/a9eaed26451a0381ed8a599597aa1c8629a2f3d1
Bug 1217282 - Add test coverage. r=gfritzsche

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3da21787e06a
https://hg.mozilla.org/mozilla-central/rev/a9eaed26451a

Comment 24

[Alessio Placitelli [:Dexter]]

Alexandra, can you verify this when it hits Nightly?

Comment 25

[Ada [:adalucinet]]

Reproduced with latest 45.0a2 (from 2015-12-22) under Mac OS X 10.11.1 → with STR from bug 1193089 comment 42, Experiments are enabled although telemetry is disabled; this lines https://goo.gl/nd7bUu are displayed via Browser Console.
Verified fixed with latest Nightly 46.0a1 from (2015-12-22) e10s on/off, across platforms [1].

[1] Mac OS X 10.11.1, Ubuntu 12.04 32-bit and Windows 7 64-bit

Comment 26

[Kamil Jozwiak [:kjozwiak]]

From the looks of the logs that have been attached in comment # 25, it looks like the experiment wasn't being installed due to an incorrect channel being used rather then being rejected due to telemetry being turned off/disabled:

>> 1450863933297 Browser.Experiments.Experiments TRACE Experiments #0::evaluateExperiments() - added
>> EXPERIMENT_ACTIVATION to TelemetryLog: ["REJECTED","[e-mail]","channel"]

I'm not sure if the channel verification takes precedence over the telemetry enabled check, maybe Felipe will know? However if the channel check does take precedence, this wasn't tested correctly as you would have ran into the ["REJECTED","[e-mail]","channel"] error. The current experiment that's currently active under http://kamiljozwiak.io/exp.html is set for the BETA channel [1] and would have failed as testing was on under the nightly channel.

[1] http://kamiljozwiak.io/firefox-manifest.json

Comment 27

[Alessio Placitelli [:Dexter]]

gExperimentsEnabled [0] is checked before checking if the conditions for the particular experiment are correct [1] (e.g. being on the right channel).

[0] - https://dxr.mozilla.org/mozilla-central/rev/388bdc46ba51ee31da8b8abe977e0ca38d117434/browser/experiments/Experiments.jsm#1240
[1] - https://dxr.mozilla.org/mozilla-central/rev/388bdc46ba51ee31da8b8abe977e0ca38d117434/browser/experiments/Experiments.jsm#1245

Comment 28

[Kamil Jozwiak [:kjozwiak]]

Thanks for checking Alessio!