Bug 1089695 - Fixing wrong dependency in Places shutdown. r=mak
☠☠ backed out by 764e07ab1ad0 ☠ ☠
authorDavid Rajchenbach-Teller <dteller@mozilla.com>
Mon, 10 Aug 2015 11:07:54 +0200
changeset 260139 78c891ba5de1e144c74a997653f05ded12e62504
parent 260138 83552c5bb31de8da251a22012e875710961bfbca
child 260140 6b768986595f2166774b1e473ecffa6e99d60d36
push id14948
push userryanvm@gmail.com
push dateMon, 31 Aug 2015 20:36:20 +0000
treeherderfx-team@1daf4a74ada5 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmak
bugs1089695
milestone43.0a1
Bug 1089695 - Fixing wrong dependency in Places shutdown. r=mak
browser/base/content/sanitize.js
browser/components/places/tests/unit/test_clearHistory_shutdown.js
toolkit/components/places/PlacesUtils.jsm
--- a/browser/base/content/sanitize.js
+++ b/browser/base/content/sanitize.js
@@ -60,17 +60,23 @@ Sanitizer.prototype = {
   sanitize: function (aItemsToClear)
   {
     var deferred = Promise.defer();
     var seenError = false;
     if (Array.isArray(aItemsToClear)) {
       var itemsToClear = [...aItemsToClear];
     } else {
       let branch = Services.prefs.getBranch(this.prefDomain);
-      itemsToClear = Object.keys(this.items).filter(itemName => branch.getBoolPref(itemName));
+      itemsToClear = Object.keys(this.items).filter(itemName => {
+        try {
+          return branch.getBoolPref(itemName);
+        } catch (ex) {
+          return false;
+        }
+      });
     }
 
     // Ensure open windows get cleared first, if they're in our list, so that they don't stick
     // around in the recently closed windows list, and so we can cancel the whole thing
     // if the user selects to keep a window open from a beforeunload prompt.
     let openWindowsIndex = itemsToClear.indexOf("openWindows");
     if (openWindowsIndex != -1) {
       itemsToClear.splice(openWindowsIndex, 1);
--- a/browser/components/places/tests/unit/test_clearHistory_shutdown.js
+++ b/browser/components/places/tests/unit/test_clearHistory_shutdown.js
@@ -66,45 +66,37 @@ add_task(function* test_execute() {
   for (let aUrl of URIS) {
     yield PlacesTestUtils.addVisits({
       uri: uri(aUrl), visitDate: timeInMicroseconds++,
       transition: PlacesUtils.history.TRANSITION_TYPED
     });
   }
   do_print("Add cache.");
   yield storeCache(FTP_URL, "testData");
-});
 
-add_task(function* run_test_continue() {
   do_print("Simulate and wait shutdown.");
   yield shutdownPlaces();
 
-  let stmt = DBConn().createStatement(
+  let stmt = DBConn(true).createStatement(
     "SELECT id FROM moz_places WHERE url = :page_url "
   );
 
   try {
     URIS.forEach(function(aUrl) {
       stmt.params.page_url = aUrl;
       do_check_false(stmt.executeStep());
       stmt.reset();
     });
   } finally {
     stmt.finalize();
   }
 
   do_print("Check cache");
   // Check cache.
-  let promiseCacheChecked = checkCache(FTP_URL);
-
-  do_print("Shutdown the download manager");
-  // Shutdown the download manager.
-  Services.obs.notifyObservers(null, "quit-application", null);
-
-  yield promiseCacheChecked;
+  yield checkCache(FTP_URL);
 });
 
 function storeCache(aURL, aContent) {
   let cache = Services.cache2;
   let storage = cache.diskCacheStorage(LoadContextInfo.default, false);
 
   return new Promise(resolve => {
     let storeCacheListener = {
--- a/toolkit/components/places/PlacesUtils.jsm
+++ b/toolkit/components/places/PlacesUtils.jsm
@@ -2066,51 +2066,82 @@ XPCOMUtils.defineLazyGetter(PlacesUtils,
 
 XPCOMUtils.defineLazyGetter(this, "bundle", function() {
   const PLACES_STRING_BUNDLE_URI = "chrome://places/locale/places.properties";
   return Cc["@mozilla.org/intl/stringbundle;1"].
          getService(Ci.nsIStringBundleService).
          createBundle(PLACES_STRING_BUNDLE_URI);
 });
 
+// A promise resolved once the Sqlite.jsm connections
+// can be closed.
+let promiseCanCloseConnection = function() {
+  let TOPIC = "places-will-close-connection";
+  return new Promise(resolve => {
+    let observer = function() {
+      Services.obs.removeObserver(observer, TOPIC);
+      resolve();
+    }
+    Services.obs.addObserver(observer, TOPIC, false)
+  });
+};
+
 XPCOMUtils.defineLazyGetter(this, "gAsyncDBConnPromised",
   () => new Promise((resolve) => {
     Sqlite.cloneStorageConnection({
       connection: PlacesUtils.history.DBConnection,
       readOnly:   true
     }).then(conn => {
       try {
-        Sqlite.shutdown.addBlocker(
-          "PlacesUtils read-only connection closing",
-          conn.close.bind(conn));
-        PlacesUtils.history.shutdownClient.jsclient.addBlocker(
-          "PlacesUtils read-only connection closing",
-          conn.close.bind(conn));
+        let promiseReady = promiseCanCloseConnection();
+        let state = "0. not started";
+        Sqlite.shutdown.addBlocker("PlacesUtils read-only connection closing",
+          Task.async(function*() {
+            // Don't close the connection as long as it might be used.
+            state = "1. waiting for `places-will-close-connection`";
+            yield promiseReady;
+
+            // But close the connection before Sqlite shutdown.
+            state = "2. closing the connection";
+            yield conn.close();
+
+            state = "3. done";
+          }),
+          () => state);
       } catch(ex) {
         // It's too late to block shutdown, just close the connection.
         conn.close();
         throw ex;
       }
       resolve(conn);
     });
   })
 );
 
 XPCOMUtils.defineLazyGetter(this, "gAsyncDBWrapperPromised",
   () => new Promise((resolve) => {
     Sqlite.wrapStorageConnection({
       connection: PlacesUtils.history.DBConnection,
     }).then(conn => {
       try {
-        Sqlite.shutdown.addBlocker(
-          "PlacesUtils wrapped connection closing",
-          conn.close.bind(conn));
-        PlacesUtils.history.shutdownClient.jsclient.addBlocker(
-          "PlacesUtils wrapped connection closing",
-          conn.close.bind(conn));
+        let promiseReady = promiseCanCloseConnection();
+        let state = "0. not started";
+        Sqlite.shutdown.addBlocker("PlacesUtils wrapped connection closing",
+          Task.async(function*() {
+            // Don't close the connection as long as it might be used.
+            state = "1. waiting for `places-will-close-connection`";
+            yield promiseReady;
+
+            // But close the connection before Sqlite shutdown.
+            state = "2. closing the connection";
+            yield conn.close();
+
+            state = "3. done";
+          }),
+          () => state);
       } catch(ex) {
         // It's too late to block shutdown, just close the connection.
         conn.close();
         throw ex;
       }
       resolve(conn);
     });
   })