1221958 - Make TelemetryController.enableTelemetryRecording pref checks more robust

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P1)

Description

[Alessio Placitelli [:Dexter]]

We should make the preference checks in [0] more robust against edge cases such as "toolkit.telemetry.enabled" being set to a string value "false" rather than the boolean |false|.

[0] - https://dxr.mozilla.org/mozilla-central/rev/6077f51254c69a1e14e1b61acba4af451bf1783e/toolkit/components/telemetry/TelemetryController.jsm#674

Comment 1

[Alessio Placitelli [:Dexter]]

Attachment: bug1221958.patch

May be worth adding test coverage for weird prefs values? (e.g. "false")

Comment 2

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8684997 [details] [diff] [review]
bug1221958.patch

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

Please make sure to check you change all the places, some use PREF_ENABLED, PREF_TELEMETRY_ENABLED, etc.
Also, please add simple test-coverage.

Comment 3

[Alessio Placitelli [:Dexter]]

Attachment: bug1221958.patch

I was missing this one.

Comment 4

[Alessio Placitelli [:Dexter]]

As discussed, I'm not adding test coverage for this one at the moment, as I found no simple way to set toolkit.telemetry.enabled to a string in the test code without causing a failure.

Comment 5

[Alessio Placitelli [:Dexter]]

Attachment: bug1221958_test.patch

I was finally able to add simple test coverage to that (thanks Georg for pointing me to the right direction).

Comment 6

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8688401 [details] [diff] [review]
bug1221958_test.patch

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

::: toolkit/components/telemetry/tests/unit/test_TelemetryController.js
@@ +447,5 @@
> +  let defaultPrefBranch = Services.prefs.getDefaultBranch(null);
> +  defaultPrefBranch.deleteBranch(PREF_ENABLED);
> +
> +  // Set the preferences controlling the Telemetry status to a string.
> +  Preferences.reset(PREF_ENABLED, "true");

Lets use "false"? I think that was the surprising string value we saw in the wild.

Comment 7

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8688320 [details] [diff] [review]
bug1221958.patch

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

::: toolkit/components/telemetry/TelemetryController.jsm
@@ +670,5 @@
>      // Configure base Telemetry recording.
>      // Unified Telemetry makes it opt-out unless the unifedOptin pref is set.
>      // Additionally, we make Telemetry opt-out for a 5% sample.
>      // If extended Telemetry is enabled, base recording is always on as well.
> +    const enabled = Preferences.get(PREF_ENABLED, false) === true;

Lets consolidate those pref checks as, say, TelemetryUtils.preferences.telemetryEnabled

Comment 8

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Add test coverage

Thanks!

Comment 9

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Robustly check if telemetry is enabled

Thanks Georg. I've consolidated that in TelemetryUtils.jsm. I've also removed the PREF constant from TelemetryController.jsm: nothing else was using it, except two tests.

Comment 10

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8688457 [details] [diff] [review]
part 1 - Robustly check if telemetry is enabled

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

::: toolkit/components/telemetry/TelemetryUtils.jsm
@@ +34,5 @@
>    /**
> +   * Returns the state of the Telemetry enabled preference, making sure
> +   * it correctly evaluates to a boolean type.
> +   */
> +  get telemetryEnabled() {

Reading here this should be: isTelemetryEnabled

::: toolkit/components/telemetry/tests/unit/test_TelemetryControllerBuildID.js
@@ +25,5 @@
>            .getService(Ci.nsISupports)
>            .wrappedJSObject);
>  
>  // Force the Telemetry enabled preference so that TelemetrySession.reset() doesn't exit early.
> +const PREF_TELEMETRY_ENABLED = "toolkit.telemetry.enabled";

This can be consolidated in head.js, we need in nearly all Telemetry xpcshell tests anyway.

Comment 11

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Robustly check if telemetry is enabled

Thanks Georg! I've consolidated the pref in head.js and made all the tests use it.

Comment 12

[Alessio Placitelli [:Dexter]]

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

Comment 13

[Alessio Placitelli [:Dexter]]

https://hg.mozilla.org/integration/fx-team/rev/488f10ee25129e1b3442c6598e2f4e929175d5ab
Bug 1221958 - Make TelemetryController.enableTelemetryRecording pref checks more robust. r=gfritzsche

https://hg.mozilla.org/integration/fx-team/rev/76d07933d91811548a2930320bf591adbeef07fa
Bug 1221958 - Add test coverage. r=gfritzsche

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/488f10ee2512
https://hg.mozilla.org/mozilla-central/rev/76d07933d918