Make TelemetryController.enableTelemetryRecording pref checks more robust

RESOLVED FIXED in Firefox 45

Status

()

defect
P1
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Dexter, Assigned: Dexter)

Tracking

(Blocks 1 bug)

Trunk
mozilla45
Points:
1
Dependency tree / graph

Firefox Tracking Flags

(firefox42 wontfix, firefox43 affected, firefox44 affected, firefox45 fixed)

Details

(Whiteboard: [measurement:client])

Attachments

(2 attachments, 4 obsolete attachments)

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
Blocks: 1201022
Points: --- → 1
Priority: -- → P2
Whiteboard: [measurement:client]
Assignee: nobody → alessio.placitelli
Posted patch bug1221958.patch (obsolete) — Splinter Review
May be worth adding test coverage for weird prefs values? (e.g. "false")
Attachment #8684997 - Flags: review?(gfritzsche)
Priority: P2 → P1
Status: NEW → ASSIGNED
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.
Attachment #8684997 - Flags: review?(gfritzsche) → review+
Posted patch bug1221958.patch (obsolete) — Splinter Review
I was missing this one.
Attachment #8684997 - Attachment is obsolete: true
Attachment #8688320 - Flags: review?(gfritzsche)
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.
Posted patch bug1221958_test.patch (obsolete) — Splinter Review
I was finally able to add simple test coverage to that (thanks Georg for pointing me to the right direction).
Attachment #8688401 - Flags: review?(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.
Attachment #8688401 - Flags: review?(gfritzsche) → review+
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
Attachment #8688320 - Flags: review?(gfritzsche)
Thanks!
Attachment #8688401 - Attachment is obsolete: true
Attachment #8688446 - Flags: review+
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.
Attachment #8688320 - Attachment is obsolete: true
Attachment #8688457 - Flags: review?(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.
Attachment #8688457 - Flags: review?(gfritzsche) → review+
Thanks Georg! I've consolidated the pref in head.js and made all the tests use it.
Attachment #8688457 - Attachment is obsolete: true
Attachment #8688475 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/488f10ee2512
https://hg.mozilla.org/mozilla-central/rev/76d07933d918
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.