Bug 1318351 - Ensure that window ids are internally handled as numbers. draft
authorHenrik Skupin <mail@hskupin.info>
Tue, 09 May 2017 10:25:26 +0200
changeset 575484 94bc3b53eb7bd0b6c0c867cab25d0be3b01b7e9f
parent 575483 210c02862ba4a883aa4900f4be00a888c3af9564
child 575485 284272d67aeace79346abd0be78b73f275199d83
push id58064
push userbmo:hskupin@gmail.com
push dateWed, 10 May 2017 13:57:21 +0000
bugs1318351
milestone55.0a1
Bug 1318351 - Ensure that window ids are internally handled as numbers. Right now we retrieve window handles at different locations and as such those are sometimes numbers but also strings. Given that we have strict comparisons when checking for the id, checks can fail. The patch also ensures that both close() and closeChromeWindow() methods are returning the window ids as string. MozReview-Commit-ID: ImOLe0Z2OE1
testing/marionette/driver.js
testing/marionette/harness/marionette_harness/tests/unit/test_window_handles_chrome.py
testing/marionette/harness/marionette_harness/tests/unit/test_window_handles_content.py
--- a/testing/marionette/driver.js
+++ b/testing/marionette/driver.js
@@ -500,34 +500,32 @@ GeckoDriver.prototype.registerBrowser = 
     // if we're already in a remote frame, or it will point to some
     // random window, which will hopefully cause an href mismatch.
     // Currently this only happens in B2G for OOP frames registered in
     // Marionette:switchToFrame, so we'll acknowledge the switchToFrame
     // message here.
     //
     // TODO: Should have a better way of determining that this message
     // is from a remote frame.
-    this.curBrowser.frameManager.currentRemoteFrame.targetFrameId =
-        this.generateFrameId(id);
+    this.curBrowser.frameManager.currentRemoteFrame.targetFrameId = id;
   }
 
   let reg = {};
   // this will be sent to tell the content process if it is the main content
   let mainContent = this.curBrowser.mainContentId === null;
   // We want to ignore frames that are XUL browsers that aren't in the "main"
   // tabbrowser, but accept things on Fennec (which doesn't have a
   // xul:tabbrowser), and accept HTML iframes (because tests depend on it),
   // as well as XUL frames. Ideally this should be cleaned up and we should
   // keep track of browsers a different way.
   if (this.appName != "Firefox" || be.namespaceURI != XUL_NS ||
       be.nodeName != "browser" || be.getTabBrowser()) {
     // curBrowser holds all the registered frames in knownFrames
-    let uid = this.generateFrameId(id);
-    reg.id = uid;
-    reg.remotenessChange = this.curBrowser.register(uid, be);
+    reg.id = id;
+    reg.remotenessChange = this.curBrowser.register(id, be);
   }
 
   // set to true if we updated mainContentId
   mainContent = mainContent && this.curBrowser.mainContentId !== null;
   if (mainContent) {
     this.mainContentFrameId = this.curBrowser.curFrameId;
   }
 
@@ -1228,17 +1226,16 @@ GeckoDriver.prototype.getIdForBrowser = 
   }
   let permKey = browser.permanentKey;
   if (this._browserIds.has(permKey)) {
     return this._browserIds.get(permKey);
   }
 
   let winId = browser.outerWindowID;
   if (winId) {
-    winId = winId.toString();
     this._browserIds.set(permKey, winId);
     return winId;
   }
   return null;
 },
 
 /**
  * Get the current window's handle. On desktop this typically corresponds
@@ -1251,23 +1248,23 @@ GeckoDriver.prototype.getIdForBrowser = 
  * @return {string}
  *     Unique window handle.
  */
 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;
+    resp.body.value = this.curBrowser.curFrameId.toString();
     return;
   }
 
   for (let i in this.browsers) {
     if (this.curBrowser == this.browsers[i]) {
-      resp.body.value = i;
+      resp.body.value = i.toString();
       return;
     }
   }
 };
 
 /**
  * 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
@@ -1434,24 +1431,30 @@ GeckoDriver.prototype.setWindowRect = fu
  *
  * @param {string} name
  *     Target name or ID of the window to switch to.
  * @param {boolean=} focus
  *      A boolean value which determines whether to focus
  *      the window. Defaults to true.
  */
 GeckoDriver.prototype.switchToWindow = function* (cmd, resp) {
-  let switchTo = cmd.parameters.name;
   let focus = (cmd.parameters.focus !== undefined) ? cmd.parameters.focus : true;
-  let found;
+
+  // Window IDs are internally handled as numbers, but here it could also be the
+  // name of the window.
+  let switchTo = parseInt(cmd.parameters.name);
+  if (isNaN(switchTo)) {
+    switchTo = cmd.parameters.name;
+  }
 
   let byNameOrId = function (name, windowId) {
     return switchTo === name || switchTo === windowId;
   };
 
+  let found;
   let winEn = Services.wm.getEnumerator(null);
   while (winEn.hasMoreElements()) {
     let win = winEn.getNext();
     let outerId = getOuterWindowId(win);
     let tabBrowser = browser.getTabBrowser(win);
 
     if (byNameOrId(win.name, outerId)) {
       // In case the wanted window is a chrome window, we are done.
@@ -2603,17 +2606,17 @@ GeckoDriver.prototype.close = function (
   if (nwins == 1) {
     return [];
   }
 
   if (this.mm != globalMessageManager) {
     this.mm.removeDelayedFrameScript(FRAME_SCRIPT);
   }
 
-  return this.curBrowser.closeTab().then(() => this.windowHandles);
+  return this.curBrowser.closeTab().then(() => this.windowHandles.map(String));
 };
 
 /**
  * Close the currently selected chrome window.
  *
  * If it is the last window currently open, the chrome window will not be
  * closed to prevent a shutdown of the application. Instead the returned
  * list of chrome window handles is empty.
@@ -2642,17 +2645,17 @@ GeckoDriver.prototype.closeChromeWindow 
 
   // reset frame to the top-most frame
   this.curFrame = null;
 
   if (this.mm != globalMessageManager) {
     this.mm.removeDelayedFrameScript(FRAME_SCRIPT);
   }
 
-  return this.curBrowser.closeWindow().then(() => this.chromeWindowHandles);
+  return this.curBrowser.closeWindow().then(() => this.chromeWindowHandles.map(String));
 };
 
 /** Delete Marionette session. */
 GeckoDriver.prototype.deleteSession = function (cmd, resp) {
   if (this.curBrowser !== null) {
     // frame scripts can be safely reused
     Preferences.set(CONTENT_LISTENER_PREF, false);
 
@@ -3151,25 +3154,16 @@ GeckoDriver.prototype.uninstallAddon = f
   let id = cmd.parameters.id;
   if (typeof id == "undefined" || typeof id != "string") {
     throw new InvalidArgumentError();
   }
 
   return addon.uninstall(id);
 };
 
-/**
- * Helper function to convert an outerWindowID into a UID that Marionette
- * tracks.
- */
-GeckoDriver.prototype.generateFrameId = function (id) {
-  let uid = id + (this.appName == "B2G" ? "-b2g" : "");
-  return uid;
-};
-
 /** Receives all messages from content messageManager. */
 GeckoDriver.prototype.receiveMessage = function (message) {
   switch (message.name) {
     case "Marionette:ok":
     case "Marionette:done":
     case "Marionette:error":
       // check if we need to remove the mozbrowserclose listener
       if (this.mozBrowserClose !== null) {
@@ -3428,14 +3422,12 @@ function copy (obj) {
  *
  * @param {nsIDOMWindow} win
  *     Window whose browser we need to access.
  *
  * @return {string}
  *     Returns the unique window ID.
  */
 function getOuterWindowId(win) {
-  let id = win.QueryInterface(Ci.nsIInterfaceRequestor)
+  return win.QueryInterface(Ci.nsIInterfaceRequestor)
       .getInterface(Ci.nsIDOMWindowUtils)
       .outerWindowID;
-
-  return id.toString();
 }
--- 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
@@ -1,13 +1,15 @@
 # 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 import By, Wait
+import types
+
+from marionette_driver import By, errors
 
 from marionette_harness import MarionetteTestCase, WindowManagerMixin
 
 
 class TestWindowHandles(WindowManagerMixin, MarionetteTestCase):
 
     def setUp(self):
         super(TestWindowHandles, self).setUp()
@@ -19,25 +21,40 @@ class TestWindowHandles(WindowManagerMix
         self.marionette.set_context("chrome")
 
     def tearDown(self):
         self.close_all_windows()
         self.close_all_tabs()
 
         super(TestWindowHandles, self).tearDown()
 
+    def assert_window_handles(self):
+        try:
+            self.assertIsInstance(self.marionette.current_chrome_window_handle, types.StringTypes)
+            self.assertIsInstance(self.marionette.current_window_handle, types.StringTypes)
+        except errors.NoSuchWindowException:
+            pass
+
+        for handle in self.marionette.chrome_window_handles:
+            self.assertIsInstance(handle, types.StringTypes)
+
+        for handle in self.marionette.window_handles:
+            self.assertIsInstance(handle, types.StringTypes)
+
     def test_chrome_window_handles_with_scopes(self):
         # Open a browser and a non-browser (about window) chrome window
         self.open_window(
             trigger=lambda: self.marionette.execute_script("window.open();"))
+        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)
 
         self.open_window(
             trigger=lambda: self.marionette.find_element(By.ID, "aboutName").click())
+        self.assert_window_handles()
         self.assertEqual(len(self.marionette.chrome_window_handles), len(self.start_windows) + 2)
         self.assertEqual(self.marionette.current_chrome_window_handle, self.start_window)
 
         chrome_window_handles_in_chrome_scope = self.marionette.chrome_window_handles
         window_handles_in_chrome_scope = self.marionette.window_handles
 
         with self.marionette.using_context("content"):
             self.assertEqual(self.marionette.chrome_window_handles,
@@ -48,119 +65,134 @@ class TestWindowHandles(WindowManagerMix
     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)
+        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)
         with self.marionette.using_context("content"):
             self.assertEqual(self.marionette.get_url(), self.empty_page)
 
         # Ensure navigate works in our current window
         other_page = self.marionette.absolute_url("test.html")
         with self.marionette.using_context("content"):
             self.marionette.navigate(other_page)
             self.assertEqual(self.marionette.get_url(), other_page)
 
         # Close the opened window and carry on in our original tab.
         self.marionette.close()
+        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_window_handles_after_opening_new_tab(self):
         def open_with_link():
             with self.marionette.using_context("content"):
                 link = self.marionette.find_element(By.ID, "new-tab")
                 link.click()
 
         new_tab = self.open_tab(trigger=open_with_link)
+        self.assert_window_handles()
         self.assertEqual(len(self.marionette.window_handles), len(self.start_tabs) + 1)
         self.assertEqual(self.marionette.current_window_handle, self.start_tab)
 
         self.marionette.switch_to_window(new_tab)
+        self.assert_window_handles()
         self.assertEqual(self.marionette.current_window_handle, new_tab)
         with self.marionette.using_context("content"):
             self.assertEqual(self.marionette.get_url(), self.empty_page)
 
         # Ensure navigate works in our current tab
         other_page = self.marionette.absolute_url("test.html")
         with self.marionette.using_context("content"):
             self.marionette.navigate(other_page)
             self.assertEqual(self.marionette.get_url(), other_page)
 
         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)
 
         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_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)
+        self.assert_window_handles()
         self.assertEqual(len(self.marionette.window_handles), len(self.start_tabs) + 1)
         self.assertEqual(self.marionette.current_window_handle, self.start_tab)
 
         # Check that the new tab has the correct page loaded
         self.marionette.switch_to_window(new_tab)
+        self.assert_window_handles()
         self.assertEqual(self.marionette.current_window_handle, new_tab)
         with self.marionette.using_context("content"):
             self.assertEqual(self.marionette.get_url(), self.empty_page)
 
         # Ensure navigate works in our current window
         other_page = self.marionette.absolute_url("test.html")
         with self.marionette.using_context("content"):
             self.marionette.navigate(other_page)
             self.assertEqual(self.marionette.get_url(), other_page)
 
         # Close the opened window and carry on in our original 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.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_closing_original_tab(self):
         def open_with_link():
             with self.marionette.using_context("content"):
                 link = self.marionette.find_element(By.ID, "new-tab")
                 link.click()
 
         new_tab = self.open_tab(trigger=open_with_link)
+        self.assert_window_handles()
         self.assertEqual(len(self.marionette.window_handles), len(self.start_tabs) + 1)
         self.assertEqual(self.marionette.current_window_handle, self.start_tab)
 
         self.marionette.close()
+        self.assert_window_handles()
         self.assertEqual(len(self.marionette.window_handles), len(self.start_tabs))
 
         self.marionette.switch_to_window(new_tab)
+        self.assert_window_handles()
         self.assertEqual(self.marionette.current_window_handle, new_tab)
         with self.marionette.using_context("content"):
             self.assertEqual(self.marionette.get_url(), self.empty_page)
 
     def test_window_handles_no_switch(self):
         """Regression test for bug 1294456.
         This test is testing the case where Marionette attempts to send a
         command to a window handle when the browser has opened and selected
@@ -175,25 +207,33 @@ class TestWindowHandles(WindowManagerMix
         queued and never sent, since the remoteness flip in the new tab was
         never going to happen.
         """
         def open_with_menu():
             menu_new_tab = self.marionette.find_element(By.ID, 'menu_newNavigatorTab')
             menu_new_tab.click()
 
         new_tab = self.open_tab(trigger=open_with_menu)
+        self.assert_window_handles()
 
         # We still have the default tab set as our window handle. This
         # get_url command should be sent immediately, and not be forever-queued.
         with self.marionette.using_context("content"):
             self.assertEqual(self.marionette.get_url(), self.test_page)
 
         self.assertEqual(len(self.marionette.window_handles), len(self.start_tabs) + 1)
         self.assertEqual(self.marionette.current_window_handle, self.start_tab)
 
         self.marionette.switch_to_window(new_tab)
+        self.assert_window_handles()
         self.assertEqual(self.marionette.current_window_handle, 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.assert_window_handles()
         self.assertEqual(self.marionette.current_window_handle, self.start_tab)
+
+    def test_window_handles_after_closing_last_window(self):
+        self.close_all_windows()
+        self.assertEqual(self.marionette.close_chrome_window(), [])
--- 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
@@ -1,13 +1,15 @@
 # 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 import By, Wait
+import types
+
+from marionette_driver import By, errors, Wait
 
 from marionette_harness import MarionetteTestCase, skip_if_mobile, WindowManagerMixin
 
 
 class TestWindowHandles(WindowManagerMixin, MarionetteTestCase):
 
     def setUp(self):
         super(TestWindowHandles, self).setUp()
@@ -16,103 +18,131 @@ class TestWindowHandles(WindowManagerMix
         self.test_page = self.marionette.absolute_url("windowHandles.html")
         self.marionette.navigate(self.test_page)
 
     def tearDown(self):
         self.close_all_tabs()
 
         super(TestWindowHandles, self).tearDown()
 
+    def assert_window_handles(self):
+        try:
+            self.assertIsInstance(self.marionette.current_window_handle, types.StringTypes)
+        except errors.NoSuchWindowException:
+            pass
+
+        for handle in self.marionette.window_handles:
+            self.assertIsInstance(handle, types.StringTypes)
+
     def test_window_handles_after_opening_new_tab(self):
         def open_with_link():
             link = self.marionette.find_element(By.ID, "new-tab")
             link.click()
 
         new_tab = self.open_tab(trigger=open_with_link)
+        self.assert_window_handles()
         self.assertEqual(len(self.marionette.window_handles), len(self.start_tabs) + 1)
         self.assertEqual(self.marionette.current_window_handle, self.start_tab)
 
         self.marionette.switch_to_window(new_tab)
+        self.assert_window_handles()
         self.assertEqual(self.marionette.current_window_handle, new_tab)
         Wait(self.marionette, timeout=self.marionette.timeout.page_load).until(
             lambda mn: mn.get_url() == self.empty_page,
             message="{} did not load after opening a new tab".format(self.empty_page))
 
         self.marionette.switch_to_window(self.start_tab)
         self.assertEqual(self.marionette.current_window_handle, self.start_tab)
         self.assertEqual(self.marionette.get_url(), self.test_page)
 
         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.assert_window_handles()
         self.assertEqual(self.marionette.current_window_handle, self.start_tab)
 
     def test_window_handles_after_opening_new_browser_window(self):
         def open_with_link():
             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)
+        self.assert_window_handles()
         self.assertEqual(len(self.marionette.window_handles), len(self.start_tabs) + 1)
         self.assertEqual(self.marionette.current_window_handle, self.start_tab)
 
         # Check that the new tab has the correct page loaded
         self.marionette.switch_to_window(new_tab)
+        self.assert_window_handles()
         self.assertEqual(self.marionette.current_window_handle, new_tab)
         Wait(self.marionette, self.marionette.timeout.page_load).until(
             lambda _: self.marionette.get_url() == self.empty_page,
             message="The expected page '{}' has not been loaded".format(self.empty_page))
 
         # Ensure navigate works in our current window
         other_page = self.marionette.absolute_url("test.html")
         self.marionette.navigate(other_page)
         self.assertEqual(self.marionette.get_url(), other_page)
 
         # Close the opened window and carry on in our original 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.assert_window_handles()
         self.assertEqual(self.marionette.current_window_handle, self.start_tab)
         self.assertEqual(self.marionette.get_url(), self.test_page)
 
     @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)
+        self.assert_window_handles()
         self.assertEqual(len(self.marionette.window_handles), len(self.start_tabs) + 1)
         self.assertEqual(self.marionette.current_window_handle, self.start_tab)
 
         self.marionette.switch_to_window(new_tab)
+        self.assert_window_handles()
         self.assertEqual(self.marionette.current_window_handle, new_tab)
 
         # Close the opened window and carry on in our original 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.assert_window_handles()
         self.assertEqual(self.marionette.current_window_handle, self.start_tab)
 
     def test_window_handles_after_closing_original_tab(self):
         def open_with_link():
             link = self.marionette.find_element(By.ID, "new-tab")
             link.click()
 
         new_tab = self.open_tab(trigger=open_with_link)
+        self.assert_window_handles()
         self.assertEqual(len(self.marionette.window_handles), len(self.start_tabs) + 1)
         self.assertEqual(self.marionette.current_window_handle, self.start_tab)
 
         self.marionette.close()
+        self.assert_window_handles()
         self.assertEqual(len(self.marionette.window_handles), len(self.start_tabs))
 
         self.marionette.switch_to_window(new_tab)
+        self.assert_window_handles()
         self.assertEqual(self.marionette.current_window_handle, new_tab)
         Wait(self.marionette, self.marionette.timeout.page_load).until(
             lambda _: self.marionette.get_url() == self.empty_page,
             message="The expected page '{}' has not been loaded".format(self.empty_page))
+
+    def test_window_handles_after_closing_last_tab(self):
+        self.close_all_tabs()
+        self.assertEqual(self.marionette.close(), [])