Bug 1249596 - Part 2: Remove FHR client id migration code. r?dexter draft
authorJeremy Lempereur <jeremy.lempereur@gmail.com>
Thu, 18 Jan 2018 23:51:24 +0100
changeset 751217 a69ad9f485ecf825a2d525894aaf47de1d31c8c0
parent 751216 f88dd3e58a94d2fdfa4b60ec5f4b99e27eb916e4
push id97902
push userbmo:jeremy.lempereur@gmail.com
push dateMon, 05 Feb 2018 19:16:12 +0000
reviewersdexter
bugs1249596, 1431544
milestone60.0a1
Bug 1249596 - Part 2: Remove FHR client id migration code. r?dexter Removed a fallback import from a legacy FHR file when there is no valid ID in the DRS file. This commit is related to bug 1431544 MozReview-Commit-ID: AACq7InWJpy
browser/components/migration/FirefoxProfileMigrator.js
browser/components/migration/tests/unit/test_fx_telemetry.js
toolkit/modules/ClientID.jsm
toolkit/modules/tests/xpcshell/test_client_id.js
--- a/browser/components/migration/FirefoxProfileMigrator.js
+++ b/browser/components/migration/FirefoxProfileMigrator.js
@@ -239,52 +239,32 @@ FirefoxProfileMigrator.prototype._getRes
       let createSubDir = (name) => {
         let dir = currentProfileDir.clone();
         dir.append(name);
         dir.create(Ci.nsIFile.DIRECTORY_TYPE, FileUtils.PERMS_DIRECTORY);
         return dir;
       };
 
       // If the 'datareporting' directory exists we migrate files from it.
-      let haveStateFile = false;
       let dataReportingDir = this._getFileObject(sourceProfileDir, "datareporting");
       if (dataReportingDir && dataReportingDir.isDirectory()) {
         // Copy only specific files.
         let toCopy = ["state.json", "session-state.json"];
 
         let dest = createSubDir("datareporting");
         let enumerator = dataReportingDir.directoryEntries;
         while (enumerator.hasMoreElements()) {
           let file = enumerator.getNext().QueryInterface(Ci.nsIFile);
           if (file.isDirectory() || toCopy.indexOf(file.leafName) == -1) {
             continue;
           }
-
-          if (file.leafName == "state.json") {
-            haveStateFile = true;
-          }
           file.copyTo(dest, "");
         }
       }
 
-      if (!haveStateFile) {
-        // Fall back to migrating the state file that contains the client id from healthreport/.
-        // We first moved the client id management from the FHR implementation to the datareporting
-        // service.
-        // Consequently, we try to migrate an existing FHR state file here as a fallback.
-        let healthReportDir = this._getFileObject(sourceProfileDir, "healthreport");
-        if (healthReportDir && healthReportDir.isDirectory()) {
-          let stateFile = this._getFileObject(healthReportDir, "state.json");
-          if (stateFile) {
-            let dest = createSubDir("healthreport");
-            stateFile.copyTo(dest, "");
-          }
-        }
-      }
-
       aCallback(true);
     },
   };
 
   return [places, cookies, passwords, formData, dictionary, bookmarksBackups,
           session, sync, times, telemetry, favicons].filter(r => r);
 };
 
--- a/browser/components/migration/tests/unit/test_fx_telemetry.js
+++ b/browser/components/migration/tests/unit/test_fx_telemetry.js
@@ -136,39 +136,16 @@ add_task(async function test_migrate_fil
   checkDirectoryContains(targetDir, {
     "datareporting": {
       "state.json": stateContent,
       "session-state.json": sessionStateContent,
     },
   });
 });
 
-add_task(async function test_fallback_fhr_state() {
-  let [srcDir, targetDir] = getTestDirs();
-
-  // Test that we fall back to migrating FHR state if the datareporting
-  // state file does not exist.
-  let stateContent = JSON.stringify({
-    clientId: "68d5474e-19dc-45c1-8e9a-81fca592707c",
-  });
-  let subDir = createSubDir(srcDir, "healthreport");
-  writeToFile(subDir, "state.json", stateContent);
-
-  // Perform migration.
-  let ok = await promiseTelemetryMigrator(srcDir, targetDir);
-  Assert.ok(ok, "callback should have been true");
-
-  checkDirectoryContains(targetDir, {
-    "healthreport": {
-      "state.json": stateContent,
-    },
-  });
-});
-
-
 add_task(async function test_datareporting_not_dir() {
   let [srcDir, targetDir] = getTestDirs();
 
   writeToFile(srcDir, "datareporting", "I'm a file but should be a directory");
 
   let ok = await promiseTelemetryMigrator(srcDir, targetDir);
   Assert.ok(ok, "callback should have been true even though the directory was a file");
 
--- a/toolkit/modules/ClientID.jsm
+++ b/toolkit/modules/ClientID.jsm
@@ -40,18 +40,17 @@ const PREF_CACHED_CLIENTID = "toolkit.te
 function isValidClientID(id) {
   const UUID_REGEX = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i;
   return UUID_REGEX.test(id);
 }
 
 this.ClientID = Object.freeze({
   /**
    * This returns a promise resolving to the the stable client ID we use for
-   * data reporting (FHR & Telemetry). Previously exising FHR client IDs are
-   * migrated to this.
+   * data reporting (FHR & Telemetry).
    *
    * WARNING: This functionality is duplicated for Android (see GeckoProfile.getClientId
    * for more). There are Java tests (TestGeckoProfile) to ensure the functionality is
    * consistent and Gecko tests to come (bug 1249156). However, THIS IS NOT FOOLPROOF.
    * Be careful when changing this code and, in particular, the underlying file format.
    *
    * @return {Promise<string>} The stable client ID.
    */
@@ -91,45 +90,32 @@ var ClientIDImpl = {
     }
 
     this._loadClientIdTask = this._doLoadClientID();
     let clear = () => this._loadClientIdTask = null;
     this._loadClientIdTask.then(clear, clear);
     return this._loadClientIdTask;
   },
 
+  /**
+   * Load the Client ID from the DataReporting Service state file.
+   * If no Client ID is found, we generate a new one.
+   */
   async _doLoadClientID() {
-    // As we want to correlate FHR and telemetry data (and move towards unifying the two),
-    // we first moved the ID management from the FHR implementation to the datareporting
-    // service, then to a common shared module.
-    // Consequently, we try to import an existing FHR ID, so we can keep using it.
-
-    // Try to load the client id from the DRS state file first.
+    // Try to load the client id from the DRS state file.
     try {
       let state = await CommonUtils.readJSON(gStateFilePath);
       if (state && this.updateClientID(state.clientID)) {
         return this._clientID;
       }
     } catch (e) {
       // fall through to next option
     }
 
-    // If we dont have DRS state yet, try to import from the FHR state.
-    try {
-      let fhrStatePath = OS.Path.join(OS.Constants.Path.profileDir, "healthreport", "state.json");
-      let state = await CommonUtils.readJSON(fhrStatePath);
-      if (state && this.updateClientID(state.clientID)) {
-        this._saveClientID();
-        return this._clientID;
-      }
-    } catch (e) {
-      // fall through to next option
-    }
-
-    // We dont have an id from FHR yet, generate a new ID.
+    // We dont have an id from the DRS state file yet, generate a new ID.
     this.updateClientID(CommonUtils.generateUUID());
     this._saveClientIdTask = this._saveClientID();
 
     // Wait on persisting the id. Otherwise failure to save the ID would result in
     // the client creating and subsequently sending multiple IDs to the server.
     // This would appear as multiple clients submitting similar data, which would
     // result in orphaning.
     await this._saveClientIdTask;
@@ -146,18 +132,17 @@ var ClientIDImpl = {
     let obj = { clientID: this._clientID };
     await OS.File.makeDir(gDatareportingPath);
     await CommonUtils.writeJSON(obj, gStateFilePath);
     this._saveClientIdTask = null;
   },
 
   /**
    * This returns a promise resolving to the the stable client ID we use for
-   * data reporting (FHR & Telemetry). Previously exising FHR client IDs are
-   * migrated to this.
+   * data reporting (FHR & Telemetry).
    *
    * @return {Promise<string>} The stable client ID.
    */
   getClientID() {
     if (!this._clientID) {
       return this._loadClientID();
     }
 
--- a/toolkit/modules/tests/xpcshell/test_client_id.js
+++ b/toolkit/modules/tests/xpcshell/test_client_id.js
@@ -12,81 +12,43 @@ ChromeUtils.import("resource://gre/modul
 
 function run_test() {
   do_get_profile();
   run_next_test();
 }
 
 add_task(async function() {
   const drsPath = OS.Path.join(OS.Constants.Path.profileDir, "datareporting", "state.json");
-  const fhrDir  = OS.Path.join(OS.Constants.Path.profileDir, "healthreport");
-  const fhrPath = OS.Path.join(fhrDir, "state.json");
   const uuidRegex = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i;
   const invalidIDs = [
     [-1, "setIntPref"],
     [0.5, "setIntPref"],
     ["INVALID-UUID", "setStringPref"],
     [true, "setBoolPref"],
     ["", "setStringPref"],
     ["3d1e1560-682a-4043-8cf2-aaaaaaaaaaaZ", "setStringPref"],
   ];
   const PREF_CACHED_CLIENTID = "toolkit.telemetry.cachedClientID";
 
-  await OS.File.makeDir(fhrDir);
-
-  // Check that we are importing the FHR client ID.
-  let clientID = CommonUtils.generateUUID();
-  await CommonUtils.writeJSON({clientID}, fhrPath);
-  Assert.equal(clientID, await ClientID.getClientID());
-
-  // We should persist the ID in DRS now and not pick up a differing ID from FHR.
+  // If there is no DRS file, we should get a new client ID.
   await ClientID._reset();
-  await CommonUtils.writeJSON({clientID: CommonUtils.generateUUID()}, fhrPath);
-  Assert.equal(clientID, await ClientID.getClientID());
-
-  // We should be guarded against broken FHR data.
-  for (let invalidID of invalidIDs) {
-    await ClientID._reset();
-    await OS.File.remove(drsPath);
-    await CommonUtils.writeJSON({clientID: invalidID}, fhrPath);
-    clientID = await ClientID.getClientID();
-    Assert.equal(typeof(clientID), "string");
-    Assert.ok(uuidRegex.test(clientID));
-  }
-
-  // We should be guarded against invalid FHR json.
-  await ClientID._reset();
-  await OS.File.remove(drsPath);
-  await OS.File.writeAtomic(fhrPath, "abcd", {encoding: "utf-8", tmpPath: fhrPath + ".tmp"});
-  clientID = await ClientID.getClientID();
+  let clientID = await ClientID.getClientID();
   Assert.equal(typeof(clientID), "string");
   Assert.ok(uuidRegex.test(clientID));
 
-  // We should be guarded against broken DRS data too and fall back to loading
-  // the FHR ID.
-  for (let invalidID of invalidIDs) {
-    await ClientID._reset();
-    clientID = CommonUtils.generateUUID();
-    await CommonUtils.writeJSON({clientID}, fhrPath);
-    await CommonUtils.writeJSON({clientID: invalidID}, drsPath);
-    Assert.equal(clientID, await ClientID.getClientID());
-  }
-
-  // We should be guarded against invalid DRS json too.
+  // We should be guarded against invalid DRS json.
   await ClientID._reset();
-  await OS.File.remove(fhrPath);
   await OS.File.writeAtomic(drsPath, "abcd", {encoding: "utf-8", tmpPath: drsPath + ".tmp"});
   clientID = await ClientID.getClientID();
   Assert.equal(typeof(clientID), "string");
   Assert.ok(uuidRegex.test(clientID));
 
-  // If both the FHR and DSR data are broken, we should end up with a new client ID.
+  // If the DRS data is broken, we should end up with a new client ID.
   for (let [invalidID, ] of invalidIDs) {
     await ClientID._reset();
-    await CommonUtils.writeJSON({clientID: invalidID}, fhrPath);
     await CommonUtils.writeJSON({clientID: invalidID}, drsPath);
     clientID = await ClientID.getClientID();
     Assert.equal(typeof(clientID), "string");
     Assert.ok(uuidRegex.test(clientID));
   }
 
   // Assure that cached IDs are being checked for validity.
   for (let [invalidID, prefFunc] of invalidIDs) {