Bug 1452653 - [marionette] Fix race condition for closing a browser and chrome window. r=ato
☠☠ backed out by b31bf225f2cd ☠ ☠
authorHenrik Skupin <mail@hskupin.info>
Tue, 17 Apr 2018 10:43:27 +0200
changeset 419044 3558cd78821d557168827d3281d966ab4a0ca905
parent 419043 bce37e838f6ab69f66794bb65426a4813025c5e6
child 419045 0f7100b128f23a3b41ebef5ee741d0f3798d7ad1
push id64212
push userhskupin@mozilla.com
push dateSat, 19 May 2018 20:30:32 +0000
treeherderautoland@3558cd78821d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersato
bugs1452653
milestone62.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 1452653 - [marionette] Fix race condition for closing a browser and chrome window. r=ato Until now Marionette assumed that the events `TabClose` and `unload` indicate that a top-level browsing context or chrome window has been closed. But both events are fired when the browsing context or chrome window is about to close. As such a race condition can be seen for slow running builds. To clearly wait until the top-level browsing context or chrome window has been closed, the appropriate message manager needs to be observed for its destroyed state. MozReview-Commit-ID: DCdaIiULqey
testing/marionette/browser.js
testing/marionette/proxy.js
testing/marionette/sync.js
--- a/testing/marionette/browser.js
+++ b/testing/marionette/browser.js
@@ -6,16 +6,19 @@
 /* global frame */
 
 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,
+} = 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";
 
@@ -163,17 +166,21 @@ browser.Context = class {
         this.driver.isReftestBrowser(this.tabBrowser)) {
       return this.tabBrowser;
     }
 
     return null;
   }
 
   get messageManager() {
-    return this.contentBrowser.messageManager;
+    if (this.contentBrowser) {
+      return this.contentBrowser.messageManager;
+    }
+
+    return null;
   }
 
   /**
    * Checks if the browsing context has been discarded.
    *
    * The browsing context will have been discarded if the content
    * browser, represented by the <code>&lt;xul:browser&gt;</code>,
    * has been detached.
@@ -272,17 +279,24 @@ browser.Context = class {
   /**
    * Close the current window.
    *
    * @return {Promise}
    *     A promise which is resolved when the current window has been closed.
    */
   closeWindow() {
     return new Promise(resolve => {
-      this.window.addEventListener("unload", resolve, {once: true});
+      // Wait for the window message manager to be destroyed
+      let destroyed = new MessageManagerDestroyedPromise(
+          this.window.messageManager);
+
+      this.window.addEventListener("unload", async () => {
+        await destroyed;
+        resolve();
+      }, {once: true});
       this.window.close();
     });
   }
 
   /**
    * Close the current tab.
    *
    * @return {Promise}
@@ -297,24 +311,32 @@ browser.Context = class {
     if (!this.tabBrowser ||
         !this.tabBrowser.tabs ||
         this.tabBrowser.tabs.length === 1 ||
         !this.tab) {
       return this.closeWindow();
     }
 
     return new Promise((resolve, reject) => {
+      // Wait for the browser message manager to be destroyed
+      let browserDetached = async () => {
+        await new MessageManagerDestroyedPromise(this.messageManager);
+        resolve();
+      };
+
       if (this.tabBrowser.closeTab) {
         // Fennec
-        this.tabBrowser.deck.addEventListener("TabClose", resolve, {once: true});
+        this.tabBrowser.deck.addEventListener(
+            "TabClose", browserDetached, {once: true});
         this.tabBrowser.closeTab(this.tab);
 
       } else if (this.tabBrowser.removeTab) {
         // Firefox
-        this.tab.addEventListener("TabClose", resolve, {once: true});
+        this.tab.addEventListener(
+            "TabClose", browserDetached, {once: true});
         this.tabBrowser.removeTab(this.tab);
 
       } else {
         reject(new UnsupportedOperationError(
             `closeTab() not supported in ${this.driver.appName}`));
       }
     });
   }
--- a/testing/marionette/proxy.js
+++ b/testing/marionette/proxy.js
@@ -9,16 +9,19 @@ ChromeUtils.import("resource://gre/modul
 ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
 
 const {
   error,
   WebDriverError,
 } = ChromeUtils.import("chrome://marionette/content/error.js", {});
 ChromeUtils.import("chrome://marionette/content/evaluate.js");
 ChromeUtils.import("chrome://marionette/content/modal.js");
+const {
+  MessageManagerDestroyedPromise,
+} = ChromeUtils.import("chrome://marionette/content/sync.js", {});
 
 this.EXPORTED_SYMBOLS = ["proxy"];
 
 XPCOMUtils.defineLazyServiceGetter(
     this, "uuidgen", "@mozilla.org/uuid-generator;1", "nsIUUIDGenerator");
 
 const log = Log.repository.getLogger("Marionette");
 
@@ -134,28 +137,34 @@ proxy.AsyncMessageChannel = class {
             reject(err);
             break;
 
           default:
             throw new TypeError(`Unknown async response type: ${type}`);
         }
       };
 
-      // The currently selected tab or window has been closed. No clean-up
-      // is necessary to do because all loaded listeners are gone.
-      this.closeHandler = ({type, target}) => {
+      // The currently selected tab or window is closing. Make sure to wait
+      // until it's fully gone.
+      this.closeHandler = async ({type, target}) => {
         log.debug(`Received DOM event ${type} for ${target}`);
 
+        let messageManager;
         switch (type) {
-          case "TabClose":
           case "unload":
-            this.removeHandlers();
-            resolve();
+            messageManager = this.browser.window.messageManager;
+            break;
+          case "TabClose":
+            messageManager = this.browser.messageManager;
             break;
         }
+
+        await new MessageManagerDestroyedPromise(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) => {
         log.debug(`Received observer notification ${topic}`);
 
@@ -203,17 +212,19 @@ proxy.AsyncMessageChannel = class {
     modal.removeHandler(this.dialogueObserver_);
 
     if (this.browser) {
       this.browser.window.removeEventListener("unload", this.closeHandler);
 
       if (this.browser.tab) {
         let node = this.browser.tab.addEventListener ?
             this.browser.tab : this.browser.contentBrowser;
-        node.removeEventListener("TabClose", this.closeHandler);
+        if (node) {
+          node.removeEventListener("TabClose", this.closeHandler);
+        }
       }
     }
   }
 
   /**
    * Reply to an asynchronous request.
    *
    * Passing an {@link WebDriverError} prototype will cause the receiving
--- a/testing/marionette/sync.js
+++ b/testing/marionette/sync.js
@@ -1,21 +1,32 @@
 /* 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/. */
 
 "use strict";
 
+ChromeUtils.import("resource://gre/modules/Log.jsm");
+ChromeUtils.import("resource://gre/modules/Services.jsm");
+
 const {
   error,
   TimeoutError,
 } = ChromeUtils.import("chrome://marionette/content/error.js", {});
 
-/* exported PollPromise, TimedPromise */
-this.EXPORTED_SYMBOLS = ["PollPromise", "TimedPromise"];
+this.EXPORTED_SYMBOLS = [
+  /* exported PollPromise, TimedPromise */
+  "PollPromise",
+  "TimedPromise",
+
+  /* exported MessageManagerDestroyedPromise */
+  "MessageManagerDestroyedPromise",
+];
+
+const logger = Log.repository.getLogger("Marionette");
 
 const {TYPE_ONE_SHOT, TYPE_REPEATING_SLACK} = Ci.nsITimer;
 
 /**
  * @callback Condition
  *
  * @param {function(*)} resolve
  *     To be called when the condition has been met.  Will return the
@@ -159,8 +170,48 @@ function TimedPromise(fn, {timeout = 150
   }).then(res => {
     timer.cancel();
     return res;
   }, err => {
     timer.cancel();
     throw err;
   });
 }
+
+/**
+ * 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) {
+      logger.debug(`Received observer notification ${topic}`);
+
+      if (subject == messageManager) {
+        Services.obs.removeObserver(this, "message-manager-disconnect");
+        resolve();
+      }
+    }
+
+    Services.obs.addObserver(observe, "message-manager-disconnect");
+  });
+}