Bug 1913765: profileGroupId should default to the clientId for existing clients and should reset at the same time. r=chutten

This also changes `removeClientId` to `resetIdentifiers` and actually
generates the new identifiers. Everywhere that currently calls
`removeClientId` already immediately re-creates the client ID anyway.

Differential Revision: https://phabricator.services.mozilla.com/D219594

browser/modules/test/unit/test_discovery.js

@@ -131,7 +131,7 @@ add_task(async function test_discovery() {
     equal(cookie.host, uri.host, "cookie exists for host");
     return true;
   });
-  await ClientID.removeClientID();
+  await ClientID.resetIdentifiers();
   await ClientID.getClientID();
   await changed;
 

toolkit/components/telemetry/app/ClientID.sys.mjs

@@ -9,6 +9,9 @@ const LOGGER_NAME = "Toolkit.Telemetry";
 const LOGGER_PREFIX = "ClientID::";
 // Must match ID in TelemetryUtils
 const CANARY_CLIENT_ID = "c0ffeec0-ffee-c0ff-eec0-ffeec0ffeec0";
+const CANARY_PROFILE_GROUP_ID = "decafdec-afde-cafd-ecaf-decafdecafde";
+
+const DRS_STATE_VERSION = 2;
 
 const lazy = {};
 
@@ -109,29 +112,26 @@ export var ClientID = Object.freeze({
   },
 
   /**
-   * Sets the client ID to the canary (known) client ID,
+   * Sets the client ID and profile group ID to the canary (known) identifiers,
    * writing it to disk and updating the cached version.
    *
-   * Use `removeClientID` followed by `getClientID` to clear the
+   * Use `resetIdentifiers` to clear the existing identifiers and generate new,
-   * existing ID and generate a new, random one if required.
+   * random ones if required.
    *
    * @return {Promise<void>}
    */
-  setCanaryClientID() {
+  setCanaryIdentifiers() {
-    return ClientIDImpl.setCanaryClientID();
+    return ClientIDImpl.setCanaryIdentifiers();
   },
 
   /**
-   * Clears the client ID asynchronously, removing it
+   * Assigns new random values to client ID and profile group ID. Should only be
-   * from disk. Use `getClientID()` to generate
+   * used if a reset is explicitly requested by the user.
-   * a fresh ID after calling this method.
-   *
-   * Should only be used if a reset is explicitly requested by the user.
    *
    * @return {Promise<void>}
    */
-  removeClientID() {
+  resetIdentifiers() {
-    return ClientIDImpl.removeClientID();
+    return ClientIDImpl.resetIdentifiers();
   },
 
   /**
@@ -150,7 +150,7 @@ var ClientIDImpl = {
   _profileGroupID: null,
   _loadClientIdTask: null,
   _saveDataReportingStateTask: null,
-  _removeClientIdTask: null,
+  _resetIdentifiersTask: null,
   _logger: null,
 
   _loadDataReportingState() {
@@ -171,7 +171,7 @@ var ClientIDImpl = {
   async _doLoadDataReportingState() {
     this._log.trace(`_doLoadDataReportingState`);
     // If there's a removal in progress, let's wait for it
-    await this._removeClientIdTask;
+    await this._resetIdentifiersTask;
 
     // Try to load the client id from the DRS state file.
     let hasCurrentClientID = false;
@@ -179,10 +179,26 @@ var ClientIDImpl = {
     try {
       let state = await IOUtils.readJSON(lazy.gStateFilePath);
       if (state) {
+        if (!("version" in state)) {
+          // Old version, clear out any previously generated profile group ID.
+          delete state.profileGroupID;
+        }
+
         hasCurrentClientID = this.updateClientID(state.clientID);
         hasCurrentProfileGroupID = this.updateProfileGroupID(
           state.profileGroupID
         );
+
+        if (!hasCurrentProfileGroupID && hasCurrentClientID) {
+          // A pre-existing profile should be assigned the existing client ID.
+          hasCurrentProfileGroupID = this.updateProfileGroupID(this._clientID);
+
+          if (hasCurrentProfileGroupID) {
+            this._saveDataReportingStateTask = this._saveDataReportingState();
+            await this._saveDataReportingStateTask;
+          }
+        }
+
         if (hasCurrentClientID && hasCurrentProfileGroupID) {
           this._log.trace(
             `_doLoadDataReportingState: Client and Group IDs loaded from state.`
@@ -250,6 +266,7 @@ var ClientIDImpl = {
     try {
       this._log.trace(`_saveDataReportingState`);
       let obj = {
+        version: DRS_STATE_VERSION,
         clientID: this._clientID,
         profileGroupID: this._profileGroupID,
       };
@@ -438,47 +455,44 @@ var ClientIDImpl = {
     this._profileGroupID = null;
   },
 
-  async setCanaryClientID() {
+  async setCanaryIdentifiers() {
-    this._log.trace("setCanaryClientID");
+    this._log.trace("setCanaryIdentifiers");
     this.updateClientID(CANARY_CLIENT_ID);
+    this.updateProfileGroupID(CANARY_PROFILE_GROUP_ID);
 
     this._saveDataReportingStateTask = this._saveDataReportingState();
     await this._saveDataReportingStateTask;
     return this._clientID;
   },
 
-  async _doRemoveClientID() {
+  async _doResetIdentifiers() {
-    this._log.trace("_doRemoveClientID");
+    this._log.trace("_doResetIdentifiers");
 
     // Reset the cached client ID.
-    this._clientID = null;
+    this.updateClientID(lazy.CommonUtils.generateUUID());
     this._clientIDHash = null;
 
-    // Clear the client id from the preference cache.
+    // Reset the cached profile group ID.
-    Services.prefs.clearUserPref(PREF_CACHED_CLIENTID);
+    this.updateProfileGroupID(lazy.CommonUtils.generateUUID());
 
     // If there is a save in progress, wait for it to complete.
     await this._saveDataReportingStateTask;
 
-    // Remove the client-id-containing state file from disk
+    // Save the new identifiers to disk.
-    await IOUtils.remove(lazy.gStateFilePath);
+    this._saveDataReportingStateTask = this._saveDataReportingState();
+    await this._saveDataReportingStateTask;
   },
 
-  async removeClientID() {
+  async resetIdentifiers() {
-    this._log.trace("removeClientID");
+    this._log.trace("resetIdentifiers");
-
-    if (AppConstants.platform != "android") {
-      // We can't clear the client_id in Glean, but we can make it the canary.
-      Glean.legacyTelemetry.clientId.set(CANARY_CLIENT_ID);
-    }
 
     // Wait for the removal.
     // Asynchronous calls to getClientID will also be blocked on this.
-    this._removeClientIdTask = this._doRemoveClientID();
+    this._resetIdentifiersTask = this._doResetIdentifiers();
-    let clear = () => (this._removeClientIdTask = null);
+    let clear = () => (this._resetIdentifiersTask = null);
-    this._removeClientIdTask.then(clear, clear);
+    this._resetIdentifiersTask.then(clear, clear);
 
-    await this._removeClientIdTask;
+    await this._resetIdentifiersTask;
   },
 
   /**

toolkit/components/telemetry/app/TelemetryControllerParent.sys.mjs

@@ -157,6 +157,8 @@ export var TelemetryController = Object.freeze({
    * @param {Boolean} [aOptions.usePingSender=false] if true, send the ping using the PingSender.
    * @param {String} [aOptions.overrideClientId=undefined] if set, override the
    *                 client id to the provided value. Implies aOptions.addClientId=true.
+   * @param {String} [aOptions.overrideProfileGroupId=undefined] if set, override the
+   *                 profile group id to the provided value. Implies aOptions.addClientId=true.
    * @returns {Promise} Test-only - a promise that resolves with the ping id once the ping is stored or sent.
    */
   submitExternalPing(aType, aPayload, aOptions = {}) {
@@ -192,6 +194,8 @@ export var TelemetryController = Object.freeze({
    * @param {Object}  [aOptions.overrideEnvironment=null] set to override the environment data.
    * @param {String} [aOptions.overrideClientId=undefined] if set, override the
    *                 client id to the provided value. Implies aOptions.addClientId=true.
+   * @param {String} [aOptions.overrideProfileGroupId=undefined] if set, override the
+   *                 profile group id to the provided value. Implies aOptions.addClientId=true.
    *
    * @returns {Promise} A promise that resolves with the ping id when the ping is saved to
    *                    disk.
@@ -374,6 +378,8 @@ var Impl = {
    * @param {Object}  [aOptions.overrideEnvironment=null] set to override the environment data.
    * @param {String} [aOptions.overrideClientId=undefined] if set, override the
    *                 client id to the provided value. Implies aOptions.addClientId=true.
+   * @param {String} [aOptions.overrideProfileGroupId=undefined] if set, override the
+   *                 profile group id to the provided value. Implies aOptions.addClientId=true.
    * @param {Boolean} [aOptions.useEncryption=false] if true, encrypt data client-side before sending.
    * @param {Object}  [aOptions.publicKey=null] the public key to use if encryption is enabled (JSON Web Key).
    * @param {String}  [aOptions.encryptionKeyId=null] the public key ID to use if encryption is enabled.
@@ -406,9 +412,14 @@ var Impl = {
       payload,
     };
 
-    if (aOptions.addClientId || aOptions.overrideClientId) {
+    if (
-      pingData.clientId = aOptions.overrideClientId || this._clientID;
+      aOptions.addClientId ||
-      pingData.profileGroupId = this._profileGroupID;
+      aOptions.overrideClientId ||
+      aOptions.overrideProfileGroupId
+    ) {
+      pingData.clientId = aOptions.overrideClientId ?? this._clientID;
+      pingData.profileGroupId =
+        aOptions.overrideProfileGroupId ?? this._profileGroupID;
     }
 
     if (aOptions.addEnvironment) {
@@ -460,16 +471,23 @@ var Impl = {
    *                  pioneer id to the provided value. Only works if aOptions.addPioneerId=true.
    * @param {String} [aOptions.overrideClientId=undefined] if set, override the
    *                 client id to the provided value. Implies aOptions.addClientId=true.
+   * @param {String} [aOptions.overrideProfileGroupId=undefined] if set, override the
+   *                 profile group id to the provided value. Implies aOptions.addClientId=true.
    * @returns {Promise} Test-only - a promise that is resolved with the ping id once the ping is stored or sent.
    */
   async _submitPingLogic(aType, aPayload, aOptions) {
     // Make sure to have a clientId if we need one. This cover the case of submitting
     // a ping early during startup, before Telemetry is initialized, if no client id was
     // cached.
-    if (
+    let needsIdentifiers =
-      aOptions.addClientId &&
+      aOptions.addClientId ||
-      (!this._profileGroupID || (!this._clientID && !aOptions.overrideClientId))
+      aOptions.overrideClientId ||
-    ) {
+      aOptions.overrideProfileGroupId;
+    let hasClientId = aOptions.overrideClientId ?? this._clientID;
+    let hasProfileGroupId =
+      aOptions.overrideProfileGroupId ?? this._profileGroupID;
+
+    if (needsIdentifiers && !(hasClientId && hasProfileGroupId)) {
       this._log.trace(
         "_submitPingLogic - Waiting on client id or profile group id"
       );
@@ -582,6 +600,8 @@ var Impl = {
    *                  pioneer id to the provided value. Only works if aOptions.addPioneerId=true.
    * @param {String} [aOptions.overrideClientId=undefined] if set, override the
    *                 client id to the provided value. Implies aOptions.addClientId=true.
+   * @param {String} [aOptions.overrideProfileGroupId=undefined] if set, override the
+   *                 profile group id to the provided value. Implies aOptions.addClientId=true.
    * @returns {Promise} Test-only - a promise that is resolved with the ping id once the ping is stored or sent.
    */
   submitExternalPing: function send(aType, aPayload, aOptions) {
@@ -646,6 +666,8 @@ var Impl = {
    * @param {Object}  [aOptions.overrideEnvironment=null] set to override the environment data.
    * @param {String} [aOptions.overrideClientId=undefined] if set, override the
    *                 client id to the provided value. Implies aOptions.addClientId=true.
+   * @param {String} [aOptions.overrideProfileGroupId=undefined] if set, override the
+   *                 profile group id to the provided value. Implies aOptions.addClientId=true.
    *
    * @returns {Promise} A promise that resolves with the ping id when the ping is saved to
    *                    disk.
@@ -834,18 +856,28 @@ var Impl = {
             TelemetryUtils.Preferences.FhrUploadEnabled,
             false
           );
-          if (uploadEnabled && this._clientID == Utils.knownClientID) {
+          if (
+            uploadEnabled &&
+            (this._clientID == Utils.knownClientID ||
+              this._profileGroupID == Utils.knownProfileGroupID)
+          ) {
             this._log.trace(
-              "Upload enabled, but got canary client ID. Resetting."
+              "Upload enabled, but got canary identifiers. Resetting."
             );
-            await lazy.ClientID.removeClientID();
+            await lazy.ClientID.resetIdentifiers();
             this._clientID = await lazy.ClientID.getClientID();
-          } else if (!uploadEnabled && this._clientID != Utils.knownClientID) {
+            this._profileGroupID = await lazy.ClientID.getProfileGroupID();
+          } else if (
+            !uploadEnabled &&
+            (this._clientID != Utils.knownClientID ||
+              this._profileGroupID != Utils.knownProfileGroupID)
+          ) {
             this._log.trace(
               "Upload disabled, but got a valid client ID. Setting canary client ID."
             );
-            await lazy.ClientID.setCanaryClientID();
+            await lazy.ClientID.setCanaryIdentifiers();
             this._clientID = await lazy.ClientID.getClientID();
+            this._profileGroupID = await lazy.ClientID.getProfileGroupID();
           }
 
           await lazy.TelemetrySend.setup(this._testMode);
@@ -1073,17 +1105,18 @@ var Impl = {
     );
     if (uploadEnabled) {
       this._log.trace(
-        "_onUploadPrefChange - upload was enabled again. Resetting client ID"
+        "_onUploadPrefChange - upload was enabled again. Resetting identifiers"
       );
 
-      // Delete cached client ID immediately, so other usage is forced to refetch it.
+      // Delete cached identifiers immediately, so other usage is forced to refetch it.
       this._clientID = null;
+      this._profileGroupID = null;
 
       // Generate a new client ID and make sure this module uses the new version
       let p = (async () => {
-        await lazy.ClientID.removeClientID();
+        await lazy.ClientID.resetIdentifiers();
-        let id = await lazy.ClientID.getClientID();
+        this._clientID = await lazy.ClientID.getClientID();
-        this._clientID = id;
+        this._profileGroupID = await lazy.ClientID.getProfileGroupID();
         Services.telemetry.scalarSet("telemetry.data_upload_optin", true);
 
         await this.saveUninstallPing().catch(e =>
@@ -1125,17 +1158,22 @@ var Impl = {
           /* clear */ true
         );
 
-        // 6. Set ClientID to a known value
+        // 6. Set identifiers to a known values
         let oldClientId = await lazy.ClientID.getClientID();
-        await lazy.ClientID.setCanaryClientID();
+        let oldProfileGroupId = await lazy.ClientID.getProfileGroupID();
+        await lazy.ClientID.setCanaryIdentifiers();
         this._clientID = await lazy.ClientID.getClientID();
+        this._profileGroupID = await lazy.ClientID.getProfileGroupID();
 
         // 7. Send the deletion-request ping.
         this._log.trace("_onUploadPrefChange - Sending deletion-request ping.");
         this.submitExternalPing(
           PING_TYPE_DELETION_REQUEST,
           { scalars },
-          { overrideClientId: oldClientId }
+          {
+            overrideClientId: oldClientId,
+            overrideProfileGroupId: oldProfileGroupId,
+          }
         );
         this._deletionRequestPingSubmittedPromise = null;
       }

toolkit/components/telemetry/app/TelemetryUtils.sys.mjs

@@ -96,6 +96,13 @@ export var TelemetryUtils = {
     return "c0ffeec0-ffee-c0ff-eec0-ffeec0ffeec0";
   },
 
+  /**
+   * A fixed valid profile group ID used when Telemetry upload is disabled.
+   */
+  get knownProfileGroupID() {
+    return "decafdec-afde-cafd-ecaf-decafdecafde";
+  },
+
   /**
    * True if this is a content process.
    */

toolkit/components/telemetry/tests/integration/tests/conftest.py

@@ -19,6 +19,7 @@ from six import reraise
 from telemetry_harness.ping_server import PingServer
 
 CANARY_CLIENT_ID = "c0ffeec0-ffee-c0ff-eec0-ffeec0ffeec0"
+CANARY_PROFILE_GROUP_ID = "decafdec-afde-cafd-ecaf-decafdecafde"
 SERVER_ROOT = "toolkit/components/telemetry/tests/marionette/harness/www"
 UUID_PATTERN = re.compile(
     r"^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$"
@@ -295,6 +296,7 @@ class Helpers(object):
         assert value is not None
         assert value != ""
         assert value != CANARY_CLIENT_ID
+        assert value != CANARY_PROFILE_GROUP_ID
         assert re.match(UUID_PATTERN, value) is not None
 
     def wait_for_ping(self, action_func, ping_filter):

toolkit/components/telemetry/tests/integration/tests/test_deletion_request_ping.py

@@ -12,15 +12,19 @@ from telemetry_harness.ping_filters import (
 def test_deletion_request_ping(browser, helpers):
     """Test the "deletion-request" ping behaviour across sessions"""
     # Get the client_id after installing an addon
-    client_id = helpers.wait_for_ping(browser.install_addon, ANY_PING)["clientId"]
+    ping = helpers.wait_for_ping(browser.install_addon, ANY_PING)
+    client_id = ping["clientId"]
+    profile_group_id = ping["profileGroupId"]
 
     # Make sure it's a valid UUID
     helpers.assert_is_valid_uuid(client_id)
+    helpers.assert_is_valid_uuid(profile_group_id)
 
     # Trigger a "deletion-request" ping.
     ping = helpers.wait_for_ping(browser.disable_telemetry, DELETION_REQUEST_PING)
 
     assert "clientId" in ping
+    assert "profileGroupId" in ping
     assert "payload" in ping
     assert "environment" not in ping["payload"]
 
@@ -48,6 +52,11 @@ def test_deletion_request_ping(browser, helpers):
     helpers.assert_is_valid_uuid(new_client_id)
     assert new_client_id != client_id
 
+    assert "profileGroupId" in main_ping
+    new_profile_group_id = main_ping["profileGroupId"]
+    helpers.assert_is_valid_uuid(new_profile_group_id)
+    assert new_profile_group_id != profile_group_id
+
     # Ensure we note in the ping that the user opted in.
     parent_scalars = main_ping["payload"]["processes"]["parent"]["scalars"]
 
@@ -58,6 +67,8 @@ def test_deletion_request_ping(browser, helpers):
     for ping in browser.ping_server.pings:
         if "clientId" in ping:
             helpers.assert_is_valid_uuid(ping["clientId"])
+        if "profileGroupId" in ping:
+            helpers.assert_is_valid_uuid(ping["profileGroupId"])
 
 
 if __name__ == "__main__":

toolkit/components/telemetry/tests/marionette/harness/telemetry_harness/testcase.py

@@ -16,6 +16,7 @@ from marionette_harness.runner.mixins.window_manager import WindowManagerMixin
 from telemetry_harness.ping_server import PingServer
 
 CANARY_CLIENT_ID = "c0ffeec0-ffee-c0ff-eec0-ffeec0ffeec0"
+CANARY_PROFILE_GROUP_ID = "decafdec-afde-cafd-ecaf-decafdecafde"
 UUID_PATTERN = re.compile(
     r"^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$"
 )
@@ -85,6 +86,9 @@ class TelemetryTestCase(WindowManagerMixin, MarionetteTestCase):
 
         # Check for client ID that is used when Telemetry upload is disabled
         self.assertNotEqual(value, CANARY_CLIENT_ID, msg="UUID is CANARY CLIENT ID")
+        self.assertNotEqual(
+            value, CANARY_PROFILE_GROUP_ID, msg="UUID is CANARY PROFILE GROUP ID"
+        )
 
         self.assertIsNotNone(
             re.match(UUID_PATTERN, value),

toolkit/components/telemetry/tests/marionette/tests/client/test_deletion_request_ping.py

@@ -75,7 +75,8 @@ class TestDeletionRequestPing(TelemetryTestCase):
         self.assertIn("clientId", main_ping)
         self.assertIsValidUUID(main_ping["clientId"])
         self.assertNotEqual(main_ping["clientId"], client_id)
-        self.assertEqual(main_ping["profileGroupId"], profile_group_id)
+        self.assertIsValidUUID(main_ping["profileGroupId"])
+        self.assertNotEqual(main_ping["profileGroupId"], profile_group_id)
 
         # Ensure we note in the ping that the user opted in.
         parent_scalars = main_ping["payload"]["processes"]["parent"]["scalars"]
@@ -87,3 +88,5 @@ class TestDeletionRequestPing(TelemetryTestCase):
         for ping in self.ping_server.pings:
             if "clientId" in ping:
                 self.assertIsValidUUID(ping["clientId"])
+            if "profileGroupId" in ping:
+                self.assertIsValidUUID(ping["profileGroupId"])

toolkit/components/telemetry/tests/unit/test_TelemetryClientID_reset.js

@@ -91,10 +91,15 @@ add_task(async function test_clientid_reset_after_reenabling() {
   let clientId = await ClientID.getClientID();
   Assert.equal(TelemetryUtils.knownClientID, clientId);
   let profileGroupId = await ClientID.getProfileGroupID();
-  Assert.equal(
+  Assert.notEqual(
     firstProfileGroupId,
     profileGroupId,
-    "The profile group ID should not have been reset."
+    "The profile group ID should have been reset."
+  );
+  Assert.notEqual(
+    profileGroupId,
+    clientId,
+    "The profile group ID should not match the new client ID."
   );
 
   // Now shutdown the instance
@@ -119,10 +124,20 @@ add_task(async function test_clientid_reset_after_reenabling() {
     "Client ID should be newly generated"
   );
   let newProfileGroupId = await ClientID.getProfileGroupID();
-  Assert.equal(
+  Assert.notEqual(
+    TelemetryUtils.knownProfileGroupID,
+    newProfileGroupId,
+    "The profile group ID should be valid and random"
+  );
+  Assert.notEqual(
     firstProfileGroupId,
     newProfileGroupId,
-    "The profile group ID should not have been reset."
+    "The profile group ID should have been reset."
+  );
+  Assert.notEqual(
+    newProfileGroupId,
+    newClientId,
+    "The profile group ID should not match the client ID."
   );
 });
 
@@ -155,6 +170,16 @@ add_task(async function test_clientid_canary_after_disabling() {
     firstClientId,
     "Client ID should be valid and random"
   );
+  Assert.notEqual(
+    TelemetryUtils.knownProfileGroupID,
+    firstProfileGroupId,
+    "Profile Group ID should be valid and random"
+  );
+  Assert.notEqual(
+    firstClientId,
+    firstProfileGroupId,
+    "Profile Group ID should be valid and not match the client ID"
+  );
 
   // Disable FHR upload: this should trigger a deletion-request ping.
   Services.prefs.setBoolPref(
@@ -173,11 +198,7 @@ add_task(async function test_clientid_canary_after_disabling() {
   let clientId = await ClientID.getClientID();
   Assert.equal(TelemetryUtils.knownClientID, clientId);
   let profileGroupId = await ClientID.getProfileGroupID();
-  Assert.equal(
+  Assert.equal(TelemetryUtils.knownProfileGroupID, profileGroupId);
-    firstProfileGroupId,
-    profileGroupId,
-    "Profile group ID should not have been reset"
-  );
 
   Services.prefs.setBoolPref(TelemetryUtils.Preferences.FhrUploadEnabled, true);
   await sendPing();
@@ -188,10 +209,15 @@ add_task(async function test_clientid_canary_after_disabling() {
     ping.clientId,
     "Client ID should be newly generated"
   );
-  Assert.equal(
+  Assert.notEqual(
     firstProfileGroupId,
     ping.profileGroupId,
-    "Profile group ID should not have been reset"
+    "Profile group ID should be newly generated"
+  );
+  Assert.notEqual(
+    ping.profileGroupId,
+    ping.clientId,
+    "Profile group ID should not match the client ID"
   );
 
   // Now shutdown the instance
@@ -215,9 +241,9 @@ add_task(async function test_clientid_canary_after_disabling() {
   );
   let newProfileGroupId = await ClientID.getProfileGroupID();
   Assert.equal(
-    firstProfileGroupId,
+    TelemetryUtils.knownProfileGroupID,
     newProfileGroupId,
-    "Profile group ID should not have been reset"
+    "Profile group ID should be a canary when upload disabled"
   );
 });
 

toolkit/components/telemetry/tests/unit/test_TelemetryController.js

@@ -208,7 +208,11 @@ add_task(async function test_disableDataUpload() {
     firstClientId,
     "Client ID should be valid and random"
   );
-  Assert.ok(firstProfileGroupId, "Test ping needs a profile group ID");
+  Assert.notEqual(
+    TelemetryUtils.knownProfileGroupID,
+    firstProfileGroupId,
+    "Profile group ID should be valid and random"
+  );
 
   // The next step should trigger an event, watch for it.
   let disableObserved = TestUtils.topicObserved(
@@ -262,10 +266,10 @@ add_task(async function test_disableDataUpload() {
   );
   let secondProfileGroupId =
     TelemetryController.getCurrentPingData().profileGroupId;
-  Assert.equal(
+  Assert.notEqual(
     firstProfileGroupId,
     secondProfileGroupId,
-    "The profile group id must not have changed"
+    "The profile group id must have changed"
   );
   // Simulate a failure in sending the deletion-request ping by disabling the HTTP server.
   await PingServer.stop();
@@ -332,10 +336,10 @@ add_task(async function test_disableDataUpload() {
     ping.clientId,
     "Client ID should be different from the previous value"
   );
-  Assert.equal(
+  Assert.notEqual(
     firstProfileGroupId,
     ping.profileGroupId,
-    "The profile group ID should not change"
+    "The profile group ID should change"
   );
 
   // The "deletion-request" ping should come next, as it was pending.
@@ -347,9 +351,9 @@ add_task(async function test_disableDataUpload() {
     "Deletion must be requested for correct client id"
   );
   Assert.equal(
-    firstProfileGroupId,
+    secondProfileGroupId,
     ping.profileGroupId,
-    "The profile group ID should not change"
+    "Deletion must be requested for correct profile group id"
   );
 
   // Wait on ping activity to settle before moving on to the next test. If we were

toolkit/components/telemetry/tests/unit/test_TelemetrySession.js

@@ -1288,6 +1288,7 @@ add_task(async function test_sendShutdownPing() {
   await TelemetryController.testShutdown();
   // After re-enabling FHR, wait for the new client ID
   gClientID = await ClientID.getClientID();
+  gProfileGroupID = await ClientID.getProfileGroupID();
 
   // Check that the "shutdown" ping was correctly saved to disk.
   await checkPendingShutdownPing();

toolkit/components/telemetry/tests/unit/test_client_id.js

@@ -131,11 +131,15 @@ add_task(async function test_profile_group_id() {
 
   // If there is no DRS file, and no cached id, we should get a new profile group ID.
   await ClientID._reset();
+  Services.prefs.clearUserPref(PREF_CACHED_CLIENTID);
   Services.prefs.clearUserPref(PREF_CACHED_PROFILEGROUPID);
   await IOUtils.remove(drsPath, { ignoreAbsent: true });
+  let clientID = await ClientID.getClientID();
   let profileGroupID = await ClientID.getProfileGroupID();
   Assert.equal(typeof profileGroupID, "string");
   Assert.ok(uuidRegex.test(profileGroupID));
+  // A new client should have distinct client ID and profile group ID.
+  Assert.notEqual(profileGroupID, clientID);
   if (AppConstants.platform != "android") {
     Assert.equal(
       profileGroupID,
@@ -145,6 +149,7 @@ add_task(async function test_profile_group_id() {
 
   // We should be guarded against invalid DRS json.
   await ClientID._reset();
+  Services.prefs.clearUserPref(PREF_CACHED_CLIENTID);
   Services.prefs.clearUserPref(PREF_CACHED_PROFILEGROUPID);
   await IOUtils.writeUTF8(drsPath, "abcd", {
     tmpPath: drsPath + ".tmp",
@@ -174,17 +179,42 @@ add_task(async function test_profile_group_id() {
     }
   }
 
-  // Test that valid DRS actually works.
   const validProfileGroupID = "5afebd62-a33c-416c-b519-5c60fb988e8e";
   const validClientID = "d06361a2-67d8-4d41-b804-6fab6ddf5461";
+
+  // An older version of DRS should reset the profile group ID to the client ID.
   await ClientID._reset();
+  Services.prefs.clearUserPref(PREF_CACHED_CLIENTID);
+  Services.prefs.clearUserPref(PREF_CACHED_PROFILEGROUPID);
   await IOUtils.writeJSON(drsPath, {
     clientID: validClientID,
     profileGroupID: validProfileGroupID,
   });
+  clientID = await ClientID.getClientID();
+  profileGroupID = await ClientID.getProfileGroupID();
+  Assert.equal(typeof profileGroupID, "string");
+  Assert.ok(uuidRegex.test(profileGroupID));
+  Assert.equal(clientID, validClientID);
+  Assert.equal(profileGroupID, clientID);
+  if (AppConstants.platform != "android") {
+    Assert.equal(
+      profileGroupID,
+      Glean.legacyTelemetry.profileGroupId.testGetValue()
+    );
+  }
+
+  // Test that valid DRS actually works.
+  await ClientID._reset();
+  Services.prefs.clearUserPref(PREF_CACHED_CLIENTID);
+  Services.prefs.clearUserPref(PREF_CACHED_PROFILEGROUPID);
+  await IOUtils.writeJSON(drsPath, {
+    version: 2,
+    clientID: validClientID,
+    profileGroupID: validProfileGroupID,
+  });
   profileGroupID = await ClientID.getProfileGroupID();
   Assert.equal(profileGroupID, validProfileGroupID);
-  let clientID = await ClientID.getClientID();
+  clientID = await ClientID.getClientID();
   Assert.equal(clientID, validClientID);
   if (AppConstants.platform != "android") {
     Assert.equal(
@@ -197,9 +227,12 @@ add_task(async function test_profile_group_id() {
   // Test that valid DRS actually works when the client ID is missing.
   await ClientID._reset();
   await IOUtils.writeJSON(drsPath, {
+    version: 2,
     profileGroupID: validProfileGroupID,
   });
+  clientID = await ClientID.getClientID();
   profileGroupID = await ClientID.getProfileGroupID();
+  Assert.equal(clientID, validClientID);
   Assert.equal(profileGroupID, validProfileGroupID);
   if (AppConstants.platform != "android") {
     Assert.equal(
@@ -211,6 +244,7 @@ add_task(async function test_profile_group_id() {
 
   // Test that reloading a valid DRS works.
   await ClientID._reset();
+  Services.prefs.clearUserPref(PREF_CACHED_CLIENTID);
   Services.prefs.clearUserPref(PREF_CACHED_PROFILEGROUPID);
   profileGroupID = await ClientID.getProfileGroupID();
   Assert.equal(profileGroupID, validProfileGroupID);
@@ -367,33 +401,48 @@ add_task(async function test_set_profile_group_id() {
   }
 });
 
-add_task(async function test_setCanaryClientID() {
+add_task(async function test_setCanaryIdentifiers() {
-  const KNOWN_UUID = "c0ffeec0-ffee-c0ff-eec0-ffeec0ffeec0";
+  const KNOWN_CLIENT_UUID = "c0ffeec0-ffee-c0ff-eec0-ffeec0ffeec0";
+  const KNOWN_PROFILE_GROUP_UUID = "decafdec-afde-cafd-ecaf-decafdecafde";
 
   await ClientID._reset();
 
   // We should be able to set a valid UUID
-  await ClientID.setCanaryClientID();
+  await ClientID.setCanaryIdentifiers();
   let clientID = await ClientID.getClientID();
-  Assert.equal(KNOWN_UUID, clientID);
+  Assert.equal(KNOWN_CLIENT_UUID, clientID);
+  let profileGroupID = await ClientID.getProfileGroupID();
+  Assert.equal(KNOWN_PROFILE_GROUP_UUID, profileGroupID);
   if (AppConstants.platform != "android") {
     Assert.equal(clientID, Glean.legacyTelemetry.clientId.testGetValue());
+    Assert.equal(
+      profileGroupID,
+      Glean.legacyTelemetry.profileGroupId.testGetValue()
+    );
   }
 });
 
 add_task(async function test_removeParallelGet() {
   // We should get a valid UUID after reset
-  await ClientID.removeClientID();
+  await ClientID.resetIdentifiers();
   let firstClientID = await ClientID.getClientID();
+  let firstProfileGroupID = await ClientID.getProfileGroupID();
   if (AppConstants.platform != "android") {
     Assert.equal(firstClientID, Glean.legacyTelemetry.clientId.testGetValue());
+    Assert.equal(
+      firstProfileGroupID,
+      Glean.legacyTelemetry.profileGroupId.testGetValue()
+    );
   }
 
+  // The IDs should differ after being reset.
+  Assert.notEqual(firstClientID, firstProfileGroupID);
+
   // We should get the same ID twice when requesting it in parallel to a reset.
-  let promiseRemoveClientID = ClientID.removeClientID();
+  let promiseResetIdentifiers = ClientID.resetIdentifiers();
   let p = ClientID.getClientID();
   let newClientID = await ClientID.getClientID();
-  await promiseRemoveClientID;
+  await promiseResetIdentifiers;
   let otherClientID = await p;
 
   Assert.notEqual(