Bug 1387092 - Read capabilities off top-level object. r=whimboo draft
authorHenrik Skupin <mail@hskupin.info>
Fri, 11 Aug 2017 11:06:15 +0200
changeset 644779 8dba4ddd4657d3ce2303968db73ca9d4ffc2d6d0
parent 644169 5322c03f4c8587fe526172d3f87160031faa6d75
child 644780 168d7a0f5bd76db51ed6dca2b14e42c3688d4b07
push id73551
push userbmo:hskupin@gmail.com
push dateFri, 11 Aug 2017 10:35:47 +0000
reviewerswhimboo
bugs1387092, 1387380
milestone57.0a1
Bug 1387092 - Read capabilities off top-level object. r=whimboo geckodriver sends capabilities as a JSON Object in the body of the command, like this: [0,1,"newSession",{"acceptInsecureCerts":true}] With https://bugzil.la/1387380 we wanted the Marionette Python client to match this behaviour, however the patch overlooked the fact that the server reads cmd.parameters.capabilities, meaning it looks for a "capabilities" field on this object instead of treating the object as the dictionary of capabilities. As a follow-up to that bug, this patch removes the ability to override the session ID by specifying a "sessionId" field. This functionality was only used for in-app restart tests. When Firefox restarts, the Marionette session is arguably not the same, and sessions should not live on between restarts. This patch will fix capabilities passed from geckodriver and align the Marionette Python client. For backwards compatibility reasons, it needs to be possible to use the Python client with older Firefoxen that reads cmd.parameters.capabilities instead of cmd.parameters. This is why we duplicate the capabilities object, like geckodriver does. MozReview-Commit-ID: DCpaxl9hOLe
testing/marionette/client/marionette_driver/marionette.py
testing/marionette/driver.js
testing/marionette/harness/marionette_harness/tests/unit/test_quit_restart.py
testing/marionette/harness/marionette_harness/tests/unit/test_session.py
--- a/testing/marionette/client/marionette_driver/marionette.py
+++ b/testing/marionette/client/marionette_driver/marionette.py
@@ -1121,17 +1121,16 @@ class Marionette(object):
         :param callback: If provided and `in_app` is True, the callback will be
                          used to trigger the restart.
         """
         if not self.instance:
             raise errors.MarionetteException("restart() can only be called "
                                              "on Gecko instances launched by Marionette")
 
         context = self._send_message("getContext", key="value")
-        session_id = self.session_id
 
         if in_app:
             if clean:
                 raise ValueError("An in_app restart cannot be triggered with the clean flag set")
 
             if callable(callback):
                 self._send_message("acceptConnections", {"value": False})
                 callback()
@@ -1150,17 +1149,17 @@ class Marionette(object):
                     self.cleanup()
                     raise exc, "Requested restart of the application was aborted", tb
 
         else:
             self.delete_session()
             self.instance.restart(clean=clean)
             self.raise_for_port(timeout=self.DEFAULT_STARTUP_TIMEOUT)
 
-        self.start_session(session_id=session_id)
+        self.start_session()
 
         # Restore the context as used before the restart
         self.set_context(context)
 
         if in_app and self.process_id:
             # In some cases Firefox restarts itself by spawning into a new process group.
             # As long as mozprocess cannot track that behavior (bug 1284864) we assist by
             # informing about the new process id.
@@ -1170,30 +1169,28 @@ class Marionette(object):
         '''
         Returns an absolute url for files served from Marionette's www directory.
 
         :param relative_url: The url of a static file, relative to Marionette's www directory.
         '''
         return "{0}{1}".format(self.baseurl, relative_url)
 
     @do_process_check
-    def start_session(self, capabilities=None, session_id=None, timeout=60):
+    def start_session(self, capabilities=None, timeout=60):
         """Create a new WebDriver session.
 
         This method must be called before performing any other action.
 
         :param capabilities: An optional dictionary of
             Marionette-recognised capabilities.  It does not
             accept a WebDriver conforming capabilities dictionary
             (including alwaysMatch, firstMatch, desiredCapabilities,
             or requriedCapabilities), and only recognises extension
             capabilities that are specific to Marionette.
         :param timeout: Timeout in seconds for the server to be ready.
-        :param session_id: Unique identifier for the session. If no
-            session ID is passed in then one will be generated.
 
         :returns: A dictionary of the capabilities offered.
 
         """
         self.crashed = 0
 
         if self.instance:
             returncode = self.instance.runner.returncode
@@ -1207,17 +1204,34 @@ class Marionette(object):
             self.socket_timeout)
 
         # Call wait_for_port() before attempting to connect in
         # the event gecko hasn't started yet.
         timeout = timeout or self.startup_timeout
         self.wait_for_port(timeout=timeout)
         self.protocol, _ = self.client.connect()
 
-        body = {"capabilities": capabilities, "sessionId": session_id}
+        body = capabilities
+        if body is None:
+            body = {}
+
+        # Duplicate capabilities object so the body we end up
+        # sending looks like this:
+        #
+        #     {acceptInsecureCerts: true, {capabilities: {acceptInsecureCerts: true}}}
+        #
+        # We do this because geckodriver sends the capabilities at the
+        # top-level, and after bug 1388424 removed support for overriding
+        # the session ID, we also do this with this client.  However,
+        # because this client is used with older Firefoxen (through upgrade
+        # tests et al.) we need to preserve backwards compatibility until
+        # Firefox 60.
+        if "capabilities" not in body and capabilities is not None:
+            body["capabilities"] = dict(capabilities)
+
         resp = self._send_message("newSession", body)
 
         self.session_id = resp["sessionId"]
         self.session = resp["value"] if self.protocol == 1 else resp["capabilities"]
         # fallback to processId can be removed in Firefox 55
         self.process_id = self.session.get("moz:processID", self.session.get("processId"))
         self.profile = self.session.get("moz:profile")
 
--- a/testing/marionette/driver.js
+++ b/testing/marionette/driver.js
@@ -767,22 +767,21 @@ GeckoDriver.prototype.listeningPromise =
  * @throws {SessionNotCreatedError}
  *     If, for whatever reason, a session could not be created.
  */
 GeckoDriver.prototype.newSession = function* (cmd, resp) {
   if (this.sessionID) {
     throw new SessionNotCreatedError("Maximum number of active sessions");
   }
 
-  this.sessionID = cmd.parameters.sessionId || element.generateUUID();
+  this.sessionID = element.generateUUID();
   this.newSessionCommandId = cmd.id;
 
   try {
-    this.capabilities = session.Capabilities.fromJSON(
-        cmd.parameters.capabilities);
+    this.capabilities = session.Capabilities.fromJSON(cmd.parameters);
   } catch (e) {
     throw new SessionNotCreatedError(e);
   }
 
   if (!this.secureTLS) {
     logger.warn("TLS certificate errors will be ignored for this session");
     let acceptAllCerts = new cert.InsecureSweepingOverride();
     cert.installOverride(acceptAllCerts);
--- a/testing/marionette/harness/marionette_harness/tests/unit/test_quit_restart.py
+++ b/testing/marionette/harness/marionette_harness/tests/unit/test_quit_restart.py
@@ -97,27 +97,27 @@ class TestQuitRestart(MarionetteTestCase
               flags |= Ci.nsIAppStartup.eRestart;
             }
             Services.startup.quit(flags);
         """, script_args=(restart,))
 
     def test_force_clean_restart(self):
         self.marionette.restart(clean=True)
         self.assertNotEqual(self.marionette.profile, self.profile)
-        self.assertEqual(self.marionette.session_id, self.session_id)
+        self.assertNotEqual(self.marionette.session_id, self.session_id)
 
         # A forced restart will cause a new process id
         self.assertNotEqual(self.marionette.process_id, self.pid)
         self.assertNotEqual(self.marionette.get_pref("startup.homepage_welcome_url"),
                             "about:")
 
     def test_force_restart(self):
         self.marionette.restart()
         self.assertEqual(self.marionette.profile, self.profile)
-        self.assertEqual(self.marionette.session_id, self.session_id)
+        self.assertNotEqual(self.marionette.session_id, self.session_id)
 
         # A forced restart will cause a new process id
         self.assertNotEqual(self.marionette.process_id, self.pid)
         self.assertNotEqual(self.marionette.get_pref("startup.homepage_welcome_url"),
                             "about:")
 
     def test_force_clean_quit(self):
         self.marionette.quit(clean=True)
--- a/testing/marionette/harness/marionette_harness/tests/unit/test_session.py
+++ b/testing/marionette/harness/marionette_harness/tests/unit/test_session.py
@@ -31,23 +31,16 @@ class TestSession(MarionetteTestCase):
 
     def test_get_session_id(self):
         # Sends newSession
         self.marionette.start_session()
 
         self.assertTrue(self.marionette.session_id is not None)
         self.assertTrue(isinstance(self.marionette.session_id, unicode))
 
-    def test_set_the_session_id(self):
-        # Sends newSession
-        self.marionette.start_session(session_id="ILoveCheese")
-
-        self.assertEqual(self.marionette.session_id, "ILoveCheese")
-        self.assertTrue(isinstance(self.marionette.session_id, unicode))
-
     def test_session_already_started(self):
         self.marionette.start_session()
         self.assertTrue(isinstance(self.marionette.session_id, unicode))
         with self.assertRaises(errors.SessionNotCreatedException):
             self.marionette._send_message("newSession", {})
 
     def test_no_session(self):
         with self.assertRaises(errors.InvalidSessionIdException):