HookSystem: improve callback safety

hyprwm · Apr 20, 2024 · 4ad739e · 4ad739e
1 parent 1055e6b
commit 4ad739e
Show file tree

Hide file tree

Showing 18 changed files with 89 additions and 105 deletions.
diff --git a/src/debug/HyprNotificationOverlay.cpp b/src/debug/HyprNotificationOverlay.cpp
@@ -3,7 +3,7 @@
 #include <pango/pangocairo.h>
 
 CHyprNotificationOverlay::CHyprNotificationOverlay() {
-    g_pHookSystem->hookDynamic("focusedMon", [&](void* self, SCallbackInfo& info, std::any param) {
+    static auto P = g_pHookSystem->hookDynamic("focusedMon", [&](void* self, SCallbackInfo& info, std::any param) {
         if (m_dNotifications.size() == 0)
             return;
 

diff --git a/src/desktop/Workspace.hpp b/src/desktop/Workspace.hpp
@@ -78,11 +78,11 @@ class CWorkspace {
     void        markInert();
 
   private:
-    void                      init(PHLWORKSPACE self);
+    void                              init(PHLWORKSPACE self);
 
-    HOOK_CALLBACK_FN*         m_pFocusedWindowHook = nullptr;
-    bool                      m_bInert             = true;
-    std::weak_ptr<CWorkspace> m_pSelf;
+    std::shared_ptr<HOOK_CALLBACK_FN> m_pFocusedWindowHook;
+    bool                              m_bInert = true;
+    std::weak_ptr<CWorkspace>         m_pSelf;
 };
 
 inline bool valid(const PHLWORKSPACE& ref) {

diff --git a/src/helpers/WLClasses.hpp b/src/helpers/WLClasses.hpp
@@ -270,9 +270,9 @@ struct STabletPad {
 };
 
 struct SIdleInhibitor {
-    wlr_idle_inhibitor_v1* pWlrInhibitor   = nullptr;
-    CWindow*               pWindow         = nullptr;
-    HOOK_CALLBACK_FN*      onWindowDestroy = nullptr;
+    wlr_idle_inhibitor_v1*            pWlrInhibitor = nullptr;
+    CWindow*                          pWindow       = nullptr;
+    std::shared_ptr<HOOK_CALLBACK_FN> onWindowDestroy;
 
     DYNLISTENER(Destroy);
 

diff --git a/src/hyprerror/HyprError.cpp b/src/hyprerror/HyprError.cpp
@@ -6,15 +6,15 @@ CHyprError::CHyprError() {
     m_fFadeOpacity.create(AVARTYPE_FLOAT, g_pConfigManager->getAnimationPropertyConfig("fadeIn"), AVARDAMAGE_NONE);
     m_fFadeOpacity.registerVar();
 
-    g_pHookSystem->hookDynamic("focusedMon", [&](void* self, SCallbackInfo& info, std::any param) {
+    static auto P = g_pHookSystem->hookDynamic("focusedMon", [&](void* self, SCallbackInfo& info, std::any param) {
         if (!m_bIsCreated)
             return;
 
         g_pHyprRenderer->damageMonitor(g_pCompositor->m_pLastMonitor);
         m_bMonitorChanged = true;
     });
 
-    g_pHookSystem->hookDynamic("preRender", [&](void* self, SCallbackInfo& info, std::any param) {
+    static auto P2 = g_pHookSystem->hookDynamic("preRender", [&](void* self, SCallbackInfo& info, std::any param) {
         if (!m_bIsCreated)
             return;
 

diff --git a/src/managers/CursorManager.cpp b/src/managers/CursorManager.cpp
@@ -50,7 +50,7 @@ CCursorManager::CCursorManager() {
 
     updateTheme();
 
-    g_pHookSystem->hookDynamic("monitorLayoutChanged", [this](void* self, SCallbackInfo& info, std::any param) { this->updateTheme(); });
+    static auto P = g_pHookSystem->hookDynamic("monitorLayoutChanged", [this](void* self, SCallbackInfo& info, std::any param) { this->updateTheme(); });
 }
 
 void CCursorManager::dropBufferRef(CCursorManager::CCursorBuffer* ref) {

diff --git a/src/managers/HookSystemManager.cpp b/src/managers/HookSystemManager.cpp
@@ -7,38 +7,40 @@ CHookSystemManager::CHookSystemManager() {
 }
 
 // returns the pointer to the function
-HOOK_CALLBACK_FN* CHookSystemManager::hookDynamic(const std::string& event, HOOK_CALLBACK_FN fn, HANDLE handle) {
-    const auto PVEC = getVecForEvent(event);
-    const auto PFN  = &m_lCallbackFunctions.emplace_back(fn);
-    PVEC->emplace_back(SCallbackFNPtr{PFN, handle});
-    return PFN;
+std::shared_ptr<HOOK_CALLBACK_FN> CHookSystemManager::hookDynamic(const std::string& event, HOOK_CALLBACK_FN fn, HANDLE handle) {
+    std::shared_ptr<HOOK_CALLBACK_FN> hookFN = std::make_shared<HOOK_CALLBACK_FN>(fn);
+    m_mRegisteredHooks[event].emplace_back(SCallbackFNPtr{.fn = hookFN, .handle = handle});
+    return hookFN;
 }
 
-void CHookSystemManager::hookStatic(const std::string& event, HOOK_CALLBACK_FN* fn, HANDLE handle) {
-    const auto PVEC = getVecForEvent(event);
-    PVEC->emplace_back(SCallbackFNPtr{fn, handle});
-}
+void CHookSystemManager::unhook(std::shared_ptr<HOOK_CALLBACK_FN> fn) {
+    for (auto& [k, v] : m_mRegisteredHooks) {
+        std::erase_if(v, [&](const auto& other) {
+            std::shared_ptr<HOOK_CALLBACK_FN> fn_ = other.fn.lock();
 
-void CHookSystemManager::unhook(HOOK_CALLBACK_FN* fn) {
-    std::erase_if(m_lCallbackFunctions, [&](const auto& other) { return &other == fn; });
-    for (auto& [k, v] : m_lpRegisteredHooks) {
-        std::erase_if(v, [&](const auto& other) { return other.fn == fn; });
+            return fn_.get() == fn.get();
+        });
     }
 }
 
-void CHookSystemManager::emit(const std::vector<SCallbackFNPtr>* callbacks, SCallbackInfo& info, std::any data) {
+void CHookSystemManager::emit(std::vector<SCallbackFNPtr>* const callbacks, SCallbackInfo& info, std::any data) {
     if (callbacks->empty())
         return;
 
     std::vector<HANDLE> faultyHandles;
+    bool                needsDeadCleanup = false;
 
     for (auto& cb : *callbacks) {
 
         m_bCurrentEventPlugin = false;
 
         if (!cb.handle) {
             // we don't guard hl hooks
-            (*cb.fn)(cb.fn, info, data);
+
+            if (std::shared_ptr<HOOK_CALLBACK_FN> fn = cb.fn.lock())
+                (*fn)(fn.get(), info, data);
+            else
+                needsDeadCleanup = true;
             continue;
         }
 
@@ -48,9 +50,12 @@ void CHookSystemManager::emit(const std::vector<SCallbackFNPtr>* callbacks, SCal
             continue;
 
         try {
-            if (!setjmp(m_jbHookFaultJumpBuf))
-                (*cb.fn)(cb.fn, info, data);
-            else {
+            if (!setjmp(m_jbHookFaultJumpBuf)) {
+                if (std::shared_ptr<HOOK_CALLBACK_FN> fn = cb.fn.lock())
+                    (*fn)(fn.get(), info, data);
+                else
+                    needsDeadCleanup = true;
+            } else {
                 // this module crashed.
                 throw std::exception();
             }
@@ -61,19 +66,18 @@ void CHookSystemManager::emit(const std::vector<SCallbackFNPtr>* callbacks, SCal
         }
     }
 
+    if (needsDeadCleanup)
+        std::erase_if(*callbacks, [](const auto& fn) { return !fn.fn.lock(); });
+
     if (!faultyHandles.empty()) {
         for (auto& h : faultyHandles)
             g_pPluginSystem->unloadPlugin(g_pPluginSystem->getPluginByHandle(h), true);
     }
 }
 
 std::vector<SCallbackFNPtr>* CHookSystemManager::getVecForEvent(const std::string& event) {
-    auto IT = std::find_if(m_lpRegisteredHooks.begin(), m_lpRegisteredHooks.end(), [&](const auto& other) { return other.first == event; });
-
-    if (IT != m_lpRegisteredHooks.end())
-        return &IT->second;
-
-    Debug::log(LOG, "[hookSystem] New hook event registered: {}", event);
+    if (!m_mRegisteredHooks.contains(event))
+        Debug::log(LOG, "[hookSystem] New hook event registered: {}", event);
 
-    return &m_lpRegisteredHooks.emplace_back(std::make_pair<>(event, std::vector<SCallbackFNPtr>{})).second;
+    return &m_mRegisteredHooks[event];
 }
diff --git a/src/managers/HookSystemManager.hpp b/src/managers/HookSystemManager.hpp
@@ -16,8 +16,8 @@
 typedef std::function<void(void*, SCallbackInfo& info, std::any data)> HOOK_CALLBACK_FN;
 
 struct SCallbackFNPtr {
-    HOOK_CALLBACK_FN* fn     = nullptr;
-    HANDLE            handle = nullptr;
+    std::weak_ptr<HOOK_CALLBACK_FN> fn;
+    HANDLE                          handle = nullptr;
 };
 
 #define EMIT_HOOK_EVENT(name, param)                                                                                                                                               \
@@ -40,21 +40,21 @@ class CHookSystemManager {
   public:
     CHookSystemManager();
 
-    // returns the pointer to the function
-    HOOK_CALLBACK_FN*            hookDynamic(const std::string& event, HOOK_CALLBACK_FN fn, HANDLE handle = nullptr);
-    void                         hookStatic(const std::string& event, HOOK_CALLBACK_FN* fn, HANDLE handle = nullptr);
-    void                         unhook(HOOK_CALLBACK_FN* fn);
+    // returns the pointer to the function.
+    // losing this pointer (letting it get destroyed)
+    // will equal to unregistering the callback.
+    [[nodiscard("Losing this pointer instantly unregisters the callback")]] std::shared_ptr<HOOK_CALLBACK_FN> hookDynamic(const std::string& event, HOOK_CALLBACK_FN fn,
+                                                                                                                          HANDLE handle = nullptr);
+    void                                                                                                      unhook(std::shared_ptr<HOOK_CALLBACK_FN> fn);
 
-    void                         emit(const std::vector<SCallbackFNPtr>* callbacks, SCallbackInfo& info, std::any data = 0);
+    void                         emit(std::vector<SCallbackFNPtr>* const callbacks, SCallbackInfo& info, std::any data = 0);
     std::vector<SCallbackFNPtr>* getVecForEvent(const std::string& event);
 
     bool                         m_bCurrentEventPlugin = false;
     jmp_buf                      m_jbHookFaultJumpBuf;
 
   private:
-    // todo: this is slow. Maybe static ptrs should be somehow allowed. unique ptr for vec?
-    std::list<std::pair<std::string, std::vector<SCallbackFNPtr>>> m_lpRegisteredHooks;
-    std::list<HOOK_CALLBACK_FN>                                    m_lCallbackFunctions;
+    std::unordered_map<std::string, std::vector<SCallbackFNPtr>> m_mRegisteredHooks;
 };
 
 inline std::unique_ptr<CHookSystemManager> g_pHookSystem;
diff --git a/src/managers/KeybindManager.cpp b/src/managers/KeybindManager.cpp
@@ -85,7 +85,7 @@ CKeybindManager::CKeybindManager() {
 
     m_tScrollTimer.reset();
 
-    g_pHookSystem->hookDynamic("configReloaded", [this](void* hk, SCallbackInfo& info, std::any param) {
+    static auto P = g_pHookSystem->hookDynamic("configReloaded", [this](void* hk, SCallbackInfo& info, std::any param) {
         // clear cuz realloc'd
         m_pActiveKeybind = nullptr;
         m_vPressedSpecialBinds.clear();

diff --git a/src/managers/input/InputMethodRelay.cpp b/src/managers/input/InputMethodRelay.cpp
@@ -3,7 +3,7 @@
 #include "../../Compositor.hpp"
 
 CInputMethodRelay::CInputMethodRelay() {
-    g_pHookSystem->hookDynamic("keyboardFocus", [&](void* self, SCallbackInfo& info, std::any param) { onKeyboardFocus(std::any_cast<wlr_surface*>(param)); });
+    static auto P = g_pHookSystem->hookDynamic("keyboardFocus", [&](void* self, SCallbackInfo& info, std::any param) { onKeyboardFocus(std::any_cast<wlr_surface*>(param)); });
 }
 
 void CInputMethodRelay::onNewIME(wlr_input_method_v2* pIME) {

diff --git a/src/plugins/PluginAPI.cpp b/src/plugins/PluginAPI.cpp
@@ -13,30 +13,18 @@ APICALL const char* __hyprland_api_get_hash() {
     return GIT_COMMIT_HASH;
 }
 
-APICALL bool HyprlandAPI::registerCallbackStatic(HANDLE handle, const std::string& event, HOOK_CALLBACK_FN* fn) {
-    auto* const PLUGIN = g_pPluginSystem->getPluginByHandle(handle);
-
-    if (!PLUGIN)
-        return false;
-
-    g_pHookSystem->hookStatic(event, fn, handle);
-    PLUGIN->registeredCallbacks.emplace_back(std::make_pair<>(event, fn));
-
-    return true;
-}
-
-APICALL HOOK_CALLBACK_FN* HyprlandAPI::registerCallbackDynamic(HANDLE handle, const std::string& event, HOOK_CALLBACK_FN fn) {
+APICALL std::shared_ptr<HOOK_CALLBACK_FN> HyprlandAPI::registerCallbackDynamic(HANDLE handle, const std::string& event, HOOK_CALLBACK_FN fn) {
     auto* const PLUGIN = g_pPluginSystem->getPluginByHandle(handle);
 
     if (!PLUGIN)
         return nullptr;
 
-    auto* const PFN = g_pHookSystem->hookDynamic(event, fn, handle);
+    auto PFN = g_pHookSystem->hookDynamic(event, fn, handle);
     PLUGIN->registeredCallbacks.emplace_back(std::make_pair<>(event, PFN));
     return PFN;
 }
 
-APICALL bool HyprlandAPI::unregisterCallback(HANDLE handle, HOOK_CALLBACK_FN* fn) {
+APICALL bool HyprlandAPI::unregisterCallback(HANDLE handle, std::shared_ptr<HOOK_CALLBACK_FN> fn) {
     auto* const PLUGIN = g_pPluginSystem->getPluginByHandle(handle);
 
     if (!PLUGIN)

diff --git a/src/plugins/PluginAPI.hpp b/src/plugins/PluginAPI.hpp
@@ -132,28 +132,20 @@ namespace HyprlandAPI {
     */
     APICALL Hyprlang::CConfigValue* getConfigValue(HANDLE handle, const std::string& name);
 
-    /*
-        Register a static (pointer) callback to a selected event.
-        Pointer must be kept valid until unregisterCallback() is called.
-
-        returns: true on success, false on fail
-    */
-    APICALL bool registerCallbackStatic(HANDLE handle, const std::string& event, HOOK_CALLBACK_FN* fn);
-
     /*
         Register a dynamic (function) callback to a selected event.
         Pointer will be free'd by Hyprland on unregisterCallback().
 
         returns: a pointer to the newly allocated function. nullptr on fail.
     */
-    APICALL HOOK_CALLBACK_FN* registerCallbackDynamic(HANDLE handle, const std::string& event, HOOK_CALLBACK_FN fn);
+    APICALL std::shared_ptr<HOOK_CALLBACK_FN> registerCallbackDynamic(HANDLE handle, const std::string& event, HOOK_CALLBACK_FN fn);
 
     /*
         Unregisters a callback. If the callback was dynamic, frees the memory.
 
         returns: true on success, false on fail
     */
-    APICALL bool unregisterCallback(HANDLE handle, HOOK_CALLBACK_FN* fn);
+    APICALL bool unregisterCallback(HANDLE handle, std::shared_ptr<HOOK_CALLBACK_FN> fn);
 
     /*
         Calls a hyprctl command.

diff --git a/src/plugins/PluginSystem.hpp b/src/plugins/PluginSystem.hpp
@@ -8,22 +8,22 @@ class IHyprWindowDecoration;
 
 class CPlugin {
   public:
-    std::string                                            name        = "";
-    std::string                                            description = "";
-    std::string                                            author      = "";
-    std::string                                            version     = "";
+    std::string                                                            name        = "";
+    std::string                                                            description = "";
+    std::string                                                            author      = "";
+    std::string                                                            version     = "";
 
-    std::string                                            path = "";
+    std::string                                                            path = "";
 
-    bool                                                   m_bLoadedWithConfig = false;
+    bool                                                                   m_bLoadedWithConfig = false;
 
-    HANDLE                                                 m_pHandle = nullptr;
+    HANDLE                                                                 m_pHandle = nullptr;
 
-    std::vector<IHyprLayout*>                              registeredLayouts;
-    std::vector<IHyprWindowDecoration*>                    registeredDecorations;
-    std::vector<std::pair<std::string, HOOK_CALLBACK_FN*>> registeredCallbacks;
-    std::vector<std::string>                               registeredDispatchers;
-    std::vector<std::shared_ptr<SHyprCtlCommand>>          registeredHyprctlCommands;
+    std::vector<IHyprLayout*>                                              registeredLayouts;
+    std::vector<IHyprWindowDecoration*>                                    registeredDecorations;
+    std::vector<std::pair<std::string, std::shared_ptr<HOOK_CALLBACK_FN>>> registeredCallbacks;
+    std::vector<std::string>                                               registeredDispatchers;
+    std::vector<std::shared_ptr<SHyprCtlCommand>>                          registeredHyprctlCommands;
 };
 
 class CPluginSystem {

diff --git a/src/protocols/Screencopy.hpp b/src/protocols/Screencopy.hpp
@@ -20,21 +20,21 @@ class CScreencopyClient {
     CScreencopyClient();
     ~CScreencopyClient();
 
-    int               ref      = 0;
-    wl_resource*      resource = nullptr;
+    int                               ref      = 0;
+    wl_resource*                      resource = nullptr;
 
-    eClientOwners     clientOwner = CLIENT_SCREENCOPY;
+    eClientOwners                     clientOwner = CLIENT_SCREENCOPY;
 
-    int               frameCounter           = 0;
-    int               framesInLastHalfSecond = 0;
-    CTimer            lastMeasure;
-    CTimer            lastFrame;
-    bool              sentScreencast = false;
+    int                               frameCounter           = 0;
+    int                               framesInLastHalfSecond = 0;
+    CTimer                            lastMeasure;
+    CTimer                            lastFrame;
+    bool                              sentScreencast = false;
 
-    void              onTick();
-    HOOK_CALLBACK_FN* tickCallback = nullptr;
+    void                              onTick();
+    std::shared_ptr<HOOK_CALLBACK_FN> tickCallback;
 
-    bool              operator==(const CScreencopyClient& other) const {
+    bool                              operator==(const CScreencopyClient& other) const {
         return resource == other.resource;
     }
 };

diff --git a/src/protocols/TearingControl.cpp b/src/protocols/TearingControl.cpp
@@ -4,7 +4,7 @@
 #include "../Compositor.hpp"
 
 CTearingControlProtocol::CTearingControlProtocol(const wl_interface* iface, const int& ver, const std::string& name) : IWaylandProtocol(iface, ver, name) {
-    g_pHookSystem->hookDynamic("destroyWindow", [this](void* self, SCallbackInfo& info, std::any param) { this->onWindowDestroy(std::any_cast<CWindow*>(param)); });
+    static auto P = g_pHookSystem->hookDynamic("destroyWindow", [this](void* self, SCallbackInfo& info, std::any param) { this->onWindowDestroy(std::any_cast<CWindow*>(param)); });
 }
 
 void CTearingControlProtocol::bindManager(wl_client* client, void* data, uint32_t ver, uint32_t id) {

diff --git a/src/protocols/XDGOutput.cpp b/src/protocols/XDGOutput.cpp
@@ -33,9 +33,9 @@ void CXDGOutputProtocol::bindManager(wl_client* client, void* data, uint32_t ver
 }
 
 CXDGOutputProtocol::CXDGOutputProtocol(const wl_interface* iface, const int& ver, const std::string& name) : IWaylandProtocol(iface, ver, name) {
-    g_pHookSystem->hookDynamic("monitorLayoutChanged", [this](void* self, SCallbackInfo& info, std::any param) { this->updateAllOutputs(); });
-    g_pHookSystem->hookDynamic("configReloaded", [this](void* self, SCallbackInfo& info, std::any param) { this->updateAllOutputs(); });
-    g_pHookSystem->hookDynamic("monitorRemoved", [this](void* self, SCallbackInfo& info, std::any param) {
+    static auto P  = g_pHookSystem->hookDynamic("monitorLayoutChanged", [this](void* self, SCallbackInfo& info, std::any param) { this->updateAllOutputs(); });
+    static auto P2 = g_pHookSystem->hookDynamic("configReloaded", [this](void* self, SCallbackInfo& info, std::any param) { this->updateAllOutputs(); });
+    static auto P3 = g_pHookSystem->hookDynamic("monitorRemoved", [this](void* self, SCallbackInfo& info, std::any param) {
         const auto PMONITOR = std::any_cast<CMonitor*>(param);
         for (auto& o : m_vXDGOutputs) {
             if (o->monitor == PMONITOR)

diff --git a/src/render/OpenGL.cpp b/src/render/OpenGL.cpp
@@ -52,7 +52,7 @@ CHyprOpenGLImpl::CHyprOpenGLImpl() {
     Debug::log(WARN, "!RENDERER: Using the legacy GLES2 renderer!");
 #endif
 
-    g_pHookSystem->hookDynamic("preRender", [&](void* self, SCallbackInfo& info, std::any data) { preRender(std::any_cast<CMonitor*>(data)); });
+    static auto P = g_pHookSystem->hookDynamic("preRender", [&](void* self, SCallbackInfo& info, std::any data) { preRender(std::any_cast<CMonitor*>(data)); });
 
     RASSERT(eglMakeCurrent(wlr_egl_get_display(g_pCompositor->m_sWLREGL), EGL_NO_SURFACE, EGL_NO_SURFACE, EGL_NO_CONTEXT), "Couldn't unset current EGL!");