Bug 1202392 - Improve exception handling and message details in Marionette. r?automatedtester draft
authorHenrik Skupin <mail@hskupin.info>
Thu, 28 Jul 2016 15:00:25 +0200
changeset 394300 3ba1ab362f374159fcda741d4d2c597b36b3e264
parent 393304 fef429fba4c64c5b9c0c823a6ab713edbbcd4220
child 526786 e191a70e9a625337e4320f66122db7ac6ef9dc14
push id24546
push userbmo:hskupin@gmail.com
push dateFri, 29 Jul 2016 14:07:09 +0000
reviewersautomatedtester
bugs1202392
milestone50.0a1
Bug 1202392 - Improve exception handling and message details in Marionette. r?automatedtester MozReview-Commit-ID: 5cvQDMlkMGn
testing/marionette/client/marionette_driver/decorators.py
testing/marionette/client/marionette_driver/geckoinstance.py
testing/marionette/client/marionette_driver/marionette.py
testing/marionette/client/marionette_driver/transport.py
testing/marionette/client/marionette_driver/wait.py
testing/marionette/harness/marionette/runner/base.py
testing/marionette/harness/marionette/runner/httpd.py
testing/marionette/harness/marionette/runner/mixins/browsermob-proxy-py/browsermobproxy/server.py
testing/marionette/harness/marionette/tests/unit/test_timeouts.py
--- a/testing/marionette/client/marionette_driver/decorators.py
+++ b/testing/marionette/client/marionette_driver/decorators.py
@@ -1,13 +1,13 @@
 # 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 errors import MarionetteException, TimeoutException
+from errors import MarionetteException
 from functools import wraps
 import socket
 import sys
 import traceback
 
 
 def _find_marionette_in_args(*args, **kwargs):
     try:
@@ -35,23 +35,26 @@ def do_process_check(func, always=False)
             try:
                 m.check_for_crash()
             except:
                 # don't want to lose the original exception
                 traceback.print_exc()
 
         try:
             return func(*args, **kwargs)
-        except (MarionetteException, socket.error, IOError) as e:
+        except (MarionetteException, IOError) as e:
             exc, val, tb = sys.exc_info()
+
+            # In case of socket failures force a shutdown of the application
+            if type(e) in (socket.error, socket.timeout):
+                m.force_shutdown()
+
             if not isinstance(e, MarionetteException) or type(e) is MarionetteException:
                 if not always:
                     check_for_crash()
-            if not isinstance(e, MarionetteException) or type(e) is TimeoutException:
-                m.force_shutdown()
             raise exc, val, tb
         finally:
             if always:
                 check_for_crash(m)
     return _
 
 
 def uses_marionette(func):
--- a/testing/marionette/client/marionette_driver/geckoinstance.py
+++ b/testing/marionette/client/marionette_driver/geckoinstance.py
@@ -167,17 +167,17 @@ class GeckoInstance(object):
 
         if self.runner:
             self.runner.stop()
             self.runner.cleanup()
 
     def restart(self, prefs=None, clean=True):
         self.close(restart=True)
 
-        if clean:
+        if clean and self.profile:
             self.profile.cleanup()
             self.profile = None
 
         if prefs:
             self.prefs = prefs
         else:
             self.prefs = None
         self.start()
@@ -215,21 +215,19 @@ class FennecInstance(GeckoInstance):
     def start(self):
         self._update_profile()
         self.runner = self.runner_class(**self._get_runner_args())
         try:
             if self.connect_to_running_emulator:
                 self.runner.device.connect()
             self.runner.start()
         except Exception as e:
-            message = 'Error possibly due to runner or device args.'
-            e.args += (message,)
-            if hasattr(e, 'strerror') and e.strerror:
-                e.strerror = ', '.join([e.strerror, message])
-            raise e
+            exc, val, tb = sys.exc_info()
+            message = 'Error possibly due to runner or device args: {}'
+            raise exc, message.format(e.message), tb
         # gecko_log comes from logcat when running with device/emulator
         logcat_args = {
             'filterspec': 'Gecko',
             'serial': self.runner.device.dm._deviceSerial
         }
         if self.gecko_log == '-':
             logcat_args['stream'] = sys.stdout
         else:
--- a/testing/marionette/client/marionette_driver/marionette.py
+++ b/testing/marionette/client/marionette_driver/marionette.py
@@ -538,16 +538,17 @@ class Marionette(object):
 
     CONTEXT_CHROME = 'chrome'  # non-browser content: windows, dialogs, etc.
     CONTEXT_CONTENT = 'content'  # browser content: iframes, divs, etc.
     TIMEOUT_SEARCH = 'implicit'
     TIMEOUT_SCRIPT = 'script'
     TIMEOUT_PAGE = 'page load'
     DEFAULT_SOCKET_TIMEOUT = 360
     DEFAULT_STARTUP_TIMEOUT = 120
+    DEFAULT_SHUTDOWN_TIMEOUT = 65  # Firefox will kill hanging threads after 60s
 
     def __init__(self, host='localhost', port=2828, app=None, bin=None,
                  baseurl=None, timeout=None, socket_timeout=DEFAULT_SOCKET_TIMEOUT,
                  startup_timeout=None, **instance_args):
         """
         :param host: address for Marionette connection
         :param port: integer port for Marionette connection
         :param baseurl: where to look for files served from Marionette's www directory
@@ -611,17 +612,17 @@ class Marionette(object):
     def profile_path(self):
         if self.instance and self.instance.profile:
             return self.instance.profile.profile
 
     def cleanup(self):
         if self.session:
             try:
                 self.delete_session()
-            except (errors.MarionetteException, socket.error, IOError):
+            except (errors.MarionetteException, IOError):
                 # These exceptions get thrown if the Marionette server
                 # hit an exception/died or the connection died. We can
                 # do no further server-side cleanup in this case.
                 pass
             self.session = None
         if self.instance:
             self.instance.close()
 
@@ -643,17 +644,17 @@ class Marionette(object):
 
     def wait_for_port(self, timeout=None):
         timeout = timeout or self.DEFAULT_STARTUP_TIMEOUT
         return transport.wait_for_port(self.host, self.port, timeout=timeout)
 
     @do_process_check
     def raise_for_port(self, port_obtained):
         if not port_obtained:
-            raise IOError("Timed out waiting for port!")
+            raise socket.timeout("Timed out waiting for port {}!".format(self.port))
 
     @do_process_check
     def _send_message(self, name, params=None, key=None):
         """Send a blocking message to the server.
 
         Marionette provides an asynchronous, non-blocking interface and
         this attempts to paper over this by providing a synchronous API
         to the user.
@@ -679,17 +680,18 @@ class Marionette(object):
 
             else:
                 msg = self.client.request(name, params)
 
         except socket.timeout:
             self.session = None
             self.window = None
             self.client.close()
-            raise errors.TimeoutException("Connection timed out")
+
+            raise
 
         res, err = msg.result, msg.error
         if err:
             self._handle_error(err)
 
         if key is not None:
             return self._unwrap_response(res.get(key))
         else:
@@ -749,27 +751,29 @@ class Marionette(object):
 
         If we've launched the binary we are connected to, wait for it to shut down.
         In the case when it doesn't happen, force its shut down.
 
         """
         if self.instance:
             exc, val, tb = sys.exc_info()
 
-            returncode = self.instance.runner.returncode
+            # Give the application some time to shutdown
+            returncode = self.instance.runner.wait(timeout=self.DEFAULT_STARTUP_TIMEOUT)
             if returncode is None:
-                self.instance.runner.stop()
-                message = 'Process killed because the connection was lost'
+                self.cleanup()
+                message = ('Process killed because the connection to Marionette server is lost.'
+                           ' Check gecko.log for errors')
             else:
-                message = 'Process died with returncode "{returncode}"'
+                message = 'Process has been closed (Exit code: {returncode})'
 
             if exc:
                 message += ' (Reason: {reason})'
 
-            raise exc, message.format(returncode=returncode, reason=val), tb
+            raise IOError, message.format(returncode=returncode, reason=val), tb
 
     @staticmethod
     def convert_keys(*string):
         typing = []
         for val in string:
             if isinstance(val, Keys):
                 typing.append(val)
             elif isinstance(val, int):
@@ -988,18 +992,18 @@ class Marionette(object):
     def enforce_gecko_prefs(self, prefs):
         """
         Checks if the running instance has the given prefs. If not, it will kill the
         currently running instance, and spawn a new instance with the requested preferences.
 
         : param prefs: A dictionary whose keys are preference names.
         """
         if not self.instance:
-            raise errors.MarionetteException("enforce_gecko_prefs can only be called "
-                                             "on gecko instances launched by Marionette")
+            raise errors.MarionetteException("enforce_gecko_prefs() can only be called "
+                                             "on Gecko instances launched by Marionette")
         pref_exists = True
         self.set_context(self.CONTEXT_CHROME)
         for pref, value in prefs.iteritems():
             if type(value) is not str:
                 value = json.dumps(value)
             pref_exists = self.execute_script("""
             let prefInterface = Components.classes["@mozilla.org/preferences-service;1"]
                                           .getService(Components.interfaces.nsIPrefBranch);
@@ -1022,51 +1026,59 @@ class Marionette(object):
         self.set_context(self.CONTEXT_CONTENT)
         if not pref_exists:
             self.delete_session()
             self.instance.restart(prefs)
             self.raise_for_port(self.wait_for_port())
             self.start_session()
             self.reset_timeouts()
 
+    @do_process_check
     def quit_in_app(self):
         """
         This will terminate the currently running instance.
         """
         if not self.instance:
-            raise errors.MarionetteException("quit_in_app can only be called "
-                                             "on gecko instances launched by Marionette")
+            raise errors.MarionetteException("quit_in_app() can only be called "
+                                             "on Gecko instances launched by Marionette")
         # Values here correspond to constants in nsIAppStartup.
         # See http://mzl.la/1X0JZsC
         restart_flags = [
             "eForceQuit",
             "eRestart",
         ]
         self._send_message("quitApplication", {"flags": restart_flags})
         self.client.close()
-        self.raise_for_port(self.wait_for_port())
+
+        try:
+            self.raise_for_port(self.wait_for_port())
+        except socket.timeout:
+            if self.instance.runner.returncode is not None:
+                exc, val, tb = sys.exc_info()
+                self.cleanup()
+                raise exc, 'Requested restart of the application was aborted', tb
 
     def restart(self, clean=False, in_app=False):
         """
         This will terminate the currently running instance, and spawn a new instance
         with the same profile and then reuse the session id when creating a session again.
 
         : param clean: If False the same profile will be used after the restart. Note
                        that the in app initiated restart always maintains the same
                        profile.
         : param in_app: If True, marionette will cause a restart from within the
                         browser. Otherwise the browser will be restarted immediately
                         by killing the process.
         """
         if not self.instance:
-            raise errors.MarionetteException("restart can only be called "
-                                             "on gecko instances launched by Marionette")
+            raise errors.MarionetteException("restart() can only be called "
+                                             "on Gecko instances launched by Marionette")
         if in_app:
             if clean:
-                raise ValueError
+                raise ValueError("An in_app restart cannot be triggered with the clean flag set")
             self.quit_in_app()
         else:
             self.delete_session()
             self.instance.restart(clean=clean)
             self.raise_for_port(self.wait_for_port())
 
         self.start_session(session_id=self.session_id)
         self.reset_timeouts()
@@ -1458,18 +1470,23 @@ class Marionette(object):
         "timeout" response status.
 
         :param timeout_type: A string value specifying the timeout
             type. This must be one of three types: 'implicit', 'script'
             or 'page load'
         :param ms: A number value specifying the timeout length in
             milliseconds (ms)
         """
-        if timeout_type not in [self.TIMEOUT_SEARCH, self.TIMEOUT_SCRIPT, self.TIMEOUT_PAGE]:
-            raise ValueError("Unknown timeout type: %s" % timeout_type)
+        timeout_types = (self.TIMEOUT_PAGE,
+                         self.TIMEOUT_SCRIPT,
+                         self.TIMEOUT_SEARCH,
+                         )
+        if timeout_type not in timeout_types:
+            raise ValueError("Unknown timeout type: {0} (should be one "
+                             "of {1})".format(timeout_type, timeout_types))
         body = {"type": timeout_type, "ms": ms}
         self._send_message("timeouts", body)
 
     def go_back(self):
         """Causes the browser to perform a back navigation."""
         self._send_message("goBack")
 
     def go_forward(self):
--- a/testing/marionette/client/marionette_driver/transport.py
+++ b/testing/marionette/client/marionette_driver/transport.py
@@ -1,14 +1,13 @@
 # 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/.
 
 import datetime
-import errno
 import json
 import socket
 import time
 
 
 class SocketTimeout(object):
     def __init__(self, socket, timeout):
         self.sock = socket
@@ -111,17 +110,16 @@ class TcpTransport(object):
 
         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.
     """
     max_packet_length = 4096
-    connection_lost_msg = "Connection to Marionette server is lost. Check gecko.log for errors."
 
     def __init__(self, addr, port, socket_timeout=360.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
@@ -168,17 +166,17 @@ class TcpTransport(object):
         while self.socket_timeout is None or (time.time() - now < self.socket_timeout):
             try:
                 chunk = self.sock.recv(bytes_to_recv)
                 data += chunk
             except socket.timeout:
                 pass
             else:
                 if not chunk:
-                    raise IOError(self.connection_lost_msg)
+                    raise socket.error("No data received over socket")
 
             sep = data.find(":")
             if sep > -1:
                 length = data[0:sep]
                 remaining = data[sep + 1:]
 
                 if len(remaining) == int(length):
                     if unmarshal:
@@ -192,17 +190,17 @@ class TcpTransport(object):
                                 self.expected_responses.remove(msg)
 
                         return msg
                     else:
                         return remaining
 
                 bytes_to_recv = int(length) - len(remaining)
 
-        raise socket.timeout("connection timed out after %ds" % self.socket_timeout)
+        raise socket.timeout("Connection timed out after %ds" % self.socket_timeout)
 
     def connect(self):
         """Connect to the server and process the hello message we expect
         to receive in response.
 
         Returns a tuple of the protocol level and the application type.
         """
         try:
@@ -234,29 +232,22 @@ class TcpTransport(object):
             data = obj.to_msg()
             self.expected_responses.append(obj)
         else:
             data = json.dumps(obj)
         payload = "%s:%s" % (len(data), data)
 
         totalsent = 0
         while totalsent < len(payload):
-            try:
-                sent = self.sock.send(payload[totalsent:])
-                if sent == 0:
-                    raise IOError("socket error after sending %d of %d bytes" %
-                                  (totalsent, len(payload)))
-                else:
-                    totalsent += sent
-
-            except IOError as e:
-                if e.errno == errno.EPIPE:
-                    raise IOError("%s: %s" % (str(e), self.connection_lost_msg))
-                else:
-                    raise e
+            sent = self.sock.send(payload[totalsent:])
+            if sent == 0:
+                raise IOError("Socket error after sending %d of %d bytes" %
+                              (totalsent, len(payload)))
+            else:
+                totalsent += sent
 
     def respond(self, obj):
         """Send a response to a command.  This can be an arbitrary JSON
         serialisable object or an ``Exception``.
         """
         res, err = None, None
         if isinstance(obj, Exception):
             err = obj
--- a/testing/marionette/client/marionette_driver/wait.py
+++ b/testing/marionette/client/marionette_driver/wait.py
@@ -116,19 +116,19 @@ class Wait(object):
         rv = None
         last_exc = None
         until = is_true or until_pred
         start = self.clock.now
 
         while not until(self.clock, self.end):
             try:
                 rv = condition(self.marionette)
-            except (KeyboardInterrupt, SystemExit) as e:
-                raise e
-            except self.exceptions as e:
+            except (KeyboardInterrupt, SystemExit):
+                raise
+            except self.exceptions:
                 last_exc = sys.exc_info()
 
             if not rv:
                 self.clock.sleep(self.interval)
                 continue
 
             if rv is not None:
                 return rv
--- a/testing/marionette/harness/marionette/runner/base.py
+++ b/testing/marionette/harness/marionette/runner/base.py
@@ -611,19 +611,19 @@ class BaseMarionetteTestRunner(object):
             for path in list(self.testvars_paths):
                 path = os.path.abspath(os.path.expanduser(path))
                 if not os.path.exists(path):
                     raise IOError('--testvars file %s does not exist' % path)
                 try:
                     with open(path) as f:
                         data.append(json.loads(f.read()))
                 except ValueError as e:
-                    raise Exception("JSON file (%s) is not properly "
-                                    "formatted: %s" % (os.path.abspath(path),
-                                                       e.message))
+                    exc, val, tb = sys.exc_info()
+                    msg = "JSON file ({0}) is not properly formatted: {1}"
+                    raise exc, msg.format(os.path.abspath(path), e.message), tb
         return data
 
     @property
     def capabilities(self):
         if self._capabilities:
             return self._capabilities
 
         self.marionette.start_session()
@@ -732,18 +732,20 @@ class BaseMarionetteTestRunner(object):
                     'connect_to_running_emulator': True,
                 })
             if not self.bin and not self.emulator:
                 try:
                     #establish a socket connection so we can vertify the data come back
                     connection = socket.socket(socket.AF_INET,socket.SOCK_STREAM)
                     connection.connect((host,int(port)))
                     connection.close()
-                except Exception, e:
-                    raise Exception("Connection attempt to %s:%s failed with error: %s" %(host,port,e))
+                except Exception as e:
+                    exc, val, tb = sys.exc_info()
+                    msg = "Connection attempt to {0}:{1} failed with error: {2}"
+                    raise exc, msg.format(host, port, e), tb
         if self.workspace:
             kwargs['workspace'] = self.workspace_path
         return kwargs
 
     def launch_test_container(self):
         if self.marionette.session is None:
             self.marionette.start_session()
         self.marionette.set_context(self.marionette.CONTEXT_CONTENT)
--- a/testing/marionette/harness/marionette/runner/httpd.py
+++ b/testing/marionette/harness/marionette/runner/httpd.py
@@ -6,17 +6,17 @@ import os
 
 from wptserve import server, handlers, routes as default_routes
 
 
 class FixtureServer(object):
 
     def __init__(self, root, host="127.0.0.1", port=0):
         if not os.path.isdir(root):
-            raise Exception("Server root is not a valid path: %s" % root)
+            raise IOError("Server root is not a valid path: %s" % root)
         self.root = root
         self.host = host
         self.port = port
         self._server = None
 
     def start(self, block=False):
         if self.alive:
             return
@@ -39,17 +39,17 @@ class FixtureServer(object):
         self._server = None
 
     @property
     def alive(self):
         return self._server is not None
 
     def get_url(self, path="/"):
         if not self.alive:
-            raise "Server not started"
+            raise Exception("Server not started")
         return self._server.get_url(path)
 
     @property
     def routes(self):
         return self._server.router.routes
 
 
 @handlers.handler
--- a/testing/marionette/harness/marionette/runner/mixins/browsermob-proxy-py/browsermobproxy/server.py
+++ b/testing/marionette/harness/marionette/runner/mixins/browsermob-proxy-py/browsermobproxy/server.py
@@ -26,18 +26,18 @@ class Server(object):
 
         exec_not_on_path = True
         for directory in os.environ['PATH'].split(path_var_sep):
             if(os.path.isfile(os.path.join(directory, path))):
                 exec_not_on_path = False
                 break
 
         if not os.path.isfile(path) and exec_not_on_path:
-            raise Exception("Browsermob-Proxy binary couldn't be found in path"
-                            " provided: %s" % path)
+            raise IOError("Browsermob-Proxy binary couldn't be found in path"
+                          " provided: %s" % path)
 
         self.path = path
         self.port = options.get('port', 8080)
         self.process = None
 
         if platform.system() == 'Darwin':
             self.command = ['sh']
         else:
--- a/testing/marionette/harness/marionette/tests/unit/test_timeouts.py
+++ b/testing/marionette/harness/marionette/tests/unit/test_timeouts.py
@@ -6,16 +6,22 @@ from marionette import MarionetteTestCas
 from marionette_driver.marionette import HTMLElement
 from marionette_driver.errors import (NoSuchElementException,
                                       MarionetteException,
                                       ScriptTimeoutException)
 from marionette_driver.by import By
 
 
 class TestTimeouts(MarionetteTestCase):
+
+    def tearDown(self):
+        self.marionette.reset_timeouts()
+
+        MarionetteTestCase.tearDown(self)
+
     def test_pagetimeout_notdefinetimeout_pass(self):
         test_html = self.marionette.absolute_url("test.html")
         self.marionette.navigate(test_html)
 
     def test_pagetimeout_fail(self):
         self.marionette.timeouts("page load", 0)
         test_html = self.marionette.absolute_url("test.html")
         self.assertRaises(MarionetteException, self.marionette.navigate, test_html)
@@ -57,8 +63,13 @@ class TestTimeouts(MarionetteTestCase):
     def test_no_timeout_settimeout(self):
         test_html = self.marionette.absolute_url("test.html")
         self.marionette.navigate(test_html)
         self.marionette.timeouts("script", 10000)
         self.assertTrue(self.marionette.execute_async_script("""
              var callback = arguments[arguments.length - 1];
              setTimeout(function() { callback(true); }, 500);
              """))
+
+    def test_invalid_timeout_type(self):
+        self.assertRaises(ValueError, self.marionette.timeouts, "foobar", 1000)
+        self.assertRaises(ValueError, self.marionette.timeouts, 42, 1000)
+        self.assertRaises(MarionetteException, self.marionette.timeouts, "page load", "foobar")