Bug 1635408 - abort non-critical RemoteSettingsWorker tasks at shutdown, r=leplatrem

Differential Revision: https://phabricator.services.mozilla.com/D73858

services/settings/RemoteSettingsWorker.js

@@ -84,8 +84,7 @@ const Agent = {
       return false;
     }
     const buffer = await resp.arrayBuffer();
-    const bytes = new Uint8Array(buffer);
-    return this.checkContentHash(bytes, size, hash);
+    return this.checkContentHash(buffer, size, hash);
   },
 
   /**

services/settings/RemoteSettingsWorker.jsm

@@ -55,17 +55,19 @@ class Worker {
     this.idleTimeoutId = null;
   }
 
-  async _execute(method, args = []) {
+  async _execute(method, args = [], options = {}) {
     if (gShutdown) {
       throw new RemoteSettingsWorkerError("Remote Settings has shut down.");
     }
+    const { mustComplete = false } = options;
+
     // (Re)instantiate the worker if it was terminated.
     if (!this.worker) {
       this.worker = new ChromeWorker(this.source);
       this.worker.onmessage = this._onWorkerMessage.bind(this);
       this.worker.onerror = error => {
         // Worker crashed. Reject each pending callback.
-        for (const [, reject] of this.callbacks.values()) {
+        for (const { reject } of this.callbacks.values()) {
           reject(error);
         }
         this.callbacks.clear();
@@ -77,16 +79,21 @@ class Worker {
     if (this.idleTimeoutId) {
       clearTimeout(this.idleTimeoutId);
     }
+    let identifier = method + "-";
+    // Include the collection details in the importJSONDump case.
+    if (identifier == "importJSONDump") {
+      identifier += `${args[0]}-${args[1]}-`;
+    }
     return new Promise((resolve, reject) => {
-      const callbackId = `${method}-${++this.lastCallbackId}`;
-      this.callbacks.set(callbackId, [resolve, reject]);
+      const callbackId = `${identifier}${++this.lastCallbackId}`;
+      this.callbacks.set(callbackId, { resolve, reject, mustComplete });
       this.worker.postMessage({ callbackId, method, args });
     });
   }
 
   _onWorkerMessage(event) {
     const { callbackId, result, error } = event.data;
-    const [resolve, reject] = this.callbacks.get(callbackId);
+    const { resolve, reject } = this.callbacks.get(callbackId);
     if (error) {
       reject(new RemoteSettingsWorkerError(error));
     } else {
@@ -110,6 +117,31 @@ class Worker {
     }
   }
 
+  /**
+   * Called at shutdown to abort anything the worker is doing that isn't
+   * critical.
+   */
+  _abortCancelableRequests() {
+    // End all tasks that we can.
+    const callbackCopy = Array.from(this.callbacks.entries());
+    const error = new Error("Shutdown, aborting read-only worker requests.");
+    for (const [id, { reject, mustComplete }] of callbackCopy) {
+      if (!mustComplete) {
+        this.callbacks.delete(id);
+        reject(error);
+      }
+    }
+    // There might be nothing left now:
+    if (!this.callbacks.size) {
+      this.stop();
+      if (gShutdownResolver) {
+        gShutdownResolver();
+      }
+    }
+    // If there was something left, we'll stop as soon as we get messages from
+    // those tasks, too.
+  }
+
   stop() {
     this.worker.terminate();
     this.worker = null;
@@ -125,7 +157,9 @@ class Worker {
   }
 
   async importJSONDump(bucket, collection) {
-    return this._execute("importJSONDump", [bucket, collection]);
+    return this._execute("importJSONDump", [bucket, collection], {
+      mustComplete: true,
+    });
   }
 
   async checkFileHash(filepath, size, hash) {
@@ -156,10 +190,16 @@ try {
       ) {
         return null;
       }
-      // Otherwise, return a promise that the worker will resolve.
-      return new Promise(resolve => {
+      // Otherwise, there's something left to do. Set up a promise:
+      let finishedPromise = new Promise(resolve => {
         gShutdownResolver = resolve;
       });
+
+      // Try to cancel most of the work:
+      RemoteSettingsWorker._abortCancelableRequests();
+
+      // Return a promise that the worker will resolve.
+      return finishedPromise;
     },
     {
       fetchState() {

services/settings/test/unit/test_shutdown_handling.js

@@ -3,9 +3,18 @@ http://creativecommons.org/publicdomain/zero/1.0/ */
 
 "use strict";
 
+const { AppConstants } = ChromeUtils.import(
+  "resource://gre/modules/AppConstants.jsm"
+);
 const { Database } = ChromeUtils.import(
   "resource://services-settings/Database.jsm"
 );
+const { RemoteSettingsWorker } = ChromeUtils.import(
+  "resource://services-settings/RemoteSettingsWorker.jsm"
+);
+const { RemoteSettingsClient } = ChromeUtils.import(
+  "resource://services-settings/RemoteSettingsClient.jsm"
+);
 
 add_task(async function test_shutdown_abort_after_start() {
   // Start a forever transaction:
@@ -82,4 +91,41 @@ add_task(async function test_shutdown_immediate_abort() {
     rejection = e;
   });
   ok(rejection, "Directly aborted promise should also have rejected.");
+  // Now clear the shutdown flag and rejection error:
+  Database._cancelShutdown();
+});
+
+add_task(async function test_shutdown_worker() {
+  // Fallback for android:
+  let importPromise = Promise.resolve();
+  let client;
+  // Android has no dumps, so only run the import if we do have dumps:
+  if (AppConstants.platform != "android") {
+    client = new RemoteSettingsClient("language-dictionaries", {
+      bucketNamePref: "services.settings.default_bucket",
+    });
+    const before = await client.get({ syncIfEmpty: false });
+    Assert.equal(before.length, 0);
+
+    importPromise = RemoteSettingsWorker.importJSONDump(
+      "main",
+      "language-dictionaries"
+    );
+  }
+  let stringifyPromise = RemoteSettingsWorker.canonicalStringify(
+    [],
+    [],
+    Date.now()
+  );
+  RemoteSettingsWorker._abortCancelableRequests();
+  await Assert.rejects(
+    stringifyPromise,
+    /Shutdown/,
+    "Should have aborted the stringify request at shutdown."
+  );
+  await importPromise.catch(e => ok(false, "importing failed!"));
+  if (client) {
+    const after = await client.get();
+    Assert.ok(after.length > 0);
+  }
 });