Bug 1501329 - Set valid client ID on Fennec if canary is detected. r=chutten, a=pascalc
authorJan-Erik Rediger <jrediger@mozilla.com>
Fri, 26 Oct 2018 18:17:48 +0000
changeset 492969 7247a70272a2
parent 492968 ea5bceb1d4ac
child 492970 565452061d53
push id1844
push userryanvm@gmail.com
push dateMon, 05 Nov 2018 15:21:08 +0000
treeherdermozilla-release@805f775f35e2 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerschutten, pascalc
bugs1501329
milestone63.0.2
Bug 1501329 - Set valid client ID on Fennec if canary is detected. r=chutten, a=pascalc Differential Revision: https://phabricator.services.mozilla.com/D9544
toolkit/components/telemetry/TelemetryController.jsm
toolkit/components/telemetry/tests/unit/test_TelemetryClientID_reset.js
--- a/toolkit/components/telemetry/TelemetryController.jsm
+++ b/toolkit/components/telemetry/TelemetryController.jsm
@@ -654,23 +654,31 @@ var Impl = {
         // TODO: This should probably happen after all the delayed init here.
         this._initialized = true;
         TelemetryEnvironment.delayedInit();
 
         // Load the ClientID.
         this._clientID = await ClientID.getClientID();
 
         // Fix-up a canary client ID if detected.
-        const uploadEnabled = Services.prefs.getBoolPref(TelemetryUtils.Preferences.FhrUploadEnabled, false);
-        if (uploadEnabled && this._clientID == Utils.knownClientID) {
+        if (IS_UNIFIED_TELEMETRY) {
+          // On desktop respect the upload preference.
+          const uploadEnabled = Services.prefs.getBoolPref(TelemetryUtils.Preferences.FhrUploadEnabled, false);
+          if (uploadEnabled && this._clientID == Utils.knownClientID) {
             this._log.trace("Upload enabled, but got canary client ID. Resetting.");
             this._clientID = await ClientID.resetClientID();
-        } else if (!uploadEnabled && this._clientID != Utils.knownClientID) {
+          } else if (!uploadEnabled && this._clientID != Utils.knownClientID) {
             this._log.trace("Upload disabled, but got a valid client ID. Setting canary client ID.");
             this._clientID = await ClientID.setClientID(TelemetryUtils.knownClientID);
+          }
+        } else if (this._clientID == Utils.knownClientID) {
+          // On Fennec (non-unified Telemetry) we might have set a canary client ID in the past by mistake.
+          // We now always reset to a valid random client ID if this is detected (Bug 1501329).
+          this._log.trace("Not unified, but got canary client ID. Resetting.");
+          this._clientID = await ClientID.resetClientID();
         }
 
         await TelemetrySend.setup(this._testMode);
 
         // Perform TelemetrySession delayed init.
         await TelemetrySession.delayedInit();
 
         if (Services.prefs.getBoolPref(TelemetryUtils.Preferences.NewProfilePingEnabled, false) &&
--- a/toolkit/components/telemetry/tests/unit/test_TelemetryClientID_reset.js
+++ b/toolkit/components/telemetry/tests/unit/test_TelemetryClientID_reset.js
@@ -147,11 +147,86 @@ add_task(async function test_clientid_ca
 
   // Start the instance
   await TelemetryController.testReset();
 
   let newClientId = await ClientID.getClientID();
   Assert.equal(TelemetryUtils.knownClientID, newClientId, "Client ID should be a canary when upload disabled");
 });
 
+/**
+ * On Android Telemetry is not unified.
+ * This tests that we reset the client ID if it was previously reset to a canary client ID by accident.
+ */
+add_task(async function test_clientid_canary_reset_canary_on_nonunified() {
+  const isUnified = Preferences.get(TelemetryUtils.Preferences.Unified, false);
+  if (isUnified) {
+    // Skipping the test if unified telemetry is on.
+    return;
+  }
+
+  await sendPing();
+  let ping = await PingServer.promiseNextPing();
+  Assert.equal(ping.type, TEST_PING_TYPE, "The ping must be a test ping");
+  Assert.ok("clientId" in ping);
+
+  let firstClientId = ping.clientId;
+  Assert.notEqual(TelemetryUtils.knownClientID, firstClientId, "Client ID should be valid and random");
+
+  // Force a canary client ID
+  let clientId = await ClientID.setClientID(TelemetryUtils.knownClientID);
+  Assert.equal(TelemetryUtils.knownClientID, clientId);
+
+  // Now shutdown the instance
+  await TelemetryController.testShutdown();
+  await TelemetryStorage.testClearPendingPings();
+
+  // Start the instance
+  await TelemetryController.testReset();
+
+  let newClientId = await ClientID.getClientID();
+  Assert.notEqual(TelemetryUtils.knownClientID, newClientId, "Client ID should be valid and random");
+  Assert.notEqual(firstClientId, newClientId, "Client ID should be valid and random");
+});
+
+/**
+ * On Android Telemetry is not unified.
+ * This tests that we don't touch the client ID if the pref is toggled for some reason.
+ */
+add_task(async function test_clientid_canary_nonunified_no_pref_trigger() {
+  const isUnified = Preferences.get(TelemetryUtils.Preferences.Unified, false);
+  if (isUnified) {
+    // Skipping the test if unified telemetry is on.
+    return;
+  }
+
+  await sendPing();
+  let ping = await PingServer.promiseNextPing();
+  Assert.equal(ping.type, TEST_PING_TYPE, "The ping must be a test ping");
+  Assert.ok("clientId" in ping);
+
+  let firstClientId = ping.clientId;
+  Assert.notEqual(TelemetryUtils.knownClientID, firstClientId, "Client ID should be valid and random");
+
+  // Flip the pref again
+  Preferences.set(TelemetryUtils.Preferences.FhrUploadEnabled, true);
+
+  // Restart the instance
+  await TelemetryController.testShutdown();
+  await TelemetryController.testReset();
+
+  let newClientId = await ClientID.getClientID();
+  Assert.equal(firstClientId, newClientId, "Client ID should be unmodified");
+
+  // Flip the pref again
+  Preferences.set(TelemetryUtils.Preferences.FhrUploadEnabled, false);
+
+  // Restart the instance
+  await TelemetryController.testShutdown();
+  await TelemetryController.testReset();
+
+  newClientId = await ClientID.getClientID();
+  Assert.equal(firstClientId, newClientId, "Client ID should be unmodified");
+});
+
 add_task(async function stopServer() {
   await PingServer.stop();
 });