Bug 1409030 - Remove Marionette socket protocol levels below 3. draft
authorHenrik Skupin <mail@hskupin.info>
Mon, 16 Oct 2017 16:42:06 +0200
changeset 680904 8bac83e3863d11cf3e6fb6c99c37e562dee90f6c
parent 680782 c6a2643362a67cdf7a87ac165454fce4b383debb
child 736009 5981fbb644c08e2fab571f0766d1534f657d50ba
push id84672
push userbmo:hskupin@gmail.com
push dateMon, 16 Oct 2017 14:59:22 +0000
bugs1409030
milestone58.0a1
Bug 1409030 - Remove Marionette socket protocol levels below 3. MozReview-Commit-ID: LmjyuBuRvhk
testing/marionette/client/marionette_driver/marionette.py
testing/marionette/client/marionette_driver/transport.py
testing/marionette/harness/marionette_harness/tests/unit/test_marionette.py
testing/marionette/harness/marionette_harness/tests/unit/test_transport.py
--- a/testing/marionette/client/marionette_driver/marionette.py
+++ b/testing/marionette/client/marionette_driver/marionette.py
@@ -172,18 +172,17 @@ class HTMLElement(object):
         This will return a dictionary with the following:
 
           * x and y represent the top left coordinates of the ``HTMLElement``
             relative to top left corner of the document.
           * height and the width will contain the height and the width
             of the DOMRect of the ``HTMLElement``.
         """
         body = {"id": self.id}
-        return self.marionette._send_message(
-            "getElementRect", body, key="value" if self.marionette.protocol == 1 else None)
+        return self.marionette._send_message("getElementRect", body)
 
     def value_of_css_property(self, property_name):
         """Gets the value of the specified CSS property name.
 
         :param property_name: Property name to get the value of.
         """
         body = {"id": self.id, "propertyName": property_name}
         return self.marionette._send_message(
@@ -736,25 +735,17 @@ class Marionette(object):
         :returns: Full response from the server, or if `key` is given,
             the value of said key in the response.
         """
 
         if not self.session_id and name != "newSession":
             raise errors.MarionetteException("Please start a session")
 
         try:
-            if self.protocol < 3:
-                data = {"name": name}
-                if params is not None:
-                    data["parameters"] = params
-                self.client.send(data)
-                msg = self.client.receive()
-
-            else:
-                msg = self.client.request(name, params)
+            msg = self.client.request(name, params)
 
         except IOError:
             self.delete_session(send_request=False)
             raise
 
         res, err = msg.result, msg.error
         if err:
             self._handle_error(err)
@@ -772,28 +763,19 @@ class Marionette(object):
             else:
                 return HTMLElement(self, value.get(W3C_WEBELEMENT_KEY))
         elif isinstance(value, list):
             return list(self._unwrap_response(item) for item in value)
         else:
             return value
 
     def _handle_error(self, obj):
-        if self.protocol == 1:
-            if "error" not in obj or not isinstance(obj["error"], dict):
-                raise errors.MarionetteException(
-                    "Malformed packet, expected key 'error' to be a dict: {}".format(obj))
-            error = obj["error"].get("status")
-            message = obj["error"].get("message")
-            stacktrace = obj["error"].get("stacktrace")
-
-        else:
-            error = obj["error"]
-            message = obj["message"]
-            stacktrace = obj["stacktrace"]
+        error = obj["error"]
+        message = obj["message"]
+        stacktrace = obj["stacktrace"]
 
         raise errors.lookup(error)(message, stacktrace=stacktrace)
 
     def check_for_crash(self):
         """Check if the process crashed.
 
         :returns: True, if a crash happened since the method has been called the last time.
         """
@@ -1255,17 +1237,17 @@ class Marionette(object):
         # 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"]
+        self.session = 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")
 
         return self.session
 
     @property
     def test_name(self):
@@ -1399,18 +1381,17 @@ class Marionette(object):
 
     def get_window_position(self):
         """Get the current window's position.
 
         :returns: a dictionary with x and y
         """
         warnings.warn("get_window_position() has been deprecated, please use get_window_rect()",
                       DeprecationWarning)
-        return self._send_message(
-            "getWindowPosition", key="value" if self.protocol == 1 else None)
+        return self._send_message("getWindowPosition")
 
     def set_window_position(self, x, y):
         """Set the position of the current window
 
         :param x: x coordinate for the top left of the window
         :param y: y coordinate for the top left of the window
         """
         warnings.warn("set_window_position() has been deprecated, please use set_window_rect()",
@@ -1456,30 +1437,28 @@ class Marionette(object):
         chrome context, it will list all available windows, not just
         browser windows (e.g. not just navigator.browser).
 
         Each window handle is assigned by the server, and the list of
         strings returned does not have a guaranteed ordering.
 
         :returns: Unordered list of unique window handles as strings
         """
-        return self._send_message(
-            "getWindowHandles", key="value" if self.protocol == 1 else None)
+        return self._send_message("getWindowHandles")
 
     @property
     def chrome_window_handles(self):
         """Get a list of currently open chrome windows.
 
         Each window handle is assigned by the server, and the list of
         strings returned does not have a guaranteed ordering.
 
         :returns: Unordered list of unique chrome window handles as strings
         """
-        return self._send_message(
-            "getChromeWindowHandles", key="value" if self.protocol == 1 else None)
+        return self._send_message("getChromeWindowHandles")
 
     @property
     def page_source(self):
         """A string representation of the DOM."""
         return self._send_message("getPageSource", key="value")
 
     def close(self):
         """Close the current window, ending the session if it's the last
@@ -1898,23 +1877,20 @@ class Marionette(object):
             "tag", target might equal "div".  If method = "id", target would be
             an element id.
         :param id: If specified, search for elements only inside the element
             with the specified id.
         """
         body = {"value": target, "using": method}
         if id:
             body["element"] = id
-        return self._send_message(
-            "findElements", body, key="value" if self.protocol == 1 else None)
+        return self._send_message("findElements", body)
 
     def get_active_element(self):
         el_or_ref = self._send_message("getActiveElement", key="value")
-        if self.protocol < 3:
-            return HTMLElement(self, el_or_ref)
         return el_or_ref
 
     def add_cookie(self, cookie):
         """Adds a cookie to your current session.
 
         :param cookie: A dictionary object, with required keys - "name"
             and "value"; optional keys - "path", "domain", "secure",
             "expiry".
@@ -1970,17 +1946,17 @@ class Marionette(object):
     def get_cookies(self):
         """Get all the cookies for the current domain.
 
         This is the equivalent of calling `document.cookie` and
         parsing the result.
 
         :returns: A list of cookies for the current domain.
         """
-        return self._send_message("getCookies", key="value" if self.protocol == 1 else None)
+        return self._send_message("getCookies")
 
     def screenshot(self, element=None, highlights=None, format="base64",
                    full=True, scroll=True):
         """Takes a screenshot of a web element or the current frame.
 
         The screen capture is returned as a lossless PNG image encoded
         as a base 64 string by default. If the `element` argument is defined the
         capture area will be limited to the bounding box of that
@@ -2065,18 +2041,17 @@ class Marionette(object):
         Will return the current browser window size in pixels. Refers to
         window outerWidth and outerHeight values, which include scroll bars,
         title bars, etc.
 
         :returns: Window rect.
         """
         warnings.warn("window_size property has been deprecated, please use get_window_rect()",
                       DeprecationWarning)
-        return self._send_message("getWindowSize",
-                                  key="value" if self.protocol == 1 else None)
+        return self._send_message("getWindowSize")
 
     def set_window_size(self, width, height):
         """Resize the browser window currently in focus.
 
         The supplied ``width`` and ``height`` values refer to the window `outerWidth`
         and `outerHeight` values, which include scroll bars, title bars, etc.
 
         An error will be returned if the requested window size would result
--- a/testing/marionette/client/marionette_driver/transport.py
+++ b/testing/marionette/client/marionette_driver/transport.py
@@ -72,67 +72,41 @@ class Response(Message):
 
     @staticmethod
     def from_msg(payload):
         data = json.loads(payload)
         assert data[0] == Response.TYPE
         return Response(data[1], data[2], data[3])
 
 
-class Proto2Command(Command):
-    """Compatibility shim that marshals messages from a protocol level
-    2 and below remote into ``Command`` objects.
-    """
-
-    def __init__(self, name, params):
-        Command.__init__(self, None, name, params)
-
-
-class Proto2Response(Response):
-    """Compatibility shim that marshals messages from a protocol level
-    2 and below remote into ``Response`` objects.
-    """
-
-    def __init__(self, error, result):
-        Response.__init__(self, None, error, result)
-
-    @staticmethod
-    def from_data(data):
-        err, res = None, None
-        if "error" in data:
-            err = data
-        else:
-            res = data
-        return Proto2Response(err, res)
-
-
 class TcpTransport(object):
     """Socket client that communciates with Marionette via TCP.
 
     It speaks the protocol of the remote debugger in Gecko, in which
     messages are always preceded by the message length and a colon, e.g.:
 
         7:MESSAGE
 
     On top of this protocol it uses a Marionette message format, that
     depending on the protocol level offered by the remote server, varies.
-    Supported protocol levels are 1 and above.
+    Supported protocol levels are `min_protocol_level` and above.
     """
     max_packet_length = 4096
+    min_protocol_level = 3
 
     def __init__(self, addr, port, socket_timeout=60.0):
         """If `socket_timeout` is `0` or `0.0`, non-blocking socket mode
         will be used.  Setting it to `1` or `None` disables timeouts on
         socket operations altogether.
         """
         self.addr = addr
         self.port = port
         self._socket_timeout = socket_timeout
 
-        self.protocol = 1
+        self.protocol = self.min_protocol_level
         self.application_type = None
         self.last_id = 0
         self.expected_response = None
         self.sock = None
 
     @property
     def socket_timeout(self):
         return self._socket_timeout
@@ -149,22 +123,16 @@ class TcpTransport(object):
         # protocol 3 and above
         if self.protocol >= 3:
             typ = int(packet[1])
             if typ == Command.TYPE:
                 msg = Command.from_msg(packet)
             elif typ == Response.TYPE:
                 msg = Response.from_msg(packet)
 
-        # protocol 2 and below
-        else:
-            data = json.loads(packet)
-
-            msg = Proto2Response.from_data(data)
-
         return msg
 
     def receive(self, unmarshal=True):
         """Wait for the next complete response from the remote.
 
         :param unmarshal: Default is to deserialise the packet and
             return a ``Message`` type.  Setting this to false will return
             the raw packet.
@@ -188,23 +156,20 @@ class TcpTransport(object):
                 length = data[0:sep]
                 remaining = data[sep + 1:]
 
                 if len(remaining) == int(length):
                     if unmarshal:
                         msg = self._unmarshal(remaining)
                         self.last_id = msg.id
 
-                        if self.protocol >= 3:
-                            self.last_id = msg.id
-
-                            # keep reading incoming responses until
-                            # we receive the user's expected response
-                            if isinstance(msg, Response) and msg != self.expected_response:
-                                return self.receive(unmarshal)
+                        # keep reading incoming responses until
+                        # we receive the user's expected response
+                        if isinstance(msg, Response) and msg != self.expected_response:
+                            return self.receive(unmarshal)
 
                         return msg
 
                     else:
                         return remaining
 
                 bytes_to_recv = int(length) - len(remaining)
 
@@ -227,17 +192,17 @@ class TcpTransport(object):
             self.sock = None
             raise
 
         with SocketTimeout(self.sock, 2.0):
             # first packet is always a JSON Object
             # which we can use to tell which protocol level we are at
             raw = self.receive(unmarshal=False)
         hello = json.loads(raw)
-        self.protocol = hello.get("marionetteProtocol", 1)
+        self.protocol = hello.get("marionetteProtocol", self.min_protocol_level)
         self.application_type = hello.get("applicationType")
 
         return (self.protocol, self.application_type)
 
     def send(self, obj):
         """Send message to the remote server.  Allowed input is a
         ``Message`` instance or a JSON serialisable object.
         """
--- a/testing/marionette/harness/marionette_harness/tests/unit/test_marionette.py
+++ b/testing/marionette/harness/marionette_harness/tests/unit/test_marionette.py
@@ -26,42 +26,8 @@ class TestMarionette(MarionetteTestCase)
     @skip_if_mobile("Bug 1322993 - Missing temporary folder")
     def test_wait_for_port_non_existing_process(self):
         """Test that wait_for_port doesn't run into a timeout if instance is not running."""
         self.marionette.quit()
         self.assertIsNotNone(self.marionette.instance.runner.returncode)
         start_time = time.time()
         self.assertFalse(self.marionette.wait_for_port(timeout=5))
         self.assertLess(time.time() - start_time, 5)
-
-
-class TestProtocol2Errors(MarionetteTestCase):
-    def setUp(self):
-        MarionetteTestCase.setUp(self)
-        self.op = self.marionette.protocol
-        self.marionette.protocol = 2
-
-    def tearDown(self):
-        self.marionette.protocol = self.op
-        MarionetteTestCase.tearDown(self)
-
-    def test_malformed_packet(self):
-        req = ["error", "message", "stacktrace"]
-        ps = []
-        for p in [p for i in range(0, len(req) + 1) for p in itertools.permutations(req, i)]:
-            ps.append(dict((x, None) for x in p))
-
-        for p in filter(lambda p: len(p) < 3, ps):
-            self.assertRaises(KeyError, self.marionette._handle_error, p)
-
-    def test_known_error_status(self):
-        with self.assertRaises(errors.NoSuchElementException):
-            self.marionette._handle_error(
-                {"error": errors.NoSuchElementException.status,
-                 "message": None,
-                 "stacktrace": None})
-
-    def test_unknown_error_status(self):
-        with self.assertRaises(errors.MarionetteException):
-            self.marionette._handle_error(
-                {"error": "barbera",
-                 "message": None,
-                 "stacktrace": None})
--- a/testing/marionette/harness/marionette_harness/tests/unit/test_transport.py
+++ b/testing/marionette/harness/marionette_harness/tests/unit/test_transport.py
@@ -6,17 +6,17 @@ import json
 
 from marionette_driver.transport import (
     Command,
     Proto2Command,
     Proto2Response,
     Response,
 )
 
-from marionette_harness import MarionetteTestCase, skip_unless_protocol
+from marionette_harness import MarionetteTestCase
 
 
 get_current_url = ("getCurrentUrl", None)
 execute_script = ("executeScript", {"script": "return 42"})
 
 
 class TestMessageSequencing(MarionetteTestCase):
     @property
@@ -28,29 +28,16 @@ class TestMessageSequencing(MarionetteTe
         self.marionette.client.last_id = new_id
 
     def send(self, name, params):
         self.last_id = self.last_id + 1
         cmd = Command(self.last_id, name, params)
         self.marionette.client.send(cmd)
         return self.last_id
 
-    @skip_unless_protocol("Skip for level < 3", lambda level: level >= 3)
-    def test_discard_older_messages(self):
-        first = self.send(*get_current_url)
-        second = self.send(*execute_script)
-        resp = self.marionette.client.receive()
-        self.assertEqual(second, resp.id)
-
-    @skip_unless_protocol("Skip for level < 3", lambda level: level >= 3)
-    def test_last_id_incremented(self):
-        before = self.last_id
-        self.send(*get_current_url)
-        self.assertGreater(self.last_id, before)
-
 
 class MessageTestCase(MarionetteTestCase):
     def assert_attr(self, obj, attr):
         self.assertTrue(hasattr(obj, attr),
                         "object does not have attribute {}".format(attr))
 
 
 class TestCommand(MessageTestCase):