Fix macOS SIGSEGV crash in live webcam mode (tkinter thread-safety)#1720
Open
llulioscesar wants to merge 4 commits intohacksider:mainfrom
Open
Fix macOS SIGSEGV crash in live webcam mode (tkinter thread-safety)#1720llulioscesar wants to merge 4 commits intohacksider:mainfrom
llulioscesar wants to merge 4 commits intohacksider:mainfrom
Conversation
Contributor
Reviewer's GuideRefactors tkinter status update helpers to marshal all widget configuration calls onto the main Tk thread via ROOT.after, preventing macOS SIGSEGV crashes caused by updating widgets from background threads, and adds existence guards while removing a risky ROOT.update() call. Sequence diagram for thread-safe status updates via ROOT.aftersequenceDiagram
actor User
participant BackgroundThread
participant ui_module
participant ROOT
participant TkMainLoop
participant status_label
User->>BackgroundThread: trigger_long_running_task
BackgroundThread->>ui_module: update_status(text)
activate ui_module
ui_module->>ui_module: translated = _(text)
alt ROOT_is_not_none
ui_module->>ROOT: after(0, _do)
ROOT->>TkMainLoop: schedule_callback(_do)
activate TkMainLoop
TkMainLoop->>ui_module: execute _do()
activate ui_module
alt status_label_is_not_none
ui_module->>status_label: configure(text=translated)
end
deactivate ui_module
deactivate TkMainLoop
else ROOT_is_none
ui_module-->>BackgroundThread: no_ui_update
deactivate ui_module
end
Sequence diagram for thread-safe popup live status updatessequenceDiagram
actor User
participant BackgroundThread
participant ui_module
participant ROOT
participant TkMainLoop
participant POPUP_LIVE
participant popup_status_label_live
User->>BackgroundThread: start_live_webcam
BackgroundThread->>ui_module: update_pop_live_status(text)
activate ui_module
ui_module->>ui_module: translated = _(text)
alt ROOT_is_not_none
ui_module->>ROOT: after(0, _do)
ROOT->>TkMainLoop: schedule_callback(_do)
activate TkMainLoop
TkMainLoop->>ui_module: execute _do()
activate ui_module
alt popup_status_label_live_and_POPUP_LIVE_valid
ui_module->>POPUP_LIVE: winfo_exists()
POPUP_LIVE-->>ui_module: exists
ui_module->>popup_status_label_live: configure(text=translated)
else popup_or_label_invalid
ui_module-->>TkMainLoop: skip_update
end
deactivate ui_module
deactivate TkMainLoop
else ROOT_is_none
ui_module-->>BackgroundThread: no_ui_update
deactivate ui_module
end
Flow diagram for guarded popup status update logicflowchart TD
A["update_pop_status(text) called"] --> B["translated = _(text)"]
B --> C{"ROOT is not None"}
C -- "No" --> Z["Return without scheduling UI update"]
C -- "Yes" --> D["Call ROOT.after(0, _do)"]
D --> E["Later on Tk main thread: execute _do()"]
E --> F{"popup_status_label is not None"}
F -- "No" --> Z
F -- "Yes" --> G{"POPUP is not None"}
G -- "No" --> Z
G -- "Yes" --> H{"POPUP.winfo_exists() is True"}
H -- "No" --> Z
H -- "Yes" --> I["popup_status_label.configure(text=translated)"]
I --> Z["End"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- When scheduling callbacks with ROOT.after, consider also checking ROOT.winfo_exists() (or handling TclError) to avoid crashes if the root window is destroyed before the callback is queued.
- Even with existence checks before configure(), there is still a race if the widget is destroyed between the check and the configure; consider wrapping the configure() calls in a small try/except TclError to make this fully robust against teardown races.
- The three update_* functions now share a very similar pattern (translate → inner _do → ROOT.after); extracting a small helper to schedule safe label updates on the main thread would reduce duplication and keep future changes to this behavior in one place.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- When scheduling callbacks with ROOT.after, consider also checking ROOT.winfo_exists() (or handling TclError) to avoid crashes if the root window is destroyed before the callback is queued.
- Even with existence checks before configure(), there is still a race if the widget is destroyed between the check and the configure; consider wrapping the configure() calls in a small try/except TclError to make this fully robust against teardown races.
- The three update_* functions now share a very similar pattern (translate → inner _do → ROOT.after); extracting a small helper to schedule safe label updates on the main thread would reduce duplication and keep future changes to this behavior in one place.
## Individual Comments
### Comment 1
<location path="modules/ui.py" line_range="760-761" />
<code_context>
+ if status_label is not None:
+ status_label.configure(text=translated)
+
+ if ROOT is not None:
+ ROOT.after(0, _do)
</code_context>
<issue_to_address>
**issue:** Consider guarding against a destroyed ROOT before calling `after` to avoid potential `TclError`.
If `ROOT` has been destroyed but the reference is still non-`None`, `ROOT.after` may raise `TclError`. To harden this, also check `ROOT.winfo_exists()` before scheduling the callback, mirroring the popup checks.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…d-safety On macOS, calling tkinter widget.configure() from background threads causes a segfault in Tk_FreeGC -> TkButtonWorldChanged -> ConfigureButton. This commit replaces direct widget updates in update_status(), update_pop_status(), and update_pop_live_status() with ROOT.after(0, ...) to schedule UI changes on the main Tk event loop thread. Also removes ROOT.update() from update_status() which re-enters the Tk event loop from background threads, another source of crashes.
Addresses review feedback: if ROOT has been destroyed but the reference is still non-None, ROOT.after() may raise TclError. Now all three functions check winfo_exists() before scheduling, consistent with the popup widget guards.
9ee8a2e to
9694b90
Compare
The display loop checked PREVIEW.state() == "withdrawn" on its first iteration, but PREVIEW.deiconify() had not yet completed, causing the preview to close immediately. Add a small delay (100ms) before the first frame check to let the window finish appearing.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
SIGSEGVcrash (Tk_FreeGC→TkButtonWorldChanged→ConfigureButton) that occurs on macOS when starting live webcam modeROOT.after(0, ...)to schedule UI changes on the main Tk event loopROOT.update()call fromupdate_status()which re-enters the Tk event loop from background threadsProblem
On macOS (tested on Apple Silicon M2 Max, macOS 26.3.1), clicking "Live" in the webcam preview causes an immediate crash with:
This happens because
update_status(),update_pop_status(), andupdate_pop_live_status()callwidget.configure()directly from background threads. Tkinter is not thread-safe on macOS, and modifying widgets from non-main threads causes segfaults in Tk's graphics context management.Fix
ROOT.after(0, callback)to schedule all widget updates on the main Tk threadROOT.update()fromupdate_status()(unnecessary withROOT.after()and dangerous from background threads)Test plan
python run.pyon macOSSummary by Sourcery
Route tkinter status label updates through the main Tk event loop to prevent macOS crashes when updating the UI from background threads.
Bug Fixes:
Enhancements: