Experiments are enabled even when telemetry is disabled

VERIFIED FIXED in Firefox 46

Status

()

Toolkit
Telemetry
P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: Felipe, Assigned: Dexter)

Tracking

Trunk
mozilla46
Points:
2

Firefox Tracking Flags

(firefox41 wontfix, firefox42 wontfix, firefox43 wontfix, firefox44 affected, firefox45 affected, firefox46 verified)

Details

(Whiteboard: [measurement:client])

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

2 years ago
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
(Reporter)

Comment 1

2 years ago
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
Do we know which versions are affected?
Priority: -- → P3
Whiteboard: [measurement:client]
(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

2 years ago
bumped to p1
Priority: P3 → P1
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!
status-firefox41: --- → affected
status-firefox42: --- → affected
status-firefox43: --- → affected
Flags: needinfo?(felipc)
(Reporter)

Comment 6

2 years ago
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.
Flags: needinfo?(felipc)
I see we would also have to re-sign it. Let's assume we're going ahead with this for tomorrow.
Points: --- → 2
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
(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.
Assignee: nobody → gfritzsche
Priority: P1 → P2
Assignee: gfritzsche → nobody
(Assignee)

Updated

2 years ago
Assignee: nobody → alessio.placitelli
(Assignee)

Updated

2 years ago
Priority: P2 → P1
(Assignee)

Comment 10

2 years ago
(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
(Assignee)

Comment 11

2 years ago
Created attachment 8694657 [details] [diff] [review]
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
Attachment #8694657 - Flags: review?(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?
Attachment #8694657 - Flags: review?(gfritzsche)
(Assignee)

Updated

2 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 13

2 years ago
Created attachment 8695962 [details] [diff] [review]
bug1217282.patch

Test coverage coming in a separate patch.
Attachment #8694657 - Attachment is obsolete: true
Attachment #8695962 - Flags: review?(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.
Attachment #8695962 - Flags: review?(gfritzsche) → review+
QA Contact: alexandra.lucinet
status-firefox41: affected → wontfix
status-firefox42: affected → wontfix
status-firefox43: affected → wontfix
status-firefox45: --- → affected
(Assignee)

Comment 15

2 years ago
Created attachment 8698080 [details] [diff] [review]
bug1217282_tests.patch
Attachment #8698080 - Flags: review?(gfritzsche)
(Assignee)

Comment 16

2 years ago
(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 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.
Attachment #8698080 - Flags: review?(gfritzsche)
(Assignee)

Comment 18

2 years ago
Created attachment 8698422 [details] [diff] [review]
bug1217282_tests.patch

Good catch. I've fixed the test, and I stumbled upon another issue (see bug 1232648).
Attachment #8698080 - Attachment is obsolete: true
Attachment #8698422 - Flags: review?(gfritzsche)
Attachment #8698422 - Flags: review?(gfritzsche) → review+
(Assignee)

Comment 19

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ae2940680cc9
(Assignee)

Comment 20

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d2a9b687071
(Assignee)

Comment 21

2 years ago
Created attachment 8699378 [details] [diff] [review]
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.
Attachment #8698422 - Attachment is obsolete: true
Attachment #8699378 - Flags: review+
Attachment #8699378 - Flags: feedback?(gfritzsche)
Attachment #8699378 - Flags: feedback?(gfritzsche) → feedback+
(Assignee)

Comment 22

2 years ago
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3da21787e06a
https://hg.mozilla.org/mozilla-central/rev/a9eaed26451a
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
(Assignee)

Comment 24

2 years ago
Alexandra, can you verify this when it hits Nightly?
Flags: qe-verify-
Flags: needinfo?(alexandra.lucinet)

Comment 25

2 years ago
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
Status: RESOLVED → VERIFIED
status-firefox46: fixed → verified
Flags: qe-verify-
Flags: needinfo?(alexandra.lucinet)
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","unified-urlbar@experiments.mozilla.org","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","unified-urlbar@experiments.mozilla.org","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
Flags: needinfo?(felipc)
(Assignee)

Comment 27

2 years ago
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
Thanks for checking Alessio!
Flags: needinfo?(felipc)
You need to log in before you can comment on or make changes to this bug.