Bug 1090157 - History.remove should block async shutdown. r=mak

toolkit/components/places/History.jsm

@@ -65,6 +65,8 @@ this.EXPORTED_SYMBOLS = [ "History" ];
 const { classes: Cc, interfaces: Ci, results: Cr, utils: Cu } = Components;
 
 Cu.import("resource://gre/modules/XPCOMUtils.jsm");
+XPCOMUtils.defineLazyModuleGetter(this, "AsyncShutdown",
+                                  "resource://gre/modules/AsyncShutdown.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "Services",
                                   "resource://gre/modules/Services.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "NetUtil",
@@ -82,6 +84,21 @@ XPCOMUtils.defineLazyModuleGetter(this, "PlacesUtils",
                                   "resource://gre/modules/PlacesUtils.jsm");
 Cu.importGlobalProperties(["URL"]);
 
+/**
+ * Whenever we update or remove numerous pages, it is preferable
+ * to yield time to the main thread every so often to avoid janking.
+ * This constant determines the maximal number of notifications we
+ * may emit before we yield.
+ */
+const NOTIFICATION_CHUNK_SIZE = 300;
+
+/**
+ * Private shutdown barrier blocked by ongoing operations.
+ */
+XPCOMUtils.defineLazyGetter(this, "operationsBarrier", () =>
+  new AsyncShutdown.Barrier("Sqlite.jsm: wait until all connections are closed")
+);
+
 /**
  * Shared connection
  */
@@ -90,8 +107,19 @@ XPCOMUtils.defineLazyGetter(this, "DBConnPromised",
     Sqlite.wrapStorageConnection({ connection: PlacesUtils.history.DBConnection } )
           .then(db => {
       try {
-        Sqlite.shutdown.addBlocker("Places History.jsm: Closing database wrapper",
-                                   () => db.close());
+        Sqlite.shutdown.addBlocker(
+          "Places History.jsm: Closing database wrapper",
+          Task.async(function*() {
+            yield operationsBarrier.wait();
+            gIsClosed = true;
+            yield db.close();
+          }),
+          () => ({
+            fetchState: () => ({
+              isClosed: gIsClosed,
+              operations: operationsBarrier.state,
+            })
+          }));
       } catch (ex) {
         // It's too late to block shutdown of Sqlite, so close the connection
         // immediately.
@@ -103,6 +131,16 @@ XPCOMUtils.defineLazyGetter(this, "DBConnPromised",
   })
 );
 
+/**
+ * `true` once this module has been shutdown.
+ */
+let gIsClosed = false;
+function ensureModuleIsOpen() {
+  if (gIsClosed) {
+    throw new Error("History.jsm has been shutdown");
+  }
+}
+
 this.History = Object.freeze({
   /**
    * Fetch the available information for one page.
@@ -209,6 +247,8 @@ this.History = Object.freeze({
    *       is an empty array.
    */
   remove: function (pages, onResult = null) {
+    ensureModuleIsOpen();
+
     // Normalize and type-check arguments
     if (Array.isArray(pages)) {
       if (pages.length == 0) {
@@ -230,6 +270,8 @@ this.History = Object.freeze({
         urls.push(normalized.href);
       }
     }
+    let normalizedPages = {guids: guids, urls: urls};
+
     // At this stage, we know that either `guids` is not-empty
     // or `urls` is not-empty.
 
@@ -237,8 +279,29 @@ this.History = Object.freeze({
       throw new TypeError("Invalid function: " + onResult);
     }
 
-    // Now perform queries
-    return remove({guids: guids, urls: urls}, onResult);
+    return Task.spawn(function*() {
+      let promise = remove(normalizedPages, onResult);
+
+      operationsBarrier.client.addBlocker(
+        "History.remove",
+        promise,
+        {
+          // In case of crash, we do not want to upload information on
+          // which urls are being cleared, for privacy reasons. GUIDs
+          // are safe wrt privacy, but useless.
+          fetchState: () => ({
+            guids: guids.length,
+            urls: normalizedPages.urls.map(u => u.protocol),
+          })
+        });
+
+      try {
+        return (yield promise);
+      } finally {
+        // Cleanup the barrier.
+        operationsBarrier.client.removeBlocker(promise);
+      }
+    });
   },
 
   /**