Bug 1442443 - remove all event loop spinning from Async.js. r?eoger draft
authorMark Hammond <mhammond@skippinet.com.au>
Fri, 02 Mar 2018 10:20:18 +1100
changeset 762257 2bf2ed262d2ca6bfe9d824b6f43442c1316ad10d
parent 762250 2667f0b010c959940d7a12b4311d54a6abd74ac5
push id101113
push userbmo:markh@mozilla.com
push dateThu, 01 Mar 2018 23:21:06 +0000
reviewerseoger
bugs1442443
milestone60.0a1
Bug 1442443 - remove all event loop spinning from Async.js. r?eoger MozReview-Commit-ID: 9Sc9kfx47yU
services/common/async.js
services/sync/tests/unit/test_bookmark_tracker.js
services/sync/tests/unit/test_service_startup.js
services/sync/tps/extensions/tps/resource/modules/bookmarks.jsm
--- a/services/common/async.js
+++ b/services/common/async.js
@@ -1,21 +1,14 @@
 /* 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/. */
 
 var EXPORTED_SYMBOLS = ["Async"];
 
-// Constants for makeSyncCallback, waitForSyncCallback.
-const CB_READY = {};
-const CB_COMPLETE = {};
-const CB_FAIL = {};
-
-const REASON_ERROR = Ci.mozIStorageStatementCallback.REASON_ERROR;
-
 ChromeUtils.import("resource://gre/modules/Services.jsm");
 ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
 
 /*
  * Helpers for various async operations.
  */
 var Async = {
 
@@ -45,75 +38,16 @@ var Async = {
         let args = Array.slice(arguments).concat(callback);
         let f = funcs.shift();
         f.apply(thisObj, args);
       }
     };
   },
 
   /**
-   * Helpers for making asynchronous calls within a synchronous API possible.
-   *
-   * If you value your sanity, do not look closely at the following functions.
-   */
-
-  /**
-   * Create a sync callback that remembers state, in particular whether it has
-   * been called.
-   * The returned callback can be called directly passing an optional arg which
-   * will be returned by waitForSyncCallback().  The callback also has a
-   * .throw() method, which takes an error object and will cause
-   * waitForSyncCallback to fail with the error object thrown as an exception
-   * (but note that the .throw method *does not* itself throw - it just causes
-   * the wait function to throw).
-   */
-  makeSyncCallback: function makeSyncCallback() {
-    // The main callback remembers the value it was passed, and that it got data.
-    let onComplete = function onComplete(data) {
-      onComplete.state = CB_COMPLETE;
-      onComplete.value = data;
-    };
-
-    // Initialize private callback data in preparation for being called.
-    onComplete.state = CB_READY;
-    onComplete.value = null;
-
-    // Allow an alternate callback to trigger an exception to be thrown.
-    onComplete.throw = function onComplete_throw(data) {
-      onComplete.state = CB_FAIL;
-      onComplete.value = data;
-    };
-
-    return onComplete;
-  },
-
-  /**
-   * Wait for a sync callback to finish.
-   */
-  waitForSyncCallback: function waitForSyncCallback(callback) {
-    // Grab the current thread so we can make it give up priority.
-    let tm = Cc["@mozilla.org/thread-manager;1"].getService();
-
-    // Keep waiting until our callback is triggered (unless the app is quitting).
-    tm.spinEventLoopUntil(() => !Async.checkAppReady || callback.state != CB_READY);
-
-    // Reset the state of the callback to prepare for another call.
-    let state = callback.state;
-    callback.state = CB_READY;
-
-    // Throw the value the callback decided to fail with.
-    if (state == CB_FAIL) {
-      throw callback.value;
-    }
-
-    // Return the value passed to the callback.
-    return callback.value;
-  },
-
-  /**
    * Check if the app is still ready (not quitting). Returns true, or throws an
    * exception if not ready.
    */
   checkAppReady: function checkAppReady() {
     // Watch for app-quit notification to stop any sync calls
     Services.obs.addObserver(function onQuitApplication() {
       Services.obs.removeObserver(onQuitApplication, "quit-application");
       Async.checkAppReady = Async.promiseYield = function() {
@@ -146,42 +80,16 @@ var Async = {
    * this will be used in exception handlers to allow such exceptions to
    * make their way to the top frame and allow the app to actually terminate.
    */
   isShutdownException(exception) {
     return exception && exception.appIsShuttingDown === true;
   },
 
   /**
-   * Return the two things you need to make an asynchronous call synchronous
-   * by spinning the event loop.
-   */
-  makeSpinningCallback: function makeSpinningCallback() {
-    let cb = Async.makeSyncCallback();
-    function callback(error, ret) {
-      if (error)
-        cb.throw(error);
-      else
-        cb(ret);
-    }
-    callback.wait = () => Async.waitForSyncCallback(cb);
-    return callback;
-  },
-
-  promiseSpinningly(promise) {
-    let cb = Async.makeSpinningCallback();
-    promise.then(result => {
-      cb(null, result);
-    }, err => {
-      cb(err || new Error("Promise rejected without explicit error"));
-    });
-    return cb.wait();
-  },
-
-  /**
    * A "tight loop" of promises can still lock up the browser for some time.
    * Periodically waiting for a promise returned by this function will solve
    * that.
    * You should probably not use this method directly and instead use jankYielder
    * below.
    * Some reference here:
    * - https://gist.github.com/jesstelford/bbb30b983bddaa6e5fef2eb867d37678
    * - https://bugzilla.mozilla.org/show_bug.cgi?id=1094248
--- a/services/sync/tests/unit/test_bookmark_tracker.js
+++ b/services/sync/tests/unit/test_bookmark_tracker.js
@@ -33,16 +33,41 @@ async function verifyTrackerEmpty() {
   equal(tracker.score, 0);
 }
 
 async function resetTracker() {
   await PlacesTestUtils.markBookmarksAsSynced();
   tracker.resetScore();
 }
 
+// We have some tests that uses Places "batch mode", which isn't async aware,
+// so a couple of these tests spin an event loop waiting for a promise.
+function promiseSpinningly(promise) {
+  let resolved = false;
+  let rv, rerror;
+  promise.then(result => {
+    rv = result;
+  }, err => {
+    rerror = err || new Error("Promise rejected without explicit error");
+  }).finally(() => {
+    resolved = true;
+  });
+  let tm = Cc["@mozilla.org/thread-manager;1"].getService();
+
+  // Keep waiting until the promise resolves.
+  tm.spinEventLoopUntil(() => resolved);
+  if (rerror) {
+    throw rerror;
+  }
+  return rv;
+}
+
+
+
+
 async function cleanup() {
   engine.lastSync = 0;
   engine._needWeakUpload.clear();
   await store.wipe();
   await resetTracker();
   await tracker.stop();
 }
 
@@ -349,17 +374,17 @@ add_task(async function test_batch_track
 
   PlacesUtils.bookmarks.runInBatchMode({
     runBatched() {
       PlacesUtils.bookmarks.createFolder(
         PlacesUtils.bookmarks.bookmarksMenuFolder,
         "Test Folder", PlacesUtils.bookmarks.DEFAULT_INDEX);
       // We should be tracking the new folder and its parent (and need to jump
       // through blocking hoops...)
-      Async.promiseSpinningly(verifyTrackedCount(2));
+      promiseSpinningly(verifyTrackedCount(2));
       // But not have bumped the score.
       Assert.equal(tracker.score, 0);
     }
   }, null);
 
   // Out of batch mode - tracker should be the same, but score should be up.
   await verifyTrackedCount(2);
   Assert.equal(tracker.score, SCORE_INCREMENT_XLARGE);
@@ -376,24 +401,24 @@ add_task(async function test_nested_batc
 
       PlacesUtils.bookmarks.runInBatchMode({
         runBatched() {
           PlacesUtils.bookmarks.createFolder(
             PlacesUtils.bookmarks.bookmarksMenuFolder,
             "Test Folder", PlacesUtils.bookmarks.DEFAULT_INDEX);
           // We should be tracking the new folder and its parent (and need to jump
           // through blocking hoops...)
-          Async.promiseSpinningly(verifyTrackedCount(2));
+          promiseSpinningly(verifyTrackedCount(2));
           // But not have bumped the score.
           Assert.equal(tracker.score, 0);
         }
       }, null);
       _("inner batch complete.");
       // should still not have a score as the outer batch is pending.
-      Async.promiseSpinningly(verifyTrackedCount(2));
+      promiseSpinningly(verifyTrackedCount(2));
       Assert.equal(tracker.score, 0);
     }
   }, null);
 
   // Out of both batches - tracker should be the same, but score should be up.
   await verifyTrackedCount(2);
   Assert.equal(tracker.score, SCORE_INCREMENT_XLARGE);
   await cleanup();
--- a/services/sync/tests/unit/test_service_startup.js
+++ b/services/sync/tests/unit/test_service_startup.js
@@ -1,49 +1,45 @@
 /* Any copyright is dedicated to the Public Domain.
  * http://creativecommons.org/publicdomain/zero/1.0/ */
 
 ChromeUtils.import("resource://services-common/observers.js");
 ChromeUtils.import("resource://services-sync/engines.js");
 ChromeUtils.import("resource://services-sync/util.js");
 
 Svc.Prefs.set("registerEngines", "Tab,Bookmarks,Form,History");
-ChromeUtils.import("resource://services-sync/service.js");
 
-function run_test() {
+add_task(async function run_test() {
   validate_all_future_pings();
   _("When imported, Service.onStartup is called");
 
   let xps = Cc["@mozilla.org/weave/service;1"]
               .getService(Ci.nsISupports)
               .wrappedJSObject;
   Assert.ok(!xps.enabled);
 
   // Test fixtures
+  let {Service} = ChromeUtils.import("resource://services-sync/service.js", {});
   Service.identity.username = "johndoe";
   Assert.ok(xps.enabled);
 
-  ChromeUtils.import("resource://services-sync/service.js");
-
   _("Service is enabled.");
   Assert.equal(Service.enabled, true);
 
   _("Observers are notified of startup");
-  do_test_pending();
-
   Assert.ok(!Service.status.ready);
   Assert.ok(!xps.ready);
 
-  Async.promiseSpinningly(promiseOneObserver("weave:service:ready"));
+  await promiseOneObserver("weave:service:ready");
 
   Assert.ok(Service.status.ready);
   Assert.ok(xps.ready);
 
   _("Engines are registered.");
   let engines = Service.engineManager.getAll();
   Assert.ok(Utils.deepEquals(engines.map(engine => engine.name),
                              ["tabs", "bookmarks", "forms", "history"]));
 
   // Clean up.
   Svc.Prefs.resetBranch("");
 
   do_test_finished();
-}
+});
--- a/services/sync/tps/extensions/tps/resource/modules/bookmarks.jsm
+++ b/services/sync/tps/extensions/tps/resource/modules/bookmarks.jsm
@@ -9,17 +9,16 @@
 
 var EXPORTED_SYMBOLS = ["PlacesItem", "Bookmark", "Separator", "Livemark",
                         "BookmarkFolder", "DumpBookmarks"];
 
 ChromeUtils.import("resource://gre/modules/PlacesBackups.jsm");
 ChromeUtils.import("resource://gre/modules/PlacesSyncUtils.jsm");
 ChromeUtils.import("resource://gre/modules/PlacesUtils.jsm");
 ChromeUtils.import("resource://gre/modules/Services.jsm");
-ChromeUtils.import("resource://services-common/async.js");
 ChromeUtils.import("resource://tps/logger.jsm");
 
 async function DumpBookmarks() {
   let [bookmarks, ] = await PlacesBackups.getBookmarksTree();
   Logger.logInfo("Dumping Bookmarks...\n" + JSON.stringify(bookmarks, undefined, 2) + "\n\n");
 }
 
 /**