Bug 1452653 - [marionette] Fix race condition for closing a browser and chrome window. draft
authorHenrik Skupin <mail@hskupin.info>
Tue, 17 Apr 2018 10:43:27 +0200
changeset 799338 b836062a2f8d44f7a3c7af112763d4b7da5f9f96
parent 799337 8eb10c5d3b363dc3107775c868eac3019311e5d1
push id111013
push userbmo:hskupin@gmail.com
push dateThu, 24 May 2018 13:52:15 +0000
bugs1452653
milestone62.0a1
Bug 1452653 - [marionette] Fix race condition for closing a browser and chrome window. 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");
+  });
+}