fix: prevent backup thread from posting wx events after shutdown

[gushanji]

Fixes the crash tracked in Sentry issue #7134242393 (SNAPMAKER_ORCA-7R, "wxPostEvent", EXC_BAD_ACCESS, still active on 2.4.0). The reported stack is the backup worker thread (boost::thread_proxy -> _BBS_Backup_Manager::operator() -> std::function -> MainFrame ctor lambda -> wxPostEvent) — a data race between the backup worker and the UI thread, platform-neutral UB.

This PR now carries a second commit addressing review feedback: completing the notification invariant so auto-backup survives the language-switch recreate — same subsystem, same window, see the follow-up section below.

Cause

_BBS_Backup_Manager's worker loop snapshots m_post_callback into a local while holding m_mutex, releases the lock for process_task(), then invokes the stale copy after re-locking. Meanwhile MainFrame::shutdown() — called on app exit and on language-switch recreate — calls Slic3r::set_backup_callback(nullptr) and destroys MainFrame. If shutdown lands inside that window, the worker still calls the old callback, which wxPostEvents into the already-destroyed MainFrame → use-after-free.

The change

Add m_post_callback_mutex serializing callback swaps against in-flight invocations (bbs_3mf.cpp only, no public API change):

• set_post_callback() swaps under the new mutex
• the worker re-reads m_post_callback under it at fire time — no stale local copy

Guarantee: once set_backup_callback(nullptr) returns, the worker can never invoke an old callback. Lock order m_mutex -> m_post_callback_mutex is documented at the member; m_mutex critical sections are otherwise unchanged (the callback was already invoked under it). The UI-thread read in process_ui_task() (callback(1)) is untouched — same-thread with the only writer, no race there.

Verification

A/B tested on Windows (MSVC 2022, RelWithDebInfo) with a test-only build aid: a 2.5 s sleep in the worker's post-task window plus error-level logging at the callback swap/invoke sites (never committed). Quit during a backup cycle, 5 trials per build:

• pre-fix: set_backup_callback(nullptr) landed mid-window in 4/5 runs; in every one the worker invoked the stale callback 150–540 ms after the swap, entering the lambda that captures the dying MainFrame (logged). Across runs this manifested as silent invalid calls, hangs at exit, and abnormal exit codes — the same UAF Sentry reports as EXC_BAD_ACCESS on macOS.
• fixed: identical interleaving (swap mid-window 5/5), but the worker re-read m_post_callback as null under the new mutex in 5/5 and skipped the call — zero invocations after the swap, no hangs or crashes, all exits clean.

Sentry cross-check: the issue's newest event stack matches the fixed path frame-by-frame (worker → std::function → MainFrame lambda → wxPostEvent).

[zhangzhend0ng]

Backup UI task stall — m_ui_tasks invariant broken after shutdown (src/libslic3r/Format/bbs_3mf.cpp, the m_ui_tasks.size() == 1 branch in operator()())

EVT_BACKUP_POST only fires when m_ui_tasks.size() == 1. After shutdown() nulls the callback, the worker skips callback(0) for the first queued task, leaving the queue at size ≥ 1. Once a language switch recreates MainFrame (GUI_App::recreate_GUI → MainFrame::shutdown(true), GUI_App.cpp:3921) and installs a new callback, every subsequent push makes size ≥ 2, so the condition never holds again → EVT_BACKUP_POST never fires → run_backup_ui_tasks() never runs.

This is not idle UI bookkeeping — it is the main path of the auto-backup pipeline:

• edits (Plater.cpp:11192, backup_soon()) or the interval timer schedule a Backup task; the worker's process_task(Backup) is a no-op ("do it in response") that only forwards it into m_ui_tasks;
• the only place a backup .3mf is actually written is MainFrame.cpp:604, export_3mf(..., SaveStrategy::Backup) inside callback(1) — reached exclusively via this event chain (grep: sole SaveStrategy::Backup call site in the codebase).

So after a language switch, project auto-backup stops writing to disk entirely (and m_temp_model clone cleanup plus RemoveBackup directory cleanup stall as well) until remove_backup(removeAll) happens to run or the process exits. The stall window requires a task to land in m_ui_tasks between the null-swap and the new callback install — real in practice, since the worker's process_task does file I/O and can straddle that window.

Pre-fix, this same window manifested as the UAF crash; this fix trades the crash for the stall, which is the right direction — but the invariant (first queued task ⇒ notification) is left incomplete.

[zhangzhend0ng]

VERDICT: APPROVE_WITH_COMMENTS

Findings

#	Location	Severity	Issue & suggested fix
1	bbs_3mf.cpp, set_post_callback() catch-up block (first hunk, after assigning m_post_callback)	QUESTION	The new catch-up calls c(0) while holding m_mutex. c is an arbitrary std::function — likely UI/wx notification code — and calling user code under a lock violates the thread-safety “never call user code while holding a lock” rule. If c directly or indirectly re-enters _BBS_Backup_Manager (e.g. calls run_ui_tasks() or set_post_callback()), it will self-deadlock on the non-recursive boost::mutex. Suggested fix: capture bool should_notify = !m_ui_tasks.empty(); under m_mutex, release the lock, then invoke c(0) outside the critical section.
2	bbs_3mf.cpp, operator()() notification block (second hunk, if (m_ui_tasks.size() == 1) { ... m_post_callback(0); })	QUESTION	The modified code still invokes m_post_callback(0) while holding m_mutex, and now additionally holds m_post_callback_mutex for the duration of the call. Holding two locks across an arbitrary callback increases reentrancy/deadlock risk. If the callback tries to call set_post_callback() or anything that acquires m_post_callback_mutex, it deadlocks. Suggested fix: copy the callback under the callback mutex, release both locks, then invoke the copy outside the lock scope.
3	bbs_3mf.cpp, set_post_callback() catch-up (first hunk)	QUESTION	The catch-up notification is not de-duplicated against operator()’s existing size()==1 notification. Interleaving where the worker pushes the first m_ui_tasks entry and main calls set_post_callback(c) before the worker takes m_post_callback_mutex can cause both threads to invoke c(0). If the callback is not idempotent, this posts duplicate UI events or runs UI tasks twice. Suggested fix: track a m_ui_tasks_notify_pending flag or perform catch-up only when transitioning from a null callback to non-null under a common lock.
4	bbs_3mf.cpp, new member boost::mutex m_post_callback_mutex; (third hunk)	MINOR	SnapmakerOrca coding standards item 9 says std::mutex + std::lock_guard only. This adds a new boost::mutex and uses boost::lock_guard. If the module has an existing Boost synchronization exception, document it; otherwise use std::mutex / std::lock_guard for new members.

Checklist coverage

Applied: cpp/concurrency/thread-safety.md, projects/snapmaker-orca/coding-standards.md, cpp/functions/parameter-validation.md, cpp/correctness/type-safety.md, cpp/correctness/undefined-behavior.md, cpp/memory/raii.md, format-io module checklist.

N/A or no-op for this diff: ABI compatibility, layering/dependency inversion, compile-time programming, templates, STL algorithms/containers, Qt6/QML, direct wxWidgets API usage (no wx calls appear in the diff itself), performance optimization, parsing/validation, format strings, integer safety beyond the callback boolean check.

Action items

• Move the catch-up callback invocation outside m_mutex in set_post_callback().
• Re-evaluate invoking m_post_callback(0) while holding both m_mutex and m_post_callback_mutex in operator()(); prefer copy-then-invoke after unlocking.
• Add de-duplication or document idempotency for catch-up notifications to avoid duplicate c(0) calls.
• Confirm boost::mutex vs std::mutex choice for the new mutex against current project conventions.
• Run relevant backup/shutdown race tests and TSan coverage if available.

Limitations: full operator()() and run_ui_tasks() bodies and the actual m_post_callback implementation were not provided, so callback reentrancy and UI-event idempotency could not be verified from the diff alone.

---

AI 评审（advisory），由 dev-guidelines harness 流水线生成 · head d4e268e5

[zhangzhend0ng]

VERDICT: APPROVE_WITH_COMMENTS

Findings

#	Location	Severity	Issue & suggested fix	Evidence
1	src/libslic3r/Format/bbs_3mf.cpp @H1	QUESTION	The catch-up path in set_post_callback can produce duplicate m_post_callback(0) invocations. It sets the callback under m_post_callback_mutex, then separately checks m_ui_tasks under m_mutex and calls c(0). Concurrently, the worker may have already pushed the task and be waiting on m_post_callback_mutex; after set_post_callback releases it, the worker also calls m_post_callback(0). Is the callback guaranteed idempotent? If not, double-scheduling may occur. Suggested: document/verify idempotency, or centralize notification so only one path fires for the same pending transition.	pending = !m_ui_tasks.empty(); ... if (pending) c(0); // outside m_mutex: only this thread drains m_ui_tasks
2	src/libslic3r/Format/bbs_3mf.cpp @h2	QUESTION	Worker now invokes m_post_callback(0) while holding both m_mutex and m_post_callback_mutex. If the callback can re-enter the manager (e.g., calls set_post_callback or run_ui_tasks), this will deadlock on the non-recursive mutex. This re-entrancy risk existed before with m_mutex, but the new second mutex widens the critical section and is not documented. Suggested: state the non-reentrancy contract explicitly, or invoke the callback outside the locks (capturing the callback by value) if safe.	if (m_ui_tasks.size() == 1) { ... boost::lock_guard post_lock(m_post_callback_mutex); ... if (m_post_callback) m_post_callback(0);

Checklist coverage

Applied:

• cpp/concurrency/thread-safety — data-race and deadlock review on m_post_callback / m_ui_tasks.
• projects/snapmaker-orca/coding-standards — formatting, brace style, C++17, no exceptions (diff conforms).
• projects/snapmaker-orca/AGENTS.md inherited harnesses — concurrency, type safety, RAII spot checks.

N/A for this diff:

• cpp/correctness/integer-safety, format-strings, parsing-and-validation, class-hierarchies, templates, qt6-*, wxwidgets-3-1-5 (no such constructs in diff).
• cpp/architecture/* — no new subsystem/layering changes.

Action items

• Verify m_post_callback is safe to call from both set_post_callback catch-up and worker notification without duplicate side effects.
• Document that m_post_callback must not call back into _BBS_Backup_Manager methods (set_post_callback, run_ui_tasks) while invoked from the worker under the two mutexes.
• Consider consolidating notification to avoid the duplicate-callback race noted in Finding 1.
• If not already present, add a full-file search for any other direct reads/writes of m_post_callback that still use only m_mutex, which would now race with this change.

Limitations

Only the provided diff was reviewed; the full bbs_3mf.cpp was not available, so other m_post_callback accesses, destructor/join logic, and the exact callback semantics are unverified. The duplicate-notification concern is speculative and depends on the callback implementation.

---

AI 评审（advisory），由 dev-guidelines harness 流水线生成 · head 5df83874