hyprwm / Hyprland

Hyprland is an independent, highly customizable, dynamic tiling Wayland compositor that doesn't sacrifice on its looks.
https://hyprland.org
BSD 3-Clause "New" or "Revised" License
20.94k stars 880 forks source link

Race condition touch screen #3527

Open Harmek59 opened 1 year ago

Harmek59 commented 1 year ago

Hyprland Version

499df49f7b28f9e3be9b1c53843fd5c465dec60b

Bug or Regression?

Bug

Description

I have laptop with touch screen, so I wanted to use plugin: https://github.com/horriblename/hyprgrass. However Hyprland crashes on touchUp event. I have tried to do some investigation. First it works fine with debug builds, but I find out what may be the problem. Functions: onTouchDown(wlr_touch_down_event*), onTouchUp(wlr_touch_up_event*), onTouchMove(wlr_touch_motion_event*) from CInputManager may be called from different threads (if i tap the screen fast enough with multiple fingers) and they use the same field m_sTouchData, so it looks like race condition. However i was not able to crash it without plugin loaded, so I have created minimal version of plugin hyprgrass, which creates hooks for this functions, but this hooks just call m_pOriginal, so I expect that it does nothing, but hyprland crashes onTouchUp.

#include "globals.hpp"

#include <algorithm>
#include <hyprland/src/Compositor.hpp>
#include <hyprland/src/config/ConfigManager.hpp>
#include <hyprland/src/managers/input/InputManager.hpp>

inline CFunctionHook* g_pTouchDownHook = nullptr;
inline CFunctionHook* g_pTouchUpHook   = nullptr;
inline CFunctionHook* g_pTouchMoveHook = nullptr;
typedef void (*origTouchDown)(void*, wlr_touch_down_event*);
typedef void (*origTouchUp)(void*, wlr_touch_up_event*);
typedef void (*origTouchMove)(void*, wlr_touch_motion_event*);

void hkOnTouchDown(void* thisptr, wlr_touch_down_event* e) {
    (*(origTouchDown)g_pTouchDownHook->m_pOriginal)(thisptr, e);
}

void hkOnTouchUp(void* thisptr, wlr_touch_up_event* e) {
    (*(origTouchUp)g_pTouchUpHook->m_pOriginal)(thisptr, e);
}

void hkOnTouchMove(void* thisptr, wlr_touch_motion_event* e) {
    (*(origTouchMove)g_pTouchMoveHook->m_pOriginal)(thisptr, e);
}

// Do NOT change this function.
APICALL EXPORT std::string PLUGIN_API_VERSION() {
    return HYPRLAND_API_VERSION;
}

APICALL EXPORT PLUGIN_DESCRIPTION_INFO PLUGIN_INIT(HANDLE handle) {
    PHANDLE = handle;

    g_pTouchDownHook = HyprlandAPI::createFunctionHook(
        PHANDLE, (void*)&CInputManager::onTouchDown, (void*)&hkOnTouchDown);
    g_pTouchUpHook = HyprlandAPI::createFunctionHook(
        PHANDLE, (void*)&CInputManager::onTouchUp, (void*)&hkOnTouchUp);
    g_pTouchMoveHook = HyprlandAPI::createFunctionHook(
        PHANDLE, (void*)&CInputManager::onTouchMove, (void*)&hkOnTouchMove);

    g_pTouchDownHook->hook();
    g_pTouchUpHook->hook();
    g_pTouchMoveHook->hook();

    HyprlandAPI::reloadConfig();

    return {"hyprgrass", "Touchscreen gestures", "horriblename", "0.2"};
}

APICALL EXPORT void PLUGIN_EXIT() {
}

I added mutex to m_sTouchData and it does solve the problem:

diff --git a/src/managers/input/InputManager.hpp b/src/managers/input/InputManager.hpp
index 670bc4da..66114076 100644
--- a/src/managers/input/InputManager.hpp
+++ b/src/managers/input/InputManager.hpp
@@ -2,6 +2,7 @@

 #include "../../defines.hpp"
 #include <list>
+#include <mutex>
 #include "../../helpers/WLClasses.hpp"
 #include "../../Window.hpp"
 #include "../../helpers/Timer.hpp"
@@ -114,6 +115,7 @@ class CInputManager {
     void               onPointerHoldEnd(wlr_pointer_hold_end_event*);

     STouchData         m_sTouchData;
+    std::mutex         m_sTouchDataMutex;

     // for dragging floating windows
     CWindow*       currentlyDraggedWindow = nullptr;
diff --git a/src/managers/input/Touch.cpp b/src/managers/input/Touch.cpp
index 19e29c1b..c02773cd 100644
--- a/src/managers/input/Touch.cpp
+++ b/src/managers/input/Touch.cpp
@@ -1,5 +1,6 @@
 #include "InputManager.hpp"
 #include "../../Compositor.hpp"
+#include <mutex>

 void CInputManager::onTouchDown(wlr_touch_down_event* e) {
     auto       PMONITOR = g_pCompositor->getMonitorFromName(e->touch->output_name ? e->touch->output_name : "");
@@ -24,6 +25,7 @@ void CInputManager::onTouchDown(wlr_touch_down_event* e) {

     m_bLastInputTouch = true;

+    std::lock_guard<std::mutex> l(m_sTouchDataMutex);
     m_sTouchData.touchFocusWindow  = m_pFoundWindowToFocus;
     m_sTouchData.touchFocusSurface = m_pFoundSurfaceToFocus;
     m_sTouchData.touchFocusLS      = m_pFoundLSToFocus;
@@ -52,12 +54,14 @@ void CInputManager::onTouchDown(wlr_touch_down_event* e) {
 }

 void CInputManager::onTouchUp(wlr_touch_up_event* e) {
+    std::lock_guard<std::mutex> l(m_sTouchDataMutex);
     if (m_sTouchData.touchFocusSurface) {
         wlr_seat_touch_notify_up(g_pCompositor->m_sSeat.seat, e->time_msec, e->touch_id);
     }
 }

 void CInputManager::onTouchMove(wlr_touch_motion_event* e) {
+    std::lock_guard<std::mutex> l(m_sTouchDataMutex);
     if (m_sTouchData.touchFocusWindow && g_pCompositor->windowValidMapped(m_sTouchData.touchFocusWindow)) {
         const auto PMONITOR = g_pCompositor->getMonitorFromID(m_sTouchData.touchFocusWindow->m_iMonitorID);

I am not sure what is going on, it looks like compiler does something in release build.

How to reproduce

Load plugin from snipped above. Touch screen. On touch up hyprland should crash.

Crash reports, logs, images, videos

hyprland.log hyprlandCrashReport445.txt

matt1432 commented 1 year ago

That's really interesting. For some reason I also had this issue when I was on Arch, but on NixOS it works perfectly