Bug 1202392 - Improve exception handling and message details in Marionette. r=automatedtester

MozReview-Commit-ID: 5cvQDMlkMGn

--HG--
extra : rebase_source : 0b4019023cb1fb0ffaf236c5f3a8439fac1916ee

testing/marionette/client/marionette_driver/decorators.py

@@ -2,7 +2,7 @@
 # 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
@@ -40,13 +40,16 @@ def do_process_check(func, always=False):
 
         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:

testing/marionette/client/marionette_driver/geckoinstance.py

@@ -173,7 +173,7 @@ class GeckoInstance(object):
     def restart(self, prefs=None, clean=True):
         self.close(restart=True)
 
-        if clean:
+        if clean and self.profile:
             self.profile.cleanup()
             self.profile = None
 
@@ -221,11 +221,9 @@ class FennecInstance(GeckoInstance):
                 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',

testing/marionette/client/marionette_driver/marionette.py

@@ -543,6 +543,7 @@ class Marionette(object):
     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,
@@ -616,7 +617,7 @@ class Marionette(object):
         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.
@@ -648,7 +649,7 @@ class Marionette(object):
     @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):
@@ -684,7 +685,8 @@ class Marionette(object):
             self.session = None
             self.window = None
             self.client.close()
-            raise errors.TimeoutException("Connection timed out")
+
+            raise
 
         res, err = msg.result, msg.error
         if err:
@@ -754,17 +756,19 @@ class Marionette(object):
         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):
@@ -993,8 +997,8 @@ class Marionette(object):
         : 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():
@@ -1027,13 +1031,14 @@ class Marionette(object):
             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 = [
@@ -1042,7 +1047,14 @@ class Marionette(object):
         ]
         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):
         """
@@ -1057,11 +1069,11 @@ class Marionette(object):
                         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()
@@ -1463,8 +1475,13 @@ class Marionette(object):
         :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)
 

testing/marionette/client/marionette_driver/transport.py

@@ -3,7 +3,6 @@
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
 import datetime
-import errno
 import json
 import socket
 import time
@@ -119,7 +118,6 @@ class TcpTransport(object):
     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
@@ -176,7 +174,7 @@ class TcpTransport(object):
                 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:
@@ -203,7 +201,7 @@ class TcpTransport(object):
 
                 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
@@ -246,19 +244,12 @@ class TcpTransport(object):
 
         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

testing/marionette/client/marionette_driver/wait.py

@@ -121,9 +121,9 @@ class Wait(object):
         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:

testing/marionette/harness/marionette/runner/base.py

@@ -616,9 +616,9 @@ class BaseMarionetteTestRunner(object):
                     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
@@ -733,8 +733,10 @@ class BaseMarionetteTestRunner(object):
                     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

testing/marionette/harness/marionette/runner/httpd.py

@@ -11,7 +11,7 @@ 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
@@ -44,7 +44,7 @@ class FixtureServer(object):
 
     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

testing/marionette/harness/marionette/runner/mixins/browsermob-proxy-py/browsermobproxy/server.py

@@ -31,8 +31,8 @@ class Server(object):
                 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)

testing/marionette/harness/marionette/tests/unit/test_timeouts.py

@@ -11,6 +11,12 @@ 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)
@@ -62,3 +68,8 @@ class TestTimeouts(MarionetteTestCase):
              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")