From fb06e27e5fee2611abc81a1ce1b2be2430fa0e70 Mon Sep 17 00:00:00 2001 From: Tyson Smith Date: Fri, 15 Sep 2023 09:54:55 -0700 Subject: [PATCH 1/2] Update log collection logic in close() --- src/ffpuppet/core.py | 73 +++++++++++++++++++------------------------- 1 file changed, 32 insertions(+), 41 deletions(-) diff --git a/src/ffpuppet/core.py b/src/ffpuppet/core.py index 4258e29..cc58e89 100644 --- a/src/ffpuppet/core.py +++ b/src/ffpuppet/core.py @@ -582,48 +582,39 @@ def close(self, force_close: bool = False) -> None: self._terminate() # collect crash reports and logs - if not force_close: - if self._logs.closed: # pragma: no cover - # This should not happen while everything is working as expected. - # This is here to prevent additional unexpected issues. - # Since this should never happen in normal operation this assert - # will help verify that. - # If '_proc' is not None this is the first call to close() - # in this situation the PuppetLogger should still be available. - assert self._proc is None, "PuppetLogger is closed!" - else: - LOG.debug("reviewing %d check(s)", len(self._checks)) - for check in self._checks: - if check.message is not None: - r_code = Reason.WORKER - check.dump_log( - dst_fp=self._logs.add_log(f"ffp_worker_{check.name}") - ) - # collect logs (excluding minidumps) - for log_path in self._crashreports(skip_md=True, skip_benign=False): - self._logs.add_log(log_path.name, log_path.open("rb")) - assert self.profile is not None - assert self.profile.path is not None - assert self._bin_path is not None - # check for minidumps and process them if possible - md_path = self.profile.path / "minidumps" - if any(md_path.glob("*.dmp")): - # check for local build symbols - sym_path = self._bin_path.parent / "crashreporter-symbols" - if not sym_path.is_dir(): - # use packaged symbols - sym_path = self._bin_path / "symbols" - process_minidumps( - md_path, - sym_path, - self._logs.add_log, - working_path=self._working_path, + if not force_close and not self._logs.closed: + LOG.debug("reviewing %d check(s)", len(self._checks)) + for check in self._checks: + if check.message is not None: + r_code = Reason.WORKER + check.dump_log( + dst_fp=self._logs.add_log(f"ffp_worker_{check.name}") ) - stderr_fp = self._logs.get_fp("stderr") - if stderr_fp: - if exit_code is not None: - stderr_fp.write(f"[ffpuppet] Exit code: {exit_code}\n".encode()) - stderr_fp.write(f"[ffpuppet] Reason code: {r_code.name}\n".encode()) + # collect logs (excluding minidumps) + for log_path in self._crashreports(skip_md=True, skip_benign=False): + self._logs.add_log(log_path.name, log_path.open("rb")) + assert self.profile is not None + assert self.profile.path is not None + assert self._bin_path is not None + # check for minidumps and process them if possible + md_path = self.profile.path / "minidumps" + if any(md_path.glob("*.dmp")): + # check for local build symbols + sym_path = self._bin_path.parent / "crashreporter-symbols" + if not sym_path.is_dir(): + # use packaged symbols + sym_path = self._bin_path / "symbols" + process_minidumps( + md_path, + sym_path, + self._logs.add_log, + working_path=self._working_path, + ) + stderr_fp = self._logs.get_fp("stderr") + if stderr_fp: + if exit_code is not None: + stderr_fp.write(f"[ffpuppet] Exit code: {exit_code}\n".encode()) + stderr_fp.write(f"[ffpuppet] Reason code: {r_code.name}\n".encode()) # reset remaining to closed state try: From c8aacc13ff764644f7ce88fcfc9a43ee21b6cd15 Mon Sep 17 00:00:00 2001 From: Tyson Smith Date: Fri, 15 Sep 2023 10:22:44 -0700 Subject: [PATCH 2/2] Always include browser exit code in logs --- src/ffpuppet/core.py | 13 +++++-------- src/ffpuppet/test_ffpuppet.py | 2 +- 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/src/ffpuppet/core.py b/src/ffpuppet/core.py index cc58e89..a67d189 100644 --- a/src/ffpuppet/core.py +++ b/src/ffpuppet/core.py @@ -535,8 +535,7 @@ def close(self, force_close: bool = False) -> None: # this can raise but shouldn't, we want to know if a timeout happens self._proc.wait(timeout=30) - # set reason code - exit_code: Optional[int] = None + # check state of browser processes and set the close reason if any(self._crashreports(skip_benign=True)): r_code = Reason.ALERT # Wait a moment for processes to exit automatically. @@ -568,12 +567,11 @@ def close(self, force_close: bool = False) -> None: # have a crash report... this should be revisited when time allows # https://bugzil.la/1370520 # Ignore -9 to avoid false positives due to system OOM killer - exit_code = self._proc.poll() + exit_code: Optional[int] = self._proc.poll() + assert exit_code is not None r_code = Reason.ALERT LOG.warning( - "Browser exit code: %r (%X), no crash reports found", - exit_code, - exit_code, + "No crash reports found, exit code: %d (%X)", exit_code, exit_code ) else: r_code = Reason.EXITED @@ -612,8 +610,7 @@ def close(self, force_close: bool = False) -> None: ) stderr_fp = self._logs.get_fp("stderr") if stderr_fp: - if exit_code is not None: - stderr_fp.write(f"[ffpuppet] Exit code: {exit_code}\n".encode()) + stderr_fp.write(f"[ffpuppet] Exit code: {self._proc.poll()}\n".encode()) stderr_fp.write(f"[ffpuppet] Reason code: {r_code.name}\n".encode()) # reset remaining to closed state diff --git a/src/ffpuppet/test_ffpuppet.py b/src/ffpuppet/test_ffpuppet.py index 7529918..245c5c1 100644 --- a/src/ffpuppet/test_ffpuppet.py +++ b/src/ffpuppet/test_ffpuppet.py @@ -998,7 +998,7 @@ def get_processes(self): with StubbedProc() as ffp: ffp.launch() proc = ffp._proc - ffp._proc.poll.side_effect = (None, 0, 0) + ffp._proc.poll.side_effect = (None, 0, 0, 0) ffp.close() assert ffp._proc is None assert ffp._logs.closed