Bug 1064333 - Migrate the FHR client id to the datareporting service. r=gps

services/common/utils.js

@@ -398,9 +398,8 @@ this.CommonUtils = {
    * @return a promise, as produced by OS.File.writeAtomic.
    */
   writeJSON: function(contents, path) {
-    let encoder = new TextEncoder();
-    let array = encoder.encode(JSON.stringify(contents));
-    return OS.File.writeAtomic(path, array, {tmpPath: path + ".tmp"});
+    let data = JSON.stringify(contents);
+    return OS.File.writeAtomic(path, data, {encoding: "utf-8", tmpPath: path + ".tmp"});
   },
 
 

services/datareporting/DataReportingService.js

@@ -9,6 +9,9 @@ const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
 Cu.import("resource://gre/modules/Preferences.jsm");
 Cu.import("resource://gre/modules/XPCOMUtils.jsm");
 Cu.import("resource://services-common/utils.js");
+Cu.import("resource://gre/modules/Promise.jsm");
+Cu.import("resource://gre/modules/Task.jsm");
+Cu.import("resource://gre/modules/osfile.jsm");
 
 
 const ROOT_BRANCH = "datareporting.";
@@ -61,6 +64,13 @@ this.DataReportingService = function () {
 
   this._os = Cc["@mozilla.org/observer-service;1"]
                .getService(Ci.nsIObserverService);
+
+  this._clientID = null;
+  this._loadClientIdTask = null;
+  this._saveClientIdTask = null;
+
+  this._stateDir = null;
+  this._stateFilePath = null;
 }
 
 DataReportingService.prototype = Object.freeze({
@@ -126,6 +136,9 @@ DataReportingService.prototype = Object.freeze({
           let policyPrefs = new Preferences(POLICY_BRANCH);
           this.policy = new DataReportingPolicy(policyPrefs, this._prefs, this);
 
+          this._stateDir = OS.Path.join(OS.Constants.Path.profileDir, "datareporting");
+          this._stateFilePath = OS.Path.join(this._stateDir, "state.json");
+
           this._os.addObserver(this, "sessionstore-windows-restored", true);
         } catch (ex) {
           Cu.reportError("Exception when initializing data reporting service: " +
@@ -284,6 +297,109 @@ DataReportingService.prototype = Object.freeze({
       this._prefs.set("service.firstRun", true);
     }.bind(this));
   },
+
+  _loadClientID: Task.async(function* () {
+    if (this._loadClientIdTask) {
+      return this._loadClientIdTask;
+    }
+
+    // Previously we had the stable client ID managed in FHR.
+    // As we want to start correlating FHR and telemetry data (and moving towards
+    // unifying the two), we moved the ID management to the datareporting
+    // service. Consequently, we try to import the FHR ID first, so we can keep
+    // using it.
+
+    // Try to load the client id from the DRS state file first.
+    try {
+      let state = yield CommonUtils.readJSON(this._stateFilePath);
+      if (state && 'clientID' in state && typeof(state.clientID) == 'string') {
+        this._clientID = state.clientID;
+        this._loadClientIdTask = null;
+        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 = yield CommonUtils.readJSON(fhrStatePath);
+      if (state && 'clientID' in state && typeof(state.clientID) == 'string') {
+        this._clientID = state.clientID;
+        this._loadClientIdTask = null;
+        this._saveClientID();
+        return this._clientID;
+      }
+    } catch (e) {
+      // fall through to next option
+    }
+
+    // We dont have an id from FHR yet, generate a new ID.
+    this._clientID = CommonUtils.generateUUID();
+    this._loadClientIdTask = null;
+    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.
+    yield this._saveClientIdTask;
+
+    return this._clientID;
+  }),
+
+  _saveClientID: Task.async(function* () {
+    let obj = { clientID: this._clientID };
+    yield OS.File.makeDir(this._stateDir);
+    yield CommonUtils.writeJSON(obj, this._stateFilePath);
+    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.
+   *
+   * @return Promise<string> The stable client ID.
+   */
+  getClientID: function() {
+    if (this._loadClientIdTask) {
+      return this._loadClientIdTask;
+    }
+
+    if (!this._clientID) {
+      this._loadClientIdTask = this._loadClientID();
+      return this._loadClientIdTask;
+    }
+
+    return Promise.resolve(this._clientID);
+  },
+
+  /**
+   * Reset the stable client id.
+   *
+   * @return Promise<string> The new client ID.
+   */
+  resetClientID: Task.async(function* () {
+    yield this._loadClientIdTask;
+    yield this._saveClientIdTask;
+
+    this._clientID = CommonUtils.generateUUID();
+    this._saveClientIdTask = this._saveClientID();
+    yield this._saveClientIdTask;
+
+    return this._clientID;
+  }),
+
+  /*
+   * Simulate a restart of the service. This is for testing only.
+   */
+  _reset: Task.async(function* () {
+    yield this._loadClientIdTask;
+    yield this._saveClientIdTask;
+    this._clientID = null;
+  }),
 });
 
 this.NSGetFactory = XPCOMUtils.generateNSGetFactory([DataReportingService]);

services/datareporting/policy.jsm

@@ -817,7 +817,8 @@ this.DataReportingPolicy.prototype = Object.freeze({
     this._log.info("Requesting data submission. Will expire at " +
                    requestExpiresDate);
     try {
-      this._listener[handler](this._inProgressSubmissionRequest);
+      let promise = this._listener[handler](this._inProgressSubmissionRequest);
+      chained = chained.then(() => promise, null);
     } catch (ex) {
       this._log.warn("Exception when calling " + handler + ": " +
                      CommonUtils.exceptionStr(ex));

services/datareporting/tests/xpcshell/test_client_id.js

@@ -0,0 +1,86 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
+
+Cu.import("resource://gre/modules/Task.jsm");
+Cu.import("resource://services-common/utils.js");
+Cu.import("resource://gre/modules/osfile.jsm");
+Cu.import("resource://gre/modules/XPCOMUtils.jsm");
+
+XPCOMUtils.defineLazyGetter(this, "gDatareportingService",
+  () => Cc["@mozilla.org/datareporting/service;1"]
+          .getService(Ci.nsISupports)
+          .wrappedJSObject);
+
+function run_test() {
+  do_get_profile();
+
+  // Send the needed startup notifications to the datareporting service
+  // to ensure that it has been initialized.
+  gDatareportingService.observe(null, "app-startup", null);
+  gDatareportingService.observe(null, "profile-after-change", null);
+
+  run_next_test();
+}
+
+add_task(function* () {
+  const drsPath = gDatareportingService._stateFilePath;
+  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;
+
+  yield OS.File.makeDir(fhrDir);
+
+  // Check that we are importing the FHR client ID.
+  let clientID = CommonUtils.generateUUID();
+  yield CommonUtils.writeJSON({clientID: clientID}, fhrPath);
+  Assert.equal(clientID, yield gDatareportingService.getClientID());
+
+  // We should persist the ID in DRS now and not pick up a differing ID from FHR.
+  yield gDatareportingService._reset();
+  yield CommonUtils.writeJSON({clientID: CommonUtils.generateUUID()}, fhrPath);
+  Assert.equal(clientID, yield gDatareportingService.getClientID());
+
+  // We should be guarded against broken FHR data.
+  yield gDatareportingService._reset();
+  yield OS.File.remove(drsPath);
+  yield CommonUtils.writeJSON({clientID: -1}, fhrPath);
+  clientID = yield gDatareportingService.getClientID();
+  Assert.equal(typeof(clientID), 'string');
+  Assert.ok(uuidRegex.test(clientID));
+
+  // We should be guarded against invalid FHR json.
+  yield gDatareportingService._reset();
+  yield OS.File.remove(drsPath);
+  yield OS.File.writeAtomic(fhrPath, "abcd", {encoding: "utf-8", tmpPath: fhrPath + ".tmp"});
+  clientID = yield gDatareportingService.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.
+  yield gDatareportingService._reset();
+  clientID = CommonUtils.generateUUID();
+  yield CommonUtils.writeJSON({clientID: clientID}, fhrPath);
+  yield CommonUtils.writeJSON({clientID: -1}, drsPath);
+  Assert.equal(clientID, yield gDatareportingService.getClientID());
+
+  // We should be guarded against invalid DRS json too.
+  yield gDatareportingService._reset();
+  yield OS.File.remove(fhrPath);
+  yield OS.File.writeAtomic(drsPath, "abcd", {encoding: "utf-8", tmpPath: drsPath + ".tmp"});
+  clientID = yield gDatareportingService.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.
+  yield gDatareportingService._reset();
+  yield CommonUtils.writeJSON({clientID: -1}, fhrPath);
+  yield CommonUtils.writeJSON({clientID: -1}, drsPath);
+  clientID = yield gDatareportingService.getClientID();
+  Assert.equal(typeof(clientID), 'string');
+  Assert.ok(uuidRegex.test(clientID));
+});

services/datareporting/tests/xpcshell/xpcshell.ini

@@ -5,3 +5,4 @@ skip-if = toolkit == 'android' || toolkit == 'gonk'
 
 [test_policy.js]
 [test_session_recorder.js]
+[test_client_id.js]

services/healthreport/healthreporter.jsm

@@ -130,13 +130,18 @@ HealthReporterState.prototype = Object.freeze({
     return Task.spawn(function* init() {
       yield OS.File.makeDir(this._stateDir);
 
+      let drs = Cc["@mozilla.org/datareporting/service;1"]
+                  .getService(Ci.nsISupports)
+                  .wrappedJSObject;
+      let drsClientID = yield drs.getClientID();
+
       let resetObjectState = function () {
         this._s = {
           // The payload version. This is bumped whenever there is a
           // backwards-incompatible change.
           v: 1,
           // The persistent client identifier.
-          clientID: CommonUtils.generateUUID(),
+          clientID: drsClientID,
           // Denotes the mechanism used to generate the client identifier.
           // 1: Random UUID.
           clientIDVersion: 1,
@@ -176,22 +181,7 @@ HealthReporterState.prototype = Object.freeze({
         // comes along and fixes us.
       }
 
-      let regen = false;
-      if (!this._s.clientID) {
-        this._log.warn("No client ID stored. Generating random ID.");
-        regen = true;
-      }
-
-      if (typeof(this._s.clientID) != "string") {
-        this._log.warn("Client ID is not a string. Regenerating.");
-        regen = true;
-      }
-
-      if (regen) {
-        this._s.clientID = CommonUtils.generateUUID();
-        this._s.clientIDVersion = 1;
-        yield this.save();
-      }
+      this._s.clientID = drsClientID;
 
       // Always look for preferences. This ensures that downgrades followed
       // by reupgrades don't result in excessive data loss.
@@ -255,21 +245,18 @@ HealthReporterState.prototype = Object.freeze({
 
   /**
    * Reset the client ID to something else.
-   *
-   * This fails if remote IDs are stored because changing the client ID
-   * while there is remote data will create orphaned records.
+   * Returns a promise that is resolved when completed.
    */
-  resetClientID: function () {
-    if (this.remoteIDs.length) {
-      throw new Error("Cannot reset client ID while remote IDs are stored.");
-    }
+  resetClientID: Task.async(function* () {
+    let drs = Cc["@mozilla.org/datareporting/service;1"]
+                .getService(Ci.nsISupports)
+                .wrappedJSObject;
+    yield drs.resetClientID();
+    this._s.clientID = yield drs.getClientID();
+    this._log.info("Reset client id to " + this._s.clientID + ".");
 
-    this._log.warn("Resetting client ID.");
-    this._s.clientID = CommonUtils.generateUUID();
-    this._s.clientIDVersion = 1;
-
-    return this.save();
-  },
+    yield this.save();
+  }),
 
   _migratePrefs: function () {
     let prefs = this._reporter._prefs;

services/healthreport/modules-testing/utils.jsm

@@ -182,6 +182,14 @@ InspectedHealthReporter.prototype = {
 const DUMMY_URI="http://localhost:62013/";
 
 this.getHealthReporter = function (name, uri=DUMMY_URI, inspected=false) {
+  // The healthreporters use the client id from the datareporting service,
+  // so we need to ensure it is initialized.
+  let drs = Cc["@mozilla.org/datareporting/service;1"]
+              .getService(Ci.nsISupports)
+              .wrappedJSObject;
+  drs.observe(null, "app-startup", null);
+  drs.observe(null, "profile-after-change", null);
+
   let branch = "healthreport.testing." + name + ".";
 
   let prefs = new Preferences(branch + "healthreport.");
@@ -193,12 +201,14 @@ this.getHealthReporter = function (name, uri=DUMMY_URI, inspected=false) {
   let policyPrefs = new Preferences(branch + "policy.");
   let listener = new MockPolicyListener();
   listener.onRequestDataUpload = function (request) {
-    reporter.requestDataUpload(request);
+    let promise = reporter.requestDataUpload(request);
     MockPolicyListener.prototype.onRequestDataUpload.call(this, request);
+    return promise;
   }
   listener.onRequestRemoteDelete = function (request) {
-    reporter.deleteRemoteData(request);
+    let promise = reporter.deleteRemoteData(request);
     MockPolicyListener.prototype.onRequestRemoteDelete.call(this, request);
+    return promise;
   }
   let policy = new DataReportingPolicy(policyPrefs, prefs, listener);
   let type = inspected ? InspectedHealthReporter : HealthReporter;

services/healthreport/tests/xpcshell/test_healthreporter.js

@@ -21,6 +21,12 @@ Cu.import("resource://testing-common/services/common/bagheeraserver.js");
 Cu.import("resource://testing-common/services/metrics/mocks.jsm");
 Cu.import("resource://testing-common/services/healthreport/utils.jsm");
 Cu.import("resource://testing-common/AppData.jsm");
+Cu.import("resource://gre/modules/XPCOMUtils.jsm");
+
+XPCOMUtils.defineLazyGetter(this, "gDatareportingService",
+  () => Cc["@mozilla.org/datareporting/service;1"]
+          .getService(Ci.nsISupports)
+          .wrappedJSObject);
 
 
 const DUMMY_URI = "http://localhost:62013/";
@@ -108,6 +114,13 @@ function ensureUserNotified (reporter) {
 }
 
 function run_test() {
+  do_get_profile();
+
+  // Send the needed startup notifications to the datareporting service
+  // to ensure that it has been initialized.
+  gDatareportingService.observe(null, "app-startup", null);
+  gDatareportingService.observe(null, "profile-after-change", null);
+
   run_next_test();
 }
 
@@ -745,7 +758,7 @@ add_task(function test_request_remote_data_deletion() {
     do_check_false(reporter.haveRemoteData());
     do_check_false(server.hasDocument(reporter.serverNamespace, id));
 
-    // Client ID should be updated.
+     // Client ID should be updated.
     do_check_neq(reporter._state.clientID, null);
     do_check_neq(reporter._state.clientID, clientID);
     do_check_eq(reporter._state.clientIDVersion, 1);
@@ -1171,38 +1184,6 @@ add_task(function test_state_downgrade_upgrade() {
   }
 });
 
-// Missing client ID in state should be created on state load.
-add_task(function* test_state_create_client_id() {
-  let reporter = getHealthReporter("state_create_client_id");
-
-  yield CommonUtils.writeJSON({
-    v: 1,
-    remoteIDs: ["id1", "id2"],
-    lastPingTime: Date.now(),
-    removeOutdatedLastPayload: true,
-  }, reporter._state._filename);
-
-  try {
-    yield reporter.init();
-
-    do_check_eq(reporter.lastSubmitID, "id1");
-    do_check_neq(reporter._state.clientID, null);
-    do_check_eq(reporter._state.clientID.length, 36);
-    do_check_eq(reporter._state.clientIDVersion, 1);
-
-    let clientID = reporter._state.clientID;
-
-    // The client ID should be persisted as soon as it is created.
-    reporter._shutdown();
-
-    reporter = getHealthReporter("state_create_client_id");
-    yield reporter.init();
-    do_check_eq(reporter._state.clientID, clientID);
-  } finally {
-    reporter._shutdown();
-  }
-});
-
 // Invalid stored client ID is reset automatically.
 add_task(function* test_empty_client_id() {
   let reporter = getHealthReporter("state_empty_client_id");

services/healthreport/tests/xpcshell/test_provider_appinfo.js

@@ -3,16 +3,29 @@
 
 "use strict";
 
-const {interfaces: Ci, results: Cr, utils: Cu} = Components;
+const {interfaces: Ci, results: Cr, utils: Cu, classes: Cc} = Components;
 
 Cu.import("resource://gre/modules/Metrics.jsm");
 Cu.import("resource://gre/modules/Services.jsm");
 Cu.import("resource://gre/modules/XPCOMUtils.jsm");
 Cu.import("resource://gre/modules/services/healthreport/providers.jsm");
 Cu.import("resource://testing-common/services/healthreport/utils.jsm");
+Cu.import("resource://gre/modules/XPCOMUtils.jsm");
+
+XPCOMUtils.defineLazyGetter(this, "gDatareportingService",
+  () => Cc["@mozilla.org/datareporting/service;1"]
+          .getService(Ci.nsISupports)
+          .wrappedJSObject);
 
 
 function run_test() {
+  do_get_profile();
+
+  // Send the needed startup notifications to the datareporting service
+  // to ensure that it has been initialized.
+  gDatareportingService.observe(null, "app-startup", null);
+  gDatareportingService.observe(null, "profile-after-change", null);
+
   run_next_test();
 }