Bug 1368965 - Don't inject chrome window handles into window handles. r=ato
authorHenrik Skupin <mail@hskupin.info>
Tue, 13 Jun 2017 18:08:44 +0200
changeset 363948 5343d6a9bc644de4f3d0c34c92c4a0ac1d6ee582
parent 363947 603cd9b8ff9939d49b39e4f8894fb6b787886b57
child 363949 ed85f2ec9f2ae8649dd2406f75027a29d80ebff2
push id44749
push userhskupin@mozilla.com
push dateWed, 14 Jun 2017 20:09:11 +0000
treeherderautoland@5343d6a9bc64 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersato
bugs1368965
milestone56.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1368965 - Don't inject chrome window handles into window handles. r=ato Chrome window handles and window handles are identifiers for different types of windows. As such those should not be mixed up. Especially for chrome windows without a tabbrowser we erroneously inject such a chrome window handle. It can break those tests which assume it's a tab. MozReview-Commit-ID: 1n2vOyfaSUh
testing/marionette/driver.js
testing/marionette/harness/marionette_harness/tests/unit/test_screenshot.py
testing/marionette/harness/marionette_harness/tests/unit/test_window_close_chrome.py
testing/marionette/harness/marionette_harness/tests/unit/test_window_close_content.py
testing/marionette/harness/marionette_harness/tests/unit/test_window_handles_chrome.py
testing/marionette/harness/marionette_harness/tests/unit/test_window_handles_content.py
testing/marionette/harness/marionette_harness/tests/unit/test_window_type.py
--- a/testing/marionette/driver.js
+++ b/testing/marionette/driver.js
@@ -183,26 +183,24 @@ Object.defineProperty(GeckoDriver.protot
   get: function () {
     let hs = [];
     let winEn = Services.wm.getEnumerator(null);
 
     while (winEn.hasMoreElements()) {
       let win = winEn.getNext();
       let tabBrowser = browser.getTabBrowser(win);
 
+      // Only return handles for browser windows
       if (tabBrowser) {
         tabBrowser.tabs.forEach(tab => {
           let winId = this.getIdForBrowser(browser.getBrowserForTab(tab));
           if (winId !== null) {
             hs.push(winId);
           }
         });
-      } else {
-        // For other chrome windows beside the browser window, only add the window itself.
-        hs.push(getOuterWindowId(win));
       }
     }
 
     return hs;
   },
 });
 
 Object.defineProperty(GeckoDriver.prototype, "chromeWindowHandles", {
@@ -344,26 +342,18 @@ GeckoDriver.prototype.getCurrentWindow =
       }
 
       break;
 
     case Context.CONTENT:
       if (this.curFrame !== null) {
         win = this.curFrame;
 
-      } else if (this.curBrowser !== null) {
-        if (browser.getTabBrowser(this.curBrowser.window)) {
-          // For browser windows we have to check if the current tab still exists.
-          if (this.curBrowser.tab && this.curBrowser.contentBrowser) {
-            win = this.curBrowser.window;
-          }
-        } else {
-          // For non-browser windows just return the window.
+      } else if (this.curBrowser !== null && this.curBrowser.contentBrowser) {
           win = this.curBrowser.window;
-        }
       }
 
       break;
   }
 
   return win;
 };
 
@@ -1186,32 +1176,24 @@ GeckoDriver.prototype.getIdForBrowser = 
  * to the currently selected tab.
  *
  * Return an opaque server-assigned identifier to this window that
  * uniquely identifies it within this Marionette instance.  This can
  * be used to switch to this window at a later point.
  *
  * @return {string}
  *     Unique window handle.
+ *
+ * @throws {NoSuchWindowError}
+ *     Top-level browsing context has been discarded.
  */
 GeckoDriver.prototype.getWindowHandle = function (cmd, resp) {
-  assert.window(this.getCurrentWindow(Context.CONTENT));
-
-  // curFrameId always holds the current tab.
-  if (this.curBrowser.curFrameId) {
-    resp.body.value = this.curBrowser.curFrameId.toString();
-    return;
-  }
-
-  for (let i in this.browsers) {
-    if (this.curBrowser == this.browsers[i]) {
-      resp.body.value = i.toString();
-      return;
-    }
-  }
+  assert.contentBrowser(this.curBrowser);
+
+  return this.curBrowser.curFrameId.toString();
 };
 
 /**
  * Get a list of top-level browsing contexts. On desktop this typically
  * corresponds to the set of open tabs for browser windows, or the window itself
  * for non-browser chrome windows.
  *
  * Each window handle is assigned by the server and is guaranteed unique,
@@ -1229,16 +1211,19 @@ GeckoDriver.prototype.getWindowHandles =
  * may itself contain tabs.
  *
  * Return an opaque server-assigned identifier to this window that
  * uniquely identifies it within this Marionette instance.  This can
  * be used to switch to this window at a later point.
  *
  * @return {string}
  *     Unique window handle.
+ *
+ * @throws {NoSuchWindowError}
+ *     Top-level browsing context has been discarded.
  */
 GeckoDriver.prototype.getChromeWindowHandle = function (cmd, resp) {
   assert.window(this.getCurrentWindow(Context.CHROME));
 
   for (let i in this.browsers) {
     if (this.curBrowser == this.browsers[i]) {
       resp.body.value = i;
       return;
@@ -2520,38 +2505,35 @@ GeckoDriver.prototype.deleteCookie = fun
  *     Unique window handles of remaining windows.
  *
  * @throws {NoSuchWindowError}
  *     Top-level browsing context has been discarded.
  * @throws {UnexpectedAlertOpenError}
  *     A modal dialog is open, blocking this operation.
  */
 GeckoDriver.prototype.close = function (cmd, resp) {
-  assert.window(this.getCurrentWindow());
+  assert.contentBrowser(this.curBrowser);
   assert.noUserPrompt(this.dialog);
 
   let nwins = 0;
 
   let winEn = Services.wm.getEnumerator(null);
   while (winEn.hasMoreElements()) {
     let win = winEn.getNext();
-
-    // For browser windows count the tabs. Otherwise take the window itself.
     let tabbrowser = browser.getTabBrowser(win);
+
     if (tabbrowser) {
       nwins += tabbrowser.tabs.length;
-    } else {
-      nwins++;
     }
   }
 
   // If there is only 1 window left, do not close it. Instead return a faked
   // empty array of window handles. This will instruct geckodriver to terminate
   // the application.
-  if (nwins == 1) {
+  if (nwins === 1) {
     return [];
   }
 
   if (this.mm != globalMessageManager) {
     this.mm.removeDelayedFrameScript(FRAME_SCRIPT);
   }
 
   return this.curBrowser.closeTab().then(() => this.windowHandles.map(String));
--- a/testing/marionette/harness/marionette_harness/tests/unit/test_screenshot.py
+++ b/testing/marionette/harness/marionette_harness/tests/unit/test_screenshot.py
@@ -139,17 +139,17 @@ class TestScreenCaptureChrome(WindowMana
         def opener():
             features = "chrome"
             if height is not None:
                 features += ",height={}".format(height)
             if width is not None:
                 features += ",width={}".format(width)
 
             self.marionette.execute_script("""
-                window.open(arguments[0], "", arguments[1]);
+                window.openDialog(arguments[0], "", arguments[1]);
                 """, script_args=[url, features])
 
         return self.open_window(opener)
 
     def test_capture_different_context(self):
         """Check that screenshots in content and chrome are different."""
         with self.marionette.using_context("content"):
             screenshot_content = self.marionette.screenshot()
--- a/testing/marionette/harness/marionette_harness/tests/unit/test_window_close_chrome.py
+++ b/testing/marionette/harness/marionette_harness/tests/unit/test_window_close_chrome.py
@@ -34,21 +34,22 @@ class TestCloseWindow(WindowManagerMixin
             self.marionette.execute_script("""
               window.open('chrome://marionette/content/test.xul',
                           'foo', 'chrome,centerscreen');
             """)
 
         win = self.open_window(trigger=open_window_with_js)
         self.marionette.switch_to_window(win)
 
-        self.assertIn(win, self.marionette.window_handles)
+        self.assertIn(win, self.marionette.chrome_window_handles)
+        self.assertNotIn(win, self.marionette.window_handles)
         chrome_window_handles = self.marionette.close_chrome_window()
         self.assertNotIn(win, chrome_window_handles)
         self.assertListEqual(self.start_windows, chrome_window_handles)
-        self.assertNotIn(win, self.marionette.window_handles)
+        self.assertNotIn(win, self.marionette.chrome_window_handles)
 
     def test_close_chrome_window_for_last_open_window(self):
         self.close_all_windows()
 
         self.assertListEqual([], self.marionette.close_chrome_window())
         self.assertListEqual([self.start_tab], self.marionette.window_handles)
         self.assertListEqual([self.start_window], self.marionette.chrome_window_handles)
         self.assertIsNotNone(self.marionette.session)
--- a/testing/marionette/harness/marionette_harness/tests/unit/test_window_close_content.py
+++ b/testing/marionette/harness/marionette_harness/tests/unit/test_window_close_content.py
@@ -32,17 +32,18 @@ class TestCloseWindow(WindowManagerMixin
                 self.marionette.execute_script("""
                   window.open('chrome://marionette/content/test.xul',
                               'foo', 'chrome,centerscreen');
                 """)
 
         win = self.open_window(trigger=open_window_with_js)
         self.marionette.switch_to_window(win)
 
-        self.assertIn(win, self.marionette.window_handles)
+        self.assertIn(win, self.marionette.chrome_window_handles)
+        self.assertNotIn(win, self.marionette.window_handles)
         chrome_window_handles = self.marionette.close_chrome_window()
         self.assertNotIn(win, chrome_window_handles)
         self.assertListEqual(self.start_windows, chrome_window_handles)
         self.assertNotIn(win, self.marionette.window_handles)
 
     @skip_if_mobile("Interacting with chrome windows not available for Fennec")
     def test_close_chrome_window_for_last_open_window(self):
         self.close_all_windows()
--- a/testing/marionette/harness/marionette_harness/tests/unit/test_window_handles_chrome.py
+++ b/testing/marionette/harness/marionette_harness/tests/unit/test_window_handles_chrome.py
@@ -57,16 +57,46 @@ class TestWindowHandles(WindowManagerMix
         window_handles_in_chrome_scope = self.marionette.window_handles
 
         with self.marionette.using_context("content"):
             self.assertEqual(self.marionette.chrome_window_handles,
                              chrome_window_handles_in_chrome_scope)
             self.assertEqual(self.marionette.window_handles,
                              window_handles_in_chrome_scope)
 
+    def test_chrome_window_handles_after_opening_new_dialog(self):
+        xul_dialog = "chrome://marionette/content/test_dialog.xul"
+
+        def open_via_js():
+            self.marionette.execute_script("""
+                window.openDialog(arguments[0]);
+            """, script_args=(xul_dialog,))
+
+        new_win = self.open_window(trigger=open_via_js)
+        self.assert_window_handles()
+        self.assertEqual(len(self.marionette.chrome_window_handles), len(self.start_windows) + 1)
+        self.assertEqual(self.marionette.current_chrome_window_handle, self.start_window)
+
+        # Check that the new tab has the correct page loaded
+        self.marionette.switch_to_window(new_win)
+        self.assert_window_handles()
+        self.assertEqual(self.marionette.current_chrome_window_handle, new_win)
+        self.assertEqual(self.marionette.get_url(), xul_dialog)
+
+        # Close the opened dialog and carry on in our original tab.
+        self.marionette.close_chrome_window()
+        self.assert_window_handles()
+        self.assertEqual(len(self.marionette.chrome_window_handles), len(self.start_windows))
+
+        self.marionette.switch_to_window(self.start_window)
+        self.assert_window_handles()
+        self.assertEqual(self.marionette.current_chrome_window_handle, self.start_window)
+        with self.marionette.using_context("content"):
+            self.assertEqual(self.marionette.get_url(), self.test_page)
+
     def test_chrome_window_handles_after_opening_new_window(self):
         def open_with_link():
             with self.marionette.using_context("content"):
                 link = self.marionette.find_element(By.ID, "new-window")
                 link.click()
 
         # We open a new window but are actually interested in the new tab
         new_win = self.open_window(trigger=open_with_link)
@@ -134,16 +164,50 @@ class TestWindowHandles(WindowManagerMix
         self.marionette.switch_to_window(new_tab)
         self.marionette.close()
         self.assert_window_handles()
         self.assertEqual(len(self.marionette.window_handles), len(self.start_tabs))
 
         self.marionette.switch_to_window(self.start_tab)
         self.assertEqual(self.marionette.current_window_handle, self.start_tab)
 
+    def test_window_handles_after_opening_new_dialog(self):
+        xul_dialog = "chrome://marionette/content/test_dialog.xul"
+
+        def open_via_js():
+            self.marionette.execute_script("""
+                window.openDialog(arguments[0]);
+            """, script_args=(xul_dialog,))
+
+        new_win = self.open_window(trigger=open_via_js)
+        self.assert_window_handles()
+        self.assertEqual(len(self.marionette.window_handles), len(self.start_tabs))
+        self.assertEqual(self.marionette.current_window_handle, self.start_tab)
+
+        self.marionette.switch_to_window(new_win)
+        self.assert_window_handles()
+        self.assertEqual(self.marionette.get_url(), xul_dialog)
+
+        # Check that the opened dialog is not accessible via window handles
+        with self.assertRaises(errors.NoSuchWindowException):
+            self.marionette.current_window_handle
+        with self.assertRaises(errors.NoSuchWindowException):
+            self.marionette.close()
+
+        # Close the dialog and carry on in our original tab.
+        self.marionette.close_chrome_window()
+        self.assert_window_handles()
+        self.assertEqual(len(self.marionette.window_handles), len(self.start_tabs))
+
+        self.marionette.switch_to_window(self.start_tab)
+        self.assert_window_handles()
+        self.assertEqual(self.marionette.current_window_handle, self.start_tab)
+        with self.marionette.using_context("content"):
+            self.assertEqual(self.marionette.get_url(), self.test_page)
+
     def test_window_handles_after_opening_new_window(self):
         def open_with_link():
             with self.marionette.using_context("content"):
                 link = self.marionette.find_element(By.ID, "new-window")
                 link.click()
 
         # We open a new window but are actually interested in the new tab
         new_tab = self.open_tab(trigger=open_with_link)
--- a/testing/marionette/harness/marionette_harness/tests/unit/test_window_handles_content.py
+++ b/testing/marionette/harness/marionette_harness/tests/unit/test_window_handles_content.py
@@ -98,28 +98,32 @@ class TestWindowHandles(WindowManagerMix
 
     @skip_if_mobile("Fennec doesn't support other chrome windows")
     def test_window_handles_after_opening_new_non_browser_window(self):
         def open_with_link():
             self.marionette.navigate(self.marionette.absolute_url("blob_download.html"))
             link = self.marionette.find_element(By.ID, "blob-download")
             link.click()
 
-        # We open a new window but are actually interested in the new tab
-        new_tab = self.open_tab(trigger=open_with_link)
+        new_win = self.open_window(trigger=open_with_link)
         self.assert_window_handles()
-        self.assertEqual(len(self.marionette.window_handles), len(self.start_tabs) + 1)
+        self.assertEqual(len(self.marionette.window_handles), len(self.start_tabs))
         self.assertEqual(self.marionette.current_window_handle, self.start_tab)
 
-        self.marionette.switch_to_window(new_tab)
+        self.marionette.switch_to_window(new_win)
         self.assert_window_handles()
-        self.assertEqual(self.marionette.current_window_handle, new_tab)
+
+        # Check that the opened window is not accessible via window handles
+        with self.assertRaises(errors.NoSuchWindowException):
+            self.marionette.current_window_handle
+        with self.assertRaises(errors.NoSuchWindowException):
+            self.marionette.close()
 
         # Close the opened window and carry on in our original tab.
-        self.marionette.close()
+        self.marionette.close_chrome_window()
         self.assert_window_handles()
         self.assertEqual(len(self.marionette.window_handles), len(self.start_tabs))
 
         self.marionette.switch_to_window(self.start_tab)
         self.assert_window_handles()
         self.assertEqual(self.marionette.current_window_handle, self.start_tab)
 
     def test_window_handles_after_closing_original_tab(self):
deleted file mode 100644
--- a/testing/marionette/harness/marionette_harness/tests/unit/test_window_type.py
+++ /dev/null
@@ -1,27 +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 TestWindowTypeChrome(MarionetteTestCase):
-    def setUp(self):
-        MarionetteTestCase.setUp(self)
-        self.marionette.set_context("chrome")
-        self.win = self.marionette.current_window_handle
-        self.marionette.execute_script("window.open('chrome://marionette/content/test.xul', 'foo', 'chrome,centerscreen');")
-        self.marionette.switch_to_window('foo')
-        self.assertNotEqual(self.win, self.marionette.current_window_handle)
-
-    def tearDown(self):
-        self.assertNotEqual(self.win, self.marionette.current_window_handle)
-        self.marionette.execute_script("window.close();")
-        self.marionette.switch_to_window(self.win)
-        MarionetteTestCase.tearDown(self)
-
-    def test_get_window_type(self):
-        window_type = self.marionette.execute_script("return window.document.documentElement.getAttribute('windowtype');")
-        self.assertEqual(window_type, self.marionette.get_window_type())
-        self.assertEqual('Test Type', self.marionette.get_window_type())
-