Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Close the parent process first #180

Merged
merged 1 commit into from
Oct 7, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 30 additions & 4 deletions src/ffpuppet/core.py
Original file line number Diff line number Diff line change
Expand Up @@ -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().

Expand All @@ -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
Expand Down
45 changes: 21 additions & 24 deletions src/ffpuppet/test_ffpuppet.py
Original file line number Diff line number Diff line change
Expand Up @@ -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],))
Expand All @@ -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()
Expand All @@ -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()
Expand All @@ -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()
Expand Down Expand Up @@ -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
Loading