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
--- 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><xul:browser></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");
+ });
+}