Bug 1284083 - Fix a race condition in nsPlacesExpiration that may cause intermittent failures. r=adw draft
authorMarco Bonardo <mbonardo@mozilla.com>
Wed, 26 Apr 2017 15:21:03 +0200
changeset 569972 222e73c84d5a56fd689a90e4a615682121605777
parent 568509 0f5ba06c4c5959030a05cb852656d854065e2226
child 626364 fe85975973b9d3e7cd321798398b672ce3e689ae
push id56351
push usermak77@bonardo.net
push dateFri, 28 Apr 2017 07:39:52 +0000
reviewersadw
bugs1284083
milestone55.0a1
Bug 1284083 - Fix a race condition in nsPlacesExpiration that may cause intermittent failures. r=adw MozReview-Commit-ID: GxLkokwrxuA
toolkit/components/places/nsPlacesExpiration.js
toolkit/components/places/tests/expiration/test_clearHistory.js
toolkit/components/places/tests/expiration/test_pref_maxpages.js
toolkit/components/places/tests/expiration/xpcshell.ini
--- a/toolkit/components/places/nsPlacesExpiration.js
+++ b/toolkit/components/places/nsPlacesExpiration.js
@@ -492,17 +492,17 @@ function nsPlacesExpiration() {
   XPCOMUtils.defineLazyServiceGetter(this, "_idle",
                                      "@mozilla.org/widget/idleservice;1",
                                      "nsIIdleService");
 
   this._prefBranch = Cc["@mozilla.org/preferences-service;1"].
                      getService(Ci.nsIPrefService).
                      getBranch(PREF_BRANCH);
 
-  this._loadPrefs().then(() => {
+  this._loadPrefsPromise = this._loadPrefs().then(() => {
     // Observe our preferences branch for changes.
     this._prefBranch.addObserver("", this, true);
 
     // Create our expiration timer.
     this._newTimer();
   }, Cu.reportError);
 
   // Register topic observers.
@@ -535,17 +535,17 @@ nsPlacesExpiration.prototype = {
         Date.now() - this._lastClearHistoryTime <
           SHUTDOWN_WITH_RECENT_CLEARHISTORY_TIMEOUT_SECONDS * 1000;
       if (!hasRecentClearHistory && this.status == STATUS.DIRTY) {
         this._expireWithActionAndLimit(ACTION.SHUTDOWN_DIRTY, LIMIT.LARGE);
       }
 
       this._finalizeInternalStatements();
     } else if (aTopic == TOPIC_PREF_CHANGED) {
-      this._loadPrefs().then(() => {
+      this._loadPrefsPromise = this._loadPrefs().then(() => {
         if (aData == PREF_INTERVAL_SECONDS) {
           // Renew the timer with the new interval value.
           this._newTimer();
         }
       }, Cu.reportError);
     } else if (aTopic == TOPIC_DEBUG_START_EXPIRATION) {
       // The passed-in limit is the maximum number of visits to expire when
       // history is over capacity.  Mind to correctly handle the NaN value.
@@ -700,17 +700,16 @@ nsPlacesExpiration.prototype = {
     Cu.reportError("Async statement execution returned with '" +
                    aError.result + "', '" + aError.message + "'");
   },
 
   // Number of expiration steps needed to reach a CLEAN status.
   _telemetrySteps: 1,
   handleCompletion: function PEX_handleCompletion(aReason) {
     if (aReason == Ci.mozIStorageStatementCallback.REASON_FINISHED) {
-
       if (this._mostRecentExpiredVisitDays) {
         try {
           Services.telemetry
                   .getHistogramById("PLACES_MOST_RECENT_EXPIRED_VISIT_DAYS")
                   .add(this._mostRecentExpiredVisitDays);
         } catch (ex) {
           Components.utils.reportError("Unable to report telemetry.");
         } finally {
@@ -799,22 +798,21 @@ nsPlacesExpiration.prototype = {
   get expireOnIdle() {
     return this._expireOnIdle;
   },
 
   _loadPrefs: Task.async(function* () {
     // Get the user's limit, if it was set.
     this._urisLimit = this._prefBranch.getIntPref(PREF_MAX_URIS,
                                                   PREF_MAX_URIS_NOTSET);
-
     if (this._urisLimit < 0) {
       // Some testing code expects a pref change to be synchronous, so
       // temporarily set this to a large value, while we asynchronously update
       // to the correct value.
-      this._urisLimit = 300000;
+      this._urisLimit = 100000;
 
       // The user didn't specify a custom limit, so we calculate the number of
       // unique places that may fit an optimal database size on this hardware.
       // Oldest pages over this threshold will be expired.
       let memSizeBytes = MEMSIZE_FALLBACK_BYTES;
       try {
         // Limit the size on systems with small memory.
          memSizeBytes = Services.sysinfo.getProperty("memsize");
@@ -836,29 +834,40 @@ nsPlacesExpiration.prototype = {
 
       let optimalDatabaseSize = Math.min(
         memSizeBytes * DATABASE_TO_MEMORY_PERC / 100,
         diskAvailableBytes * DATABASE_TO_DISK_PERC / 100,
         DATABASE_MAX_SIZE
       );
 
       // Calculate avg size of a URI in the database.
-      let db = yield PlacesUtils.promiseDBConnection();
-      let pageSize = (yield db.execute(`PRAGMA page_size`))[0].getResultByIndex(0);
-      let pageCount = (yield db.execute(`PRAGMA page_count`))[0].getResultByIndex(0);
-      let freelistCount = (yield db.execute(`PRAGMA freelist_count`))[0].getResultByIndex(0);
-      let dbSize = (pageCount - freelistCount) * pageSize;
-      let uriCount = (yield db.execute(`SELECT count(*) FROM moz_places`))[0].getResultByIndex(0);
-      let avgURISize = Math.ceil(dbSize / uriCount);
-      // For new profiles this value may be too large, due to the Sqlite header,
-      // or Infinity when there are no pages.  Thus we must limit it.
-      if (avgURISize > (URIENTRY_AVG_SIZE * 3)) {
-        avgURISize = URIENTRY_AVG_SIZE;
+      let db;
+      try {
+        db = yield PlacesUtils.promiseDBConnection();
+      } catch (ex) {
+        // We may have been initialized late in the shutdown process, maybe
+        // by a call to clear history on shutdown.
+        // If we're unable to get a connection clone, we'll just proceed with
+        // the default value, it should not be critical at this point in the
+        // application life-cycle.
       }
-      this._urisLimit = Math.ceil(optimalDatabaseSize / avgURISize);
+      if (db) {
+        let pageSize = (yield db.execute(`PRAGMA page_size`))[0].getResultByIndex(0);
+        let pageCount = (yield db.execute(`PRAGMA page_count`))[0].getResultByIndex(0);
+        let freelistCount = (yield db.execute(`PRAGMA freelist_count`))[0].getResultByIndex(0);
+        let dbSize = (pageCount - freelistCount) * pageSize;
+        let uriCount = (yield db.execute(`SELECT count(*) FROM moz_places`))[0].getResultByIndex(0);
+        let avgURISize = Math.ceil(dbSize / uriCount);
+        // For new profiles this value may be too large, due to the Sqlite header,
+        // or Infinity when there are no pages.  Thus we must limit it.
+        if (avgURISize > (URIENTRY_AVG_SIZE * 3)) {
+          avgURISize = URIENTRY_AVG_SIZE;
+        }
+        this._urisLimit = Math.ceil(optimalDatabaseSize / avgURISize);
+      }
     }
 
     // Expose the calculated limit to other components.
     this._prefBranch.setIntPref(PREF_READONLY_CALCULATED_MAX_URIS,
                                 this._urisLimit);
 
     // Get the expiration interval value.
     this._interval = this._prefBranch.getIntPref(PREF_INTERVAL_SECONDS,
@@ -909,32 +918,42 @@ nsPlacesExpiration.prototype = {
    * @param aAction
    *        The ACTION we are expiring for.  See the ACTION const for values.
    * @param aLimit
    *        Whether to use small, large or no limits when expiring.  See the
    *        LIMIT const for values.
    */
   _expireWithActionAndLimit:
   function PEX__expireWithActionAndLimit(aAction, aLimit) {
-    // Skip expiration during batch mode.
-    if (this._inBatchMode)
-      return;
-    // Don't try to further expire after shutdown.
-    if (this._shuttingDown && aAction != ACTION.SHUTDOWN_DIRTY) {
-      return;
-    }
+    Task.spawn(function*() {
+      // Ensure that we'll run statements with the most up-to-date pref values.
+      // On shutdown we cannot do this, since we must enqueue the expiration
+      // statements synchronously before the connection goes away.
+      // TODO (Bug 1275878): handle this properly through a shutdown blocker.
+      if (!this._shuttingDown)
+        yield this._loadPrefsPromise;
 
-    let boundStatements = [];
-    for (let queryType in EXPIRATION_QUERIES) {
-      if (EXPIRATION_QUERIES[queryType].actions & aAction)
-        boundStatements.push(this._getBoundStatement(queryType, aLimit, aAction));
-    }
+      // Skip expiration during batch mode.
+      if (this._inBatchMode)
+        return;
+      // Don't try to further expire after shutdown.
+      if (this._shuttingDown && aAction != ACTION.SHUTDOWN_DIRTY) {
+        return;
+      }
 
-    // Execute statements asynchronously in a transaction.
-    this._db.executeAsync(boundStatements, boundStatements.length, this);
+      let boundStatements = [];
+      for (let queryType in EXPIRATION_QUERIES) {
+        if (EXPIRATION_QUERIES[queryType].actions & aAction)
+          boundStatements.push(this._getBoundStatement(queryType, aLimit, aAction));
+      }
+
+
+      // Execute statements asynchronously in a transaction.
+      this._db.executeAsync(boundStatements, boundStatements.length, this);
+    }.bind(this)).catch(Cu.reportError);
   },
 
   /**
    * Finalizes all of our mozIStorageStatements so we can properly close the
    * database.
    */
   _finalizeInternalStatements: function PEX__finalizeInternalStatements() {
     for (let queryType in this._cachedStatements) {
--- a/toolkit/components/places/tests/expiration/test_clearHistory.js
+++ b/toolkit/components/places/tests/expiration/test_clearHistory.js
@@ -69,20 +69,16 @@ function add_old_anno(aIdentifier, aName
   stmt.params.anno_name = aName;
   try {
     stmt.executeStep();
   } finally {
     stmt.finalize();
   }
 }
 
-function run_test() {
-  run_next_test();
-}
-
 add_task(function* test_historyClear() {
   // Set interval to a large value so we don't expire on it.
   setInterval(3600); // 1h
 
   // Expire all expirable pages.
   setMaxPages(0);
 
   // Add some bookmarked page with visit and annotations.
@@ -121,17 +117,17 @@ add_task(function* test_historyClear() {
     as.setPageAnnotation(pageURI, "expire", "test", 0, as.EXPIRE_NEVER);
     as.setPageAnnotation(pageURI, "expire_session", "test", 0, as.EXPIRE_SESSION);
     add_old_anno(pageURI, "expire_days", "test", as.EXPIRE_DAYS, 8);
     add_old_anno(pageURI, "expire_weeks", "test", as.EXPIRE_WEEKS, 31);
     add_old_anno(pageURI, "expire_months", "test", as.EXPIRE_MONTHS, 181);
   }
 
   // Expire all visits for the bookmarks
-  yield PlacesUtils.history.clear();
+  yield PlacesTestUtils.clearHistory();
 
   ["expire_days", "expire_weeks", "expire_months", "expire_session",
    "expire"].forEach(function(aAnno) {
     let pages = as.getPagesWithAnnotation(aAnno);
     do_check_eq(pages.length, 0);
   });
 
   ["expire_days", "expire_weeks", "expire_months", "expire_session",
--- a/toolkit/components/places/tests/expiration/test_pref_maxpages.js
+++ b/toolkit/components/places/tests/expiration/test_pref_maxpages.js
@@ -9,19 +9,16 @@
  *
  * Expiration will obey to hardware spec, but user can set a custom maximum
  * number of pages to retain, to restrict history, through
  * "places.history.expiration.max_pages".
  * This limit is used at next expiration run.
  * If the pref is set to a number < 0 we will use the default value.
  */
 
-var hs = Cc["@mozilla.org/browser/nav-history-service;1"].
-         getService(Ci.nsINavHistoryService);
-
 var tests = [
 
   { desc: "Set max_pages to a negative value, with 1 page.",
     maxPages: -1,
     addPages: 1,
     expectedNotifications: 0, // Will ignore and won't expire anything.
   },
 
@@ -55,20 +52,16 @@ var tests = [
 
   { desc: "Set max_pages to 10 with 10 pages.",
     maxPages: 10,
     addPages: 10,
     expectedNotifications: 0, // We are below the limit, won't expire anything.
   },
 ];
 
-function run_test() {
-  run_next_test();
-}
-
 add_task(function* test_pref_maxpages() {
   // The pref should not exist by default.
   try {
     getMaxPages();
     do_throw("interval pref should not exist by default");
   } catch (ex) {}
 
   // Set interval to a large value so we don't expire on it.
@@ -78,43 +71,36 @@ add_task(function* test_pref_maxpages() 
     let currentTest = tests[testIndex - 1];
     print("\nTEST " + testIndex + ": " + currentTest.desc);
     currentTest.receivedNotifications = 0;
 
     // Setup visits.
     let now = getExpirablePRTime();
     for (let i = 0; i < currentTest.addPages; i++) {
       let page = "http://" + testIndex + "." + i + ".mozilla.org/";
-      yield PlacesTestUtils.addVisits({ uri: uri(page), visitDate: now++ });
+      yield PlacesTestUtils.addVisits({ uri: uri(page), visitDate: now-- });
     }
 
     // Observe history.
-    let historyObserver = {
-      onBeginUpdateBatch: function PEX_onBeginUpdateBatch() {},
-      onEndUpdateBatch: function PEX_onEndUpdateBatch() {},
-      onClearHistory() {},
-      onVisit() {},
-      onTitleChanged() {},
-      onDeleteURI(aURI) {
-        print("onDeleteURI " + aURI.spec);
-        currentTest.receivedNotifications++;
-      },
-      onPageChanged() {},
-      onDeleteVisits(aURI, aTime) {
-        print("onDeleteVisits " + aURI.spec + " " + aTime);
-      },
+    let historyObserver = new NavHistoryObserver();
+    historyObserver.onDeleteURI = aURI => {
+      print("onDeleteURI " + aURI.spec);
+      currentTest.receivedNotifications++;
     };
-    hs.addObserver(historyObserver);
+    historyObserver.onDeleteVisits = (aURI, aTime) => {
+      print("onDeleteVisits " + aURI.spec + " " + aTime);
+    };
+    PlacesUtils.history.addObserver(historyObserver);
 
     setMaxPages(currentTest.maxPages);
 
     // Expire now.
     yield promiseForceExpirationStep(-1);
 
-    hs.removeObserver(historyObserver, false);
+    PlacesUtils.history.removeObserver(historyObserver, false);
 
     do_check_eq(currentTest.receivedNotifications,
                 currentTest.expectedNotifications);
 
     // Clean up.
     yield PlacesTestUtils.clearHistory();
   }
 
--- a/toolkit/components/places/tests/expiration/xpcshell.ini
+++ b/toolkit/components/places/tests/expiration/xpcshell.ini
@@ -11,9 +11,8 @@ skip-if = toolkit == 'android'
 [test_debug_expiration.js]
 [test_idle_daily.js]
 [test_notifications.js]
 [test_notifications_onDeleteURI.js]
 [test_notifications_onDeleteVisits.js]
 [test_outdated_analyze.js]
 [test_pref_interval.js]
 [test_pref_maxpages.js]
-skip-if = (os == 'win' && debug) || (os == 'linux') # bug 1284083