Bug 1223277 - Return immediately when click command closes tab or window. draft
authorHenrik Skupin <mail@hskupin.info>
Mon, 26 Jun 2017 16:55:02 -0700
changeset 600721 6985541f8aaa2dd2e76162c6567c88aa9d56bbb9
parent 600720 6d9efd8a7491ad6d8bfd5a5ab31315cc9d60dc70
child 635084 0fc1922b4e57c0d29d6a0e717da93608184a1c49
push id65860
push userbmo:hskupin@gmail.com
push dateTue, 27 Jun 2017 20:00:08 +0000
bugs1223277
milestone56.0a1
Bug 1223277 - Return immediately when click command closes tab or window. In some cases the click command can trigger the closing of the currently selected tab or window. To not cause a hang when waiting for a response from the removed framescript, the tab and window closing events have to be observed. Also the command has to return immediately. MozReview-Commit-ID: 9WeXryrKEJr
testing/marionette/driver.js
testing/marionette/harness/marionette_harness/tests/unit/test_click.py
testing/marionette/harness/marionette_harness/tests/unit/test_crash.py
testing/marionette/harness/marionette_harness/tests/unit/test_window_management.py
testing/marionette/harness/marionette_harness/www/clicks.html
testing/marionette/harness/marionette_harness/www/test_inner_iframe.html
testing/marionette/proxy.js
--- a/testing/marionette/driver.js
+++ b/testing/marionette/driver.js
@@ -125,17 +125,18 @@ this.GeckoDriver = function (appName, se
   this.timer = null;
   this.inactivityTimer = null;
 
   this.testName = null;
 
   this.capabilities = new session.Capabilities();
 
   this.mm = globalMessageManager;
-  this.listener = proxy.toListener(() => this.mm, this.sendAsync.bind(this));
+  this.listener = proxy.toListener(() => this.mm, this.sendAsync.bind(this),
+                                   () => this.curBrowser);
 
   // points to an alert instance if a modal dialog is present
   this.dialog = null;
   this.dialogHandler = this.globalModalDialogHandler.bind(this);
 };
 
 Object.defineProperty(GeckoDriver.prototype, "a11yChecks", {
   get: function () {
--- a/testing/marionette/harness/marionette_harness/tests/unit/test_click.py
+++ b/testing/marionette/harness/marionette_harness/tests/unit/test_click.py
@@ -5,16 +5,17 @@
 import urllib
 
 from marionette_driver import By, errors
 
 from marionette_harness import (
     MarionetteTestCase,
     run_if_e10s,
     skip_if_mobile,
+    WindowManagerMixin,
 )
 
 
 def inline(doc):
     return "data:text/html;charset=utf-8,{}".format(urllib.quote(doc))
 
 
 # The <a> element in the following HTML is not interactable because it
@@ -356,8 +357,40 @@ class TestClickNavigation(MarionetteTest
             self.marionette.find_element(By.ID, "anchor")
 
         self.marionette.go_back()
         self.marionette.find_element(By.ID, "anchor")
 
         self.marionette.find_element(By.ID, "history-back").click()
         with self.assertRaises(errors.NoSuchElementException):
             self.marionette.find_element(By.ID, "anchor")
+
+
+class TestClickCloseContext(WindowManagerMixin, MarionetteTestCase):
+
+    def setUp(self):
+        super(TestClickCloseContext, self).setUp()
+
+        self.test_page = self.marionette.absolute_url("clicks.html")
+
+    def tearDown(self):
+        self.close_all_tabs()
+
+        super(TestClickCloseContext, self).tearDown()
+
+    def test_click_close_tab(self):
+        self.marionette.navigate(self.marionette.absolute_url("windowHandles.html"))
+        tab = self.open_tab(
+            lambda: self.marionette.find_element(By.ID, "new-tab").click())
+        self.marionette.switch_to_window(tab)
+
+        self.marionette.navigate(self.test_page)
+        self.marionette.find_element(By.ID, "close-window").click()
+
+    @skip_if_mobile("Fennec doesn't support other chrome windows")
+    def test_click_close_window(self):
+        self.marionette.navigate(self.marionette.absolute_url("windowHandles.html"))
+        win = self.open_window(
+            lambda: self.marionette.find_element(By.ID, "new-window").click())
+        self.marionette.switch_to_window(win)
+
+        self.marionette.navigate(self.test_page)
+        self.marionette.find_element(By.ID, "close-window").click()
--- a/testing/marionette/harness/marionette_harness/tests/unit/test_crash.py
+++ b/testing/marionette/harness/marionette_harness/tests/unit/test_crash.py
@@ -1,21 +1,23 @@
 # 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/.
 
 import glob
 import shutil
 
-from marionette_driver.errors import MarionetteException
+from marionette_driver import Wait
+from marionette_driver.errors import MarionetteException, NoSuchWindowException
+
+from marionette_harness import MarionetteTestCase, expectedFailure, run_if_e10s
+
 # Import runner module to monkey patch mozcrash module
 from mozrunner.base import runner
 
-from marionette_harness import MarionetteTestCase, expectedFailure, run_if_e10s
-
 
 class MockMozCrash(object):
     """Mock object to replace original mozcrash methods."""
 
     def __init__(self, marionette):
         self.marionette = marionette
 
         with self.marionette.using_context('chrome'):
@@ -86,34 +88,56 @@ class BaseCrashTestCase(MarionetteTestCa
             """, sandbox=sandbox)
         finally:
             self.marionette.client.socket_timeout = socket_timeout
 
 
 class TestCrash(BaseCrashTestCase):
 
     def test_crash_chrome_process(self):
-        self.assertRaisesRegexp(IOError, 'Process crashed',
-                                self.crash, chrome=True)
+        with self.assertRaises(IOError):
+            self.crash(chrome=True)
+            Wait(self.marionette, timeout=self.socket_timeout,
+                 ignored_exceptions=NoSuchWindowException).until(
+                lambda _: self.marionette.get_url(),
+                message="Expected IOError exception for content crash not raised."
+            )
+
+        # A crash results in a non zero exit code
+        self.assertNotIn(self.marionette.instance.runner.returncode, (None, 0))
+
         self.assertEqual(self.marionette.crashed, 1)
         self.assertIsNone(self.marionette.session)
         self.assertRaisesRegexp(MarionetteException, 'Please start a session',
                                 self.marionette.get_url)
 
         self.marionette.start_session()
         self.assertNotEqual(self.marionette.process_id, self.pid)
 
         self.marionette.get_url()
 
     @run_if_e10s("Content crashes only exist in e10s mode")
     def test_crash_content_process(self):
-        self.marionette.navigate(self.remote_uri)
+        # With MOZ_CRASHREPORTER_SHUTDOWN each window of the browser will be
+        # closed in case of a content crash. So the "unload" handler fires for
+        # the current window, which causes the command to return early.
+        # To check for an IOError, further commands have to be executed until
+        # Firefox has been shutdown.
+        with self.assertRaises(IOError):
+            self.crash(chrome=False)
+            Wait(self.marionette, timeout=self.socket_timeout,
+                 ignored_exceptions=NoSuchWindowException).until(
+                lambda _: self.marionette.get_url(),
+                message="Expected IOError exception for content crash not raised."
+            )
 
-        self.assertRaisesRegexp(IOError, 'Content process crashed',
-                                self.crash, chrome=False)
+        # In the case of a content crash Firefox will be closed and its
+        # returncode will report 0 (this will change with 1370520).
+        self.assertEqual(self.marionette.instance.runner.returncode, 0)
+
         self.assertEqual(self.marionette.crashed, 1)
         self.assertIsNone(self.marionette.session)
         self.assertRaisesRegexp(MarionetteException, 'Please start a session',
                                 self.marionette.get_url)
 
         self.marionette.start_session()
         self.assertNotEqual(self.marionette.process_id, self.pid)
         self.marionette.get_url()
@@ -123,31 +147,50 @@ class TestCrash(BaseCrashTestCase):
         self.crash(chrome=True)
 
 
 class TestCrashInSetUp(BaseCrashTestCase):
 
     def setUp(self):
         super(TestCrashInSetUp, self).setUp()
 
-        self.assertRaisesRegexp(IOError, 'Process crashed',
-                                self.crash, chrome=True)
+        with self.assertRaises(IOError):
+            self.crash(chrome=True)
+            Wait(self.marionette, timeout=self.socket_timeout,
+                 ignored_exceptions=NoSuchWindowException).until(
+                lambda _: self.marionette.get_url(),
+                message="Expected IOError exception for content crash not raised."
+            )
+
+        # A crash results in a non zero exit code
+        self.assertNotIn(self.marionette.instance.runner.returncode, (None, 0))
+
         self.assertEqual(self.marionette.crashed, 1)
         self.assertIsNone(self.marionette.session)
 
     def test_crash_in_setup(self):
         self.marionette.start_session()
         self.assertNotEqual(self.marionette.process_id, self.pid)
 
 
 class TestCrashInTearDown(BaseCrashTestCase):
 
     def tearDown(self):
         try:
-            self.assertRaisesRegexp(IOError, 'Process crashed',
-                                    self.crash, chrome=True)
-        finally:
+            with self.assertRaises(IOError):
+                self.crash(chrome=True)
+                Wait(self.marionette, timeout=self.socket_timeout,
+                     ignored_exceptions=NoSuchWindowException).until(
+                    lambda _: self.marionette.get_url(),
+                    message="Expected IOError exception for content crash not raised."
+                )
+
+            # A crash results in a non zero exit code
+            self.assertNotIn(self.marionette.instance.runner.returncode, (None, 0))
+
             self.assertEqual(self.marionette.crashed, 1)
             self.assertIsNone(self.marionette.session)
+
+        finally:
             super(TestCrashInTearDown, self).tearDown()
 
     def test_crash_in_teardown(self):
         pass
--- a/testing/marionette/harness/marionette_harness/tests/unit/test_window_management.py
+++ b/testing/marionette/harness/marionette_harness/tests/unit/test_window_management.py
@@ -161,13 +161,8 @@ class TestSwitchWindow(WindowManagerMixi
 
         with self.marionette.using_context('content'):
             self.assertEqual(self.marionette.title, "We Arrive Here")
 
         # Let's close and check
         self.marionette.close_chrome_window()
         self.marionette.switch_to_window(self.start_window)
         self.assertEqual(len(self.marionette.chrome_window_handles), 1)
-
-    def tearDown(self):
-        #ensure that we close the window, regardless of pass/failure
-        self.close_all_windows()
-        MarionetteTestCase.tearDown(self)
--- a/testing/marionette/harness/marionette_harness/www/clicks.html
+++ b/testing/marionette/harness/marionette_harness/www/clicks.html
@@ -31,16 +31,17 @@
 
 <div>
   <p id="js-links">Javascript links:</p>
   <ul>
     <li>Navigate in history:
       <a href="javascript:history.back();" id="history-back">Back</a>
       <a href="javascript:history.forward();" id="history-forward">Forward</a>
     <li><a href="javascript:window.open('test.html', '_blank')" id="new-window">Open a window</a>
+    <li><a href="javascript:window.close();" id="close-window">Close tab/window</a>
     <li><a id="addbuttonlistener" href="javascript:addMousedownListener();">Click</a> to
       add an event listener for: <span style="color: red;" id="showbutton">button click</span>
   </ul>
 </div>
 
 <div>
   <p id="special">Special:</p>
   <select id="option" onclick="window.location = '/slow?delay=1'">
--- a/testing/marionette/harness/marionette_harness/www/test_inner_iframe.html
+++ b/testing/marionette/harness/marionette_harness/www/test_inner_iframe.html
@@ -3,11 +3,11 @@
    - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
 
 <!doctype html>
 <html>
 <head>
 <title>Inner Iframe</title>
 </head>
 <body>
-  <iframe src="test.html" id="inner_frame"></iframe> 
+  <iframe src="test.html" id="inner_frame"></iframe>
 </body>
-</html> 
+</html>
--- a/testing/marionette/proxy.js
+++ b/testing/marionette/proxy.js
@@ -1,24 +1,29 @@
 /* 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";
 
 const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
 
+Cu.import("resource://gre/modules/Log.jsm");
+Cu.import("resource://gre/modules/Services.jsm");
+
 Cu.import("chrome://marionette/content/error.js");
 Cu.import("chrome://marionette/content/modal.js");
 
 this.EXPORTED_SYMBOLS = ["proxy"];
 
 const uuidgen = Cc["@mozilla.org/uuid-generator;1"]
     .getService(Ci.nsIUUIDGenerator);
 
+const logger = Log.repository.getLogger("Marionette");
+
 // Proxy handler that traps requests to get a property.  Will prioritise
 // properties that exist on the object's own prototype.
 var ownPriorityGetterTrap = {
   get: (obj, prop) => {
     if (obj.hasOwnProperty(prop)) {
       return obj[prop];
     }
     return (...args) => obj.send(prop, args);
@@ -39,38 +44,45 @@ this.proxy = {};
  * passed literally.  The latter specialisation is temporary to achieve
  * backwards compatibility with listener.js.
  *
  * @param {function(): (nsIMessageSender|nsIMessageBroadcaster)} mmFn
  *     Closure function returning the current message manager.
  * @param {function(string, Object, number)} sendAsyncFn
  *     Callback for sending async messages.
  */
-proxy.toListener = function (mmFn, sendAsyncFn) {
-  let sender = new proxy.AsyncMessageChannel(mmFn, sendAsyncFn);
+proxy.toListener = function (mmFn, sendAsyncFn, browserFn) {
+  let sender = new proxy.AsyncMessageChannel(mmFn, sendAsyncFn, browserFn);
   return new Proxy(sender, ownPriorityGetterTrap);
 };
 
 /**
  * Provides a transparent interface between chrome- and content space.
  *
  * The AsyncMessageChannel is an abstraction of the message manager
  * IPC architecture allowing calls to be made to any registered message
  * listener in Marionette.  The {@code #send(...)} method returns a promise
  * that gets resolved when the message handler calls {@code .reply(...)}.
  */
 proxy.AsyncMessageChannel = class {
-  constructor(mmFn, sendAsyncFn) {
+  constructor(mmFn, sendAsyncFn, browserFn) {
+    this.mmFn_ = mmFn;
     this.sendAsync = sendAsyncFn;
+    this.browserFn_ = browserFn;
+
     // TODO(ato): Bug 1242595
     this.activeMessageId = null;
 
-    this.mmFn_ = mmFn;
     this.listeners_ = new Map();
     this.dialogueObserver_ = null;
+    this.closeHandler = null;
+  }
+
+  get browser() {
+    return this.browserFn_();
   }
 
   get mm() {
     return this.mmFn_();
   }
 
   /**
    * Send a message across the channel.  The name of the function to
@@ -119,32 +131,92 @@ proxy.AsyncMessageChannel = class {
             break;
 
           default:
             throw new TypeError(
                 `Unknown async response type: ${msg.json.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 = event => {
+        logger.debug(`Received DOM event "${event.type}" for "${event.target}"`);
+
+        switch (event.type) {
+          case "TabClose":
+          case "unload":
+            this.removeHandlers();
+            resolve();
+            break;
+        }
+      }
+
+      // 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) => {
-        this.cancelAll();
+        logger.debug(`Received observer notification "${topic}"`);
+
+        this.removeAllListeners_();
+        // TODO(ato): It's not ideal to have listener specific behaviour here:
+        this.sendAsync("cancelRequest");
+
+        this.removeHandlers();
         resolve();
       };
 
-      // start content message listener
-      // and install observers for global- and tab modal dialogues
+      // start content message listener, and install handlers for
+      // modal dialogues, and window/tab state changes.
       this.addListener_(path, cb);
-      modal.addHandler(this.dialogueObserver_);
+      this.addHandlers();
 
       // sendAsync is GeckoDriver#sendAsync
       this.sendAsync(name, marshal(args), uuid);
     });
   }
 
   /**
+   * Add all necessary handlers for events and observer notifications.
+   */
+  addHandlers() {
+    modal.addHandler(this.dialogueObserver_);
+
+    // Register event handlers in case the command closes the current tab or window,
+    // and the promise has to be escaped.
+    if (this.browser) {
+      this.browser.window.addEventListener("unload", this.closeHandler, false);
+
+      if (this.browser.tab) {
+        let node = this.browser.tab.addEventListener ?
+            this.browser.tab : this.browser.contentBrowser;
+        node.addEventListener("TabClose", this.closeHandler, false);
+      }
+    }
+  }
+
+  /**
+   * Remove all registered handlers for events and observer notifications.
+   */
+  removeHandlers() {
+    modal.removeHandler(this.dialogueObserver_);
+
+    if (this.browser) {
+      this.browser.window.removeEventListener("unload", this.closeHandler, false);
+
+      if (this.browser.tab) {
+        let node = this.browser.tab.addEventListener ?
+            this.browser.tab : this.browser.contentBrowser;
+
+        node.removeEventListener("TabClose", this.closeHandler, false);
+      }
+    }
+  }
+
+  /**
    * Reply to an asynchronous request.
    *
    * Passing an WebDriverError prototype will cause the receiving channel
    * to throw this error.
    *
    * Usage:
    *
    *     let channel = proxy.AsyncMessageChannel(
@@ -204,31 +276,20 @@ proxy.AsyncMessageChannel = class {
    *
    * @return {string}
    *     Path to be used for nsIMessageListener.addMessageListener.
    */
   static makePath(uuid) {
     return "Marionette:asyncReply:" + uuid;
   }
 
-  /**
-   * Abort listening for responses, remove all modal dialogue handlers,
-   * and cancel any ongoing requests in the listener.
-   */
-  cancelAll() {
-    this.removeAllListeners_();
-    modal.removeHandler(this.dialogueObserver_);
-    // TODO(ato): It's not ideal to have listener specific behaviour here:
-    this.sendAsync("cancelRequest");
-  }
-
   addListener_(path, callback) {
     let autoRemover = msg => {
       this.removeListener_(path);
-      modal.removeHandler(this.dialogueObserver_);
+      this.removeHandlers();
       callback(msg);
     };
 
     this.mm.addMessageListener(path, autoRemover);
     this.listeners_.set(path, autoRemover);
   }
 
   removeListener_(path) {