# HG changeset patch # User Henrik Skupin # Date 1523954607 -7200 # Node ID 6a28d74472b1eaf649e360b5aabb17d8e135cd4c # Parent 787e2fd549068230488f6ced7a2fb7f39cd7009f 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 diff --git a/testing/marionette/browser.js b/testing/marionette/browser.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"; @@ -164,17 +167,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 <xul:browser>, * has been detached. @@ -273,17 +280,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} @@ -298,24 +312,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}`)); } }); } diff --git a/testing/marionette/proxy.js b/testing/marionette/proxy.js --- 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"); XPCOMUtils.defineLazyServiceGetter( this, "globalMessageManager", "@mozilla.org/globalmessagemanager;1", "nsIMessageBroadcaster"); @@ -137,28 +140,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}`); @@ -206,17 +215,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 diff --git a/testing/marionette/sync.js b/testing/marionette/sync.js --- 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 @@ -166,8 +177,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 + * (``) 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"); + }); +}