Bug 846133 - Store FHR state in a file; r=rnewman

Preferences aren't robust. So, we're using a file.

services/common/bagheeraclient.js

@@ -161,7 +161,11 @@ BagheeraClient.prototype = Object.freeze({
     request.loadFlags = this._loadFlags;
     request.timeout = this.DEFAULT_TIMEOUT_MSEC;
 
+    let deleteID;
+
     if (options.deleteID) {
+      deleteID = options.deleteID;
+      this._log.debug("Will delete " + deleteID);
       request.setHeader("X-Obsolete-Document", options.deleteID);
     }
 
@@ -186,6 +190,7 @@ BagheeraClient.prototype = Object.freeze({
     let result = new BagheeraClientRequestResult();
     result.namespace = namespace;
     result.id = id;
+    result.deleteID = deleteID;
 
     request.onComplete = this._onComplete.bind(this, request, deferred, result);
     request.post(data);

services/datareporting/DataReportingService.js

@@ -258,14 +258,13 @@ DataReportingService.prototype = Object.freeze({
       }
     }
 
-    // The reporter initializes in the background.
     this._healthReporter = new ns.HealthReporter(HEALTHREPORT_BRANCH,
                                                  this.policy,
                                                  this.sessionRecorder);
 
     // Wait for initialization to finish so if a shutdown occurs before init
     // has finished we don't adversely affect app startup on next run.
-    this._healthReporter.onInit().then(function onInit() {
+    this._healthReporter.init().then(function onInit() {
       this._prefs.set("service.firstRun", true);
     }.bind(this));
   },

services/healthreport/healthreporter.jsm

@@ -54,6 +54,188 @@ const TELEMETRY_COLLECT_DAILY = "HEALTHREPORT_COLLECT_DAILY_MS";
 const TELEMETRY_SHUTDOWN = "HEALTHREPORT_SHUTDOWN_MS";
 const TELEMETRY_COLLECT_CHECKPOINT = "HEALTHREPORT_POST_COLLECT_CHECKPOINT_MS";
 
+
+/**
+ * Helper type to assist with management of Health Reporter state.
+ *
+ * Instances are not meant to be created outside of a HealthReporter instance.
+ *
+ * Please note that remote IDs are treated as a queue. When a new remote ID is
+ * added, it goes at the back of the queue. When we look for the current ID, we
+ * pop the ID at the front of the queue. This helps ensure that all IDs on the
+ * server are eventually deleted. This should eventually become irrelevant once
+ * the server supports multiple ID deletion.
+ */
+function HealthReporterState(reporter) {
+  this._reporter = reporter;
+
+  let profD = OS.Constants.Path.profileDir;
+
+  if (!profD || !profD.length) {
+    throw new Error("Could not obtain profile directory. OS.File not " +
+                    "initialized properly?");
+  }
+
+  this._log = reporter._log;
+
+  this._stateDir = OS.Path.join(profD, "healthreport");
+
+  // To facilitate testing.
+  let leaf = reporter._stateLeaf || "state.json";
+
+  this._filename = OS.Path.join(this._stateDir, leaf);
+  this._log.debug("Storing state in " + this._filename);
+  this._s = null;
+}
+
+HealthReporterState.prototype = Object.freeze({
+  get lastPingDate() {
+    return new Date(this._s.lastPingTime);
+  },
+
+  get lastSubmitID() {
+    return this._s.remoteIDs[0];
+  },
+
+  get remoteIDs() {
+    return this._s.remoteIDs;
+  },
+
+  init: function () {
+    return Task.spawn(function init() {
+      try {
+        OS.File.makeDir(this._stateDir);
+      } catch (ex if ex instanceof OS.FileError) {
+        if (!ex.becauseExists) {
+          throw ex;
+        }
+      }
+
+      let resetObjectState = function () {
+        this._s = {
+          v: 1,
+          remoteIDs: [],
+          lastPingTime: 0,
+        };
+      }.bind(this);
+
+      try {
+        this._s = yield CommonUtils.readJSON(this._filename);
+      } catch (ex if ex instanceof OS.File.Error) {
+        if (!ex.becauseNoSuchFile) {
+          throw ex;
+        }
+
+        this._log.warn("Saved state file does not exist.");
+        resetObjectState();
+      } catch (ex) {
+        this._log.error("Exception when reading state from disk: " +
+                        CommonUtils.exceptionStr(ex));
+        resetObjectState();
+
+        // Don't save in case it goes away on next run.
+      }
+
+      if (typeof(this._s) != "object") {
+        this._log.warn("Read state is not an object. Resetting state.");
+        this._s = {
+          v: 1,
+          remoteIDs: [],
+          lastPingTime: 0,
+        };
+        yield this.save();
+      }
+
+      if (this._s.v != 1) {
+        this._log.warn("Unknown version in state file: " + this._s.v);
+        this._s = {
+          v: 1,
+          remoteIDs: [],
+          lastPingTime: 0,
+        };
+        // We explicitly don't save here in the hopes an application re-upgrade
+        // comes along and fixes us.
+      }
+
+      // Always look for preferences. This ensures that downgrades followed
+      // by reupgrades don't result in excessive data loss.
+      for (let promise of this._migratePrefs()) {
+        yield promise;
+      }
+    }.bind(this));
+  },
+
+  save: function () {
+    this._log.info("Writing state file: " + this._filename);
+    return CommonUtils.writeJSON(this._s, this._filename);
+  },
+
+  addRemoteID: function (id) {
+    this._log.warn("Recording new remote ID: " + id);
+    this._s.remoteIDs.push(id);
+    return this.save();
+  },
+
+  removeRemoteID: function (id) {
+    this._log.warn("Removing document from remote ID list: " + id);
+    let filtered = this._s.remoteIDs.filter((x) => x != id);
+
+    if (filtered.length == this._s.remoteIDs.length) {
+      return Promise.resolve();
+    }
+
+    this._s.remoteIDs = filtered;
+    return this.save();
+  },
+
+  setLastPingDate: function (date) {
+    this._s.lastPingTime = date.getTime();
+
+    return this.save();
+  },
+
+  updateLastPingAndRemoveRemoteID: function (date, id) {
+    if (!id) {
+      return this.setLastPingDate(date);
+    }
+
+    this._log.info("Recording last ping time and deleted remote document.");
+    this._s.lastPingTime = date.getTime();
+    return this.removeRemoteID(id);
+  },
+
+  _migratePrefs: function () {
+    let prefs = this._reporter._prefs;
+
+    let lastID = prefs.get("lastSubmitID", null);
+    let lastPingDate = CommonUtils.getDatePref(prefs, "lastPingTime",
+                                               0, this._log, OLDEST_ALLOWED_YEAR);
+
+    // If we have state from prefs, migrate and save it to a file then clear
+    // out old prefs.
+    if (lastID || (lastPingDate && lastPingDate.getTime() > 0)) {
+      this._log.warn("Migrating saved state from preferences.");
+
+      if (lastID) {
+        this._log.info("Migrating last saved ID: " + lastID);
+        this._s.remoteIDs.push(lastID);
+      }
+
+      let ourLast = this.lastPingDate;
+
+      if (lastPingDate && lastPingDate.getTime() > ourLast.getTime()) {
+        this._log.info("Migrating last ping time: " + lastPingDate);
+        this._s.lastPingTime = lastPingDate.getTime();
+      }
+
+      yield this.save();
+      prefs.reset(["lastSubmitID", "lastPingTime"]);
+    } else {
+      this._log.warn("No prefs data found.");
+    }
+  },
+});
+
 /**
  * This is the abstract base class of `HealthReporter`. It exists so that
  * we can sanely divide work on platforms where control of Firefox Health
@@ -83,6 +265,7 @@ function AbstractHealthReporter(branch, policy, sessionRecorder) {
   this._storageInProgress = false;
   this._providerManager = null;
   this._providerManagerInProgress = false;
+  this._initializeStarted = false;
   this._initialized = false;
   this._initializeHadError = false;
   this._initializedDeferred = Promise.defer();
@@ -99,18 +282,6 @@ function AbstractHealthReporter(branch, policy, sessionRecorder) {
   let hasFirstRun = this._prefs.get("service.firstRun", false);
   this._initHistogram = hasFirstRun ? TELEMETRY_INIT : TELEMETRY_INIT_FIRSTRUN;
   this._dbOpenHistogram = hasFirstRun ? TELEMETRY_DB_OPEN : TELEMETRY_DB_OPEN_FIRSTRUN;
-
-  TelemetryStopwatch.start(this._initHistogram, this);
-
-  // As soon as we could have storage, we need to register cleanup or
-  // else bad things (like hangs) happen on shutdown.
-  Services.obs.addObserver(this, "quit-application", false);
-  Services.obs.addObserver(this, "profile-before-change", false);
-
-  this._storageInProgress = true;
-  TelemetryStopwatch.start(this._dbOpenHistogram, this);
-  Metrics.Storage(this._dbName).then(this._onStorageCreated.bind(this),
-                                     this._onInitError.bind(this));
 }
 
 AbstractHealthReporter.prototype = Object.freeze({
@@ -125,6 +296,27 @@ AbstractHealthReporter.prototype = Object.freeze({
     return this._initialized;
   },
 
+  /**
+   * Initialize the instance.
+   *
+   * This must be called once after object construction or the instance is
+   * useless.
+   */
+  init: function () {
+    if (this._initializeStarted) {
+      throw new Error("We have already started initialization.");
+    }
+
+    this._initializeStarted = true;
+
+    TelemetryStopwatch.start(this._initHistogram, this);
+
+    this._initializeState().then(this._onStateInitialized.bind(this),
+                                 this._onInitError.bind(this));
+
+    return this.onInit();
+  },
+
   //----------------------------------------------------
   // SERVICE CONTROL FUNCTIONS
   //
@@ -146,6 +338,23 @@ AbstractHealthReporter.prototype = Object.freeze({
     // useful error message.
   },
 
+  _initializeState: function () {
+    return Promise.resolve();
+  },
+
+  _onStateInitialized: function () {
+    // As soon as we have could storage, we need to register cleanup or
+    // else bad things happen on shutdown.
+    Services.obs.addObserver(this, "quit-application", false);
+    Services.obs.addObserver(this, "profile-before-change", false);
+
+    this._storageInProgress = true;
+    TelemetryStopwatch.start(this._dbOpenHistogram, this);
+
+    Metrics.Storage(this._dbName).then(this._onStorageCreated.bind(this),
+                                       this._onInitError.bind(this));
+  },
+
   // Called when storage has been opened.
   _onStorageCreated: function (storage) {
     TelemetryStopwatch.finish(this._dbOpenHistogram, this);
@@ -775,38 +984,6 @@ AbstractHealthReporter.prototype = Object.freeze({
     throw new Task.Result(o);
   },
 
-  get _stateDir() {
-    let profD = OS.Constants.Path.profileDir;
-
-    // Work around bug 810543 until OS.File is more resilient.
-    if (!profD || !profD.length) {
-      throw new Error("Could not obtain profile directory. OS.File not " +
-                      "initialized properly?");
-    }
-
-    return OS.Path.join(profD, "healthreport");
-  },
-
-  _ensureDirectoryExists: function (path) {
-    let deferred = Promise.defer();
-
-    OS.File.makeDir(path).then(
-      function onResult() {
-        deferred.resolve(true);
-      },
-      function onError(error) {
-        if (error.becauseExists) {
-          deferred.resolve(true);
-          return;
-        }
-
-        deferred.reject(error);
-      }
-    );
-
-    return deferred.promise;
-  },
-
   _now: function _now() {
     return new Date();
   },
@@ -919,7 +1096,9 @@ AbstractHealthReporter.prototype = Object.freeze({
  * @param policy
  *        (HealthReportPolicy) Policy driving execution of HealthReporter.
  */
-function HealthReporter(branch, policy, sessionRecorder) {
+function HealthReporter(branch, policy, sessionRecorder, stateLeaf=null) {
+  this._stateLeaf = stateLeaf;
+
   AbstractHealthReporter.call(this, branch, policy, sessionRecorder);
 
   if (!this.serverURI) {
@@ -929,6 +1108,8 @@ function HealthReporter(branch, policy, sessionRecorder) {
   if (!this.serverNamespace) {
     throw new Error("No server namespace defined. Did you forget a pref?");
   }
+
+  this._state = new HealthReporterState(this);
 }
 
 HealthReporter.prototype = Object.freeze({
@@ -936,6 +1117,10 @@ HealthReporter.prototype = Object.freeze({
 
   QueryInterface: XPCOMUtils.generateQI([Ci.nsIObserver]),
 
+  get lastSubmitID() {
+    return this._state.lastSubmitID;
+  },
+
   /**
    * When we last successfully submitted data to the server.
    *
@@ -944,13 +1129,7 @@ HealthReporter.prototype = Object.freeze({
    * similar data in the policy is only used for forensic purposes.
    */
   get lastPingDate() {
-    return CommonUtils.getDatePref(this._prefs, "lastPingTime", 0, this._log,
-                                   OLDEST_ALLOWED_YEAR);
-  },
-
-  set lastPingDate(value) {
-    CommonUtils.setDatePref(this._prefs, "lastPingTime", value,
-                            OLDEST_ALLOWED_YEAR);
+    return this._state.lastPingDate;
   },
 
   /**
@@ -994,23 +1173,6 @@ HealthReporter.prototype = Object.freeze({
     this._prefs.set("documentServerNamespace", value);
   },
 
-  /**
-   * The document ID for data to be submitted to the server.
-   *
-   * This should be a UUID.
-   *
-   * We generate a new UUID when we upload data to the server. When we get a
-   * successful response for that upload, we record that UUID in this value.
-   * On the subsequent upload, this ID will be deleted from the server.
-   */
-  get lastSubmitID() {
-    return this._prefs.get("lastSubmitID", null) || null;
-  },
-
-  set lastSubmitID(value) {
-    this._prefs.set("lastSubmitID", value || "");
-  },
-
   /**
    * Whether this instance will upload data to a server.
    */
@@ -1025,7 +1187,7 @@ HealthReporter.prototype = Object.freeze({
    * @return bool
    */
   haveRemoteData: function () {
-    return !!this.lastSubmitID;
+    return !!this._state.lastSubmitID;
   },
 
   /**
@@ -1062,13 +1224,17 @@ HealthReporter.prototype = Object.freeze({
    * deleted.
    */
   requestDeleteRemoteData: function (reason) {
-    if (!this.lastSubmitID) {
+    if (!this.haveRemoteData()) {
       return;
     }
 
     return this._policy.deleteRemoteData(reason);
   },
 
+  _initializeState: function() {
+    return this._state.init();
+  },
+
   /**
    * Override default handler to incur an upload describing the error.
    */
@@ -1110,53 +1276,45 @@ HealthReporter.prototype = Object.freeze({
   _onBagheeraResult: function (request, isDelete, date, result) {
     this._log.debug("Received Bagheera result.");
 
-    let promise = CommonUtils.laterTickResolvingPromise(null);
-    let hrProvider = this.getProvider("org.mozilla.healthreport");
+    return Task.spawn(function onBagheeraResult() {
+      let hrProvider = this.getProvider("org.mozilla.healthreport");
 
-    if (!result.transportSuccess) {
-      // The built-in provider may not be initialized if this instance failed
-      // to initialize fully.
-      if (hrProvider && !isDelete) {
-        hrProvider.recordEvent("uploadTransportFailure", date);
+      if (!result.transportSuccess) {
+        // The built-in provider may not be initialized if this instance failed
+        // to initialize fully.
+        if (hrProvider && !isDelete) {
+          hrProvider.recordEvent("uploadTransportFailure", date);
+        }
+
+        request.onSubmissionFailureSoft("Network transport error.");
+        throw new Task.Result(false);
       }
 
-      request.onSubmissionFailureSoft("Network transport error.");
-      return promise;
-    }
+      if (!result.serverSuccess) {
+        if (hrProvider && !isDelete) {
+          hrProvider.recordEvent("uploadServerFailure", date);
+        }
 
-    if (!result.serverSuccess) {
-      if (hrProvider && !isDelete) {
-        hrProvider.recordEvent("uploadServerFailure", date);
+        request.onSubmissionFailureHard("Server failure.");
+        throw new Task.Result(false);
       }
 
-      request.onSubmissionFailureHard("Server failure.");
-      return promise;
-    }
+      if (hrProvider && !isDelete) {
+        hrProvider.recordEvent("uploadSuccess", date);
+      }
 
-    if (hrProvider && !isDelete) {
-      hrProvider.recordEvent("uploadSuccess", date);
-    }
+      if (isDelete) {
+        this._log.warn("Marking delete as successful.");
+        yield this._state.removeRemoteID(result.id);
+      } else {
+        this._log.warn("Marking upload as successful.");
+        yield this._state.updateLastPingAndRemoveRemoteID(date, result.deleteID);
+      }
 
-    if (isDelete) {
-      this.lastSubmitID = null;
-    } else {
-      this.lastSubmitID = result.id;
-      this.lastPingDate = date;
-    }
+      request.onSubmissionSuccess(this._now());
 
-    request.onSubmissionSuccess(this._now());
-
-#ifndef RELEASE_BUILD
-    // Intended to be temporary until we a) assess the impact b) bug 846133
-    // deploys more robust storage for state.
-    try {
-      Services.prefs.savePrefFile(null);
-    } catch (ex) {
-      this._log.warn("Error forcing prefs save: " + CommonUtils.exceptionStr(ex));
-    }
-#endif
-
-    return promise;
+      throw new Task.Result(true);
+    }.bind(this));
   },
 
   _onSubmitDataRequestFailure: function (error) {
@@ -1183,10 +1341,13 @@ HealthReporter.prototype = Object.freeze({
       let histogram = Services.telemetry.getHistogramById(TELEMETRY_PAYLOAD_SIZE_UNCOMPRESSED);
       histogram.add(payload.length);
 
+      let lastID = this.lastSubmitID;
+      yield this._state.addRemoteID(id);
+
       let hrProvider = this.getProvider("org.mozilla.healthreport");
       if (hrProvider) {
-        let event = this.lastSubmitID ? "continuationUploadAttempt"
-                                      : "firstDocumentUploadAttempt";
+        let event = lastID ? "continuationUploadAttempt"
+                           : "firstDocumentUploadAttempt";
         hrProvider.recordEvent(event, now);
       }
 
@@ -1194,7 +1355,7 @@ HealthReporter.prototype = Object.freeze({
       let result;
       try {
         let options = {
-          deleteID: this.lastSubmitID,
+          deleteID: lastID,
           telemetryCompressed: TELEMETRY_PAYLOAD_SIZE_COMPRESSED,
         };
         result = yield client.uploadJSON(this.serverNamespace, id, payload,
@@ -1219,7 +1380,7 @@ HealthReporter.prototype = Object.freeze({
    *        (DataSubmissionRequest) Tracks progress of this request.
    */
   deleteRemoteData: function (request) {
-    if (!this.lastSubmitID) {
+    if (!this._state.lastSubmitID) {
       this._log.info("Received request to delete remote data but no data stored.");
       request.onNoDataAvailable();
       return;

services/healthreport/modules-testing/utils.jsm

@@ -216,8 +216,8 @@ this.createFakeCrash = function (submitted=false, date=new Date()) {
  *
  * The purpose of this type is to aid testing of startup and shutdown.
  */
-this.InspectedHealthReporter = function (branch, policy) {
-  HealthReporter.call(this, branch, policy);
+this.InspectedHealthReporter = function (branch, policy, recorder, stateLeaf) {
+  HealthReporter.call(this, branch, policy, recorder, stateLeaf);
 
   this.onStorageCreated = null;
   this.onProviderManagerInitialized = null;

services/healthreport/tests/xpcshell/test_healthreporter.js

@@ -6,10 +6,12 @@
 const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
 
 Cu.import("resource://services-common/observers.js");
+Cu.import("resource://services-common/utils.js");
 Cu.import("resource://gre/modules/commonjs/sdk/core/promise.js");
 Cu.import("resource://gre/modules/Metrics.jsm");
+Cu.import("resource://gre/modules/osfile.jsm");
 Cu.import("resource://gre/modules/Preferences.jsm");
-Cu.import("resource://gre/modules/services/healthreport/healthreporter.jsm");
+let bsp = Cu.import("resource://gre/modules/services/healthreport/healthreporter.jsm");
 Cu.import("resource://gre/modules/services/healthreport/providers.jsm");
 Cu.import("resource://gre/modules/services/datareporting/policy.jsm");
 Cu.import("resource://gre/modules/Services.jsm");
@@ -25,6 +27,8 @@ const SERVER_PORT = 8080;
 const SERVER_URI = "http://" + SERVER_HOSTNAME + ":" + SERVER_PORT;
 const MILLISECONDS_PER_DAY = 24 * 60 * 60 * 1000;
 
+const HealthReporterState = bsp.HealthReporterState;
+
 
 function defineNow(policy, now) {
   print("Adjusting fake system clock to " + now);
@@ -59,7 +63,9 @@ function getJustReporter(name, uri=SERVER_URI, inspected=false) {
   });
 
   let type = inspected ? InspectedHealthReporter : HealthReporter;
-  reporter = new type(branch + "healthreport.", policy);
+  // Define per-instance state file so tests don't interfere with each other.
+  reporter = new type(branch + "healthreport.", policy, null,
+                      "state-" + name + ".json");
 
   return reporter;
 }
@@ -67,7 +73,7 @@ function getJustReporter(name, uri=SERVER_URI, inspected=false) {
 function getReporter(name, uri, inspected) {
   return Task.spawn(function init() {
     let reporter = getJustReporter(name, uri, inspected);
-    yield reporter.onInit();
+    yield reporter.init();
 
     yield reporter._providerManager.registerProviderFromType(
       HealthReportProvider);
@@ -127,11 +133,9 @@ add_task(function test_constructor() {
   try {
     do_check_eq(reporter.lastPingDate.getTime(), 0);
     do_check_null(reporter.lastSubmitID);
-
-    reporter.lastSubmitID = "foo";
-    do_check_eq(reporter.lastSubmitID, "foo");
-    reporter.lastSubmitID = null;
-    do_check_null(reporter.lastSubmitID);
+    do_check_eq(typeof(reporter._state), "object");
+    do_check_eq(reporter._state.lastPingDate.getTime(), 0);
+    do_check_eq(reporter._state.remoteIDs.length, 0);
 
     let failed = false;
     try {
@@ -165,6 +169,8 @@ add_task(function test_shutdown_storage_in_progress() {
     reporter._initiateShutdown();
   };
 
+  reporter.init();
+
   reporter._waitForShutdown();
   do_check_eq(reporter.providerManagerShutdownCount, 0);
   do_check_eq(reporter.storageCloseCount, 1);
@@ -181,6 +187,8 @@ add_task(function test_shutdown_provider_manager_in_progress() {
     reporter._initiateShutdown();
   };
 
+  reporter.init();
+
   // This will hang if shutdown logic is busted.
   reporter._waitForShutdown();
   do_check_eq(reporter.providerManagerShutdownCount, 1);
@@ -197,6 +205,8 @@ add_task(function test_shutdown_when_provider_manager_errors() {
     throw new Error("Fake error during provider manager initialization.");
   };
 
+  reporter.init();
+
   // This will hang if shutdown logic is busted.
   reporter._waitForShutdown();
   do_check_eq(reporter.providerManagerShutdownCount, 1);
@@ -285,7 +295,8 @@ add_task(function test_json_payload_simple() {
     do_check_eq(Object.keys(original.data.days).length, 0);
     do_check_false("notInitialized" in original);
 
-    reporter.lastPingDate = new Date(now.getTime() - 24 * 60 * 60 * 1000 - 10);
+    yield reporter._state.setLastPingDate(
+      new Date(now.getTime() - 24 * 60 * 60 * 1000 - 10));
 
     original = JSON.parse(yield reporter.getJSONPayload());
     do_check_eq(original.lastPingDate, reporter._formatDate(reporter.lastPingDate));
@@ -608,6 +619,16 @@ add_task(function test_data_submission_success() {
     do_check_eq(data.uploadSuccess, 1);
     do_check_eq(Object.keys(data).length, 3);
 
+    let d = reporter.lastPingDate;
+    let id = reporter.lastSubmitID;
+
+    reporter._shutdown();
+
+    // Ensure reloading state works.
+    reporter = yield getReporter("data_submission_success");
+    do_check_eq(reporter.lastSubmitID, id);
+    do_check_eq(reporter.lastPingDate.getTime(), d.getTime());
+
     reporter._shutdown();
   } finally {
     yield shutdownServer(server);
@@ -761,8 +782,9 @@ add_task(function test_collect_when_upload_disabled() {
   let name = "healthreport-testing-collect_when_upload_disabled-healthreport-lastDailyCollection";
   let pref = "app.update.lastUpdateTime." + name;
   do_check_false(Services.prefs.prefHasUserValue(pref));
+
   try {
-    yield reporter.onInit();
+    yield reporter.init();
     do_check_true(Services.prefs.prefHasUserValue(pref));
 
     // We would ideally ensure the timer fires and does the right thing.
@@ -828,7 +850,7 @@ add_task(function test_upload_on_init_failure() {
   reporter._policy.recordUserAcceptance();
   let error = false;
   try {
-    yield reporter.onInit();
+    yield reporter.init();
   } catch (ex) {
     error = true;
   } finally {
@@ -852,3 +874,161 @@ add_task(function test_upload_on_init_failure() {
   yield shutdownServer(server);
 });
 
+add_task(function test_state_prefs_conversion_simple() {
+  let reporter = getJustReporter("state_prefs_conversion");
+  let prefs = reporter._prefs;
+
+  let lastSubmit = new Date();
+  prefs.set("lastSubmitID", "lastID");
+  CommonUtils.setDatePref(prefs, "lastPingTime", lastSubmit);
+
+  try {
+    yield reporter.init();
+
+    do_check_eq(reporter._state.lastSubmitID, "lastID");
+    do_check_eq(reporter._state.remoteIDs.length, 1);
+    do_check_eq(reporter._state.lastPingDate.getTime(), lastSubmit.getTime());
+    do_check_eq(reporter._state.lastPingDate.getTime(), reporter.lastPingDate.getTime());
+    do_check_eq(reporter._state.lastSubmitID, reporter.lastSubmitID);
+    do_check_true(reporter.haveRemoteData());
+
+    // User set preferences should have been wiped out.
+    do_check_false(prefs.isSet("lastSubmitID"));
+    do_check_false(prefs.isSet("lastPingTime"));
+  } finally {
+    reporter._shutdown();
+  }
+});
+
+// If the saved JSON file does not contain an object, we should reset
+// automatically.
+add_task(function test_state_no_json_object() {
+  let reporter = getJustReporter("state_shared");
+  yield CommonUtils.writeJSON("hello", reporter._state._filename);
+
+  try {
+    yield reporter.init();
+
+    do_check_eq(reporter.lastPingDate.getTime(), 0);
+    do_check_null(reporter.lastSubmitID);
+
+    let o = yield CommonUtils.readJSON(reporter._state._filename);
+    do_check_eq(typeof(o), "object");
+    do_check_eq(o.v, 1);
+    do_check_eq(o.lastPingTime, 0);
+    do_check_eq(o.remoteIDs.length, 0);
+  } finally {
+    reporter._shutdown();
+  }
+});
+
+// If we encounter a future version, we reset state to the current version.
+add_task(function test_state_future_version() {
+  let reporter = getJustReporter("state_shared");
+  yield CommonUtils.writeJSON({v: 2, remoteIDs: ["foo"], lastPingTime: 2412},
+                              reporter._state._filename);
+  try {
+    yield reporter.init();
+
+    do_check_eq(reporter.lastPingDate.getTime(), 0);
+    do_check_null(reporter.lastSubmitID);
+
+    // While the object is updated, we don't save the file.
+    let o = yield CommonUtils.readJSON(reporter._state._filename);
+    do_check_eq(o.v, 2);
+    do_check_eq(o.lastPingTime, 2412);
+    do_check_eq(o.remoteIDs.length, 1);
+  } finally {
+    reporter._shutdown();
+  }
+});
+
+// Test recovery if the state file contains invalid JSON.
+add_task(function test_state_invalid_json() {
+  let reporter = getJustReporter("state_shared");
+
+  let encoder = new TextEncoder();
+  let arr = encoder.encode("{foo: bad value, 'bad': as2,}");
+  let path = reporter._state._filename;
+  yield OS.File.writeAtomic(path, arr, {tmpPath: path + ".tmp"});
+
+  try {
+    yield reporter.init();
+
+    do_check_eq(reporter.lastPingDate.getTime(), 0);
+    do_check_null(reporter.lastSubmitID);
+  } finally {
+    reporter._shutdown();
+  }
+});
+
+add_task(function test_state_multiple_remote_ids() {
+  let [reporter, server] = yield getReporterAndServer("state_multiple_remote_ids");
+
+  let now = new Date(Date.now() - 5000);
+
+  server.setDocument(reporter.serverNamespace, "id1", "foo");
+  server.setDocument(reporter.serverNamespace, "id2", "bar");
+
+  try {
+    yield reporter._state.addRemoteID("id1");
+    yield reporter._state.addRemoteID("id2");
+    yield reporter._state.setLastPingDate(now);
+    do_check_eq(reporter._state.remoteIDs.length, 2);
+    do_check_eq(reporter._state.remoteIDs[0], "id1");
+    do_check_eq(reporter._state.remoteIDs[1], "id2");
+    do_check_eq(reporter.lastSubmitID, "id1");
+
+    let deferred = Promise.defer();
+    let request = new DataSubmissionRequest(deferred, now);
+    reporter.requestDataUpload(request);
+    yield deferred.promise;
+
+    do_check_eq(reporter._state.remoteIDs.length, 2);
+    do_check_eq(reporter._state.remoteIDs[0], "id2");
+    do_check_true(reporter.lastPingDate.getTime() > now.getTime());
+    do_check_false(server.hasDocument(reporter.serverNamespace, "id1"));
+    do_check_true(server.hasDocument(reporter.serverNamespace, "id2"));
+
+    let o = CommonUtils.readJSON(reporter._state._filename);
+    do_check_eq(reporter._state.remoteIDs.length, 2);
+    do_check_eq(reporter._state.remoteIDs[0], "id2");
+    do_check_eq(reporter._state.remoteIDs[1], reporter._state.remoteIDs[1]);
+  } finally {
+    yield shutdownServer(server);
+    reporter._shutdown();
+  }
+});
+
+// If we have a state file then downgrade to prefs, the prefs should be
+// reimported and should supplement existing state.
+add_task(function test_state_downgrade_upgrade() {
+  let reporter = getJustReporter("state_shared");
+
+  let now = new Date();
+
+  yield CommonUtils.writeJSON({v: 1, remoteIDs: ["id1", "id2"], lastPingTime: now.getTime()},
+                              reporter._state._filename);
+
+  let prefs = reporter._prefs;
+  prefs.set("lastSubmitID", "prefID");
+  prefs.set("lastPingTime", "" + (now.getTime() + 1000));
+
+  try {
+    yield reporter.init();
+
+    do_check_eq(reporter.lastSubmitID, "id1");
+    do_check_eq(reporter._state.remoteIDs.length, 3);
+    do_check_eq(reporter._state.remoteIDs[2], "prefID");
+    do_check_eq(reporter.lastPingDate.getTime(), now.getTime() + 1000);
+    do_check_false(prefs.isSet("lastSubmitID"));
+    do_check_false(prefs.isSet("lastPingTime"));
+
+    let o = yield CommonUtils.readJSON(reporter._state._filename);
+    do_check_eq(o.remoteIDs.length, 3);
+    do_check_eq(o.lastPingTime, now.getTime() + 1000);
+  } finally {
+    reporter._shutdown();
+  }
+});
+