Bug 1504756 - [marionette] Use waitForObserverTopic when waiting for observer notifications. r=ato
☠☠ backed out by 49bd1dd914a4 ☠ ☠
authorHenrik Skupin <mail@hskupin.info>
Tue, 04 Dec 2018 21:59:39 +0000
changeset 508562 a8ea6d01fbe1de92009842a4028c9179646a88f5
parent 508561 ba627a1b61dcebdd2cde178567fd800e0cbe8208
child 508563 30d345cce5be1236aed9380e7c4d121a99800d70
push id1905
push userffxbld-merge
push dateMon, 21 Jan 2019 12:33:13 +0000
treeherdermozilla-release@c2fca1944d8c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersato
bugs1504756
milestone65.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 1504756 - [marionette] Use waitForObserverTopic when waiting for observer notifications. r=ato Depends on D13661 Differential Revision: https://phabricator.services.mozilla.com/D13662
testing/marionette/browser.js
testing/marionette/doc/internals/sync.rst
testing/marionette/driver.js
testing/marionette/proxy.js
testing/marionette/sync.js
testing/marionette/test/unit/test_sync.js
--- a/testing/marionette/browser.js
+++ b/testing/marionette/browser.js
@@ -7,18 +7,18 @@
 
 const {WebElementEventTarget} = ChromeUtils.import("chrome://marionette/content/dom.js", {});
 ChromeUtils.import("chrome://marionette/content/element.js");
 const {
   NoSuchWindowError,
   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"];
 
 /** @namespace */
 this.browser = {};
 
 const XUL_NS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
@@ -283,23 +283,25 @@ browser.Context = class {
 
   /**
    * Close the current window.
    *
    * @return {Promise}
    *     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]);
   }
 
   /**
    * Close the current tab.
    *
    * @return {Promise}
    *     A promise which is resolved when the current tab has been closed.
    *
@@ -311,17 +313,21 @@ browser.Context = class {
     // same if only one remaining tab is open, or no tab selected at all.
     if (!this.tabBrowser ||
         !this.tabBrowser.tabs ||
         this.tabBrowser.tabs.length === 1 ||
         !this.tab) {
       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) {
       // Fennec
       tabClosed = waitForEvent(this.tabBrowser.deck, "TabClose");
       this.tabBrowser.closeTab(this.tab);
 
     } else if (this.tabBrowser.removeTab) {
@@ -329,17 +335,17 @@ browser.Context = class {
       tabClosed = waitForEvent(this.tab, "TabClose");
       this.tabBrowser.removeTab(this.tab);
 
     } else {
       throw new UnsupportedOperationError(
         `closeTab() not supported in ${this.driver.appName}`);
     }
 
-    return Promise.all([destroyed, tabClosed]);
+    return Promise.all([disconnected, tabClosed]);
   }
 
   /**
    * Opens a tab with given URI.
    *
    * @param {string} uri
    *      URI to open.
    */
--- a/testing/marionette/doc/internals/sync.rst
+++ b/testing/marionette/doc/internals/sync.rst
@@ -1,22 +1,21 @@
 sync module
 ===========
 
 Provides an assortment of synchronisation primitives.
 
 .. js:autofunction:: executeSoon
 
-.. js:autoclass:: MessageManagerDestroyedPromise
-  :members:
-
 .. js:autoclass:: PollPromise
   :members:
 
 .. js:autoclass:: Sleep
   :members:
 
 .. js:autoclass:: TimedPromise
   :members:
 
 .. js:autofunction:: waitForEvent
 
 .. js:autofunction:: waitForMessage
+
+.. js:autofunction:: waitForObserverTopic
--- a/testing/marionette/driver.js
+++ b/testing/marionette/driver.js
@@ -58,16 +58,17 @@ const {MarionettePrefs} = ChromeUtils.im
 ChromeUtils.import("chrome://marionette/content/proxy.js");
 ChromeUtils.import("chrome://marionette/content/reftest.js");
 const {
   DebounceCallback,
   IdlePromise,
   PollPromise,
   TimedPromise,
   waitForEvent,
+  waitForObserverTopic,
 } = ChromeUtils.import("chrome://marionette/content/sync.js", {});
 
 XPCOMUtils.defineLazyGetter(this, "logger", Log.get);
 XPCOMUtils.defineLazyGlobalGetters(this, ["URL"]);
 
 this.EXPORTED_SYMBOLS = ["GeckoDriver"];
 
 const APP_ID_FIREFOX = "{ec8030f7-c20a-464f-9b0e-13a3a9e97384}";
@@ -3293,25 +3294,20 @@ GeckoDriver.prototype.quit = async funct
   } else {
     mode = Ci.nsIAppStartup.eAttemptQuit;
   }
 
   this._server.acceptConnections = false;
   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) {
   assert.desktop();
 
   let path = cmd.parameters.path;
   let temp = cmd.parameters.temporary || false;
   if (typeof path == "undefined" || typeof path != "string" ||
--- a/testing/marionette/proxy.js
+++ b/testing/marionette/proxy.js
@@ -10,17 +10,17 @@ ChromeUtils.import("resource://gre/modul
 const {
   error,
   WebDriverError,
 } = ChromeUtils.import("chrome://marionette/content/error.js", {});
 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"];
 
 XPCOMUtils.defineLazyGetter(this, "log", Log.get);
 XPCOMUtils.defineLazyServiceGetter(
     this, "uuidgen", "@mozilla.org/uuid-generator;1", "nsIUUIDGenerator");
 
@@ -151,17 +151,19 @@ proxy.AsyncMessageChannel = class {
           case "unload":
             messageManager = this.browser.window.messageManager;
             break;
           case "TabClose":
             messageManager = this.browser.messageManager;
             break;
         }
 
-        await new MessageManagerDestroyedPromise(messageManager);
+        await waitForObserverTopic("message-manager-disconnect",
+            subject => subject === messageManager);
+
         this.removeHandlers();
         resolve();
       };
 
       // A modal or tab modal dialog has been opened. To be able to handle it,
       // the active command has to be aborted. Therefore remove all handlers,
       // and cancel any ongoing requests in the listener.
       this.dialogueObserver_ = (subject, topic) => {
--- a/testing/marionette/sync.js
+++ b/testing/marionette/sync.js
@@ -17,22 +17,22 @@ const {truncate} = ChromeUtils.import("c
 const {Log} = ChromeUtils.import("chrome://marionette/content/log.js", {});
 
 XPCOMUtils.defineLazyGetter(this, "log", Log.get);
 
 this.EXPORTED_SYMBOLS = [
   "executeSoon",
   "DebounceCallback",
   "IdlePromise",
-  "MessageManagerDestroyedPromise",
   "PollPromise",
   "Sleep",
   "TimedPromise",
   "waitForEvent",
   "waitForMessage",
+  "waitForObserverTopic",
 ];
 
 const {TYPE_ONE_SHOT, TYPE_REPEATING_SLACK} = Ci.nsITimer;
 
 const PROMISE_TIMEOUT = AppConstants.DEBUG ? 4500 : 1500;
 
 
 /**
@@ -251,56 +251,16 @@ function TimedPromise(fn,
 function Sleep(timeout) {
   if (typeof timeout != "number") {
     throw new TypeError();
   }
   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).
  *
  * @param {ChromeWindow} win
  *     Window to request the animation frame from.
  *
  * @return Promise
  */
@@ -506,8 +466,56 @@ function waitForMessage(messageManager, 
       if (checkFn && !checkFn(msg)) {
         return;
       }
       messageManager.removeMessageListener(messageName, onMessage);
       resolve(msg.data);
     });
   });
 }
+
+/**
+ * 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);
+  });
+}
--- a/testing/marionette/test/unit/test_sync.js
+++ b/testing/marionette/test/unit/test_sync.js
@@ -1,20 +1,23 @@
 /* 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/. */
 
+ChromeUtils.import("resource://gre/modules/Services.jsm");
+
 const {
   DebounceCallback,
   IdlePromise,
   PollPromise,
   Sleep,
   TimedPromise,
   waitForEvent,
   waitForMessage,
+  waitForObserverTopic,
 } = ChromeUtils.import("chrome://marionette/content/sync.js", {});
 
 const DEFAULT_TIMEOUT = 2000;
 
 /**
  * Mimic a DOM node for listening for events.
  */
 class MockElement {
@@ -418,8 +421,41 @@ add_task(async function test_waitForMess
 
     messageManager = new MessageManager();
     let sent = waitForMessage(messageManager, "message", {checkFn});
     messageManager.send("message", data1);
     messageManager.send("message", data2);
     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);
+  }
+});