Bug 1336233 - Fix intermittent failures in browser_multi_redirect_frecency.js by ensuring the idle service updates for frecency are ignored. r?mak draft
authorMark Banner <standard8@mozilla.com>
Mon, 06 Feb 2017 14:16:20 +0000
changeset 479855 b40c14a0f5bdaf9120c7874ca81d634d708b091c
parent 479651 af8a2573d0f1e9cc6f2ba0ab67d7a702a197f177
child 544812 91858a3d571112cc4809eacd955045568f18cb59
push id44390
push userbmo:standard8@mozilla.com
push dateTue, 07 Feb 2017 12:50:56 +0000
reviewersmak
bugs1336233
milestone54.0a1
Bug 1336233 - Fix intermittent failures in browser_multi_redirect_frecency.js by ensuring the idle service updates for frecency are ignored. r?mak MozReview-Commit-ID: L8AgWOAGF9g
toolkit/components/places/nsNavHistory.cpp
toolkit/components/places/tests/PlacesTestUtils.jsm
toolkit/components/places/tests/browser/browser_multi_redirect_frecency.js
toolkit/components/places/tests/browser/browser_redirect.js
toolkit/components/places/tests/unit/test_frecency_decay.js
toolkit/components/places/tests/unit/xpcshell.ini
--- a/toolkit/components/places/nsNavHistory.cpp
+++ b/toolkit/components/places/nsNavHistory.cpp
@@ -99,16 +99,20 @@ using namespace mozilla::places;
 #define PREF_FREC_DEFAULT_VISIT_BONUS_DEF       0
 #define PREF_FREC_UNVISITED_BOOKMARK_BONUS      "places.frecency.unvisitedBookmarkBonus"
 #define PREF_FREC_UNVISITED_BOOKMARK_BONUS_DEF  140
 #define PREF_FREC_UNVISITED_TYPED_BONUS         "places.frecency.unvisitedTypedBonus"
 #define PREF_FREC_UNVISITED_TYPED_BONUS_DEF     200
 #define PREF_FREC_RELOAD_VISIT_BONUS            "places.frecency.reloadVisitBonus"
 #define PREF_FREC_RELOAD_VISIT_BONUS_DEF        0
 
+// This is a 'hidden' pref for the purposes of unit tests.
+#define PREF_FREC_DECAY_RATE     "places.frecency.decayRate"
+#define PREF_FREC_DECAY_RATE_DEF 0.975
+
 // In order to avoid calling PR_now() too often we use a cached "now" value
 // for repeating stuff.  These are milliseconds between "now" cache refreshes.
 #define RENEW_CACHED_NOW_TIMEOUT ((int32_t)3 * PR_MSEC_PER_SEC)
 
 // character-set annotation
 #define CHARSET_ANNO NS_LITERAL_CSTRING("URIProperties/characterSet")
 
 // These macros are used when splitting history by date.
@@ -3088,28 +3092,33 @@ public:
 } // namespace
 
 nsresult
 nsNavHistory::DecayFrecency()
 {
   nsresult rv = FixInvalidFrecencies();
   NS_ENSURE_SUCCESS(rv, rv);
 
+  float decayRate = Preferences::GetFloat(PREF_FREC_DECAY_RATE, PREF_FREC_DECAY_RATE_DEF);
+
   // Globally decay places frecency rankings to estimate reduced frecency
   // values of pages that haven't been visited for a while, i.e., they do
   // not get an updated frecency.  A scaling factor of .975 results in .5 the
   // original value after 28 days.
   // When changing the scaling factor, ensure that the barrier in
   // moz_places_afterupdate_frecency_trigger still ignores these changes.
   nsCOMPtr<mozIStorageAsyncStatement> decayFrecency = mDB->GetAsyncStatement(
-    "UPDATE moz_places SET frecency = ROUND(frecency * .975) "
+    "UPDATE moz_places SET frecency = ROUND(frecency * :decay_rate) "
     "WHERE frecency > 0"
   );
   NS_ENSURE_STATE(decayFrecency);
 
+  rv = decayFrecency->BindDoubleByName(NS_LITERAL_CSTRING("decay_rate"), decayRate);
+  NS_ENSURE_SUCCESS(rv, rv);
+
   // Decay potentially unused adaptive entries (e.g. those that are at 1)
   // to allow better chances for new entries that will start at 1.
   nsCOMPtr<mozIStorageAsyncStatement> decayAdaptive = mDB->GetAsyncStatement(
     "UPDATE moz_inputhistory SET use_count = use_count * .975"
   );
   NS_ENSURE_STATE(decayAdaptive);
 
   // Delete any adaptive entries that won't help in ordering anymore.
--- a/toolkit/components/places/tests/PlacesTestUtils.jsm
+++ b/toolkit/components/places/tests/PlacesTestUtils.jsm
@@ -156,16 +156,35 @@ this.PlacesTestUtils = Object.freeze({
       `SELECT count(*) FROM moz_historyvisits v
        JOIN moz_places h ON h.id = v.place_id
        WHERE url_hash = hash(:url) AND url = :url`,
       { url });
     return rows[0].getResultByIndex(0);
   }),
 
   /**
+   * Asynchronously checks the frecency for a specified page.
+   * @param aURI
+   *        nsIURI or address to look for.
+   *
+   * @return {Promise}
+   * @resolves Returns the frecency.
+   * @rejects JavaScript exception.
+   */
+  frecencyInDB: Task.async(function* (aURI) {
+    let url = aURI instanceof Ci.nsIURI ? new URL(aURI.spec) : new URL(aURI);
+    let db = yield PlacesUtils.promiseDBConnection();
+    let rows = yield db.executeCached(
+      `SELECT frecency FROM moz_places
+       WHERE url_hash = hash(:url) AND url = :url`,
+      { url: url.href });
+    return rows[0].getResultByIndex(0);
+  }),
+
+  /**
    * Marks all syncable bookmarks as synced by setting their sync statuses to
    * "NORMAL", resetting their change counters, and removing all tombstones.
    * Used by tests to avoid calling `PlacesSyncUtils.bookmarks.pullChanges`
    * and `PlacesSyncUtils.bookmarks.pushChanges`.
    *
    * @resolves When all bookmarks have been updated.
    * @rejects JavaScript exception.
    */
--- a/toolkit/components/places/tests/browser/browser_multi_redirect_frecency.js
+++ b/toolkit/components/places/tests/browser/browser_multi_redirect_frecency.js
@@ -7,17 +7,22 @@ const REDIRECT_SOURCE_VISIT_BONUS =
   Services.prefs.getIntPref("places.frecency.redirectSourceVisitBonus");
 // const LINK_VISIT_BONUS =
 //   Services.prefs.getIntPref("places.frecency.linkVisitBonus");
 // const TYPED_VISIT_BONUS =
 //   Services.prefs.getIntPref("places.frecency.typedVisitBonus");
 const PERM_REDIRECT_VISIT_BONUS =
   Services.prefs.getIntPref("places.frecency.permRedirectVisitBonus");
 
+// Ensure that decay frecency doesn't kick in during tests (as a result
+// of idle-daily).
+Services.prefs.setCharPref("places.frecency.decayRate", "1.0");
+
 registerCleanupFunction(function*() {
+  Services.prefs.clearUserPref("places.frecency.decayRate");
   yield PlacesTestUtils.clearHistory();
 });
 
 function promiseVisitedURIObserver(redirectURI, targetURI, expectedTargetFrecency) {
   // Create and add history observer.
   return new Promise(resolve => {
     let historyObserver = {
       _redirectNotified: false,
--- a/toolkit/components/places/tests/browser/browser_redirect.js
+++ b/toolkit/components/places/tests/browser/browser_redirect.js
@@ -7,17 +7,22 @@ const TARGET_URI = NetUtil.newURI("http:
 
 const REDIRECT_SOURCE_VISIT_BONUS =
   Services.prefs.getIntPref("places.frecency.redirectSourceVisitBonus");
 const LINK_VISIT_BONUS =
   Services.prefs.getIntPref("places.frecency.linkVisitBonus");
 const TYPED_VISIT_BONUS =
   Services.prefs.getIntPref("places.frecency.typedVisitBonus");
 
+// Ensure that decay frecency doesn't kick in during tests (as a result
+// of idle-daily).
+Services.prefs.setCharPref("places.frecency.decayRate", "1.0");
+
 registerCleanupFunction(function*() {
+  Services.prefs.clearUserPref("places.frecency.decayRate");
   yield PlacesTestUtils.clearHistory();
 });
 
 function promiseVisitedWithFrecency(expectedRedirectFrecency, expectedTargetFrecency) {
   // Create and add history observer.
   return new Promise(resolve => {
     let historyObserver = {
       _redirectNotified: false,
new file mode 100644
--- /dev/null
+++ b/toolkit/components/places/tests/unit/test_frecency_decay.js
@@ -0,0 +1,71 @@
+Cu.import("resource://gre/modules/PromiseUtils.jsm");
+
+const PREF_FREC_DECAY_RATE_DEF = 0.975;
+
+/**
+ * Promises that the frecency has been changed, and is the new value.
+ *
+ * @param {nsIURI} expectedURI The URI to check frecency for.
+ * @param {Number} expectedFrecency The expected frecency for the URI.
+ * @returns {Promise} A promise which is resolved when the URI is seen.
+ */
+function promiseFrecencyChanged(expectedURI, expectedFrecency) {
+  let deferred = PromiseUtils.defer();
+  let obs = new NavHistoryObserver();
+  obs.onFrecencyChanged =
+    (uri, newFrecency, guid, hidden, visitDate) => {
+      PlacesUtils.history.removeObserver(obs);
+      Assert.ok(!!uri, "uri should not be null");
+      Assert.ok(uri.equals(NetUtil.newURI(expectedURI)), "uri should be the expected one");
+      Assert.equal(newFrecency, expectedFrecency, "Frecency should be the expected one");
+      deferred.resolve();
+    };
+  PlacesUtils.history.addObserver(obs, false);
+  return deferred.promise;
+}
+
+/**
+ * Promises that the many frecencies changed notification has been seen.
+ *
+ * @returns {Promise} A promise which is resolved when the notification is seen.
+ */
+function promiseManyFrecenciesChanged() {
+  let deferred = PromiseUtils.defer();
+  let obs = new NavHistoryObserver();
+  obs.onManyFrecenciesChanged = () => {
+    PlacesUtils.history.removeObserver(obs);
+    Assert.ok(true);
+    deferred.resolve();
+  };
+  PlacesUtils.history.addObserver(obs, false);
+  return deferred.promise;
+}
+
+add_task(function* setup() {
+  Services.prefs.setCharPref("places.frecency.decayRate", PREF_FREC_DECAY_RATE_DEF);
+});
+
+add_task(function* test_frecency_decay() {
+  let unvisitedBookmarkFrecency = Services.prefs.getIntPref("places.frecency.unvisitedBookmarkBonus");
+
+  // Add a bookmark and check its frecency.
+  let url = "http://example.com/b";
+  let promiseOne = promiseFrecencyChanged(url, unvisitedBookmarkFrecency);
+  yield PlacesUtils.bookmarks.insert({
+    url,
+    parentGuid: PlacesUtils.bookmarks.unfiledGuid
+  });
+  yield promiseOne;
+
+  // Trigger DecayFrecency.
+  let promiseMany = promiseManyFrecenciesChanged();
+  PlacesUtils.history.QueryInterface(Ci.nsIObserver)
+             .observe(null, "idle-daily", "");
+  yield promiseMany;
+
+  // Now check the new frecency is correct.
+  let newFrecency = yield PlacesTestUtils.frecencyInDB(url);
+
+  Assert.equal(newFrecency, Math.round(unvisitedBookmarkFrecency * PREF_FREC_DECAY_RATE_DEF),
+               "Frecencies should match");
+});
--- a/toolkit/components/places/tests/unit/xpcshell.ini
+++ b/toolkit/components/places/tests/unit/xpcshell.ini
@@ -89,16 +89,17 @@ skip-if = (os == "win" && os_version == 
 [test_bug636917_isLivemark.js]
 [test_childlessTags.js]
 [test_crash_476292.js]
 [test_database_replaceOnStartup.js]
 [test_download_history.js]
 # Bug 676989: test fails consistently on Android
 fail-if = os == "android"
 [test_frecency.js]
+[test_frecency_decay.js]
 [test_frecency_zero_updated.js]
 # Bug 676989: test hangs consistently on Android
 skip-if = os == "android"
 [test_getChildIndex.js]
 [test_getPlacesInfo.js]
 [test_history.js]
 [test_history_autocomplete_tags.js]
 [test_history_catobs.js]