Bug 1357878 - Prevent hang on setting window size to existing size; r?maja_zf draft
authorAndreas Tolfsen <ato@mozilla.com>
Wed, 19 Apr 2017 21:19:36 +0100
changeset 569444 9d1226683c8bb3d3fd6e84ea1b2fa6dfb16ac246
parent 569138 0b77ed3f26c5335503bc16e85b8c067382e7bb1e
child 569445 efe58458d9c782264a07a2e21183d090cc6b6500
push id56187
push userbmo:ato@mozilla.com
push dateThu, 27 Apr 2017 14:41:11 +0000
reviewersmaja_zf
bugs1357878
milestone55.0a1
Bug 1357878 - Prevent hang on setting window size to existing size; r?maja_zf When the window's size is being set to the window's existing size, Marionette unconditionally listens for the DOM resize event. When a window is not resized, no such event fires. This patch skips setting the window size when the window's current size is the requested size. This bypasses the problem of listening for an event that never occurs. It also combines the window position- and size tests into a test_window_rect.py test, since they share many of the same characteristics. Fixes: https://github.com/mozilla/geckodriver/issues/643 MozReview-Commit-ID: IUtCFXwT1fh
testing/marionette/driver.js
testing/marionette/harness/marionette_harness/tests/unit/test_set_window_size.py
testing/marionette/harness/marionette_harness/tests/unit/test_window_position.py
testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py
testing/marionette/harness/marionette_harness/tests/unit/unit-tests.ini
--- a/testing/marionette/driver.js
+++ b/testing/marionette/driver.js
@@ -1370,46 +1370,50 @@ GeckoDriver.prototype.getWindowRect = fu
  * @throws {UnsupportedOperationError}
  *     Not applicable to application.
  * @throws {NoSuchWindowError}
  *     Top-level browsing context has been discarded.
  * @throws {UnexpectedAlertOpenError}
  *     A modal dialog is open, blocking this operation.
  */
 GeckoDriver.prototype.setWindowRect = function* (cmd, resp) {
-  assert.firefox();
+  assert.firefox()
   const win = assert.window(this.getCurrentWindow());
   assert.noUserPrompt(this.dialog);
 
-  let {x, y, height, width} = cmd.parameters;
+  let {x, y, width, height} = cmd.parameters;
 
   if (height != null && width != null) {
     assert.positiveInteger(height);
     assert.positiveInteger(width);
-    yield new Promise(resolve => {
-      // When the DOM resize event claims that it fires _after_ the document
-      // view has been resized, it is lying.
-      //
-      // Because resize events fire at a high rate, DOM modifications
-      // such as updates to outerWidth/outerHeight are not guaranteed to
-      // have processed.  To overcome this... abomination... of the web
-      // platform, we throttle the event using setTimeout.  If everything
-      // was well in this world we would use requestAnimationFrame, but
-      // it does not seem to like our particular flavour of XUL.
-      const fps15 = 66;
-      const synchronousResize = () => win.setTimeout(resolve, fps15);
-      win.addEventListener("resize", synchronousResize, {once: true});
-      win.resizeTo(width, height);
-    });
+
+    if (win.outerWidth != width && win.outerHeight != height) {
+      yield new Promise(resolve => {
+        // When the DOM resize event claims that it fires _after_ the document
+        // view has been resized, it is lying.
+        //
+        // Because resize events fire at a high rate, DOM modifications
+        // such as updates to outerWidth/outerHeight are not guaranteed to
+        // have processed.  To overcome this... abomination... of the web
+        // platform, we throttle the event using setTimeout.  If everything
+        // was well in this world we would use requestAnimationFrame, but
+        // it does not seem to like our particular flavour of XUL.
+        const fps15 = 66;
+        const synchronousResize = () => win.setTimeout(resolve, fps15);
+        win.addEventListener("resize", synchronousResize, {once: true});
+        win.resizeTo(width, height);
+      });
+    }
   }
 
   if (x != null && y != null) {
     assert.integer(x);
     assert.integer(y);
-    let orig = {screenX: win.screenX, screenY: win.screenY};
+    const orig = {screenX: win.screenX, screenY: win.screenY};
+
     win.moveTo(x, y);
     yield wait.until((resolve, reject) => {
       if ((x == win.screenX && y == win.screenY) ||
         (win.screenX != orig.screenX || win.screenY != orig.screenY)) {
         resolve();
       } else {
         reject();
       }
@@ -1417,17 +1421,16 @@ GeckoDriver.prototype.setWindowRect = fu
   }
 
   return {
     "x": win.screenX,
     "y": win.screenY,
     "width": win.outerWidth,
     "height": win.outerHeight,
   };
-
 };
 
 /**
  * Switch current top-level browsing context by name or server-assigned ID.
  * Searches for windows by name, then ID.  Content windows take precedence.
  *
  * @param {string} name
  *     Target name or ID of the window to switch to.
deleted file mode 100644
--- a/testing/marionette/harness/marionette_harness/tests/unit/test_set_window_size.py
+++ /dev/null
@@ -1,84 +0,0 @@
-# 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/.
-
-from marionette_harness import MarionetteTestCase
-
-
-class TestSetWindowSize(MarionetteTestCase):
-    def setUp(self):
-        super(MarionetteTestCase, self).setUp()
-        self.start_size = self.marionette.window_size
-        self.max_width = self.marionette.execute_script("return window.screen.availWidth;")
-        self.max_height = self.marionette.execute_script("return window.screen.availHeight;")
-
-    def tearDown(self):
-        # WebDriver spec says a resize cannot result in window being maximized, an
-        # error is returned if that is the case; therefore if the window is maximized
-        # at the start of this test, returning to the original size via set_window_size
-        # size will result in error; so reset to original size minus 1 pixel width
-        if self.start_size['width'] == self.max_width and self.start_size['height'] == self.max_height:
-            self.start_size['width']-=1
-        self.marionette.set_window_size(self.start_size['width'], self.start_size['height'])
-        super(MarionetteTestCase, self).tearDown()
-
-    def test_that_we_can_get_and_set_window_size(self):
-        # event handler
-        self.marionette.execute_script("""
-        window.wrappedJSObject.rcvd_event = false;
-        window.onresize = function() {
-            window.wrappedJSObject.rcvd_event = true;
-        };
-        """)
-
-        # valid size
-        width = self.max_width - 100
-        height = self.max_height - 100
-        self.marionette.set_window_size(width, height)
-        self.wait_for_condition(lambda m: m.execute_script("return window.wrappedJSObject.rcvd_event;"))
-        size = self.marionette.window_size
-        self.assertEqual(size['width'], width,
-                         "Window width is {0} but should be {1}".format(size['width'], width))
-        self.assertEqual(size['height'], height,
-                         "Window height is {0} but should be {1}".format(size['height'], height))
-
-    def test_that_we_can_get_new_size_when_set_window_size(self):
-        actual = self.marionette.window_size
-        width = actual['width'] - 50
-        height = actual['height'] - 50
-        size = self.marionette.set_window_size(width, height)
-        self.assertIsNotNone(size, "Response is None")
-        self.assertEqual(size['width'], width,
-                         "New width is {0} but should be {1}".format(size['width'], width))
-        self.assertEqual(size['height'], height,
-                         "New height is {0} but should be {1}".format(size['height'], height))
-
-    def test_possible_to_request_window_larger_than_screen(self):
-        self.marionette.set_window_size(4 * self.max_width, 4 * self.max_height)
-        size = self.marionette.window_size
-
-        # In X the window size may be greater than the bounds of the screen
-        self.assertGreaterEqual(size["width"], self.max_width)
-        self.assertGreaterEqual(size["height"], self.max_height)
-
-    def test_that_we_can_maximise_the_window(self):
-        # valid size
-        width = self.max_width - 100
-        height = self.max_height - 100
-        self.marionette.set_window_size(width, height)
-
-        # event handler
-        self.marionette.execute_script("""
-        window.wrappedJSObject.rcvd_event = false;
-        window.onresize = function() {
-            window.wrappedJSObject.rcvd_event = true;
-        };
-        """)
-        self.marionette.maximize_window()
-        self.wait_for_condition(lambda m: m.execute_script("return window.wrappedJSObject.rcvd_event;"))
-
-        size = self.marionette.window_size
-        self.assertGreaterEqual(size['width'], self.max_width,
-                         "Window width does not use availWidth, current width: {0}, max width: {1}".format(size['width'], self.max_width))
-        self.assertGreaterEqual(size['height'], self.max_height,
-                         "Window height does not use availHeight. current width: {0}, max width: {1}".format(size['height'], self.max_height))
deleted file mode 100644
--- a/testing/marionette/harness/marionette_harness/tests/unit/test_window_position.py
+++ /dev/null
@@ -1,113 +0,0 @@
-# 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/.
-
-from marionette_driver.errors import InvalidArgumentException
-
-from marionette_harness import MarionetteTestCase
-
-
-class TestWindowPosition(MarionetteTestCase):
-
-    def setUp(self):
-        MarionetteTestCase.setUp(self)
-        self.original_position = self.marionette.get_window_position()
-
-    def tearDown(self):
-        x, y = self.original_position["x"], self.original_position["y"]
-        self.marionette.set_window_position(x, y)
-        MarionetteTestCase.tearDown(self)
-
-    def test_get_types(self):
-        position = self.marionette.get_window_position()
-        self.assertIsInstance(position["x"], int)
-        self.assertIsInstance(position["y"], int)
-
-    def test_set_types(self):
-        for x, y in (["a", "b"], [1.2, 3.4], [True, False], [[], []], [{}, {}]):
-            print("testing invalid type position ({},{})".format(x, y))
-            with self.assertRaises(InvalidArgumentException):
-                self.marionette.set_window_position(x, y)
-
-    def test_setting_window_rect_with_nulls_errors(self):
-        with self.assertRaises(InvalidArgumentException):
-            self.marionette.set_window_rect(height=None, width=None,
-                                            x=None, y=None)
-
-    def test_set_position_with_rect(self):
-        old_position = self.marionette.window_rect
-        wanted_position = {"x": old_position["x"] + 10, "y": old_position["y"] + 10}
-
-        new_position = self.marionette.set_window_rect(x=wanted_position["x"], y=wanted_position["y"])
-
-        self.assertNotEqual(old_position["x"], new_position["x"])
-        self.assertNotEqual(old_position["y"], new_position["y"])
-
-    def test_set_size_with_rect(self):
-        actual = self.marionette.window_size
-        width = actual["width"] - 50
-        height = actual["height"] - 50
-
-        size = self.marionette.set_window_rect(width=width, height=height)
-        self.assertEqual(size["width"], width,
-                         "New width is {0} but should be {1}".format(size["width"], width))
-        self.assertEqual(size["height"], height,
-                         "New height is {0} but should be {1}".format(size["height"], height))
-
-    def test_move_to_new_position(self):
-        old_position = self.marionette.get_window_position()
-        new_position = {"x": old_position["x"] + 10, "y": old_position["y"] + 10}
-        self.marionette.set_window_position(new_position["x"], new_position["y"])
-        self.assertNotEqual(old_position["x"], new_position["x"])
-        self.assertNotEqual(old_position["y"], new_position["y"])
-
-    def test_move_to_existing_position(self):
-        old_position = self.marionette.get_window_position()
-        self.marionette.set_window_position(old_position["x"], old_position["y"])
-        new_position = self.marionette.get_window_position()
-        self.assertEqual(old_position["x"], new_position["x"])
-        self.assertEqual(old_position["y"], new_position["y"])
-
-    def test_move_to_negative_coordinates(self):
-        print("Current position: {}".format(
-            self.marionette.get_window_position()))
-        self.marionette.set_window_position(-8, -8)
-        position = self.marionette.get_window_position()
-        print("Position after requesting move to negative coordinates: {}".format(position))
-
-        # Different systems will report more or less than (-8,-8)
-        # depending on the characteristics of the window manager, since
-        # the screenX/screenY position measures the chrome boundaries,
-        # including any WM decorations.
-        #
-        # This makes this hard to reliably test across different
-        # environments.  Generally we are happy when calling
-        # marionette.set_window_position with negative coordinates does
-        # not throw.
-        #
-        # Because we have to cater to an unknown set of environments,
-        # the following assertions are the most common denominator that
-        # make this test pass, irregardless of system characteristics.
-
-        os = self.marionette.session_capabilities["platformName"]
-
-        # Certain WMs prohibit windows from being moved off-screen,
-        # but we don't have this information.  It should be safe to
-        # assume a window can be moved to (0,0) or less.
-        if os == "linux":
-            # certain WMs prohibit windows from being moved off-screen
-            self.assertLessEqual(position["x"], 0)
-            self.assertLessEqual(position["y"], 0)
-
-        # On macOS, windows can only be moved off the screen on the
-        # horizontal axis.  The system menu bar also blocks windows from
-        # being moved to (0,0).
-        elif os == "darwin":
-            self.assertEqual(-8, position["x"])
-            self.assertEqual(23, position["y"])
-
-        # It turns out that Windows is the only platform on which the
-        # window can be reliably positioned off-screen.
-        elif os == "windows_nt":
-            self.assertEqual(-8, position["x"])
-            self.assertEqual(-8, position["y"])
new file mode 100644
--- /dev/null
+++ b/testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py
@@ -0,0 +1,186 @@
+# 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/.
+
+from marionette_driver.errors import InvalidArgumentException
+
+from marionette_harness import MarionetteTestCase
+
+
+class TestPosition(MarionetteTestCase):
+
+    def setUp(self):
+        MarionetteTestCase.setUp(self)
+        self.original_position = self.marionette.get_window_position()
+
+    def tearDown(self):
+        x, y = self.original_position["x"], self.original_position["y"]
+        self.marionette.set_window_position(x, y)
+        MarionetteTestCase.tearDown(self)
+
+    def test_get_types(self):
+        position = self.marionette.get_window_position()
+        self.assertIn("x", position)
+        self.assertIn("y", position)
+        self.assertIsInstance(position["x"], int)
+        self.assertIsInstance(position["y"], int)
+
+    def test_set_types(self):
+        for x, y in (["a", "b"], [1.2, 3.4], [True, False], [[], []], [{}, {}]):
+            print("testing invalid type position ({},{})".format(x, y))
+            with self.assertRaises(InvalidArgumentException):
+                self.marionette.set_window_position(x, y)
+
+    def test_setting_window_rect_with_nulls_errors(self):
+        with self.assertRaises(InvalidArgumentException):
+            self.marionette.set_window_rect(height=None, width=None,
+                                            x=None, y=None)
+
+    def test_set_position_with_rect(self):
+        old_position = self.marionette.window_rect
+        wanted_position = {"x": old_position["x"] + 10, "y": old_position["y"] + 10}
+
+        new_position = self.marionette.set_window_rect(x=wanted_position["x"], y=wanted_position["y"])
+
+        self.assertNotEqual(old_position["x"], new_position["x"])
+        self.assertNotEqual(old_position["y"], new_position["y"])
+
+    def test_move_to_new_position(self):
+        old_position = self.marionette.get_window_position()
+        new_position = {"x": old_position["x"] + 10, "y": old_position["y"] + 10}
+        self.marionette.set_window_position(new_position["x"], new_position["y"])
+        self.assertNotEqual(old_position["x"], new_position["x"])
+        self.assertNotEqual(old_position["y"], new_position["y"])
+
+    def test_move_to_existing_position(self):
+        old_position = self.marionette.get_window_position()
+        self.marionette.set_window_position(old_position["x"], old_position["y"])
+        new_position = self.marionette.get_window_position()
+        self.assertEqual(old_position["x"], new_position["x"])
+        self.assertEqual(old_position["y"], new_position["y"])
+
+    def test_move_to_negative_coordinates(self):
+        print("Current position: {}".format(
+            self.marionette.get_window_position()))
+        self.marionette.set_window_position(-8, -8)
+        position = self.marionette.get_window_position()
+        print("Position after requesting move to negative coordinates: {}".format(position))
+
+        # Different systems will report more or less than (-8,-8)
+        # depending on the characteristics of the window manager, since
+        # the screenX/screenY position measures the chrome boundaries,
+        # including any WM decorations.
+        #
+        # This makes this hard to reliably test across different
+        # environments.  Generally we are happy when calling
+        # marionette.set_window_position with negative coordinates does
+        # not throw.
+        #
+        # Because we have to cater to an unknown set of environments,
+        # the following assertions are the most common denominator that
+        # make this test pass, irregardless of system characteristics.
+
+        os = self.marionette.session_capabilities["platformName"]
+
+        # Certain WMs prohibit windows from being moved off-screen,
+        # but we don't have this information.  It should be safe to
+        # assume a window can be moved to (0,0) or less.
+        if os == "linux":
+            # certain WMs prohibit windows from being moved off-screen
+            self.assertLessEqual(position["x"], 0)
+            self.assertLessEqual(position["y"], 0)
+
+        # On macOS, windows can only be moved off the screen on the
+        # horizontal axis.  The system menu bar also blocks windows from
+        # being moved to (0,0).
+        elif os == "darwin":
+            self.assertEqual(-8, position["x"])
+            self.assertEqual(23, position["y"])
+
+        # It turns out that Windows is the only platform on which the
+        # window can be reliably positioned off-screen.
+        elif os == "windows_nt":
+            self.assertEqual(-8, position["x"])
+            self.assertEqual(-8, position["y"])
+
+
+class TestSize(MarionetteTestCase):
+
+    def setUp(self):
+        super(MarionetteTestCase, self).setUp()
+        self.max = self.marionette.execute_script("""
+            return {
+              width: window.screen.availWidth,
+              height: window.screen.availHeight,
+            }""", sandbox=None)
+
+        # WebDriver spec says a resize cannot result in window being
+        # maximised, an error is returned if that is the case; therefore if
+        # the window is maximised at the start of this test, returning to
+        # the original size via set_window_size size will result in error;
+        # so reset to original size minus 1 pixel width
+        start_size = self.marionette.window_size
+        if start_size["width"] == self.max["width"] and start_size["height"] == self.max["height"]:
+            self.start_size["width"] -= 1
+            self.start_size["height"] -= 1
+        self.marionette.set_window_size(start_size["width"], start_size["height"])
+
+        self.original_size = self.marionette.window_size
+
+    def tearDown(self):
+        self.marionette.set_window_size(
+            self.original_size["width"], self.original_size["height"])
+        super(MarionetteTestCase, self).tearDown()
+
+    def test_get_types(self):
+        size = self.marionette.window_size
+        self.assertIn("width", size)
+        self.assertIn("height", size)
+        self.assertIsInstance(size["width"], int)
+        self.assertIsInstance(size["height"], int)
+
+    def test_set_types(self):
+        for width, height in (["a", "b"], [1.2, 3.4], [True, False], [[], []], [{}, {}]):
+            print("testing invalid type size ({},{})".format(width, height))
+            with self.assertRaises(InvalidArgumentException):
+                self.marionette.set_window_size(width, height)
+
+    def test_setting_window_rect_with_nulls_errors(self):
+        with self.assertRaises(InvalidArgumentException):
+            self.marionette.set_window_rect(height=None, width=None,
+                                            x=None, y=None)
+
+    def test_set_size_with_rect(self):
+        actual = self.marionette.window_size
+        width = actual["width"] - 50
+        height = actual["height"] - 50
+
+        size = self.marionette.set_window_rect(width=width, height=height)
+        self.assertEqual(size["width"], width,
+                         "New width is {0} but should be {1}".format(size["width"], width))
+        self.assertEqual(size["height"], height,
+                         "New height is {0} but should be {1}".format(size["height"], height))
+
+    def test_resize_to_new_size(self):
+        old = self.marionette.window_size
+        new = {"width": old["width"] + 10, "height": old["height"] + 10}
+        self.marionette.set_window_size(new["width"], new["height"])
+        actual = self.marionette.window_size
+        self.assertEqual(actual["width"], new["width"])
+        self.assertEqual(actual["height"], new["height"])
+
+    def test_resize_to_existing_size(self):
+        old = self.marionette.window_size
+        self.marionette.set_window_size(old["width"], old["height"])
+        new = self.marionette.window_size
+        self.assertEqual(old["width"], new["width"])
+        self.assertEqual(old["height"], new["height"])
+
+    def test_resize_larger_than_screen(self):
+        self.marionette.set_window_size(
+            self.max["width"] * 2, self.max["height"] * 2)
+        new = self.marionette.window_size
+
+        # in X the window size may be greater than the bounds of the screen
+        self.assertGreaterEqual(new["width"], self.max["width"])
+        self.assertGreaterEqual(new["height"], self.max["height"])
--- a/testing/marionette/harness/marionette_harness/tests/unit/unit-tests.ini
+++ b/testing/marionette/harness/marionette_harness/tests/unit/unit-tests.ini
@@ -67,17 +67,17 @@ skip-if = appname == 'fennec'
 
 [test_visibility.py]
 [test_window_handles_chrome.py]
 skip-if = appname == 'fennec'
 [test_window_handles_content.py]
 [test_window_close_chrome.py]
 skip-if = appname == 'fennec'
 [test_window_close_content.py]
-[test_window_position.py]
+[test_window_rect.py]
 skip-if = appname == 'fennec'
 [test_window_status_content.py]
 [test_window_status_chrome.py]
 
 [test_screenshot.py]
 [test_cookies.py]
 [test_title.py]
 [test_title_chrome.py]
@@ -95,18 +95,16 @@ skip-if = true # Bug 925688
 [test_errors.py]
 
 [test_execute_isolate.py]
 [test_click_scrolling.py]
 [test_profile_management.py]
 skip-if = manage_instance == false || appname == 'fennec' # Bug 1298921 and bug 1322993
 [test_quit_restart.py]
 skip-if = manage_instance == false || appname == 'fennec' # Bug 1298921 and bug 1322993
-[test_set_window_size.py]
-skip-if = os == "linux" || appname == 'fennec' # Bug 1085717
 [test_with_using_context.py]
 
 [test_modal_dialogs.py]
 skip-if = appname == 'fennec' # Bug 1325738
 [test_key_actions.py]
 [test_mouse_action.py]
 skip-if = appname == 'fennec'
 [test_teardown_context_preserved.py]