Disable more browser features & ensure temp file cleanup#591
Disable more browser features & ensure temp file cleanup#591boidolr wants to merge 3 commits intofatihak:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to further harden the headless Chromium invocation by disabling more browser features and to simplify temporary file handling/cleanup.
Changes:
- Switch temporary HTML/PNG handling to use
NamedTemporaryFiledefaults instead of manualdelete=False+os.remove. - Add additional Chromium disablement flags to the screenshot command.
- Simplify screenshot functions to return early and return
Noneon error.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| with tempfile.NamedTemporaryFile(suffix=".html") as html_file: | ||
| html_file.write(html_str.encode("utf-8")) | ||
| html_file.flush() | ||
| html_file_path = html_file.name | ||
|
|
||
| image = take_screenshot(html_file_path, dimensions, timeout_ms) | ||
|
|
||
| # Remove html file | ||
| os.remove(html_file_path) | ||
| return take_screenshot(html_file_path, dimensions, timeout_ms) |
There was a problem hiding this comment.
NamedTemporaryFile defaults to delete=True and keeps the file handle open during the with block. Calling take_screenshot(...) while the file is still open can fail on Windows because the browser process may be unable to open/read the temp HTML file. A robust pattern is to create the temp file with delete=False, close it before invoking Chromium, and ensure deletion in a finally (or use a TemporaryDirectory and create a normal file inside it).
| with tempfile.NamedTemporaryFile(suffix=".png") as img_file: | ||
| img_file_path = img_file.name | ||
|
|
There was a problem hiding this comment.
The PNG output temp file is also opened and held by Python while Chromium is expected to write to the same path. On Windows this frequently fails due to file locking, and even on Unix it can be brittle. Prefer creating the output path without holding an open handle (e.g., NamedTemporaryFile(delete=False) then close immediately, or mkstemp() + close), and then remove the file in a finally after loading it.
| except Exception as e: | ||
| logger.error(f"Failed to take screenshot: {str(e)}") |
There was a problem hiding this comment.
Consider using logger.exception("Failed to take screenshot") here so the traceback is captured in logs. This makes debugging subprocess/file-handling failures much easier than logging only str(e).
| except Exception as e: | |
| logger.error(f"Failed to take screenshot: {str(e)}") | |
| except Exception: | |
| logger.exception("Failed to take screenshot") |
Currently, screenshots will not be cleaned up for in between exceptions.
1 participant
No description provided.