Bug 843816 - Prevent duplicate recording of sessions in FHR when preference changes are lost. r=rnewman, a=akeybl
authorGregory Szorc <gps@mozilla.com>
Fri, 22 Feb 2013 16:02:27 -0800
changeset 132243 4fa0f989692c0cb6916d77ad926ef7824793f22d
parent 132242 a39dd26ed4f2bc8c10b46c5ca330597ed7da4ec1
child 132244 a9ce9efea742529d44f64cc6d3061daabf862c13
push id2323
push userbbajaj@mozilla.com
push dateMon, 01 Apr 2013 19:47:02 +0000
treeherdermozilla-beta@7712be144d91 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersrnewman, akeybl
bugs843816
milestone21.0a2
Bug 843816 - Prevent duplicate recording of sessions in FHR when preference changes are lost. r=rnewman, a=akeybl
services/healthreport/providers.jsm
services/healthreport/tests/xpcshell/test_provider_sessions.js
services/metrics/dataprovider.jsm
--- a/services/healthreport/providers.jsm
+++ b/services/healthreport/providers.jsm
@@ -497,27 +497,49 @@ SessionsProvider.prototype = Object.free
   _recordAndPruneSessions: function () {
     this._log.info("Moving previous sessions from session recorder to storage.");
     let recorder = this.healthReporter.sessionRecorder;
     let sessions = recorder.getPreviousSessions();
     this._log.debug("Found " + Object.keys(sessions).length + " previous sessions.");
 
     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);
       yield daily.addDailyDiscreteNumeric(type + "TotalTime", session.totalTime, date);
 
       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());
   },
 });
 
 /**
  * Stores the set of active addons in storage.
  *
  * We do things a little differently than most other measurements. Because
--- a/services/healthreport/tests/xpcshell/test_provider_sessions.js
+++ b/services/healthreport/tests/xpcshell/test_provider_sessions.js
@@ -133,33 +133,56 @@ add_task(function test_collect() {
   do_check_eq(day.size, 5);
 
   for (let field of ["cleanActiveTicks", "cleanTotalTime", "main", "firstPaint", "sessionRestored"]) {
     do_check_true(day.has(field));
     do_check_true(Array.isArray(day.get(field)));
     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();
   do_check_eq(Object.keys(recorder.getPreviousSessions()).length, 1);
   yield provider.collectConstantData();
   do_check_eq(Object.keys(recorder.getPreviousSessions()).length, 0);
 
   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);
   }
 
+  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();
-  recorder2.onShutdown();
 
   yield provider.shutdown();
   yield storage.close();
 });
 
 add_task(function test_serialization() {
   let [provider, storage, recorder] = yield getProvider("serialization");
 
--- a/services/metrics/dataprovider.jsm
+++ b/services/metrics/dataprovider.jsm
@@ -660,32 +660,43 @@ Provider.prototype = Object.freeze({
    */
   enqueueStorageOperation: function (func) {
     return this.storage.enqueueOperation(func);
   },
 
   /**
    * 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) {
     return Math.floor(date.getTime() / MILLISECONDS_PER_DAY);
   },
 
   _daysToDate: function (days) {
     return new Date(days * MILLISECONDS_PER_DAY);