Bug 1533309 - Correct process shutdown on setup error - r=Bebe
authorTarek Ziadé <tarek@mozilla.com>
Thu, 07 Mar 2019 14:31:46 +0000
changeset 520763 728658e02de5947d27e2aebd75b45de94b42e5d0
parent 520762 75921acaaf148933460e88631b0a0b95b234fed5
child 520764 41a911c24dd3c491da432881baa9b38fe0dce4d5
push id10862
push userffxbld-merge
push dateMon, 11 Mar 2019 13:01:11 +0000
treeherdermozilla-beta@a2e7f5c935da [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersBebe
bugs1533309
milestone67.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 1533309 - Correct process shutdown on setup error - r=Bebe Make sure we shutdown the mitmproxy process in case the setup fails. Differential Revision: https://phabricator.services.mozilla.com/D22467
testing/mozbase/mozproxy/mozproxy/backends/mitm.py
testing/mozbase/mozproxy/tests/manifest.ini
testing/mozbase/mozproxy/tests/test.py
testing/mozbase/mozproxy/tests/test_proxy.py
--- a/testing/mozbase/mozproxy/mozproxy/backends/mitm.py
+++ b/testing/mozbase/mozproxy/mozproxy/backends/mitm.py
@@ -71,16 +71,17 @@ POLICIES_CONTENT_OFF = """{
 
 class Mitmproxy(Playback):
     def __init__(self, config):
         self.config = config
         self.mitmproxy_proc = None
         self.mitmdump_path = None
         self.recordings = config.get("playback_recordings")
         self.browser_path = config.get("binary")
+        self.policies_dir = None
 
         # mozproxy_dir is where we will download all mitmproxy required files
         # when running locally it comes from obj_path via mozharness/mach
         if self.config.get("obj_path") is not None:
             self.mozproxy_dir = self.config.get("obj_path")
         else:
             # in production it is ../tasks/task_N/build/, in production that dir
             # is not available as an envvar, however MOZ_UPLOAD_DIR is set as
@@ -93,23 +94,25 @@ class Mitmproxy(Playback):
         self.recordings_path = self.mozproxy_dir
         LOG.info(
             "mozproxy_dir used for mitmproxy downloads and exe files: %s"
             % self.mozproxy_dir
         )
 
         # go ahead and download and setup mitmproxy
         self.download()
-
         # mitmproxy must be started before setup, so that the CA cert is available
         self.start()
 
-        # XXX if this fails, we fail to create an insatnce and mitdump has
-        # started and become a ghost
-        self.setup()
+        # In case the setup fails, we want to stop the process before raising.
+        try:
+            self.setup()
+        except Exception:
+            self.stop()
+            raise
 
     def download(self):
         """Download and unpack mitmproxy binary and pageset using tooltool"""
         if not os.path.exists(self.mozproxy_dir):
             os.makedirs(self.mozproxy_dir)
 
         LOG.info("downloading mitmproxy binary")
         _manifest = os.path.join(here, self.config["playback_binary_manifest"])
@@ -327,17 +330,17 @@ class MitmproxyDesktop(Mitmproxy):
     def stop(self):
         self.stop_mitmproxy_playback()
         self.turn_off_browser_proxy()
 
     def turn_off_browser_proxy(self):
         """Turn off the browser proxy that was used for mitmproxy playback. In Firefox
         we need to change the autoconfig files to revert the proxy; for Chromium the proxy
         was setup on the cmd line, so nothing is required here."""
-        if self.config["app"] == "firefox":
+        if self.config["app"] == "firefox" and self.policies_dir is not None:
             LOG.info("Turning off the browser proxy")
 
             self.write_policies_json(
                 self.policies_dir, policies_content=POLICIES_CONTENT_OFF
             )
 
 
 class MitmproxyAndroid(Mitmproxy):
--- a/testing/mozbase/mozproxy/tests/manifest.ini
+++ b/testing/mozbase/mozproxy/tests/manifest.ini
@@ -1,3 +1,3 @@
 [DEFAULT]
 subsuite = mozbase
-[test.py]
+[test_proxy.py]
rename from testing/mozbase/mozproxy/tests/test.py
rename to testing/mozbase/mozproxy/tests/test_proxy.py
--- a/testing/mozbase/mozproxy/tests/test.py
+++ b/testing/mozbase/mozproxy/tests/test_proxy.py
@@ -1,74 +1,96 @@
 #!/usr/bin/env python
 from __future__ import absolute_import, print_function
 import os
-import subprocess
 
-import mozprocess
+import mock
 import mozunit
 import mozinfo
 from mozproxy import get_playback
-from mozproxy import utils
-
+from mozproxy.backends import mitm
 
 here = os.path.dirname(__file__)
-# XXX we need to create a temp dir where things happen, for cleanup
-# otherwise, the source tree gets polluted
-
-# XXX will do better later using a real mock tool
-
-
-def _no_download(*args, **kw):
-    pass
 
 
-utils.tooltool_download = _no_download
-
-
-def check_output(*args, **kw):
-    return "yea ok"
-
-
-subprocess.check_output = check_output
-
-
-class ProcessHandler:
+class Process:
     def __init__(self, *args, **kw):
-        self.pid = 12345
-
-    def run(self):
         pass
 
     def poll(self):
         return None
 
+    wait = poll
 
-mozprocess.ProcessHandler = ProcessHandler
+    def kill(self, sig=9):
+        pass
+
+    pid = 1234
+    stderr = stdout = None
 
 
-def test_mitm():
-    from mozproxy.backends import mitm
-
+@mock.patch("mozprocess.processhandler.ProcessHandlerMixin.Process", new=Process)
+@mock.patch("mozproxy.backends.mitm.tooltool_download", new=mock.DEFAULT)
+def test_mitm(*args):
     mitm.MITMDUMP_SLEEP = 0.1
-
     bin_name = "mitmproxy-rel-bin-{platform}.manifest"
     pageset_name = "mitmproxy-recordings-raptor-paypal.manifest"
 
     config = {
         "playback_tool": "mitmproxy",
         "playback_binary_manifest": bin_name,
         "playback_pageset_manifest": pageset_name,
         "platform": mozinfo.os,
         "playback_recordings": os.path.join(here, "paypal.mp"),
         "run_local": True,
-        "obj_path": here,   # XXX tmp?
+        "obj_path": here,  # XXX tmp?
         "binary": "firefox",
         "app": "firefox",
         "host": "example.com",
     }
 
     playback = get_playback(config)
     assert playback is not None
+    playback.stop()
+
+
+@mock.patch("mozprocess.processhandler.ProcessHandlerMixin.Process", new=Process)
+@mock.patch("mozproxy.backends.mitm.tooltool_download", new=mock.DEFAULT)
+def test_playback_setup_failed(*args):
+    class SetupFailed(Exception):
+        pass
+
+    def setup(*args, **kw):
+        def _s(self):
+            raise SetupFailed("Failed")
+
+        return _s
+
+    mitm.MITMDUMP_SLEEP = 0.1
+    bin_name = "mitmproxy-rel-bin-{platform}.manifest"
+    pageset_name = "mitmproxy-recordings-raptor-paypal.manifest"
+
+    config = {
+        "playback_tool": "mitmproxy",
+        "playback_binary_manifest": bin_name,
+        "playback_pageset_manifest": pageset_name,
+        "platform": mozinfo.os,
+        "playback_recordings": os.path.join(here, "paypal.mp"),
+        "run_local": True,
+        "obj_path": here,  # XXX tmp?
+        "binary": "firefox",
+        "app": "firefox",
+        "host": "example.com",
+    }
+
+    prefix = "mozproxy.backends.mitm.MitmproxyDesktop."
+    with mock.patch(prefix + "setup", new_callable=setup):
+        with mock.patch(prefix + "stop_mitmproxy_playback") as p:
+            try:
+                get_playback(config)
+            except SetupFailed:
+                assert p.call_count == 1
+            except Exception:
+                raise
 
 
 if __name__ == "__main__":
     mozunit.main(runwith="pytest")