Bug 1569146 - a page redirecting to itself should appear in history. r=Standard8

The history anti-flooding system prevents repeated loads of the same url from
flooding history, generating bogus frecency values.
In some cases though, like a page redirecting to itself, it may be overzealous.
A redirect source is always marked as hidden, and if the page redirects to
itself the second visit will be skipped, because repeated.
We always want to haveat least one unhidden visit in that case, thus we can
skip the anti-flooding check if the page will be unhidden.

Differential Revision: https://phabricator.services.mozilla.com/D40426

--HG--
extra : moz-landing-system : lando

toolkit/components/places/History.cpp

@@ -529,7 +529,7 @@ class NotifyManyVisitsObservers : public Runnable {
     }
 
     if (aNow - aPlace.visitTime < RECENTLY_VISITED_URIS_MAX_AGE) {
-      mHistory->AppendToRecentlyVisitedURIs(aURI);
+      mHistory->AppendToRecentlyVisitedURIs(aURI, aPlace.hidden);
     }
     mHistory->NotifyVisited(aURI);
 
@@ -1978,7 +1978,7 @@ void History::Shutdown() {
   }
 }
 
-void History::AppendToRecentlyVisitedURIs(nsIURI* aURI) {
+void History::AppendToRecentlyVisitedURIs(nsIURI* aURI, bool aHidden) {
   // Add a new entry, if necessary.
   RecentURIKey* entry = mRecentlyVisitedURIs.GetEntry(aURI);
   if (!entry) {
@@ -1986,6 +1986,7 @@ void History::AppendToRecentlyVisitedURIs(nsIURI* aURI) {
   }
   if (entry) {
     entry->time = PR_Now();
+    entry->hidden = aHidden;
   }
 
   // Remove entries older than RECENTLY_VISITED_URIS_MAX_AGE.
@@ -1997,13 +1998,6 @@ void History::AppendToRecentlyVisitedURIs(nsIURI* aURI) {
   }
 }
 
-inline bool History::IsRecentlyVisitedURI(nsIURI* aURI) {
-  RecentURIKey* entry = mRecentlyVisitedURIs.GetEntry(aURI);
-  // Check if the entry exists and is younger than
-  // RECENTLY_VISITED_URIS_MAX_AGE.
-  return entry && (PR_Now() - entry->time) < RECENTLY_VISITED_URIS_MAX_AGE;
-}
-
 ////////////////////////////////////////////////////////////////////////////////
 //// IHistory
 
@@ -2042,16 +2036,10 @@ History::VisitURI(nsIWidget* aWidget, nsIURI* aURI, nsIURI* aLastVisitedURI,
     return NS_OK;
   }
 
-  // Do not save a reloaded uri if we have visited the same URI recently.
   bool reload = false;
   if (aLastVisitedURI) {
     rv = aURI->Equals(aLastVisitedURI, &reload);
     NS_ENSURE_SUCCESS(rv, rv);
-    if (reload && IsRecentlyVisitedURI(aURI)) {
-      // Regardless we must update the stored visit time.
-      AppendToRecentlyVisitedURIs(aURI);
-      return NS_OK;
-    }
   }
 
   nsTArray<VisitData> placeArray(1);
@@ -2110,6 +2098,26 @@ History::VisitURI(nsIWidget* aWidget, nsIURI* aURI, nsIURI* aLastVisitedURI,
     place.shouldUpdateFrecency = false;
   }
 
+  // Do not save a reloaded uri if we have visited the same URI recently.
+  if (reload) {
+    RecentURIKey* entry = mRecentlyVisitedURIs.GetEntry(aURI);
+    // Check if the entry exists and is younger than
+    // RECENTLY_VISITED_URIS_MAX_AGE.
+    if (entry && (PR_Now() - entry->time) < RECENTLY_VISITED_URIS_MAX_AGE) {
+      bool wasHidden = entry->hidden;
+      // Regardless of whether we store the visit or not, we must update the
+      // stored visit time.
+      AppendToRecentlyVisitedURIs(aURI, place.hidden);
+      // We always want to store an unhidden visit, if the previous visits were
+      // hidden, because otherwise the page may not appear in the history UI.
+      // This can happen for example at a page redirecting to itself.
+      if (!wasHidden || place.hidden) {
+        // We can skip this visit.
+        return NS_OK;
+      }
+    }
+  }
+
   // EMBED visits are session-persistent and should not go through the database.
   // They exist only to keep track of isVisited status during the session.
   if (place.transitionType == nsINavHistoryService::TRANSITION_EMBED) {

toolkit/components/places/History.h

@@ -138,8 +138,10 @@ class History final : public IHistory,
   /**
    * Helper function to append a new URI to mRecentlyVisitedURIs. See
    * mRecentlyVisitedURIs.
+   * @param {nsIURI} aURI The URI to append
+   * @param {bool} aHidden The hidden status of the visit being appended.
    */
-  void AppendToRecentlyVisitedURIs(nsIURI* aURI);
+  void AppendToRecentlyVisitedURIs(nsIURI* aURI, bool aHidden);
 
   void NotifyVisitedParent(const nsTArray<mozilla::ipc::URIParams>& aURIs);
 
@@ -230,13 +232,9 @@ class History final : public IHistory,
       MOZ_ASSERT_UNREACHABLE("Do not call me!");
     }
     MOZ_INIT_OUTSIDE_CTOR PRTime time;
+    MOZ_INIT_OUTSIDE_CTOR bool hidden;
   };
   nsTHashtable<RecentURIKey> mRecentlyVisitedURIs;
-  /**
-   * Whether aURI has been visited "recently".
-   * See RECENTLY_VISITED_URIS_MAX_AGE.
-   */
-  bool IsRecentlyVisitedURI(nsIURI* aURI);
 };
 
 }  // namespace places

toolkit/components/places/tests/browser/browser.ini

@@ -3,22 +3,72 @@ support-files =
   head.js
 
 [browser_bug399606.js]
+support-files =
+  399606-history.go-0.html
+  399606-httprefresh.html
+  399606-location.reload.html
+  399606-location.replace.html
+  399606-window.location.html
+  399606-window.location.href.html
 [browser_bug461710.js]
+support-files =
+  461710_iframe.html
+  461710_link_page-2.html
+  461710_link_page-3.html
+  461710_link_page.html
+  461710_visited_page.html
 [browser_bug646422.js]
 [browser_bug680727.js]
 skip-if = fission || verify
 [browser_double_redirect.js]
+support-files =
+  begin.html
+  final.html
+  redirect_once.sjs
+  redirect_twice.sjs
 [browser_favicon_privatebrowsing_perwindowpb.js]
 [browser_history_post.js]
+support-files =
+  history_post.html
+  history_post.sjs
 [browser_notfound.js]
 [browser_onvisit_title_null_for_navigation.js]
 skip-if = verify
 support-files =
   empty_page.html
 [browser_redirect.js]
+support-files =
+  redirect.sjs
+  redirect-target.html
+[browser_redirect_self.js]
+support-files =
+  redirect_self.sjs
 [browser_multi_redirect_frecency.js]
+support-files =
+  final.html
+  redirect_once.sjs
+  redirect_thrice.sjs
+  redirect_twice.sjs
+  redirect_twice_perma.sjs
 [browser_settitle.js]
+support-files =
+  title1.html
+  title2.html
 [browser_visited_notfound.js]
 [browser_visituri.js]
+support-files =
+  begin.html
+  final.html
+  redirect_once.sjs
+  redirect_twice.sjs
 [browser_visituri_nohistory.js]
+support-files =
+  begin.html
+  final.html
+  favicon-normal16.png
+  favicon-normal32.png
 [browser_visituri_privatebrowsing_perwindowpb.js]
+support-files =
+  begin.html
+  favicon.html
+  final.html

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

@@ -0,0 +1,46 @@
+/* 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/. */
+
+/*
+ * Tests a page that redirects to itself. On the initial visit the page should
+ * be marked as hidden, but then the second visit should unhide it.
+ * This ensures that that the history anti-flooding system doesn't skip the
+ * second visit.
+ */
+
+add_task(async function() {
+  await PlacesUtils.history.clear();
+  const url =
+    "http://mochi.test:8888/tests/toolkit/components/places/tests/browser/redirect_self.sjs";
+  let visitCount = 0;
+  function onVisitsListener(events) {
+    visitCount++;
+    Assert.equal(events.length, 1, "Right number of visits notified");
+    Assert.equal(events[0].url, url, "Got a visit for the expected url");
+    if (visitCount == 1) {
+      Assert.ok(events[0].hidden, "The visit should be hidden");
+    } else {
+      Assert.ok(!events[0].hidden, "The visit should not be hidden");
+    }
+  }
+  PlacesObservers.addListener(["page-visited"], onVisitsListener);
+  registerCleanupFunction(async function() {
+    PlacesObservers.removeListener(["page-visited"], onVisitsListener);
+    await PlacesUtils.history.clear();
+  });
+  await BrowserTestUtils.withNewTab(
+    {
+      gBrowser,
+      url,
+    },
+    async browser => {
+      await TestUtils.waitForCondition(() => visitCount == 2);
+      // Check that the visit is not hidden in the database.
+      Assert.ok(
+        !(await PlacesTestUtils.fieldInDB(url, "hidden")),
+        "The url should not be hidden in the database"
+      );
+    }
+  );
+});

toolkit/components/places/tests/browser/redirect_self.sjs

@@ -0,0 +1,27 @@
+/* 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/. */
+
+// Script that redirects to itself once.
+
+function handleRequest(request, response) {
+  if (
+    request.hasHeader("Cookie") &&
+    request.getHeader("Cookie").includes("redirect-self")
+  ) {
+    response.setStatusLine("1.0", 200, "OK");
+    // Expire the cookie.
+    response.setHeader(
+      "Set-Cookie",
+      "redirect-self=true; expires=Thu, 01 Jan 1970 00:00:00 GMT",
+      true
+    );
+    response.setHeader("Content-Type", "text/plain; charset=utf-8", false);
+    response.write("OK");
+  } else {
+    response.setStatusLine(request.httpVersion, 302, "Moved Temporarily");
+    response.setHeader("Set-Cookie", "redirect-self=true", true);
+    response.setHeader("Location", "redirect_self.sjs");
+    response.write("Moved Temporarily");
+  }
+}

toolkit/components/places/tests/moz.build

@@ -24,7 +24,10 @@ XPCSHELL_TESTS_MANIFESTS += [
     'unit/xpcshell.ini',
 ]
 
-BROWSER_CHROME_MANIFESTS += ['browser/browser.ini']
+BROWSER_CHROME_MANIFESTS += [
+    'browser/browser.ini'
+]
+
 MOCHITEST_CHROME_MANIFESTS += [
     'chrome/chrome.ini',
 ]
@@ -55,6 +58,7 @@ TEST_HARNESS_FILES.testing.mochitest.tests.toolkit.components.places.tests.brows
     'browser/redirect-target.html',
     'browser/redirect.sjs',
     'browser/redirect_once.sjs',
+    'browser/redirect_self.sjs',
     'browser/redirect_thrice.sjs',
     'browser/redirect_twice.sjs',
     'browser/redirect_twice_perma.sjs',