bug 892454 - Use a less fragile flag than zero frecency to skip the frecency update on visit.

r=Mano

toolkit/components/places/History.cpp

@@ -76,6 +76,7 @@ struct VisitData {
   , visitTime(0)
   , frecency(-1)
   , titleChanged(false)
+  , shouldUpdateFrecency(true)
   {
     guid.SetIsVoid(true);
     title.SetIsVoid(true);
@@ -91,6 +92,7 @@ struct VisitData {
   , visitTime(0)
   , frecency(-1)
   , titleChanged(false)
+  , shouldUpdateFrecency(true)
   {
     (void)aURI->GetSpec(spec);
     (void)GetReversedHostname(aURI, revHost);
@@ -157,6 +159,9 @@ struct VisitData {
 
   // TODO bug 626836 hook up hidden and typed change tracking too!
   bool titleChanged;
+
+  // Indicates whether frecency should be updated for this visit.
+  bool shouldUpdateFrecency;
 };
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -997,8 +1002,12 @@ private:
 
     // TODO (bug 623969) we shouldn't update this after each visit, but
     // rather only for each unique place to save disk I/O.
-    rv = UpdateFrecency(aPlace);
-    NS_ENSURE_SUCCESS(rv, rv);
+
+    // Don't update frecency if the page should not appear in autocomplete.
+    if (aPlace.shouldUpdateFrecency) {
+      rv = UpdateFrecency(aPlace);
+      NS_ENSURE_SUCCESS(rv, rv);
+    }
 
     return NS_OK;
   }
@@ -1168,10 +1177,7 @@ private:
    */
   nsresult UpdateFrecency(const VisitData& aPlace)
   {
-    // Don't update frecency if the page should not appear in autocomplete.
-    if (aPlace.frecency == 0) {
-      return NS_OK;
-    }
+    MOZ_ASSERT(aPlace.shouldUpdateFrecency);
 
     nsresult rv;
     { // First, set our frecency to the proper value.
@@ -2064,7 +2070,10 @@ History::InsertPlace(const VisitData& aPlace)
   NS_ENSURE_SUCCESS(rv, rv);
   rv = stmt->BindInt32ByName(NS_LITERAL_CSTRING("typed"), aPlace.typed);
   NS_ENSURE_SUCCESS(rv, rv);
-  rv = stmt->BindInt32ByName(NS_LITERAL_CSTRING("frecency"), aPlace.frecency);
+  // When inserting a page for a first visit that should not appear in
+  // autocomplete, for example an error page, use a zero frecency.
+  int32_t frecency = aPlace.shouldUpdateFrecency ? aPlace.frecency : 0;
+  rv = stmt->BindInt32ByName(NS_LITERAL_CSTRING("frecency"), frecency);
   NS_ENSURE_SUCCESS(rv, rv);
   rv = stmt->BindInt32ByName(NS_LITERAL_CSTRING("hidden"), aPlace.hidden);
   NS_ENSURE_SUCCESS(rv, rv);
@@ -2417,7 +2426,7 @@ History::VisitURI(nsIURI* aURI,
 
   // Error pages should never be autocompleted.
   if (aFlags & IHistory::UNRECOVERABLE_ERROR) {
-    place.frecency = 0;
+    place.shouldUpdateFrecency = false;
   }
 
   // EMBED visits are session-persistent and should not go through the database.

toolkit/components/places/tests/browser/Makefile.in

@@ -23,6 +23,7 @@ MOCHITEST_BROWSER_FILES = \
 	browser_favicon_setAndFetchFaviconForPage_failures.js \
 	browser_notfound.js \
 	browser_redirect.js \
+	browser_visited_notfound.js \
 	browser_visituri.js \
 	browser_visituri_nohistory.js \
 	browser_visituri_privatebrowsing_perwindowpb.js \

toolkit/components/places/tests/browser/browser_visited_notfound.js

@@ -0,0 +1,51 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+const TEST_URI = NetUtil.newURI("http://mochi.test:8888/notFoundPage.html");
+
+function test() {
+  waitForExplicitFinish();
+
+  gBrowser.selectedTab = gBrowser.addTab();
+  registerCleanupFunction(function() {
+    gBrowser.removeCurrentTab();
+  });
+
+  // First add a visit to the page, this will ensure that later we skip
+  // updating the frecency for a newly not-found page.
+  addVisits({ uri: TEST_URI }, window, () => {
+    info("Added visit");
+    fieldForUrl(TEST_URI, "frecency", aFrecency => {
+      ok(aFrecency > 0, "Frecency should be > 0");
+      continueTest(aFrecency);
+    });
+  });
+}
+
+function continueTest(aOldFrecency) {
+  // Used to verify errors are not marked as typed.
+  PlacesUtils.history.markPageAsTyped(TEST_URI);
+  gBrowser.selectedTab.linkedBrowser.loadURI(TEST_URI.spec);
+
+  // Create and add history observer.
+  let historyObserver = {
+    __proto__: NavHistoryObserver.prototype,
+    onVisit: function (aURI, aVisitID, aTime, aSessionID, aReferringID,
+                      aTransitionType) {
+      PlacesUtils.history.removeObserver(historyObserver);
+      info("Received onVisit: " + aURI.spec);
+      fieldForUrl(aURI, "frecency", function (aFrecency) {
+        is(aFrecency, aOldFrecency, "Frecency should be unchanged");
+        fieldForUrl(aURI, "hidden", function (aHidden) {
+          is(aHidden, 0, "Page should not be hidden");
+          fieldForUrl(aURI, "typed", function (aTyped) {
+            is(aTyped, 0, "page should not be marked as typed");
+            promiseClearHistory().then(finish);
+          });
+        });
+      });
+    }
+  };
+  PlacesUtils.history.addObserver(historyObserver, false);
+}

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

@@ -0,0 +1,30 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
+// Tests a zero frecency is correctly updated when inserting new valid visits.
+
+function run_test()
+{
+  run_next_test()
+}
+
+add_task(function ()
+{
+  const TEST_URI = NetUtil.newURI("http://example.com/");
+  let id = PlacesUtils.bookmarks.insertBookmark(PlacesUtils.unfiledBookmarksFolderId,
+                                                TEST_URI,
+                                                PlacesUtils.bookmarks.DEFAULT_INDEX,
+                                                "A title");
+  yield promiseAsyncUpdates();
+  do_check_true(frecencyForUrl(TEST_URI) > 0);
+
+  // Removing the bookmark should leave an orphan page with zero frecency.
+  // Note this would usually be expired later by expiration.
+  PlacesUtils.bookmarks.removeItem(id);
+  yield promiseAsyncUpdates();
+  do_check_eq(frecencyForUrl(TEST_URI), 0);
+
+  // Now add a valid visit to the page, frecency should increase.
+  yield promiseAddVisits({ uri: TEST_URI });
+  do_check_true(frecencyForUrl(TEST_URI) > 0);
+});

toolkit/components/places/tests/unit/xpcshell.ini

@@ -72,6 +72,7 @@ skip-if = os == "android"
 # Bug 676989: test fails consistently on Android
 fail-if = os == "android"
 [test_frecency.js]
+[test_frecency_zero_updated.js]
 # Bug 676989: test hangs consistently on Android
 skip-if = os == "android"
 [test_getChildIndex.js]