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

--HG--
extra : rebase_source : 3f9248ebbdc696ce5e6856ecb167ab144739a52e

testing/marionette/browser.js

@@ -11,6 +11,9 @@ 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"];
 
@@ -168,7 +171,11 @@ browser.Context = class {
   }
 
   get messageManager() {
-    return this.contentBrowser.messageManager;
+    if (this.contentBrowser) {
+      return this.contentBrowser.messageManager;
+    }
+
+    return null;
   }
 
   /**
@@ -277,7 +284,14 @@ browser.Context = class {
    */
   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();
     });
   }
@@ -302,14 +316,22 @@ browser.Context = class {
     }
 
     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 {

testing/marionette/proxy.js

@@ -14,6 +14,9 @@ const {
 } = 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"];
 
@@ -139,18 +142,24 @@ proxy.AsyncMessageChannel = class {
         }
       };
 
-      // 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,
@@ -208,7 +217,9 @@ proxy.AsyncMessageChannel = class {
       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);
+        }
       }
     }
   }

testing/marionette/sync.js

@@ -4,13 +4,24 @@
 
 "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;
 
@@ -164,3 +175,43 @@ function TimedPromise(fn, {timeout = 1500, throws = TimeoutError} = {}) {
     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");
+  });
+}