Bug 628805 - Add test for unstored sessionId to test_async_history_api.js and fix the way session ids are stored with places visits [r=sdwilsh, a=blocking-final]

toolkit/components/places/src/History.cpp

@@ -733,11 +733,23 @@ private:
     for (nsTArray<VisitData>::size_type i = 0; i < mPlaces.Length(); i++) {
       mReferrers[i].spec = mPlaces[i].referrerSpec;
 
+      // If we are inserting a place into an empty mPlaces array, we need to
+      // check to make sure we do not store a bogus session id that is higher
+      // than the current maximum session id.
+      if (i == 0) {
+        PRInt64 newSessionId = navHistory->GetNewSessionID();
+        if (mPlaces[0].sessionId > newSessionId) {
+          mPlaces[0].sessionId = newSessionId;
+        }
+      }
+
       // Speculatively get a new session id for our visit if the current session
-      // id is non-valid.  While it is true that we will use the session id from
-      // the referrer if the visit was "recent" enough, we cannot call this
-      // method off of the main thread, so we have to consume an id now.
-      if (mPlaces[i].sessionId <= 0) {
+      // id is non-valid or if it is larger than the current largest session id.
+      // While it is true that we will use the session id from the referrer if
+      // the visit was "recent" enough, we cannot call this method off of the
+      // main thread, so we have to consume an id now.
+      if (mPlaces[i].sessionId <= 0 ||
+          (i > 0 && mPlaces[i].sessionId >= mPlaces[0].sessionId)) {
         mPlaces[i].sessionId = navHistory->GetNewSessionID();
       }
 

toolkit/components/places/tests/unit/test_async_history_api.js

@@ -498,6 +498,55 @@ function test_invalid_sessionId_ignored()
   });
 }
 
+function test_unstored_sessionId_ignored()
+{
+  let place = {
+    uri: NetUtil.newURI(TEST_DOMAIN +
+                        "test_unstored_sessionId_ignored"),
+    visits: [
+      new VisitInfo(),
+    ],
+  };
+
+  // Find max session id in database.
+  let stmt = DBConn().createStatement(
+    "SELECT MAX(session) as max_session " +
+    "FROM moz_historyvisits"
+  );
+  do_check_true(stmt.executeStep());
+  let maxSessionId = stmt.row.max_session;
+  stmt.finalize();
+
+  // Create bogus sessionId that is not in database.
+  place.visits[0].sessionId = maxSessionId + 10;
+  do_check_false(gGlobalHistory.isVisited(place.uri));
+
+  gHistory.updatePlaces(place, function(aResultCode, aPlaceInfo) {
+    do_check_true(Components.isSuccessCode(aResultCode));
+    let uri = aPlaceInfo.uri;
+    do_check_true(gGlobalHistory.isVisited(uri));
+
+    // Check to make sure we do not persist bogus sessionId with the visit.
+    let visit = aPlaceInfo.visits[0];
+    do_check_neq(visit.sessionId, place.visits[0].sessionId);
+
+    // Check to make sure we do not persist bogus sessionId in the database.
+    let stmt = DBConn().createStatement(
+      "SELECT MAX(session) as max_session " +
+      "FROM moz_historyvisits"
+    );
+    do_check_true(stmt.executeStep());
+
+    // Max sessionId should increase by 1 because we will generate a new
+    // non-bogus sessionId.
+    let newMaxSessionId = stmt.row.max_session;
+    do_check_eq(maxSessionId + 1, newMaxSessionId);
+    stmt.finalize();
+
+    run_next_test();
+  });
+}
+
 function test_observer_topic_dispatched_when_complete()
 {
   // We test a normal visit, and embeded visit, and a uri that would fail
@@ -972,6 +1021,7 @@ let gTests = [
   test_invalid_referrerURI_ignored,
   test_nonnsIURI_referrerURI_ignored,
   test_invalid_sessionId_ignored,
+  test_unstored_sessionId_ignored,
   test_observer_topic_dispatched_when_complete,
   test_add_visit,
   test_properties_saved,