Bug 843816 - Prevent duplicate recording of sessions in FHR when preference changes are lost; r=rnewman

services/healthreport/providers.jsm

@@ -484,7 +484,26 @@ SessionsProvider.prototype = Object.freeze({
 
     let daily = this.getMeasurement("previous", 3);
 
-    for each (let session in sessions) {
+    // Please note the coupling here between the session recorder and our state.
+    // If the pruned index or the current index of the session recorder is ever
+    // deleted or reset to 0, our stored state of a later index would mean that
+    // new sessions would never be captured by this provider until the session
+    // recorder index catches up to our last session ID. This should not happen
+    // under normal circumstances, so we don't worry too much about it. We
+    // should, however, consider this as part of implementing bug 841561.
+    let lastRecordedSession = yield this.getState("lastSession");
+    if (lastRecordedSession === null) {
+      lastRecordedSession = -1;
+    }
+    this._log.debug("The last recorded session was #" + lastRecordedSession);
+
+    for (let [index, session] in Iterator(sessions)) {
+      if (index < lastRecordedSession) {
+        this._log.warn("Already recorded session " + index + ". Did the last " +
+                       "session crash or have an issue saving the prefs file?");
+        continue;
+      }
+
       let type = session.clean ? "clean" : "aborted";
       let date = session.startDate;
       yield daily.addDailyDiscreteNumeric(type + "ActiveTicks", session.activeTicks, date);
@@ -493,8 +512,11 @@ SessionsProvider.prototype = Object.freeze({
       for (let field of ["main", "firstPaint", "sessionRestored"]) {
         yield daily.addDailyDiscreteNumeric(field, session[field], date);
       }
+
+      lastRecordedSession = index;
     }
 
+    yield this.setState("lastSession", "" + lastRecordedSession);
     recorder.pruneOldSessions(new Date());
   },
 });

services/healthreport/tests/xpcshell/test_provider_sessions.js

@@ -138,6 +138,9 @@ add_task(function test_collect() {
     do_check_eq(day.get(field).length, 6);
   }
 
+  let lastIndex = yield provider.getState("lastSession");
+  do_check_eq(lastIndex, "5"); // 0-indexed so this is really 6.
+
   // Fake an aborted sessions.
   let recorder2 = new SessionRecorder("testing.collect.sessions.");
   recorder2.onStartup();
@@ -147,15 +150,35 @@ add_task(function test_collect() {
 
   values = yield daily.getValues();
   day = values.days.getDay(now);
+  do_check_eq(day.size, 7);
   for (let field of ["abortedActiveTicks", "abortedTotalTime"]) {
     do_check_true(day.has(field));
     do_check_true(Array.isArray(day.get(field)));
     do_check_eq(day.get(field).length, 1);
   }
 
-  recorder.onShutdown();
+  lastIndex = yield provider.getState("lastSession");
+  do_check_eq(lastIndex, "6");
+
   recorder2.onShutdown();
 
+  // If we try to insert a lower-numbered session, it will be ignored.
+  let recorder3 = new SessionRecorder("testing.collect.sessions.");
+  recorder3._currentIndex = recorder2._currentIndex - 4;
+  recorder3._prunedIndex = recorder3._currentIndex;
+  recorder3.onStartup();
+  // Session is left over from recorder2.
+  do_check_eq(Object.keys(recorder.getPreviousSessions()).length, 1);
+  yield provider.collectConstantData();
+  lastIndex = yield provider.getState("lastSession");
+  do_check_eq(lastIndex, "6");
+  values = yield daily.getValues();
+  day = values.days.getDay(now);
+  do_check_eq(day.size, 7); // We should not get additional entry.
+  recorder3.onShutdown();
+
+  recorder.onShutdown();
+
   yield provider.shutdown();
   yield storage.close();
 });

services/metrics/dataprovider.jsm

@@ -649,22 +649,33 @@ Provider.prototype = Object.freeze({
   /**
    * Obtain persisted provider state.
    *
-   * State is backend by storage.
+   * Provider state consists of key-value pairs of string names and values.
+   * Providers can stuff whatever they want into state. They are encouraged to
+   * store as little as possible for performance reasons.
+   *
+   * State is backed by storage and is robust.
+   *
+   * These functions do not enqueue on storage automatically, so they should
+   * be guarded by `enqueueStorageOperation` or some other mutex.
+   *
+   * @param key
+   *        (string) The property to retrieve.
+   *
+   * @return Promise<string|null> String value on success. null if no state
+   *         is available under this key.
    */
   getState: function (key) {
-    let name = this.name;
-    let storage = this.storage;
-    return storage.enqueueOperation(function get() {
-      return storage.getProviderState(name, key);
-    });
+    return this.storage.getProviderState(this.name, key);
   },
 
+  /**
+   * Set state for this provider.
+   *
+   * This is the complementary API for `getState` and obeys the same
+   * storage restrictions.
+   */
   setState: function (key, value) {
-    let name = this.name;
-    let storage = this.storage;
-    return storage.enqueueOperation(function set() {
-      return storage.setProviderState(name, key, value);
-    });
+    return this.storage.setProviderState(this.name, key, value);
   },
 
   _dateToDays: function (date) {