Bug 1388424 - 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

@@ -1124,10 +1124,7 @@ class Marionette(object):
         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")
@@ -1154,9 +1151,7 @@ class Marionette(object):
             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)
 
@@ -1173,11 +1168,9 @@ class Marionette(object):
         :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
@@ -1187,11 +1180,7 @@ class Marionette(object):
             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
 
@@ -1211,10 +1200,25 @@ class Marionette(object):
         timeout = timeout or self.startup_timeout
         self.wait_for_port(timeout=timeout)
         self.protocol, _ = self.client.connect()
+        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)
 
-        body = {"capabilities": capabilities, "sessionId": session_id}
         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

testing/marionette/driver.js

@@ -771,13 +771,10 @@ 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);
   }

testing/marionette/harness/marionette_harness/tests/unit/test_quit_restart.py

@@ -102,8 +102,7 @@ class TestQuitRestart(MarionetteTestCase):
     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"),
@@ -112,8 +111,7 @@ class TestQuitRestart(MarionetteTestCase):
     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"),

testing/marionette/harness/marionette_harness/tests/unit/test_session.py

@@ -35,14 +35,6 @@ class TestSession(MarionetteTestCase):
 
         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))