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

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?

(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.

bumped to 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!

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.

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

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.

(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

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 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?

Test coverage coming in a separate patch.

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.

(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.

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

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.

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

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

Alexandra, can you verify this when it hits Nightly?

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

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

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!