Merge pull request #12392 from akallabeth/x11-rails-lock-fix

[client,x11] improve rails window locking

client/X11/xf_event.c

@@ -410,9 +410,8 @@ static BOOL xf_event_Expose(xfContext* xfc, const XExposeEvent* event, BOOL app)
 	{
 		xfAppWindow* appWindow = xf_AppWindowFromX11Window(xfc, event->window);
 		if (appWindow)
-		{
 			xf_UpdateWindowArea(xfc, appWindow, x, y, w, h);
-		}
+		xf_rail_return_window(appWindow);
 	}
 
 	return TRUE;

client/X11/xf_graphics.c

@@ -251,12 +251,12 @@ static Window xf_Pointer_get_window(xfContext* xfc)
 	if (xfc->remote_app)
 	{
 		Window w = 0;
-		HashTable_Lock(xfc->railWindows);
+		xf_AppWindowsLock(xfc);
 		if (!xfc->appWindow)
 			WLog_WARN(TAG, "xf_Pointer: Invalid appWindow");
 		else
 			w = xfc->appWindow->handle;
-		HashTable_Unlock(xfc->railWindows);
+		xf_AppWindowsUnlock(xfc);
 		return w;
 	}
 	else

client/X11/xf_rail.c

@@ -159,9 +159,10 @@ BOOL xf_rail_send_activate(xfContext* xfc, Window xwindow, BOOL enabled)
 
 	WINPR_ASSERT(appWindow->windowId <= UINT32_MAX);
 	activate.windowId = (UINT32)appWindow->windowId;
+	xf_rail_return_window(appWindow);
+
 	activate.enabled = enabled;
 	const UINT rc = xfc->rail->ClientActivate(xfc->rail, &activate);
-	xf_rail_return_window(appWindow);
 	return rc == CHANNEL_RC_OK;
 }
 
@@ -381,6 +382,7 @@ static void window_state_log_style_int(wLog* log, const WINDOW_STATE_ORDER* wind
 static BOOL xf_rail_window_common(rdpContext* context, const WINDOW_ORDER_INFO* orderInfo,
                                   const WINDOW_STATE_ORDER* windowState)
 {
+	BOOL rc = FALSE;
 	xfContext* xfc = (xfContext*)context;
 
 	WINPR_ASSERT(xfc);
@@ -440,7 +442,7 @@ static BOOL xf_rail_window_common(rdpContext* context, const WINDOW_ORDER_INFO*
 		}
 
 		if (!appWindow->title)
-			return FALSE;
+			goto fail;
 
 		xf_AppWindowInit(xfc, appWindow);
 	}
@@ -518,14 +520,14 @@ static BOOL xf_rail_window_common(rdpContext* context, const WINDOW_ORDER_INFO*
 			if (!(title = _strdup("")))
 			{
 				WLog_ERR(TAG, "failed to duplicate empty window title string");
-				return FALSE;
+				goto fail;
 			}
 		}
 		else if (!(title = ConvertWCharNToUtf8Alloc(
 		               cnv.wc, windowState->titleInfo.length / sizeof(WCHAR), nullptr)))
 		{
 			WLog_ERR(TAG, "failed to convert window title");
-			return FALSE;
+			goto fail;
 		}
 
 		free(appWindow->title);
@@ -566,7 +568,7 @@ static BOOL xf_rail_window_common(rdpContext* context, const WINDOW_ORDER_INFO*
 			    (RECTANGLE_16*)calloc(appWindow->numWindowRects, sizeof(RECTANGLE_16));
 
 			if (!appWindow->windowRects)
-				return FALSE;
+				goto fail;
 
 			CopyMemory(appWindow->windowRects, windowState->windowRects,
 			           appWindow->numWindowRects * sizeof(RECTANGLE_16));
@@ -595,7 +597,7 @@ static BOOL xf_rail_window_common(rdpContext* context, const WINDOW_ORDER_INFO*
 			    (RECTANGLE_16*)calloc(appWindow->numVisibilityRects, sizeof(RECTANGLE_16));
 
 			if (!appWindow->visibilityRects)
-				return FALSE;
+				goto fail;
 
 			CopyMemory(appWindow->visibilityRects, windowState->visibilityRects,
 			           appWindow->numVisibilityRects * sizeof(RECTANGLE_16));
@@ -675,7 +677,10 @@ static BOOL xf_rail_window_common(rdpContext* context, const WINDOW_ORDER_INFO*
 	{
 	    xf_SetWindowRects(xfc, appWindow, appWindow->windowRects, appWindow->numWindowRects);
 	}*/
-	return TRUE;
+	rc = TRUE;
+fail:
+	xf_rail_return_window(appWindow);
+	return rc;
 }
 
 static BOOL xf_rail_window_delete(rdpContext* context, const WINDOW_ORDER_INFO* orderInfo)
@@ -1186,14 +1191,14 @@ static UINT xf_rail_server_local_move_size(RailClientContext* context,
 			x = localMoveSize->posX;
 			y = localMoveSize->posY;
 			/* FIXME: local keyboard moves not working */
-			return CHANNEL_RC_OK;
+			break;
 
 		case RAIL_WMSZ_KEYSIZE:
 			direction = NET_WM_MOVERESIZE_SIZE_KEYBOARD;
 			x = localMoveSize->posX;
 			y = localMoveSize->posY;
 			/* FIXME: local keyboard moves not working */
-			return CHANNEL_RC_OK;
+			break;
 		default:
 			break;
 	}
@@ -1375,13 +1380,16 @@ xfAppWindow* xf_rail_add_window(xfContext* xfc, UINT64 id, INT32 x, INT32 y, UIN
 	appWindow->width = WINPR_ASSERTING_INT_CAST(int, width);
 	appWindow->height = WINPR_ASSERTING_INT_CAST(int, height);
 
+	xf_AppWindowsLock(xfc);
 	if (!xf_AppWindowCreate(xfc, appWindow))
 		goto fail;
+
 	if (!HashTable_Insert(xfc->railWindows, &appWindow->windowId, (void*)appWindow))
 		goto fail;
 	return appWindow;
 fail:
 	rail_window_free(appWindow);
+	xf_AppWindowsUnlock(xfc);
 	return nullptr;
 }
 
@@ -1396,26 +1404,10 @@ BOOL xf_rail_del_window(xfContext* xfc, UINT64 id)
 	return HashTable_Remove(xfc->railWindows, &id);
 }
 
-xfAppWindow* xf_rail_get_window(xfContext* xfc, UINT64 id)
+void xf_rail_return_windowFrom(xfAppWindow* window, const char* file, const char* fkt, size_t line)
-{
-	if (!xfc)
-		return nullptr;
-
-	if (!xfc->railWindows)
-		return nullptr;
-
-	HashTable_Lock(xfc->railWindows);
-	xfAppWindow* window = HashTable_GetItemValue(xfc->railWindows, &id);
-	if (!window)
-		HashTable_Unlock(xfc->railWindows);
-
-	return window;
-}
-
-void xf_rail_return_window(xfAppWindow* window)
 {
 	if (!window)
 		return;
 
-	HashTable_Unlock(window->xfc->railWindows);
+	xfAppWindowsUnlockFrom(window->xfc, file, fkt, line);
 }

client/X11/xf_rail.h

@@ -125,10 +125,16 @@ BOOL xf_rail_disable_remoteapp_mode(xfContext* xfc);
 xfAppWindow* xf_rail_add_window(xfContext* xfc, UINT64 id, INT32 x, INT32 y, UINT32 width,
                                 UINT32 height, UINT32 surfaceId);
 
-void xf_rail_return_window(xfAppWindow* window);
+#define xf_rail_return_window(window) \
+	xf_rail_return_windowFrom((window), __FILE__, __func__, __LINE__)
+void xf_rail_return_windowFrom(xfAppWindow* window, const char* file, const char* fkt, size_t line);
 
-WINPR_ATTR_MALLOC(xf_rail_return_window, 1)
+#define xf_rail_get_window(xfc, id) \
-xfAppWindow* xf_rail_get_window(xfContext* xfc, UINT64 id);
+	xf_rail_get_windowFrom((xfc), (id), __FILE__, __func__, __LINE__)
+
+WINPR_ATTR_MALLOC(xf_rail_return_windowFrom, 1)
+xfAppWindow* xf_rail_get_windowFrom(xfContext* xfc, UINT64 id, const char* file, const char* fkt,
+                                    size_t line);
 
 BOOL xf_rail_del_window(xfContext* xfc, UINT64 id);
 

client/X11/xf_window.c

@@ -1435,7 +1435,31 @@ void xf_DestroyWindow(xfContext* xfc, xfAppWindow* appWindow)
 	free(appWindow);
 }
 
-xfAppWindow* xf_AppWindowFromX11Window(xfContext* xfc, Window wnd)
+static xfAppWindow* get_windowUnlocked(xfContext* xfc, UINT64 id)
+{
+	WINPR_ASSERT(xfc);
+	return HashTable_GetItemValue(xfc->railWindows, &id);
+}
+
+xfAppWindow* xf_rail_get_windowFrom(xfContext* xfc, UINT64 id, const char* file, const char* fkt,
+                                    size_t line)
+{
+	if (!xfc)
+		return nullptr;
+
+	if (!xfc->railWindows)
+		return nullptr;
+
+	xfAppWindowsLockFrom(xfc, file, fkt, line);
+	xfAppWindow* window = get_windowUnlocked(xfc, id);
+	if (!window)
+		xfAppWindowsUnlockFrom(xfc, file, fkt, line);
+
+	return window;
+}
+
+xfAppWindow* xf_AppWindowFromX11WindowFrom(xfContext* xfc, Window wnd, const char* file,
+                                           const char* fkt, size_t line)
 {
 	ULONG_PTR* pKeys = nullptr;
 
@@ -1443,29 +1467,28 @@ xfAppWindow* xf_AppWindowFromX11Window(xfContext* xfc, Window wnd)
 	if (!xfc->railWindows)
 		return nullptr;
 
-	HashTable_Lock(xfc->railWindows);
+	xfAppWindowsLockFrom(xfc, file, fkt, line);
 	size_t count = HashTable_GetKeys(xfc->railWindows, &pKeys);
 
 	for (size_t index = 0; index < count; index++)
 	{
-		xfAppWindow* appWindow = xf_rail_get_window(xfc, *(UINT64*)pKeys[index]);
+		xfAppWindow* appWindow = get_windowUnlocked(xfc, *(UINT64*)pKeys[index]);
 
 		if (!appWindow)
 		{
-			HashTable_Unlock(xfc->railWindows);
+			xfAppWindowsUnlockFrom(xfc, file, fkt, line);
 			free(pKeys);
 			return nullptr;
 		}
 
 		if (appWindow->handle == wnd)
 		{
-			HashTable_Unlock(xfc->railWindows);
 			free(pKeys);
 			return appWindow;
 		}
 	}
 
-	HashTable_Unlock(xfc->railWindows);
+	xfAppWindowsUnlockFrom(xfc, file, fkt, line);
 	free(pKeys);
 	return nullptr;
 }
@@ -1585,3 +1608,39 @@ void xf_XSetTransientForHint(xfContext* xfc, xfAppWindow* window)
 	(void)LogDynAndXSetTransientForHint(xfc->log, xfc->display, window->handle, parent->handle);
 	xf_rail_return_window(parent);
 }
+
+void xfAppWindowsLockFrom(xfContext* xfc, WINPR_ATTR_UNUSED const char* file,
+                          WINPR_ATTR_UNUSED const char* fkt, WINPR_ATTR_UNUSED size_t line)
+{
+	WINPR_ASSERT(xfc);
+
+#if defined(WITH_VERBOSE_WINPR_ASSERT)
+	const DWORD level = WLOG_TRACE;
+	if (WLog_IsLevelActive(xfc->log, level))
+		WLog_PrintTextMessage(xfc->log, level, line, file, fkt, "[rails] locking [%s]", fkt);
+#endif
+
+	HashTable_Lock(xfc->railWindows);
+
+#if defined(WITH_VERBOSE_WINPR_ASSERT)
+	WINPR_ASSERT(!xfc->isRailWindowsLocked);
+	xfc->isRailWindowsLocked = TRUE;
+#endif
+}
+
+void xfAppWindowsUnlockFrom(xfContext* xfc, WINPR_ATTR_UNUSED const char* file,
+                            WINPR_ATTR_UNUSED const char* fkt, WINPR_ATTR_UNUSED size_t line)
+{
+	WINPR_ASSERT(xfc);
+
+#if defined(WITH_VERBOSE_WINPR_ASSERT)
+	const DWORD level = WLOG_TRACE;
+	if (WLog_IsLevelActive(xfc->log, level))
+		WLog_PrintTextMessage(xfc->log, level, line, file, fkt, "[rails] unocking [%s]", fkt);
+
+	WINPR_ASSERT(xfc->isRailWindowsLocked);
+	xfc->isRailWindowsLocked = FALSE;
+#endif
+
+	HashTable_Unlock(xfc->railWindows);
+}

client/X11/xf_window.h

@@ -122,8 +122,17 @@ void xf_SetWindowMinMaxInfo(xfContext* xfc, xfAppWindow* appWindow, int maxWidth
 void xf_StartLocalMoveSize(xfContext* xfc, xfAppWindow* appWindow, int direction, int x, int y);
 void xf_EndLocalMoveSize(xfContext* xfc, xfAppWindow* appWindow);
 
-WINPR_ATTR_MALLOC(xf_rail_return_window, 1)
+#define xf_AppWindowFromX11Window(xfc, wnd) \
-xfAppWindow* xf_AppWindowFromX11Window(xfContext* xfc, Window wnd);
+	xf_AppWindowFromX11WindowFrom((xfc), (wnd), __FILE__, __func__, __LINE__)
+WINPR_ATTR_MALLOC(xf_rail_return_windowFrom, 1)
+xfAppWindow* xf_AppWindowFromX11WindowFrom(xfContext* xfc, Window wnd, const char* file,
+                                           const char* fkt, size_t line);
+
+#define xf_AppWindowsLock(xfc) xfAppWindowsLockFrom((xfc), __FILE__, __func__, __LINE__)
+void xfAppWindowsLockFrom(xfContext* xfc, const char* file, const char* fkt, size_t line);
+
+#define xf_AppWindowsUnlock(xfc) xfAppWindowsUnlockFrom((xfc), __FILE__, __func__, __LINE__)
+void xfAppWindowsUnlockFrom(xfContext* xfc, const char* file, const char* fkt, size_t line);
 
 const char* window_styles_to_string(UINT32 style, char* buffer, size_t length);
 const char* window_styles_ex_to_string(UINT32 styleEx, char* buffer, size_t length);

client/X11/xfreerdp.h

@@ -290,6 +290,10 @@ struct xf_context
 	wHashTable* railWindows;
 	xfRailIconCache* railIconCache;
 
+#if defined(WITH_VERBOSE_WINPR_ASSERT)
+	BOOL isRailWindowsLocked;
+#endif
+
 	BOOL xkbAvailable;
 	BOOL xrenderAvailable;