Bug 1261386 - Avoid history flooding from repeated reloads. r=adw

MozReview-Commit-ID: FhU8nOoNUHb

--HG--
extra : rebase_source : ff75adb252b13f4042da49d4572fb807c3d0d823

Author: mak77

browser/app/profile/firefox.js

@@ -836,6 +836,7 @@ pref("places.frecency.bookmarkVisitBonus", 75);
 pref("places.frecency.downloadVisitBonus", 0);
 pref("places.frecency.permRedirectVisitBonus", 0);
 pref("places.frecency.tempRedirectVisitBonus", 0);
+pref("places.frecency.reloadVisitBonus", 0);
 pref("places.frecency.defaultVisitBonus", 0);
 
 // bonus (in percent) for place types for frecency calculations

services/sync/modules/engines/history.js

@@ -302,8 +302,8 @@ HistoryStore.prototype = {
         continue;
       }
 
-      if (!visit.type || !(visit.type >= PlacesUtils.history.TRANSITION_LINK &&
-                           visit.type <= PlacesUtils.history.TRANSITION_FRAMED_LINK)) {
+      if (!visit.type ||
+          !Object.values(PlacesUtils.history.TRANSITIONS).includes(visit.type)) {
         this._log.warn("Encountered record with invalid visit type: " +
                        visit.type + "; ignoring.");
         continue;

toolkit/components/places/History.cpp

@@ -1210,8 +1210,8 @@ class InsertVisitedURIs final: public Runnable
                                _place.visitTime);
     NS_ENSURE_SUCCESS(rv, rv);
     uint32_t transitionType = _place.transitionType;
-    NS_ASSERTION(transitionType >= nsINavHistoryService::TRANSITION_LINK &&
-                 transitionType <= nsINavHistoryService::TRANSITION_FRAMED_LINK,
+    MOZ_ASSERT(transitionType >= nsINavHistoryService::TRANSITION_LINK &&
+               transitionType <= nsINavHistoryService::TRANSITION_RELOAD,
                  "Invalid transition type!");
     rv = stmt->BindInt32ByName(NS_LITERAL_CSTRING("visit_type"),
                                transitionType);
@@ -1970,7 +1970,7 @@ History::History()
   : mShuttingDown(false)
   , mShutdownMutex("History::mShutdownMutex")
   , mObservers(VISIT_OBSERVERS_INITIAL_CACHE_LENGTH)
-  , mRecentlyVisitedURIsNextIndex(0)
+  , mRecentlyVisitedURIs(RECENTLY_VISITED_URIS_SIZE)
 {
   NS_ASSERTION(!gService, "Ruh-roh!  This service has already been created!");
   gService = this;
@@ -2406,25 +2406,29 @@ History::Shutdown()
 
 void
 History::AppendToRecentlyVisitedURIs(nsIURI* aURI) {
-  if (mRecentlyVisitedURIs.Length() < RECENTLY_VISITED_URI_SIZE) {
-    // Append a new element while the array is not full.
-    mRecentlyVisitedURIs.AppendElement(aURI);
-  } else {
-    // Otherwise, replace the oldest member.
-    mRecentlyVisitedURIsNextIndex %= RECENTLY_VISITED_URI_SIZE;
-    mRecentlyVisitedURIs.ElementAt(mRecentlyVisitedURIsNextIndex) = aURI;
-    mRecentlyVisitedURIsNextIndex++;
+  // Add a new entry, if necessary.
+  RecentURIKey* entry = mRecentlyVisitedURIs.GetEntry(aURI);
+  if (!entry) {
+    entry = mRecentlyVisitedURIs.PutEntry(aURI);
+  }
+  if (entry) {
+    entry->time = PR_Now();
+  }
+
+  // Remove entries older than RECENTLY_VISITED_URIS_MAX_AGE.
+  for (auto iter = mRecentlyVisitedURIs.Iter(); !iter.Done(); iter.Next()) {
+    RecentURIKey* entry = iter.Get();
+    if ((PR_Now() - entry->time) > RECENTLY_VISITED_URIS_MAX_AGE) {
+      iter.Remove();
+    }
   }
 }
 
 inline bool
 History::IsRecentlyVisitedURI(nsIURI* aURI) {
-  bool equals = false;
-  RecentlyVisitedArray::index_type i;
-  for (i = 0; i < mRecentlyVisitedURIs.Length() && !equals; ++i) {
-    aURI->Equals(mRecentlyVisitedURIs.ElementAt(i), &equals);
-  }
-  return equals;
+  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;
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -2466,12 +2470,14 @@ History::VisitURI(nsIURI* aURI,
     return NS_OK;
   }
 
+  // Do not save a reloaded uri if we have visited the same URI recently.
+  bool reload = false;
   if (aLastVisitedURI) {
-    bool same;
-    rv = aURI->Equals(aLastVisitedURI, &same);
+    rv = aURI->Equals(aLastVisitedURI, &reload);
     NS_ENSURE_SUCCESS(rv, rv);
-    if (same && IsRecentlyVisitedURI(aURI)) {
-      // Do not save refresh visits if we have visited this URI recently.
+    if (reload && IsRecentlyVisitedURI(aURI)) {
+      // Regardless we must update the stored visit time.
+      AppendToRecentlyVisitedURIs(aURI);
       return NS_OK;
     }
   }
@@ -2507,6 +2513,9 @@ History::VisitURI(nsIURI* aURI,
   else if (aFlags & IHistory::REDIRECT_PERMANENT) {
     transitionType = nsINavHistoryService::TRANSITION_REDIRECT_PERMANENT;
   }
+  else if (reload) {
+    transitionType = nsINavHistoryService::TRANSITION_RELOAD;
+  }
   else if ((recentFlags & nsNavHistory::RECENT_TYPED) &&
            !(aFlags & IHistory::UNRECOVERABLE_ERROR)) {
     // Don't mark error pages as typed, even if they were actually typed by
@@ -2948,7 +2957,7 @@ History::UpdatePlaces(JS::Handle<JS::Value> aPlaceInfos,
       NS_ENSURE_SUCCESS(rv, rv);
       NS_ENSURE_ARG_RANGE(transitionType,
                           nsINavHistoryService::TRANSITION_LINK,
-                          nsINavHistoryService::TRANSITION_FRAMED_LINK);
+                          nsINavHistoryService::TRANSITION_RELOAD);
       data.SetTransitionType(transitionType);
       data.hidden = GetHiddenState(false, transitionType);
 

toolkit/components/places/History.h

@@ -33,8 +33,14 @@ class ConcurrentStatementsHolder;
 #define NS_HISTORYSERVICE_CID \
   {0x0937a705, 0x91a6, 0x417a, {0x82, 0x92, 0xb2, 0x2e, 0xb1, 0x0d, 0xa8, 0x6c}}
 
-// Max size of History::mRecentlyVisitedURIs
-#define RECENTLY_VISITED_URI_SIZE 8
+// Initial size of mRecentlyVisitedURIs.
+#define RECENTLY_VISITED_URIS_SIZE 64
+// Microseconds after which a visit can be expired from mRecentlyVisitedURIs.
+// When an URI is reloaded we only take into account the first visit to it, and
+// ignore any subsequent visits, if they happen before this time has elapsed.
+// A commonly found case is to reload a page every 5 minutes, so we pick a time
+// larger than that.
+#define RECENTLY_VISITED_URIS_MAX_AGE 6 * 60 * PR_USEC_PER_SEC
 
 class History final : public IHistory
                     , public nsIDownloadHistory
@@ -189,14 +195,26 @@ class History final : public IHistory
   nsTHashtable<KeyClass> mObservers;
 
   /**
-   * mRecentlyVisitedURIs remembers URIs which are recently added to the DB,
-   * to avoid saving these locations repeatedly in a short period.
+   * mRecentlyVisitedURIs remembers URIs which have been recently added to
+   * history, to avoid saving these locations repeatedly in a short period.
+   */
+  class RecentURIKey : public nsURIHashKey
+  {
+  public:
+    explicit RecentURIKey(const nsIURI* aURI) : nsURIHashKey(aURI)
+    {
+    }
+    RecentURIKey(const RecentURIKey& aOther) : nsURIHashKey(aOther)
+    {
+      NS_NOTREACHED("Do not call me!");
+    }
+    PRTime time;
+  };
+  nsTHashtable<RecentURIKey> mRecentlyVisitedURIs;
+  /**
+   * Whether aURI has been visited "recently".
+   * See RECENTLY_VISITED_URIS_MAX_AGE.
    */
-  typedef AutoTArray<nsCOMPtr<nsIURI>, RECENTLY_VISITED_URI_SIZE>
-          RecentlyVisitedArray;
-  RecentlyVisitedArray mRecentlyVisitedURIs;
-  RecentlyVisitedArray::index_type mRecentlyVisitedURIsNextIndex;
-
   bool IsRecentlyVisitedURI(nsIURI* aURI);
 };
 

toolkit/components/places/History.jsm

@@ -40,7 +40,7 @@
  * - date: (Date)
  *     The time the visit occurred.
  * - transition: (number)
- *     How the user reached the page. See constants `TRANSITION_*`
+ *     How the user reached the page. See constants `TRANSITIONS.*`
  *     for the possible transition types.
  * - referrer: (URL)
  *          or (nsIURI)
@@ -408,51 +408,58 @@ this.History = Object.freeze({
    * objects.
    */
 
-  /**
-   * The user followed a link and got a new toplevel window.
-   */
-  TRANSITION_LINK: Ci.nsINavHistoryService.TRANSITION_LINK,
-
-  /**
-   * The user typed the page's URL in the URL bar or selected it from
-   * URL bar autocomplete results, clicked on it from a history query
-   * (from the History sidebar, History menu, or history query in the
-   * personal toolbar or Places organizer.
-   */
-  TRANSITION_TYPED: Ci.nsINavHistoryService.TRANSITION_TYPED,
-
-  /**
-   * The user followed a bookmark to get to the page.
-   */
-  TRANSITION_BOOKMARK: Ci.nsINavHistoryService.TRANSITION_BOOKMARK,
-
-  /**
-   * Some inner content is loaded. This is true of all images on a
-   * page, and the contents of the iframe. It is also true of any
-   * content in a frame if the user did not explicitly follow a link
-   * to get there.
-   */
-  TRANSITION_EMBED: Ci.nsINavHistoryService.TRANSITION_EMBED,
-
-  /**
-   * Set when the transition was a permanent redirect.
-   */
-  TRANSITION_REDIRECT_PERMANENT: Ci.nsINavHistoryService.TRANSITION_REDIRECT_PERMANENT,
-
-  /**
-   * Set when the transition was a temporary redirect.
-   */
-  TRANSITION_REDIRECT_TEMPORARY: Ci.nsINavHistoryService.TRANSITION_REDIRECT_TEMPORARY,
-
-  /**
-   * Set when the transition is a download.
-   */
-  TRANSITION_DOWNLOAD: Ci.nsINavHistoryService.TRANSITION_REDIRECT_DOWNLOAD,
-
-  /**
-   * The user followed a link and got a visit in a frame.
-   */
-  TRANSITION_FRAMED_LINK: Ci.nsINavHistoryService.TRANSITION_FRAMED_LINK,
+  TRANSITIONS: {
+    /**
+     * The user followed a link and got a new toplevel window.
+     */
+    LINK: Ci.nsINavHistoryService.TRANSITION_LINK,
+
+    /**
+     * The user typed the page's URL in the URL bar or selected it from
+     * URL bar autocomplete results, clicked on it from a history query
+     * (from the History sidebar, History menu, or history query in the
+     * personal toolbar or Places organizer.
+     */
+    TYPED: Ci.nsINavHistoryService.TRANSITION_TYPED,
+
+    /**
+     * The user followed a bookmark to get to the page.
+     */
+    BOOKMARK: Ci.nsINavHistoryService.TRANSITION_BOOKMARK,
+
+    /**
+     * Some inner content is loaded. This is true of all images on a
+     * page, and the contents of the iframe. It is also true of any
+     * content in a frame if the user did not explicitly follow a link
+     * to get there.
+     */
+    EMBED: Ci.nsINavHistoryService.TRANSITION_EMBED,
+
+    /**
+     * Set when the transition was a permanent redirect.
+     */
+    REDIRECT_PERMANENT: Ci.nsINavHistoryService.TRANSITION_REDIRECT_PERMANENT,
+
+    /**
+     * Set when the transition was a temporary redirect.
+     */
+    REDIRECT_TEMPORARY: Ci.nsINavHistoryService.TRANSITION_REDIRECT_TEMPORARY,
+
+    /**
+     * Set when the transition is a download.
+     */
+    DOWNLOAD: Ci.nsINavHistoryService.TRANSITION_DOWNLOAD,
+
+    /**
+     * The user followed a link and got a visit in a frame.
+     */
+    FRAMED_LINK: Ci.nsINavHistoryService.TRANSITION_FRAMED_LINK,
+
+    /**
+     * The user reloaded a page.
+     */
+    RELOAD: Ci.nsINavHistoryService.TRANSITION_RELOAD,
+  },
 });
 
 /**
@@ -484,7 +491,7 @@ function validatePageInfo(pageInfo) {
   for (let inVisit of pageInfo.visits) {
     let visit = {
       date: new Date(),
-      transition: inVisit.transition || History.TRANSITION_LINK,
+      transition: inVisit.transition || History.TRANSITIONS.LINK,
     };
 
     if (!isValidTransitionType(visit.transition)) {
@@ -541,16 +548,7 @@ function convertForUpdatePlaces(pageInfo) {
  * @return (Boolean)
  */
 function isValidTransitionType(transitionType) {
-  return [
-    History.TRANSITION_LINK,
-    History.TRANSITION_TYPED,
-    History.TRANSITION_BOOKMARK,
-    History.TRANSITION_EMBED,
-    History.TRANSITION_REDIRECT_PERMANENT,
-    History.TRANSITION_REDIRECT_TEMPORARY,
-    History.TRANSITION_DOWNLOAD,
-    History.TRANSITION_FRAMED_LINK
-  ].includes(transitionType);
+  return Object.values(History.TRANSITIONS).includes(transitionType);
 }
 
 /**

toolkit/components/places/PlacesDBUtils.jsm

@@ -690,13 +690,13 @@ this.PlacesDBUtils = {
     let fixVisitStats = DBConn.createAsyncStatement(
       `UPDATE moz_places
        SET visit_count = (SELECT count(*) FROM moz_historyvisits
-                          WHERE place_id = moz_places.id AND visit_type NOT IN (0,4,7,8)),
+                          WHERE place_id = moz_places.id AND visit_type NOT IN (0,4,7,8,9)),
            last_visit_date = (SELECT MAX(visit_date) FROM moz_historyvisits
                               WHERE place_id = moz_places.id)
        WHERE id IN (
          SELECT h.id FROM moz_places h
          WHERE visit_count <> (SELECT count(*) FROM moz_historyvisits v
-                               WHERE v.place_id = h.id AND visit_type NOT IN (0,4,7,8))
+                               WHERE v.place_id = h.id AND visit_type NOT IN (0,4,7,8,9))
             OR last_visit_date <> (SELECT MAX(visit_date) FROM moz_historyvisits v
                                    WHERE v.place_id = h.id)
        )`);