Bug 881991 - Properly detect already inserted sessions; r=rnewman

--HG--
extra : rebase_source : 2655dcc1230f05d37c3f43c1d2c4fb22768bd1f5

services/healthreport/providers.jsm

@@ -611,7 +611,7 @@ SessionsProvider.prototype = Object.freeze({
     this._log.debug("The last recorded session was #" + lastRecordedSession);
 
     for (let [index, session] in Iterator(sessions)) {
-      if (index < lastRecordedSession) {
+      if (index <= lastRecordedSession) {
         this._log.warn("Already recorded session " + index + ". Did the last " +
                        "session crash or have an issue saving the prefs file?");
         continue;

services/healthreport/tests/xpcshell/test_provider_sessions.js

@@ -118,17 +118,21 @@ add_task(function test_collect() {
   recorder = new SessionRecorder("testing.collect.sessions.");
   recorder.onStartup();
 
-  yield provider.collectConstantData();
+  // Collecting the provider should prune all previous sessions.
   let sessions = recorder.getPreviousSessions();
+  do_check_eq(Object.keys(sessions).length, 6);
+  yield provider.collectConstantData();
+  sessions = recorder.getPreviousSessions();
   do_check_eq(Object.keys(sessions).length, 0);
 
+  // And those previous sessions should make it to storage.
   let daily = provider.getMeasurement("previous", 3);
   let values = yield daily.getValues();
   do_check_true(values.days.hasDay(now));
   do_check_eq(values.days.size, 1);
-
   let day = values.days.getDay(now);
   do_check_eq(day.size, 5);
+  let previousStorageCount = day.get("main").length;
 
   for (let field of ["cleanActiveTicks", "cleanTotalTime", "main", "firstPaint", "sessionRestored"]) {
     do_check_true(day.has(field));
@@ -137,9 +141,11 @@ add_task(function test_collect() {
   }
 
   let lastIndex = yield provider.getState("lastSession");
-  do_check_eq(lastIndex, "5"); // 0-indexed so this is really 6.
+  do_check_eq(lastIndex, "" + (previousStorageCount - 1)); // 0-indexed
 
-  // Fake an aborted sessions.
+  // Fake an aborted session. If we create a 2nd recorder against the same
+  // prefs branch as a running one, this simulates what would happen if the
+  // first recorder didn't shut down.
   let recorder2 = new SessionRecorder("testing.collect.sessions.");
   recorder2.onStartup();
   do_check_eq(Object.keys(recorder.getPreviousSessions()).length, 1);
@@ -148,7 +154,8 @@ add_task(function test_collect() {
 
   values = yield daily.getValues();
   day = values.days.getDay(now);
-  do_check_eq(day.size, 7);
+  do_check_eq(day.size, previousStorageCount + 1);
+  previousStorageCount = day.get("main").length;
   for (let field of ["abortedActiveTicks", "abortedTotalTime"]) {
     do_check_true(day.has(field));
     do_check_true(Array.isArray(day.get(field)));
@@ -156,25 +163,27 @@ add_task(function test_collect() {
   }
 
   lastIndex = yield provider.getState("lastSession");
-  do_check_eq(lastIndex, "6");
+  do_check_eq(lastIndex, "" + (previousStorageCount - 1));
 
+  recorder.onShutdown();
   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();
+  // If we try to insert a already-inserted session, it will be ignored.
+  recorder = new SessionRecorder("testing.collect.sessions.");
+  recorder._currentIndex = recorder._currentIndex - 1;
+  recorder._prunedIndex = recorder._currentIndex;
+  recorder.onStartup();
   // Session is left over from recorder2.
-  do_check_eq(Object.keys(recorder.getPreviousSessions()).length, 1);
+  sessions = recorder.getPreviousSessions();
+  do_check_eq(Object.keys(sessions).length, 1);
+  do_check_true(previousStorageCount - 1 in sessions);
   yield provider.collectConstantData();
   lastIndex = yield provider.getState("lastSession");
-  do_check_eq(lastIndex, "6");
+  do_check_eq(lastIndex, "" + (previousStorageCount - 1));
   values = yield daily.getValues();
   day = values.days.getDay(now);
-  do_check_eq(day.size, 7); // We should not get additional entry.
-  recorder3.onShutdown();
-
+  // We should not get additional entry.
+  do_check_eq(day.get("main").length, previousStorageCount);
   recorder.onShutdown();
 
   yield provider.shutdown();