Bug 1265584 - Move wptrunner marionette usage onto a single thread, r=ato
authorJames Graham <james@hoppipolla.co.uk>
Mon, 14 May 2018 16:57:19 +0100
changeset 419873 9b8539a9a2a6c20220cdcb07e19b7bf3a91c7436
parent 419872 a6f3af165d488712c31c2e2f43214c0820c27ace
child 419874 b99661f1fcc56bca0176bd813c166525a4ebf2e4
push id34052
push userccoroiu@mozilla.com
push dateFri, 25 May 2018 17:52:14 +0000
treeherdermozilla-central@94d7f0e1c4d0 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersato
bugs1265584
milestone62.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 1265584 - Move wptrunner marionette usage onto a single thread, r=ato Running marionette on a background thread is problematic in the case that a test times out. In this case the background thread is not terminated. If we then call into marionette again on the main thread we may race with something that happens on the runner thread. The marionette client isn't threadsafe, so this leads to buggy behaviour. The simplest fx for the problem is just to move all the marionette calls onto the main thread and instead of waiting on the main thread, spin up a thread with a timer. MozReview-Commit-ID: 3vVlMcwPHSx
testing/web-platform/tests/tools/wptrunner/wptrunner/executors/executormarionette.py
--- a/testing/web-platform/tests/tools/wptrunner/wptrunner/executors/executormarionette.py
+++ b/testing/web-platform/tests/tools/wptrunner/wptrunner/executors/executormarionette.py
@@ -432,25 +432,28 @@ class ExecuteAsyncScriptRun(object):
                 # We just want it to never time out, really, but marionette doesn't
                 # make that possible. It also seems to time out immediately if the
                 # timeout is set too high. This works at least.
                 self.protocol.base.set_timeout(2**28 - 1)
         except IOError:
             self.logger.error("Lost marionette connection before starting test")
             return Stop
 
-        executor = threading.Thread(target = self._run)
-        executor.start()
-
         if timeout is not None:
             wait_timeout = timeout + 2 * extra_timeout
         else:
             wait_timeout = None
 
-        flag = self.result_flag.wait(wait_timeout)
+        timer = threading.Timer(wait_timeout, self._timeout)
+        timer.start()
+
+        self._run()
+
+        self.result_flag.wait()
+        timer.cancel()
 
         if self.result == (None, None):
             self.logger.debug("Timed out waiting for a result")
             self.result = False, ("EXTERNAL-TIMEOUT", None)
         elif self.result[1] is None:
             # We didn't get any data back from the test, so check if the
             # browser is still responsive
             if self.protocol.is_alive:
@@ -475,16 +478,20 @@ class ExecuteAsyncScriptRun(object):
             if message:
                 message += "\n"
             message += traceback.format_exc(e)
             self.logger.warning(traceback.format_exc())
             self.result = False, ("INTERNAL-ERROR", e)
         finally:
             self.result_flag.set()
 
+    def _timeout(self):
+        self.result = False, ("EXTERNAL-TIMEOUT", None)
+        self.result_flag.set()
+
 
 class MarionetteTestharnessExecutor(TestharnessExecutor):
     supports_testdriver = True
 
     def __init__(self, browser, server_config, timeout_multiplier=1,
                  close_after_done=True, debug_info=None, capabilities=None,
                  **kwargs):
         """Marionette-based executor for testharness.js tests"""