Bug 1202392 - Improve exception handling and message details in Marionette. r=automatedtester
authorHenrik Skupin <mail@hskupin.info>
Thu, 28 Jul 2016 15:00:25 +0200
changeset 307821 186bab1ed1899e656a6688ab685618e1e87da9b5
parent 307820 65135e97b7c66c8d24c19ed5769019f8826c135b
child 307822 9d5365ee6944046216d3fff3662de79680d41aaf
push id31032
push userhskupin@mozilla.com
push dateWed, 03 Aug 2016 13:47:33 +0000
treeherderautoland@186bab1ed189 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersautomatedtester
bugs1202392
milestone51.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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
@@ -168,17 +168,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()
@@ -216,21 +216,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
@@ -114,17 +113,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
@@ -171,17 +169,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:
@@ -198,17 +196,17 @@ class TcpTransport(object):
 
                         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:
@@ -241,29 +239,22 @@ class TcpTransport(object):
             if isinstance(obj, Command):
                 self.expected_response = 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()
@@ -728,18 +728,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")