From ddf2277022b9ffbe853a330bed36800aed0609d9 Mon Sep 17 00:00:00 2001 From: Tyson Smith Date: Fri, 6 Oct 2023 14:36:02 -0700 Subject: [PATCH] Close the parent process first This will avoid closing the launcher process on Windows which can lead to hung processes that do not show up in task manager. This eventually can lead to a system OOM. --- src/ffpuppet/core.py | 34 ++++++++++++++++++++++---- src/ffpuppet/test_ffpuppet.py | 45 ++++++++++++++++------------------- 2 files changed, 51 insertions(+), 28 deletions(-) diff --git a/src/ffpuppet/core.py b/src/ffpuppet/core.py index cc3bcdc..10d42ff 100644 --- a/src/ffpuppet/core.py +++ b/src/ffpuppet/core.py @@ -271,6 +271,32 @@ def _dbg_sanity_check(cls, dbg: Debugger) -> None: if not match or float(match.group("ver")) < cls.VALGRIND_MIN_VERSION: raise OSError(f"Valgrind >= {cls.VALGRIND_MIN_VERSION:.2f} is required") + @staticmethod + def _parent_proc(processes: List[Process]) -> Optional[Process]: + """Inspect given processes and select the browser parent process. + This assumes all processes are part of the same process tree. + This is needed because the process tree is different depending on the platform. + Windows: + python -> firefox (launcher) -> firefox (parent) -> firefox (content procs) + + Linux and others: + python -> firefox (parent) -> firefox (content procs) + + Args: + processes: Processes to inspect. + + Returns: + The browser parent process if it still exists otherwise false. + """ + for proc in processes: + try: + cmd = proc.cmdline() + if "-contentproc" not in cmd and "-no-deelevate" not in cmd: + return proc + except (AccessDenied, NoSuchProcess): # pragma: no cover + pass + return None + def _terminate(self) -> None: """Call terminate() on browser processes. If terminate() fails try kill(). @@ -285,12 +311,12 @@ def _terminate(self) -> None: LOG.debug("no processes to terminate") return - assert self._proc # try terminating the parent process first, this should be all that is needed - if self._proc.poll() is None: - LOG.debug("attempting to terminate parent process (%d)", self._proc.pid) + parent = self._parent_proc(procs) + if parent: try: - self._proc.terminate() + LOG.debug("attempting to terminate parent process (%d)", parent.pid) + parent.terminate() # wait for debugger (if in use) and child processes to close procs = wait_procs(procs, timeout=10)[1] except (AccessDenied, NoSuchProcess): # pragma: no cover diff --git a/src/ffpuppet/test_ffpuppet.py b/src/ffpuppet/test_ffpuppet.py index bf99764..6ea6e1b 100644 --- a/src/ffpuppet/test_ffpuppet.py +++ b/src/ffpuppet/test_ffpuppet.py @@ -836,26 +836,20 @@ def test_ffpuppet_29(mocker): # not running mocker.patch.object(FFPuppet, "get_processes", return_value=[]) with FFPuppet() as ffp: - ffp._proc = mocker.Mock(spec=Popen) ffp._terminate() - assert ffp._proc.poll.call_count == 0 - assert ffp._proc.terminate.call_count == 0 - ffp._proc = None proc = mocker.Mock(spec_set=Process, pid=123) + proc.cmdline.return_value = [""] # running (close with parent) mocker.patch.object(FFPuppet, "get_processes", side_effect=([proc],)) fake_wait_procs.side_effect = (([], []),) with FFPuppet() as ffp: - ffp._proc = mocker.Mock(spec=Popen, pid=123) - ffp._proc.poll.return_value = None ffp._terminate() - assert ffp._proc.poll.call_count == 1 - assert ffp._proc.terminate.call_count == 1 - ffp._proc = None - assert proc.terminate.call_count == 0 + assert proc.cmdline.call_count == 1 + assert proc.terminate.call_count == 1 assert fake_wait_procs.call_count == 1 fake_wait_procs.reset_mock() + proc.reset_mock() # running (terminate() all) mocker.patch.object(FFPuppet, "get_processes", side_effect=([proc],)) @@ -864,12 +858,8 @@ def test_ffpuppet_29(mocker): ([], []), ) with FFPuppet() as ffp: - ffp._proc = mocker.Mock(spec=Popen, pid=123) - ffp._proc.poll.return_value = None ffp._terminate() - assert ffp._proc.poll.call_count == 1 - ffp._proc = None - assert proc.terminate.call_count == 1 + assert proc.terminate.call_count == 2 assert proc.kill.call_count == 0 assert fake_wait_procs.call_count == 2 fake_wait_procs.reset_mock() @@ -883,12 +873,8 @@ def test_ffpuppet_29(mocker): ([], []), ) with FFPuppet() as ffp: - ffp._proc = mocker.Mock(spec=Popen, pid=123) - ffp._proc.poll.return_value = None ffp._terminate() - assert ffp._proc.poll.call_count == 1 - ffp._proc = None - assert proc.terminate.call_count == 1 + assert proc.terminate.call_count == 2 assert proc.kill.call_count == 1 assert fake_wait_procs.call_count == 3 fake_wait_procs.reset_mock() @@ -902,12 +888,8 @@ def test_ffpuppet_29(mocker): ([], [proc]), ) with FFPuppet() as ffp: - ffp._proc = mocker.Mock(spec=Popen, pid=123) - ffp._proc.poll.return_value = None with raises(TerminateError): ffp._terminate() - assert ffp._proc.poll.call_count == 1 - ffp._proc = None assert fake_wait_procs.call_count == 3 fake_wait_procs.reset_mock() proc.reset_mock() @@ -1090,3 +1072,18 @@ def test_ffpuppet_34(mocker): proc.environ.return_value = {"FFPUPPET_UID": ffp._uid} process_iter.side_effect = ([proc, mocker.Mock(spec_set=Process)],) assert any(ffp.get_processes()) + + +@mark.parametrize("proc_count", [0, 1, 2]) +def test_ffpuppet_35(mocker, proc_count): + """test FFPuppet._parent_proc() setting reason""" + procs = [] + for _ in range(proc_count): + procs.append(mocker.Mock(spec_set=Process)) + procs[-1].cmdline.return_value = [""] + parent = FFPuppet._parent_proc(procs) + if proc_count: + assert parent + assert parent.cmdline.call_count == 1 + else: + assert parent is None