Bug 1501329 - Set valid client ID on Fennec if canary is detected. r=chutten, a=RyanVM
authorJan-Erik Rediger <jrediger@mozilla.com>
Fri, 26 Oct 2018 18:17:48 +0000
changeset 498240 6cfcc707ee835deb65fa583775d4aeec7c9647a6
parent 498239 bf07e6c3ea338ef4e0c4ce6742ec4117f322b4e5
child 498241 455c4d904d2778da6d3cd86ac960d4f20ac99eb4
push id10079
push userryanvm@gmail.com
push dateWed, 31 Oct 2018 19:35:01 +0000
treeherdermozilla-beta@cb358118e54d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerschutten, RyanVM
bugs1501329
milestone64.0
Bug 1501329 - Set valid client ID on Fennec if canary is detected. r=chutten, a=RyanVM Differential Revision: https://phabricator.services.mozilla.com/D9544
toolkit/components/telemetry/app/TelemetryController.jsm
toolkit/components/telemetry/tests/unit/test_TelemetryClientID_reset.js
--- a/toolkit/components/telemetry/app/TelemetryController.jsm
+++ b/toolkit/components/telemetry/app/TelemetryController.jsm
@@ -657,23 +657,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) {
-          this._log.trace("Upload enabled, but got canary client ID. Resetting.");
+        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) {
+            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();
-        } 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);
         }
 
         await TelemetrySend.setup(this._testMode);
 
         // Perform TelemetrySession delayed init.
         await TelemetrySession.delayedInit();
         await MemoryTelemetry.delayedInit();
 
--- 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();
 });