Bug 1598924 - make remote settings handle shutdown gracefully, r=leplatrem,asuth
authorGijs Kruitbosch <gijskruitbosch@gmail.com>
Tue, 24 Mar 2020 14:55:41 +0000
changeset 520221 354489628b3985c7dc385f039f763b15c05ccec7
parent 520220 eb0786104e9e5cd6f974009b8e75fbb8d35f5837
child 520222 87f1271cf700a606444f54f15902918479baaab6
push id37245
push useropoprus@mozilla.com
push dateTue, 24 Mar 2020 21:46:41 +0000
treeherdermozilla-central@dbabf2e388fa [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersleplatrem, asuth
bugs1598924
milestone76.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1598924 - make remote settings handle shutdown gracefully, r=leplatrem,asuth By and large, this change accomplishes two things: 1. Run db.close() in finally clauses so that even if db access fails, we close our connections. It also tries to avoid waiting on other, non-DB operations before calling close, to avoid the DB connection needlessly hanging around. 2. Intercept all async database operations from the remote settings client to kinto and ensuring they complete before the end of `profile-before-change`. Any operations started after Services.startup.isShuttingDown (so after quit/restart is initiated by the user) will throw. Operations started beforehand are put in a set of operations, and remove themselves once complete. We AsyncShutdown block on that set of operations completing. Differential Revision: https://phabricator.services.mozilla.com/D66995
services/settings/Database.jsm
services/settings/RemoteSettingsClient.jsm
services/settings/test/unit/test_remote_settings.js
toolkit/components/url-classifier/tests/unit/test_features.js
--- a/services/settings/Database.jsm
+++ b/services/settings/Database.jsm
@@ -1,19 +1,21 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 const { XPCOMUtils } = ChromeUtils.import(
   "resource://gre/modules/XPCOMUtils.jsm"
 );
+const { Services } = ChromeUtils.import("resource://gre/modules/Services.jsm");
 
 XPCOMUtils.defineLazyGlobalGetters(this, ["indexedDB"]);
 
 XPCOMUtils.defineLazyModuleGetters(this, {
+  AsyncShutdown: "resource://gre/modules/AsyncShutdown.jsm",
   CommonUtils: "resource://services-common/utils.js",
 });
 
 var EXPORTED_SYMBOLS = ["Database"];
 
 // IndexedDB name.
 const DB_NAME = "remote-settings";
 const DB_VERSION = 2;
@@ -24,22 +26,28 @@ const DB_VERSION = 2;
 class IndexedDBError extends Error {
   constructor(error, method = "") {
     super(`IndexedDB: ${method} ${error.message}`);
     this.name = error.name;
     this.stack = error.stack;
   }
 }
 
+class ShutdownError extends IndexedDBError {
+  constructor(error, method) {
+    super(error, method);
+  }
+}
+
 /**
- * Database is a tiny wrapper with the objective
+ * InternalDatabase is a tiny wrapper with the objective
  * of providing major kinto-offline-client collection API.
  * (with the objective of getting rid of kinto-offline-client)
  */
-class Database {
+class InternalDatabase {
   /* Expose the IDBError class publicly */
   static get IDBError() {
     return IndexedDBError;
   }
 
   constructor(identifier) {
     this.identifier = identifier;
     this._idb = null;
@@ -400,8 +408,117 @@ function sortObjects(order, list) {
       return -direction;
     }
     if (_isUndefined(a[field]) && _isUndefined(b[field])) {
       return 0;
     }
     return a[field] > b[field] ? direction : -direction;
   });
 }
+
+const gPendingOperations = new Set();
+const kWrappedMethods = new Set([
+  "open",
+  "close",
+  "list",
+  "importBulk",
+  "deleteBulk",
+  "getLastModified",
+  "saveLastModified",
+  "getMetadata",
+  "saveMetadata",
+  "clear",
+  "create",
+  "update",
+  "delete",
+]);
+
+/**
+ * We need to block shutdown on any pending indexeddb operations, and also
+ * want to avoid starting new ones if we already know we're going to be
+ * shutting down. In order to do this, we wrap all the database objects
+ * we create in a proxy that lets us catch any pending operations, and remove
+ * them when finished.
+ */
+function Database(identifier) {
+  ensureShutdownBlocker();
+  let db = new InternalDatabase(identifier);
+  // This helper returns a function that gets executed when consumers runs an
+  // async operation on the underlying database. It throws if run after
+  // shutdown has started. Otherwise, it wraps the result from the "real"
+  // method call and adds it as a pending operation, removing it when the
+  // operation is done.
+  function _getWrapper(target, method) {
+    return function(...args) {
+      // For everything other than `close`, check if we're shutting down:
+      if (method != "close" && Services.startup.shuttingDown) {
+        throw new ShutdownError(
+          "The application is shutting down, can't call " + method
+        );
+      }
+      // We're not shutting down yet. Kick off the actual method call,
+      // and stash the result.
+      let promise = target[method].apply(target, args);
+      // Now put a "safe" wrapper of the promise in our shutdown tracker,
+      let safePromise = promise.catch(ex => Cu.reportError(ex));
+      let operationInfo = {
+        promise: safePromise,
+        method,
+        identifier,
+      };
+      gPendingOperations.add(operationInfo);
+      // And ensure we remove it once done.
+      safePromise.then(() => gPendingOperations.delete(operationInfo));
+      // Now return the original.
+      return promise;
+    };
+  }
+
+  return new Proxy(db, {
+    get(target, property, receiver) {
+      // For tests so they can stub methods. Otherwise, we end up fetching
+      // wrappers from the proxy, and then assigning them onto the real
+      // object when the test is finished, and then callers to that method
+      // hit infinite recursion...
+      if (property == "_wrappedDBForTestOnly") {
+        return target;
+      }
+      if (kWrappedMethods.has(property)) {
+        return _getWrapper(target, property);
+      }
+      if (typeof target[property] == "function") {
+        // If we get here, someone's tried to use some other operation that we
+        // don't yet know how to proxy. Throw to ensure we fix that:
+        throw new Error(
+          `Don't know how to process access to 'database.${property}()'` +
+            " Add to kWrappedMethods or otherwise update the proxy to deal."
+        );
+      }
+      return target[property];
+    },
+  });
+}
+
+Database.IDBError = InternalDatabase.IDBError;
+Database.ShutdownError = ShutdownError;
+
+let gShutdownBlocker = false;
+function ensureShutdownBlocker() {
+  if (gShutdownBlocker) {
+    return;
+  }
+  gShutdownBlocker = true;
+  AsyncShutdown.profileBeforeChange.addBlocker(
+    "RemoteSettingsClient - finish IDB access.",
+    () => {
+      return Promise.all(Array.from(gPendingOperations).map(op => op.promise));
+    },
+    {
+      fetchState() {
+        let info = [];
+        for (let op of gPendingOperations) {
+          info.push({ method: op.method, identifier: op.identifier });
+        }
+        return info;
+      },
+    }
+  );
+}
--- a/services/settings/RemoteSettingsClient.jsm
+++ b/services/settings/RemoteSettingsClient.jsm
@@ -172,18 +172,22 @@ class AttachmentDownloader extends Downl
 
   /**
    * Delete all downloaded records attachments.
    *
    * Note: the list of attachments to be deleted is based on the
    * current list of records.
    */
   async deleteAll() {
-    const allRecords = await this._client.db.list();
-    await this._client.db.close();
+    let allRecords;
+    try {
+      allRecords = await this._client.db.list();
+    } finally {
+      await this._client.db.close();
+    }
     return Promise.all(
       allRecords.filter(r => !!r.attachment).map(r => this.delete(r))
     );
   }
 }
 
 class RemoteSettingsClient extends EventEmitter {
   static get NetworkOfflineError() {
@@ -227,21 +231,17 @@ class RemoteSettingsClient extends Event
     // to preview the changes to be approved in a real client.
     this.bucketNamePref = bucketNamePref;
     XPCOMUtils.defineLazyPreferenceGetter(
       this,
       "bucketName",
       this.bucketNamePref
     );
 
-    XPCOMUtils.defineLazyGetter(
-      this,
-      "db",
-      () => new Database(this.identifier)
-    );
+    XPCOMUtils.defineLazyGetter(this, "db", () => Database(this.identifier));
 
     XPCOMUtils.defineLazyGetter(
       this,
       "attachments",
       () => new AttachmentDownloader(this)
     );
   }
 
@@ -267,22 +267,27 @@ class RemoteSettingsClient extends Event
    * @returns {number}
    *          The timestamp in milliseconds, returns -1 if retrieving
    *          the timestamp from the kinto collection fails.
    */
   async getLastModified() {
     let timestamp = -1;
     try {
       timestamp = await this.db.getLastModified();
-      await this.db.close();
     } catch (err) {
       console.warn(
         `Error retrieving the getLastModified timestamp from ${this.identifier} RemoteSettingClient`,
         err
       );
+    } finally {
+      try {
+        await this.db.close();
+      } catch (err) {
+        console.warn(`Couldn't close db connection`);
+      }
     }
 
     return timestamp;
   }
 
   /**
    * Lists settings.
    *
@@ -321,45 +326,50 @@ class RemoteSettingsClient extends Event
       } catch (e) {
         // Report but return an empty list since there will be no data anyway.
         Cu.reportError(e);
         return [];
       }
     }
 
     // Read from the local DB.
-    const data = await this.db.list({ filters, order });
+    let data, localRecords, timestamp, metadata;
+    try {
+      data = await this.db.list({ filters, order });
 
+      if (verifySignature) {
+        console.debug(
+          `${this.identifier} verify signature of local data on read`
+        );
+        const allData = ObjectUtils.isEmpty(filters)
+          ? data
+          : await this.db.list();
+        localRecords = allData.map(r => this._cleanLocalFields(r));
+        timestamp = await this.db.getLastModified();
+        metadata = await this.db.getMetadata();
+        if (syncIfEmpty && ObjectUtils.isEmpty(metadata)) {
+          // No sync occured yet, may have records from dump but no metadata.
+          console.debug(
+            `Required metadata for ${this.identifier}, fetching from server.`
+          );
+          metadata = await this.httpClient().getData();
+          await this.db.saveMetadata(metadata);
+        }
+      }
+    } finally {
+      await this.db.close();
+    }
     if (verifySignature) {
-      console.debug(
-        `${this.identifier} verify signature of local data on read`
-      );
-      const allData = ObjectUtils.isEmpty(filters)
-        ? data
-        : await this.db.list();
-      const localRecords = allData.map(r => this._cleanLocalFields(r));
-      const timestamp = await this.db.getLastModified();
-      let metadata = await this.db.getMetadata();
-      if (syncIfEmpty && ObjectUtils.isEmpty(metadata)) {
-        // No sync occured yet, may have records from dump but no metadata.
-        console.debug(
-          `Required metadata for ${this.identifier}, fetching from server.`
-        );
-        metadata = await this.httpClient().getData();
-        await this.db.saveMetadata(metadata);
-      }
       await this._validateCollectionSignature(
         localRecords,
         timestamp,
         metadata
       );
     }
 
-    await this.db.close();
-
     // Filter the records based on `this.filterFunc` results.
     return this._filterEntries(data);
   }
 
   /**
    * Synchronize the local database with the remote server.
    *
    * @param {Object} options See #maybeSync() options.
@@ -401,17 +411,17 @@ class RemoteSettingsClient extends Event
     if (this._syncRunning) {
       console.warn(`${this.identifier} sync already running`);
       return;
     }
 
     // Prevent network requests and IndexedDB calls to be initiated
     // during shutdown.
     if (Services.startup.shuttingDown) {
-      console.warn(`${this.identifier} sync interrputed by shutdown`);
+      console.warn(`${this.identifier} sync interrupted by shutdown`);
       return;
     }
 
     this._syncRunning = true;
 
     let importedFromDump = [];
     const startedAt = new Date();
     let reportStatus = null;
@@ -644,16 +654,18 @@ class RemoteSettingsClient extends Event
   _telemetryFromError(e, options = { default: null }) {
     let reportStatus = options.default;
 
     if (e instanceof RemoteSettingsClient.NetworkOfflineError) {
       reportStatus = UptakeTelemetry.STATUS.NETWORK_OFFLINE_ERROR;
     } else if (e instanceof RemoteSettingsClient.MissingSignatureError) {
       // Collection metadata has no signature info, no need to retry.
       reportStatus = UptakeTelemetry.STATUS.SIGNATURE_ERROR;
+    } else if (e instanceof Database.ShutdownError) {
+      reportStatus = UptakeTelemetry.STATUS.SHUTDOWN_ERROR;
     } else if (/unparseable/.test(e.message)) {
       reportStatus = UptakeTelemetry.STATUS.PARSE_ERROR;
     } else if (/NetworkError/.test(e.message)) {
       reportStatus = UptakeTelemetry.STATUS.NETWORK_ERROR;
     } else if (/Timeout/.test(e.message)) {
       reportStatus = UptakeTelemetry.STATUS.TIMEOUT_ERROR;
     } else if (/HTTP 5??/.test(e.message)) {
       reportStatus = UptakeTelemetry.STATUS.SERVER_ERROR;
--- a/services/settings/test/unit/test_remote_settings.js
+++ b/services/settings/test/unit/test_remote_settings.js
@@ -680,55 +680,55 @@ add_task(async function test_telemetry_r
 
   const endHistogram = getUptakeTelemetrySnapshot(client.identifier);
   const expectedIncrements = { [UptakeTelemetry.STATUS.SERVER_ERROR]: 1 };
   checkUptakeTelemetry(startHistogram, endHistogram, expectedIncrements);
 });
 add_task(clear_state);
 
 add_task(async function test_telemetry_reports_unknown_errors() {
-  const backup = client.db.list;
+  const backup = client.db._wrappedDBForTestOnly.list;
   client.db.list = () => {
     throw new Error("Internal");
   };
   const startHistogram = getUptakeTelemetrySnapshot(client.identifier);
 
   try {
     await client.maybeSync(2000);
   } catch (e) {}
 
-  client.db.list = backup;
+  client.db._wrappedDBForTestOnly.list = backup;
   const endHistogram = getUptakeTelemetrySnapshot(client.identifier);
   const expectedIncrements = { [UptakeTelemetry.STATUS.UNKNOWN_ERROR]: 1 };
   checkUptakeTelemetry(startHistogram, endHistogram, expectedIncrements);
 });
 add_task(clear_state);
 
 add_task(async function test_telemetry_reports_indexeddb_as_custom_1() {
-  const backup = client.db.getLastModified;
+  const backup = client.db._wrappedDBForTestOnly.getLastModified;
   const msg =
     "IndexedDB getLastModified() The operation failed for reasons unrelated to the database itself";
   client.db.getLastModified = () => {
     throw new Error(msg);
   };
   const startHistogram = getUptakeTelemetrySnapshot(client.identifier);
 
   try {
     await client.maybeSync(2000);
   } catch (e) {}
 
-  client.db.getLastModified = backup;
+  client.db._wrappedDBForTestOnly.getLastModified = backup;
   const endHistogram = getUptakeTelemetrySnapshot(client.identifier);
   const expectedIncrements = { [UptakeTelemetry.STATUS.CUSTOM_1_ERROR]: 1 };
   checkUptakeTelemetry(startHistogram, endHistogram, expectedIncrements);
 });
 add_task(clear_state);
 
 add_task(async function test_telemetry_reports_error_name_as_event_nightly() {
-  const backup = client.db.list;
+  const backup = client.db._wrappedDBForTestOnly.list;
   client.db.list = () => {
     const e = new Error("Some unknown error");
     e.name = "ThrownError";
     throw e;
   };
 
   await withFakeChannel("nightly", async () => {
     try {
@@ -746,17 +746,17 @@ add_task(async function test_telemetry_r
           trigger: "manual",
           duration: v => v >= 0,
           errorName: "ThrownError",
         },
       ],
     ]);
   });
 
-  client.db.list = backup;
+  client.db._wrappedDBForTestOnly.list = backup;
 });
 add_task(clear_state);
 
 add_task(async function test_bucketname_changes_when_bucket_pref_changes() {
   equal(client.bucketName, "main");
 
   Services.prefs.setCharPref(
     "services.settings.default_bucket",
--- a/toolkit/components/url-classifier/tests/unit/test_features.js
+++ b/toolkit/components/url-classifier/tests/unit/test_features.js
@@ -1,15 +1,20 @@
 /* Any copyright is dedicated to the Public Domain.
    http://creativecommons.org/publicdomain/zero/1.0/
 */
 
 "use strict";
 
 add_test(async _ => {
+  ok(
+    Services.cookies,
+    "Force the cookie service to be initialized to avoid issues later. " +
+      "See https://bugzilla.mozilla.org/show_bug.cgi?id=1621759#c3"
+  );
   Services.prefs.setBoolPref("browser.safebrowsing.passwords.enabled", true);
 
   let classifier = Cc["@mozilla.org/url-classifier/dbservice;1"].getService(
     Ci.nsIURIClassifier
   );
   ok(!!classifier, "We have the URI-Classifier");
 
   var tests = [