Confine plugin lifecycle callbacks to UI thread

Drop the orphaned PluginCallbackList (dead
after the PluginManager migration) and hop
run_on_*_callbacks onto the UI thread via
CallAfter, snapshotting under the mutex on the
worker first. Keeps wx subscribers off the
detached load/unload workers.

src/slic3r/plugin/PluginCallbackList.hpp

@@ -1,44 +0,0 @@
-#pragma once
-
-#include <functional>
-#include <memory>
-#include <mutex>
-#include <utility>
-#include <vector>
-
-namespace Slic3r {
-
-// Thread-safe append-only callback storage. Each dispatch operates on a snapshot,
-// so callbacks may safely subscribe more callbacks without deadlocking or changing
-// the in-flight dispatch.
-template<typename Signature> class PluginCallbackList
-{
-public:
-    using Callback = std::function<Signature>;
-    using CallbackPtr = std::shared_ptr<Callback>;
-
-    void subscribe(Callback callback)
-    {
-        auto owned_callback = std::make_shared<Callback>(std::move(callback));
-        std::lock_guard<std::mutex> lock(m_mutex);
-        m_callbacks.emplace_back(std::move(owned_callback));
-    }
-
-    template<typename Dispatcher> void dispatch(Dispatcher&& dispatcher) const
-    {
-        std::vector<CallbackPtr> callbacks;
-        {
-            std::lock_guard<std::mutex> lock(m_mutex);
-            callbacks = m_callbacks;
-        }
-
-        for (const CallbackPtr& callback : callbacks)
-            dispatcher(*callback);
-    }
-
-private:
-    mutable std::mutex       m_mutex;
-    std::vector<CallbackPtr> m_callbacks;
-};
-
-} // namespace Slic3r

src/slic3r/plugin/PluginManager.cpp

@@ -1098,61 +1098,88 @@ std::vector<PluginManager::CapabilityLifecycleFn> PluginManager::copy_capability
     return it == m_capability_callbacks.end() ? std::vector<CapabilityLifecycleFn>{} : it->second;
 }
 
-// The run_on_*_callbacks below are called from detached load/unload workers. They copy the
+// The run_on_*_callbacks below are called from detached load/unload workers. They snapshot the
-// subscriber list under m_mutex and invoke it OUTSIDE the lock: subscribers re-enter the manager,
+// subscriber list under m_mutex (the copy protects the in-flight loop from a callback that
-// and no callback may run while the registry lock is held.
+// subscribes mid-dispatch, and serialises against a concurrent subscribe), then hand the snapshot
+// to the UI thread: subscribers touch wx and must not run on a worker, and none may run while the
+// registry lock is held. The dispatch runs inline when already on the UI thread, else via
+// CallAfter, since calling wx off the main thread is UB.
 void PluginManager::run_on_load_callbacks(const std::string& plugin_key)
 {
-    for (auto& fn : copy_callbacks(CallbackType::Load)) {
+    auto work = [callbacks = copy_callbacks(CallbackType::Load), plugin_key] {
-        try {
+        for (auto& fn : callbacks) {
-            fn(plugin_key);
+            try {
-        } catch (const std::exception& ex) {
+                fn(plugin_key);
-            BOOST_LOG_TRIVIAL(error) << "Plugin load completion callback failed for " << plugin_key << ": " << ex.what();
+            } catch (const std::exception& ex) {
-        } catch (...) {
+                BOOST_LOG_TRIVIAL(error) << "Plugin load completion callback failed for " << plugin_key << ": " << ex.what();
-            BOOST_LOG_TRIVIAL(error) << "Plugin load completion callback failed for " << plugin_key;
+            } catch (...) {
+                BOOST_LOG_TRIVIAL(error) << "Plugin load completion callback failed for " << plugin_key;
+            }
         }
-    }
+    };
+    if (wxTheApp == nullptr || wxIsMainThread())
+        work();
+    else
+        GUI::wxGetApp().CallAfter(std::move(work));
 }
 
 void PluginManager::run_on_unload_callbacks(const std::string& plugin_key)
 {
-    for (auto& fn : copy_callbacks(CallbackType::Unload)) {
+    auto work = [callbacks = copy_callbacks(CallbackType::Unload), plugin_key] {
-        try {
+        for (auto& fn : callbacks) {
-            fn(plugin_key);
+            try {
-        } catch (const std::exception& ex) {
+                fn(plugin_key);
-            BOOST_LOG_TRIVIAL(error) << "Plugin unload completion callback failed for " << plugin_key << ": " << ex.what();
+            } catch (const std::exception& ex) {
-        } catch (...) {
+                BOOST_LOG_TRIVIAL(error) << "Plugin unload completion callback failed for " << plugin_key << ": " << ex.what();
-            BOOST_LOG_TRIVIAL(error) << "Plugin unload completion callback failed for " << plugin_key << ": unknown error";
+            } catch (...) {
+                BOOST_LOG_TRIVIAL(error) << "Plugin unload completion callback failed for " << plugin_key << ": unknown error";
+            }
         }
-    }
+    };
+    if (wxTheApp == nullptr || wxIsMainThread())
+        work();
+    else
+        GUI::wxGetApp().CallAfter(std::move(work));
 }
 
 void PluginManager::run_on_capability_load_callbacks(const PluginCapabilityId& id)
 {
-    for (auto& fn : copy_capability_callbacks(CallbackType::Load)) {
+    auto work = [callbacks = copy_capability_callbacks(CallbackType::Load), id] {
-        try {
+        for (auto& fn : callbacks) {
-            fn(id);
+            try {
-        } catch (const std::exception& ex) {
+                fn(id);
-            BOOST_LOG_TRIVIAL(error) << "Plugin capability load callback failed for " << id.plugin_key << "/" << id.name << ": "
+            } catch (const std::exception& ex) {
-                                     << ex.what();
+                BOOST_LOG_TRIVIAL(error) << "Plugin capability load callback failed for " << id.plugin_key << "/" << id.name << ": "
-        } catch (...) {
+                                         << ex.what();
-            BOOST_LOG_TRIVIAL(error) << "Plugin capability load callback failed for " << id.plugin_key << "/" << id.name;
+            } catch (...) {
+                BOOST_LOG_TRIVIAL(error) << "Plugin capability load callback failed for " << id.plugin_key << "/" << id.name;
+            }
         }
-    }
+    };
+    if (wxTheApp == nullptr || wxIsMainThread())
+        work();
+    else
+        GUI::wxGetApp().CallAfter(std::move(work));
 }
 
 void PluginManager::run_on_capability_unload_callbacks(const PluginCapabilityId& id)
 {
-    for (auto& fn : copy_capability_callbacks(CallbackType::Unload)) {
+    auto work = [callbacks = copy_capability_callbacks(CallbackType::Unload), id] {
-        try {
+        for (auto& fn : callbacks) {
-            fn(id);
+            try {
-        } catch (const std::exception& ex) {
+                fn(id);
-            BOOST_LOG_TRIVIAL(error) << "Plugin capability unload callback failed for " << id.plugin_key << "/" << id.name << ": "
+            } catch (const std::exception& ex) {
-                                     << ex.what();
+                BOOST_LOG_TRIVIAL(error) << "Plugin capability unload callback failed for " << id.plugin_key << "/" << id.name << ": "
-        } catch (...) {
+                                         << ex.what();
-            BOOST_LOG_TRIVIAL(error) << "Plugin capability unload callback failed for " << id.plugin_key << "/" << id.name;
+            } catch (...) {
+                BOOST_LOG_TRIVIAL(error) << "Plugin capability unload callback failed for " << id.plugin_key << "/" << id.name;
+            }
         }
-    }
+    };
+    if (wxTheApp == nullptr || wxIsMainThread())
+        work();
+    else
+        GUI::wxGetApp().CallAfter(std::move(work));
 }
 
 // ── Cloud user ──────────────────────────────────────────────────────────────────────────────

tests/slic3rutils/CMakeLists.txt

@@ -3,7 +3,6 @@ add_executable(${_TEST_NAME}_tests
     ${_TEST_NAME}_tests_main.cpp
     test_action_source.cpp
     test_plugin_host_api.cpp
-    test_plugin_callback_list.cpp
     test_plugin_install.cpp
     test_plugin_lifecycle.cpp
     test_slicing_pipeline_bindings.cpp

tests/slic3rutils/test_plugin_callback_list.cpp

@@ -1,105 +0,0 @@
-#include <catch2/catch_all.hpp>
-
-#include <slic3r/plugin/PluginCallbackList.hpp>
-
-#include <atomic>
-#include <chrono>
-#include <condition_variable>
-#include <future>
-#include <mutex>
-#include <string>
-#include <vector>
-
-using Slic3r::PluginCallbackList;
-
-TEST_CASE("PluginCallbackList dispatches subscribers in subscription order", "[plugin][callback-list]")
-{
-    PluginCallbackList<void()> callbacks;
-    std::vector<int>            calls;
-
-    callbacks.subscribe([&calls] { calls.push_back(1); });
-    callbacks.subscribe([&calls] { calls.push_back(2); });
-
-    callbacks.dispatch([](const auto& callback) { callback(); });
-
-    CHECK(calls == std::vector<int>{1, 2});
-}
-
-TEST_CASE("PluginCallbackList defers subscribers added during dispatch", "[plugin][callback-list]")
-{
-    PluginCallbackList<void()> callbacks;
-    std::vector<std::string>   calls;
-
-    callbacks.subscribe([&] {
-        calls.emplace_back("original");
-        callbacks.subscribe([&calls] { calls.emplace_back("added"); });
-    });
-
-    callbacks.dispatch([](const auto& callback) { callback(); });
-    CHECK(calls == std::vector<std::string>{"original"});
-
-    callbacks.dispatch([](const auto& callback) { callback(); });
-    CHECK(calls == std::vector<std::string>{"original", "original", "added"});
-}
-
-TEST_CASE("PluginCallbackList preserves mutable callback state across dispatches", "[plugin][callback-list]")
-{
-    PluginCallbackList<void()> callbacks;
-    std::vector<int>            calls;
-
-    callbacks.subscribe([count = 0, &calls]() mutable { calls.push_back(++count); });
-
-    callbacks.dispatch([](const auto& callback) { callback(); });
-    callbacks.dispatch([](const auto& callback) { callback(); });
-
-    CHECK(calls == std::vector<int>{1, 2});
-}
-
-TEST_CASE("PluginCallbackList does not hold its mutex while invoking callbacks", "[plugin][callback-list]")
-{
-    using namespace std::chrono_literals;
-
-    PluginCallbackList<void()> callbacks;
-    std::mutex                 mutex;
-    std::condition_variable    callback_entered_cv;
-    std::condition_variable    release_callback_cv;
-    bool                       callback_entered = false;
-    bool                       release_callback = false;
-    std::atomic<int>           late_calls{0};
-
-    callbacks.subscribe([&] {
-        std::unique_lock<std::mutex> lock(mutex);
-        callback_entered = true;
-        callback_entered_cv.notify_one();
-        release_callback_cv.wait(lock, [&release_callback] { return release_callback; });
-    });
-
-    auto dispatch = std::async(std::launch::async, [&] {
-        callbacks.dispatch([](const auto& callback) { callback(); });
-    });
-
-    {
-        std::unique_lock<std::mutex> lock(mutex);
-        callback_entered_cv.wait_for(lock, 5s, [&callback_entered] { return callback_entered; });
-    }
-
-    auto subscribe = std::async(std::launch::async, [&] {
-        callbacks.subscribe([&late_calls] { ++late_calls; });
-    });
-    const bool subscribed_while_callback_blocked = subscribe.wait_for(5s) == std::future_status::ready;
-
-    {
-        std::lock_guard<std::mutex> lock(mutex);
-        release_callback = true;
-    }
-    release_callback_cv.notify_one();
-
-    dispatch.get();
-    subscribe.get();
-    CHECK(callback_entered);
-    CHECK(subscribed_while_callback_blocked);
-    CHECK(late_calls.load() == 0);
-
-    callbacks.dispatch([](const auto& callback) { callback(); });
-    CHECK(late_calls.load() == 1);
-}