Bug 1504756 - [marionette] Use waitForObserverTopic when waiting for observer notifications. r=ato

Depends on D13661

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

--HG--
extra : moz-landing-system : lando

testing/marionette/browser.js

@@ -12,8 +12,8 @@ const {
   UnsupportedOperationError,
 } = ChromeUtils.import("chrome://marionette/content/error.js", {});
 const {
-  MessageManagerDestroyedPromise,
   waitForEvent,
+  waitForObserverTopic,
 } = ChromeUtils.import("chrome://marionette/content/sync.js", {});
 
 this.EXPORTED_SYMBOLS = ["browser", "Context", "WindowState"];
@@ -288,13 +288,15 @@ browser.Context = class {
    *     A promise which is resolved when the current window has been closed.
    */
   closeWindow() {
-    let destroyed = new MessageManagerDestroyedPromise(
-        this.window.messageManager);
+    // Create a copy of the messageManager before it is disconnected
+    let messageManager = this.window.messageManager;
+    let disconnected = waitForObserverTopic("message-manager-disconnect",
+        subject => subject === messageManager);
     let unloaded = waitForEvent(this.window, "unload");
 
     this.window.close();
 
-    return Promise.all([destroyed, unloaded]);
+    return Promise.all([disconnected, unloaded]);
   }
 
   /**
@@ -316,7 +318,11 @@ browser.Context = class {
       return this.closeWindow();
     }
 
-    let destroyed = new MessageManagerDestroyedPromise(this.messageManager);
+    // Create a copy of the messageManager before it is disconnected
+    let messageManager = this.messageManager;
+    let disconnected = waitForObserverTopic("message-manager-disconnect",
+        subject => subject === messageManager);
+
     let tabClosed;
 
     if (this.tabBrowser.closeTab) {
@@ -334,7 +340,7 @@ browser.Context = class {
         `closeTab() not supported in ${this.driver.appName}`);
     }
 
-    return Promise.all([destroyed, tabClosed]);
+    return Promise.all([disconnected, tabClosed]);
   }
 
   /**

testing/marionette/doc/internals/sync.rst

@@ -5,9 +5,6 @@ Provides an assortment of synchronisation primitives.
 
 .. js:autofunction:: executeSoon
 
-.. js:autoclass:: MessageManagerDestroyedPromise
-  :members:
-
 .. js:autoclass:: PollPromise
   :members:
 
@@ -20,3 +17,5 @@ Provides an assortment of synchronisation primitives.
 .. js:autofunction:: waitForEvent
 
 .. js:autofunction:: waitForMessage
+
+.. js:autofunction:: waitForObserverTopic

testing/marionette/driver.js

@@ -63,6 +63,7 @@ const {
   PollPromise,
   TimedPromise,
   waitForEvent,
+  waitForObserverTopic,
 } = ChromeUtils.import("chrome://marionette/content/sync.js", {});
 
 XPCOMUtils.defineLazyGetter(this, "logger", Log.get);
@@ -3298,15 +3299,10 @@ GeckoDriver.prototype.quit = async function(cmd) {
   this.deleteSession();
 
   // delay response until the application is about to quit
-  let quitApplication = new Promise(resolve => {
-    Services.obs.addObserver(
-        (subject, topic, data) => resolve(data),
-        "quit-application");
-  });
-
+  let quitApplication = waitForObserverTopic("quit-application");
   Services.startup.quit(mode);
 
-  return {cause: await quitApplication};
+  return {cause: (await quitApplication).data};
 };
 
 GeckoDriver.prototype.installAddon = function(cmd) {

testing/marionette/proxy.js

@@ -15,7 +15,7 @@ ChromeUtils.import("chrome://marionette/content/evaluate.js");
 const {Log} = ChromeUtils.import("chrome://marionette/content/log.js", {});
 ChromeUtils.import("chrome://marionette/content/modal.js");
 const {
-  MessageManagerDestroyedPromise,
+  waitForObserverTopic,
 } = ChromeUtils.import("chrome://marionette/content/sync.js", {});
 
 this.EXPORTED_SYMBOLS = ["proxy"];
@@ -156,7 +156,9 @@ proxy.AsyncMessageChannel = class {
             break;
         }
 
-        await new MessageManagerDestroyedPromise(messageManager);
+        await waitForObserverTopic("message-manager-disconnect",
+            subject => subject === messageManager);
+
         this.removeHandlers();
         resolve();
       };

testing/marionette/sync.js

@@ -22,12 +22,12 @@ this.EXPORTED_SYMBOLS = [
   "executeSoon",
   "DebounceCallback",
   "IdlePromise",
-  "MessageManagerDestroyedPromise",
   "PollPromise",
   "Sleep",
   "TimedPromise",
   "waitForEvent",
   "waitForMessage",
+  "waitForObserverTopic",
 ];
 
 const {TYPE_ONE_SHOT, TYPE_REPEATING_SLACK} = Ci.nsITimer;
@@ -255,46 +255,6 @@ function Sleep(timeout) {
   return new TimedPromise(() => {}, {timeout, throws: null});
 }
 
-/**
- * Detects when the specified message manager has been destroyed.
- *
- * One can observe the removal and detachment of a content browser
- * (`<xul:browser>`) or a chrome window by its message manager
- * disconnecting.
- *
- * When a browser is associated with a tab, this is safer than only
- * relying on the event `TabClose` which signalises the _intent to_
- * remove a tab and consequently would lead to the destruction of
- * the content browser and its browser message manager.
- *
- * When closing a chrome window it is safer than only relying on
- * the event 'unload' which signalises the _intent to_ close the
- * chrome window and consequently would lead to the destruction of
- * the window and its window message manager.
- *
- * @param {MessageListenerManager} messageManager
- *     The message manager to observe for its disconnect state.
- *     Use the browser message manager when closing a content browser,
- *     and the window message manager when closing a chrome window.
- *
- * @return {Promise}
- *     A promise that resolves when the message manager has been destroyed.
- */
-function MessageManagerDestroyedPromise(messageManager) {
-  return new Promise(resolve => {
-    function observe(subject, topic) {
-      log.trace(`Received observer notification ${topic}`);
-
-      if (subject == messageManager) {
-        Services.obs.removeObserver(this, "message-manager-disconnect");
-        resolve();
-      }
-    }
-
-    Services.obs.addObserver(observe, "message-manager-disconnect");
-  });
-}
-
 /**
  * Throttle until the main thread is idle and `window` has performed
  * an animation frame (in that order).
@@ -511,3 +471,51 @@ function waitForMessage(messageManager, messageName,
     });
   });
 }
+
+/**
+ * Wait for the specified observer topic to be observed.
+ *
+ * This method has been duplicated from TestUtils.jsm.
+ *
+ * Because this function is intended for testing, any error in checkFn
+ * will cause the returned promise to be rejected instead of waiting for
+ * the next notification, since this is probably a bug in the test.
+ *
+ * @param {string} topic
+ *     The topic to observe.
+ * @param {Object=} options
+ *     Extra options.
+ * @param {function(String,Object)=} options.checkFn
+ *     Called with ``subject``, and ``data`` as arguments, should return true
+ *     if the notification is the expected one, or false if it should be
+ *     ignored and listening should continue. If not specified, the first
+ *     notification for the specified topic resolves the returned promise.
+ *
+ * @return {Promise.<Array<String, Object>>}
+ *     Promise which resolves to an array of ``subject``, and ``data`` from
+ *     the observed notification.
+ */
+function waitForObserverTopic(topic, {checkFn = null} = {}) {
+  if (typeof topic != "string") {
+    throw new TypeError();
+  }
+  if (checkFn != null && typeof checkFn != "function") {
+    throw new TypeError();
+  }
+
+  return new Promise((resolve, reject) => {
+    Services.obs.addObserver(function observer(subject, topic, data) {
+      log.trace(`Received observer notification ${topic}`);
+      try {
+        if (checkFn && !checkFn(subject, data)) {
+          return;
+        }
+        Services.obs.removeObserver(observer, topic);
+        resolve({subject, data});
+      } catch (ex) {
+        Services.obs.removeObserver(observer, topic);
+        reject(ex);
+      }
+    }, topic);
+  });
+}

testing/marionette/test/unit/test_sync.js

@@ -2,6 +2,8 @@
  * 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/. */
 
+ChromeUtils.import("resource://gre/modules/Services.jsm");
+
 const {
   DebounceCallback,
   IdlePromise,
@@ -10,6 +12,7 @@ const {
   TimedPromise,
   waitForEvent,
   waitForMessage,
+  waitForObserverTopic,
 } = ChromeUtils.import("chrome://marionette/content/sync.js", {});
 
 const DEFAULT_TIMEOUT = 2000;
@@ -423,3 +426,36 @@ add_task(async function test_waitForMessage_checkFnTypes() {
     equal(expected_data, await sent);
   }
 });
+
+add_task(async function test_waitForObserverTopic_topicTypes() {
+  for (let topic of [42, null, undefined, true, [], {}]) {
+    Assert.throws(() => waitForObserverTopic(topic), /TypeError/);
+  }
+
+  let data = {"foo": "bar"};
+  let sent = waitForObserverTopic("message");
+  Services.obs.notifyObservers(this, "message", data);
+  let result = await sent;
+  equal(this, result.subject);
+  equal(data, result.data);
+});
+
+add_task(async function test_waitForObserverTopic_checkFnTypes() {
+  for (let checkFn of ["foo", 42, true, [], {}]) {
+    Assert.throws(() => waitForObserverTopic(
+        "message", {checkFn}), /TypeError/);
+  }
+
+  let data1 = {"fo": "bar"};
+  let data2 = {"foo": "bar"};
+
+  for (let checkFn of [null, undefined, (subject, data) => data == data2]) {
+    let expected_data = (checkFn == null) ? data1 : data2;
+
+    let sent = waitForObserverTopic("message");
+    Services.obs.notifyObservers(this, "message", data1);
+    Services.obs.notifyObservers(this, "message", data2);
+    let result = await sent;
+    equal(expected_data, result.data);
+  }
+});