Bug 1501329 - Persist information about canary after resetting client ID. r=chutten, a=pascalc
authorJan-Erik Rediger <jrediger@mozilla.com>
Fri, 26 Oct 2018 18:01:13 +0000
changeset 492970 565452061d53
parent 492969 7247a70272a2
child 492971 713bae63f714
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 - Persist information about canary after resetting client ID. r=chutten, a=pascalc We erroneously reset client IDs on Fennec to a canary client ID. This is now detected and a new valid and random client ID is set. This adds a new boolean attribute "wasCanary" to the `state.json` file generated by ClientID.jsm. Depends on D9544 Differential Revision: https://phabricator.services.mozilla.com/D9903
toolkit/components/telemetry/tests/unit/test_TelemetryClientID_reset.js
toolkit/modules/ClientID.jsm
toolkit/modules/tests/xpcshell/test_client_id.js
toolkit/modules/tests/xpcshell/xpcshell.ini
--- a/toolkit/components/telemetry/tests/unit/test_TelemetryClientID_reset.js
+++ b/toolkit/components/telemetry/tests/unit/test_TelemetryClientID_reset.js
@@ -10,20 +10,20 @@ ChromeUtils.import("resource://gre/modul
 ChromeUtils.import("resource://gre/modules/TelemetrySend.jsm", this);
 ChromeUtils.import("resource://gre/modules/TelemetryUtils.jsm", this);
 ChromeUtils.import("resource://gre/modules/Preferences.jsm");
 
 const PING_FORMAT_VERSION = 4;
 const OPTOUT_PING_TYPE = "optout";
 const TEST_PING_TYPE = "test-ping-type";
 
-function sendPing() {
+function sendPing(addEnvironment = false) {
   let options = {
     addClientId: true,
-    addEnvironment: false,
+    addEnvironment,
   };
   return TelemetryController.submitExternalPing(TEST_PING_TYPE, {}, options);
 }
 
 add_task(async function test_setup() {
   // Addon manager needs a profile directory
   do_get_profile();
   // Make sure we don't generate unexpected pings due to pref changes.
@@ -206,26 +206,28 @@ add_task(async function test_clientid_ca
   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 TelemetryStorage.testClearPendingPings();
   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 TelemetryStorage.testClearPendingPings();
   await TelemetryController.testReset();
 
   newClientId = await ClientID.getClientID();
   Assert.equal(firstClientId, newClientId, "Client ID should be unmodified");
 });
 
 add_task(async function stopServer() {
   await PingServer.stop();
--- a/toolkit/modules/ClientID.jsm
+++ b/toolkit/modules/ClientID.jsm
@@ -4,19 +4,22 @@
 
 "use strict";
 
 var EXPORTED_SYMBOLS = ["ClientID"];
 
 ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
 ChromeUtils.import("resource://gre/modules/Services.jsm");
 ChromeUtils.import("resource://gre/modules/Log.jsm");
+ChromeUtils.import("resource://gre/modules/AppConstants.jsm");
 
 const LOGGER_NAME = "Toolkit.Telemetry";
 const LOGGER_PREFIX = "ClientID::";
+// Must match ID in TelemetryUtils
+const CANARY_CLIENT_ID = "c0ffeec0-ffee-c0ff-eec0-ffeec0ffeec0";
 
 ChromeUtils.defineModuleGetter(this, "CommonUtils",
                                "resource://services-common/utils.js");
 ChromeUtils.defineModuleGetter(this, "OS",
                                "resource://gre/modules/osfile.jsm");
 
 XPCOMUtils.defineLazyGetter(this, "gDatareportingPath", () => {
   return OS.Path.join(OS.Constants.Path.profileDir, "datareporting");
@@ -52,16 +55,28 @@ var ClientID = Object.freeze({
    *
    * @return {Promise<string>} The stable client ID.
    */
   getClientID() {
     return ClientIDImpl.getClientID();
   },
 
   /**
+   * This returns true if the client ID prior to the last client ID reset was a canary client ID.
+   * Android only. Always returns null on Desktop.
+   */
+  wasCanaryClientID() {
+    if (AppConstants.platform == "android") {
+      return ClientIDImpl.wasCanaryClientID();
+    }
+
+    return null;
+  },
+
+  /**
    * Get the client id synchronously without hitting the disk.
    * This returns:
    *  - the current on-disk client id if it was already loaded
    *  - the client id that we cached into preferences (if any)
    *  - null otherwise
    */
   getCachedClientID() {
     return ClientIDImpl.getCachedClientID();
@@ -102,16 +117,17 @@ var ClientID = Object.freeze({
 });
 
 var ClientIDImpl = {
   _clientID: null,
   _loadClientIdTask: null,
   _saveClientIdTask: null,
   _removeClientIdTask: null,
   _logger: null,
+  _wasCanary: null,
 
   _loadClientID() {
     if (this._loadClientIdTask) {
       return this._loadClientIdTask;
     }
 
     this._loadClientIdTask = this._doLoadClientID();
     let clear = () => this._loadClientIdTask = null;
@@ -125,16 +141,19 @@ var ClientIDImpl = {
    */
   async _doLoadClientID() {
     // If there's a removal in progress, let's wait for it
     await this._removeClientIdTask;
 
     // Try to load the client id from the DRS state file.
     try {
       let state = await CommonUtils.readJSON(gStateFilePath);
+      if (AppConstants.platform == "android" && state && "wasCanary" in state) {
+        this._wasCanary = state.wasCanary;
+      }
       if (state && this.updateClientID(state.clientID)) {
         return this._clientID;
       }
     } catch (e) {
       // fall through to next option
     }
 
     // We dont have an id from the DRS state file yet, generate a new ID.
@@ -152,16 +171,20 @@ var ClientIDImpl = {
 
   /**
    * Save the client ID to the client ID file.
    *
    * @return {Promise} A promise resolved when the client ID is saved to disk.
    */
   async _saveClientID() {
     let obj = { clientID: this._clientID };
+    // We detected a canary client ID when resetting, storing this as a flag
+    if (AppConstants.platform == "android" && this._wasCanary) {
+      obj.wasCanary = true;
+    }
     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).
@@ -172,16 +195,24 @@ var ClientIDImpl = {
     if (!this._clientID) {
       return this._loadClientID();
     }
 
     return Promise.resolve(this._clientID);
   },
 
   /**
+   * This returns true if the client ID prior to the last client ID reset was a canary client ID.
+   * Android only. Always returns null on Desktop.
+   */
+  wasCanaryClientID() {
+    return this._wasCanary;
+  },
+
+  /**
    * Get the client id synchronously without hitting the disk.
    * This returns:
    *  - the current on-disk client id if it was already loaded
    *  - the client id that we cached into preferences (if any)
    *  - null otherwise
    */
   getCachedClientID() {
     if (this._clientID) {
@@ -238,24 +269,31 @@ var ClientIDImpl = {
     // Clear the client id from the preference cache.
     Services.prefs.clearUserPref(PREF_CACHED_CLIENTID);
 
     // Remove the client id from disk
     await OS.File.remove(gStateFilePath, {ignoreAbsent: true});
   },
 
   async resetClientID() {
+    let oldClientId = this._clientID;
+
     // Wait for the removal.
     // Asynchronous calls to getClientID will also be blocked on this.
     this._removeClientIdTask = this._doRemoveClientID();
     let clear = () => this._removeClientIdTask = null;
     this._removeClientIdTask.then(clear, clear);
 
     await this._removeClientIdTask;
 
+    // On Android we detect resets after a canary client ID.
+    if (AppConstants.platform == "android" ) {
+      this._wasCanary = oldClientId == CANARY_CLIENT_ID;
+    }
+
     // Generate a new id.
     return this.getClientID();
   },
 
   /**
    * Sets the client id to the given value and updates the value cached in
    * preferences only if the given id is a valid.
    *
--- a/toolkit/modules/tests/xpcshell/test_client_id.js
+++ b/toolkit/modules/tests/xpcshell/test_client_id.js
@@ -2,16 +2,17 @@
  * http://creativecommons.org/publicdomain/zero/1.0/ */
 
 "use strict";
 
 ChromeUtils.import("resource://gre/modules/ClientID.jsm");
 ChromeUtils.import("resource://services-common/utils.js");
 ChromeUtils.import("resource://gre/modules/osfile.jsm");
 ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
+ChromeUtils.import("resource://gre/modules/AppConstants.jsm");
 
 const PREF_CACHED_CLIENTID = "toolkit.telemetry.cachedClientID";
 
 function run_test() {
   do_get_profile();
   run_next_test();
 }
 
@@ -125,8 +126,32 @@ add_task(async function test_resetParall
   // We should get the same ID twice when requesting it in parallel to a reset.
   let p = ClientID.resetClientID();
   let newClientID = await ClientID.getClientID();
   let otherClientID = await p;
 
   Assert.notEqual(firstClientID, newClientID, "After reset client ID should be different.");
   Assert.equal(newClientID, otherClientID, "Getting the client ID in parallel to a reset should give the same id.");
 });
+
+add_task({
+  skip_if: () => AppConstants.platform != "android",
+}, async function test_FennecCanaryDetect() {
+  const KNOWN_UUID = "c0ffeec0-ffee-c0ff-eec0-ffeec0ffeec0";
+
+  // We should get a valid UUID after reset
+  let firstClientID = await ClientID.resetClientID();
+  Assert.notEqual(KNOWN_UUID, firstClientID, "Client ID should be random.");
+
+  // Set the canary client ID.
+  await ClientID.setClientID(KNOWN_UUID);
+  Assert.equal(KNOWN_UUID, await ClientID.getClientID(), "Client ID should be known canary.");
+
+  let newClientID = await ClientID.resetClientID();
+  Assert.notEqual(KNOWN_UUID, newClientID, "After reset Client ID should be random.");
+  Assert.notEqual(firstClientID, newClientID, "After reset Client ID should be new.");
+  Assert.ok(ClientID.wasCanaryClientID(), "After reset we should have detected a canary client ID");
+
+  let clientID = await ClientID.resetClientID();
+  Assert.notEqual(KNOWN_UUID, clientID, "After reset Client ID should be random.");
+  Assert.notEqual(newClientID, clientID, "After reset Client ID should be new.");
+  Assert.ok(!ClientID.wasCanaryClientID(), "After reset we should not have detected a canary client ID");
+});
--- a/toolkit/modules/tests/xpcshell/xpcshell.ini
+++ b/toolkit/modules/tests/xpcshell/xpcshell.ini
@@ -7,17 +7,16 @@ support-files =
   chromeappsstore.sqlite
   corrupt.sqlite
   zips/zen.zip
 
 [test_BinarySearch.js]
 skip-if = toolkit == 'android'
 [test_CanonicalJSON.js]
 [test_client_id.js]
-skip-if = toolkit == 'android'
 [test_Color.js]
 [test_CreditCard.js]
 [test_DeferredTask.js]
 skip-if = toolkit == 'android'
 [test_FileUtils.js]
 skip-if = toolkit == 'android'
 [test_FinderIterator.js]
 [test_GMPInstallManager.js]