New subject: [PATCH 2/6] user32: Do not deactivate if thread is foreground.

27 Apr 2020

When calling SetForegroundWindow for a window in another thread, an
internal message is posted to the thread's message queue.

If this thread then calls SetForegroundWindow before processing its
messages it will execute the corresponding set_active_window first,
but then overwrite the active window later, when processing its internal
messages.

This is not always the correct behavior and these tests help determine
what should actually be done in various situations.

This aims to check the following sequences:

* window A0, A1, or A2 is foreground, then:
  * thread B sets foreground to window A0
  * thread A sets foreground to window A1

As well as these sequences where foreground is also temporarily switched
to thread B' window B0:

* window A0, A1, or A2 is foreground, then:
  * thread B sets foreground to window B0
  * thread B sets foreground to window A0
  * thread B sets foreground to window B0
  * thread A sets foreground to window A1

In addition, we also do tests with additional SetActiveWindow / SetFocus
calls to validate their influence.

Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com>
---

For system-wide input (by opposition to per-window as currently done) to
be correctly implemented, for instance for a correct implementation of
the rawinput API, but also to be able to implement focus change requests
like using _NET_ACTIVE_WINDOW request on X11, Wine focus tracking has to
precisely match system focus.

There's currently several issues with that and this patch series fixes
some focus inversions that may happen because of internal messages being
queued, then processed late, overwriting previous changes, when multiple
threads or processes are switching foreground.

Another future improvement that I think would be helpful, will be to
add timestamps to SetForegroundWindow requests, so that we can use the
system event time to tell events that are the result of window creation
from events that come from an actual system focus change.

Then, if an application is't processing its messages, we will not check
for system events and that could be a problem: when focus is changed to
a non-Wine window, no other window will receive an event. A fix for that
would be to additionally listen to focus changes events globally, for
instance in explorer.exe or a dedicated service, and tell wineserver
about them.

 dlls/user32/tests/win.c | 204 +++++++++++++++++++++++++++++++++++++---
 1 file changed, 190 insertions(+), 14 deletions(-)

diff --git a/dlls/user32/tests/win.c b/dlls/user32/tests/win.c
index 25d643f54a6e..46bc0952b73c 100644
--- a/dlls/user32/tests/win.c
+++ b/dlls/user32/tests/win.c
@@ -3243,25 +3243,165 @@ static void test_SetActiveWindow(HWND hwnd)
 struct create_window_thread_params
 {
     HWND window;
-    HANDLE window_created;
-    HANDLE test_finished;
+    HANDLE test_ready;
+    HANDLE set_foreground;
+    HANDLE test_done;
+};
+
+static int set_foreground_msgcount;
+
+static LRESULT WINAPI set_foreground_wndproc(HWND hwnd, UINT msg, WPARAM wparam, LPARAM lparam)
+{
+    switch (msg)
+    {
+    case WM_NCACTIVATE:
+    case WM_ACTIVATE:
+    case WM_SETFOCUS:
+    case WM_KILLFOCUS:
+        set_foreground_msgcount++;
+        break;
+    }
+
+    return DefWindowProcW(hwnd, msg, wparam, lparam);
+}
+
+struct set_foreground_test_desc
+{
+    int  initial_window;
+    BOOL steal_foreground;
+    BOOL call_set_active_window;
+    BOOL call_set_focus;
+
+    BOOL todo_msgcount_before_set_foreground;
+    int  msgcount_before_set_foreground;
+    BOOL todo_msgcount_after_set_foreground;
+    int  msgcount_after_set_foreground;
+    BOOL todo_msgcount_after_peek_message;
+    int  msgcount_after_peek_message;
+    BOOL todo_expected_window;
+    int  expected_window;
+};
+
+static struct set_foreground_test_desc set_foreground_tests[] = {
+    {1, FALSE, FALSE, FALSE,  FALSE, 0, FALSE, 0, FALSE, 6, FALSE, 0},
+    {1, TRUE, FALSE, FALSE,   FALSE, 0,  TRUE, 1,  TRUE, 6,  TRUE, 0},
+    {1, FALSE, TRUE, FALSE,   FALSE, 0, FALSE, 0, FALSE, 6, FALSE, 0},
+    {1, TRUE, TRUE, FALSE,    FALSE, 0,  TRUE, 1,  TRUE, 6,  TRUE, 0},
+    {1, FALSE, FALSE, TRUE,   FALSE, 0, FALSE, 0, FALSE, 6, FALSE, 0},
+    {1, TRUE, FALSE, TRUE,    FALSE, 0,  TRUE, 1,  TRUE, 6,  TRUE, 0},
+
+    {2, FALSE, FALSE, FALSE,  FALSE, 0, FALSE, 6,  TRUE, 0,  TRUE, 1},
+    {2, TRUE, FALSE, FALSE,   FALSE, 0, FALSE, 6,  TRUE, 0,  TRUE, 1},
+    {2, FALSE, TRUE, FALSE,   FALSE, 6, FALSE, 0,  TRUE, 0,  TRUE, 1},
+    {2, TRUE, TRUE, FALSE,    FALSE, 6,  TRUE, 1,  TRUE, 6,  TRUE, 0},
+    {2, FALSE, FALSE, TRUE,    TRUE, 8, FALSE, 0,  TRUE, 0,  TRUE, 1},
+    {2, TRUE, FALSE, TRUE,     TRUE, 8,  TRUE, 1,  TRUE, 6,  TRUE, 0},
+
+    {0, FALSE, FALSE, FALSE,  FALSE, 0, FALSE, 6, FALSE, 0, FALSE, 1},
+    {0, TRUE, FALSE, FALSE,   FALSE, 0, FALSE, 6,  TRUE, 0,  TRUE, 1},
+    {0, FALSE, TRUE, FALSE,   FALSE, 6, FALSE, 0, FALSE, 0, FALSE, 1},
+    {0, TRUE, TRUE, FALSE,    FALSE, 6,  TRUE, 1, FALSE, 6, FALSE, 0},
+    {0, FALSE, FALSE, TRUE,    TRUE, 8, FALSE, 0, FALSE, 0, FALSE, 1},
+    {0, TRUE, FALSE, TRUE,     TRUE, 8,  TRUE, 1, FALSE, 6, FALSE, 0},
 };
 
 static DWORD WINAPI create_window_thread(void *param)
 {
     struct create_window_thread_params *p = param;
     DWORD res;
+    WNDPROC wndprocs[3];
+    HWND windows[3];
     BOOL ret;
+    MSG msg;
+    int i;
 
-    p->window = CreateWindowA("static", NULL, WS_POPUP | WS_VISIBLE, 0, 0, 0, 0, 0, 0, 0, 0);
+    windows[1] = CreateWindowA("static", NULL, WS_POPUP | WS_VISIBLE, 0, 0, 0, 0, 0, 0, 0, 0);
+    windows[2] = CreateWindowA("static", NULL, WS_POPUP | WS_VISIBLE, 0, 0, 0, 0, 0, 0, 0, 0);
+    windows[0] = p->window = CreateWindowA("static", NULL, WS_POPUP | WS_VISIBLE, 0, 0, 0, 0, 0, 0, 0, 0);
+    trace("window[0]:%p windows[1]:%p windows[2]:%p\n", windows[0], windows[1], windows[2]);
 
-    ret = SetEvent(p->window_created);
+    ret = SetEvent(p->test_ready);
     ok(ret, "SetEvent failed, last error %#x.\n", GetLastError());
 
-    res = WaitForSingleObject(p->test_finished, INFINITE);
+    res = WaitForSingleObject(p->test_done, INFINITE);
     ok(res == WAIT_OBJECT_0, "Wait failed (%#x), last error %#x.\n", res, GetLastError());
+    ret = ResetEvent(p->test_done);
+    ok(ret, "ResetEvent failed, last error %#x.\n", GetLastError());
+
+
+    for (i = 0; i < ARRAY_SIZE(windows); ++i)
+        wndprocs[i] = (WNDPROC)SetWindowLongPtrA(windows[i], GWLP_WNDPROC, (LONG_PTR)set_foreground_wndproc);
+
+    flush_events(TRUE);
+
+    for (i = 0; i < ARRAY_SIZE(set_foreground_tests); ++i)
+    {
+        struct set_foreground_test_desc *test = set_foreground_tests + i;
+        HWND initial_window = windows[test->initial_window];
+        HWND expected_window = windows[test->expected_window];
+
+        SetForegroundWindow(initial_window);
+        flush_events(TRUE);
+        check_wnd_state(initial_window, initial_window, initial_window, 0);
+
+        ret = SetEvent(p->test_ready);
+        ok(ret, "SetEvent failed, last error %#x.\n", GetLastError());
+
+        res = WaitForSingleObject(p->set_foreground, INFINITE);
+        ok(res == WAIT_OBJECT_0, "Wait failed (%#x), last error %#x.\n", res, GetLastError());
+        ret = ResetEvent(p->set_foreground);
+        ok(ret, "ResetEvent failed, last error %#x.\n", GetLastError());
+
+        set_foreground_msgcount = 0;
+
+        if (test->call_set_active_window)
+            SetActiveWindow(windows[1]);
+        if (test->call_set_focus)
+            SetFocus(windows[1]);
+
+        todo_wine_if(test->todo_msgcount_before_set_foreground)
+        ok(set_foreground_msgcount == test->msgcount_before_set_foreground,
+           "%d: Unexpected number of messages received before SetForegroundWindow: %d\n", i, set_foreground_msgcount);
+        set_foreground_msgcount = 0;
+
+        SetForegroundWindow(windows[1]);
+
+        todo_wine_if(test->todo_msgcount_after_set_foreground)
+        ok(set_foreground_msgcount == test->msgcount_after_set_foreground,
+           "%d: Unexpected number of messages received after SetForegroundWindow: %d\n", i, set_foreground_msgcount);
+        set_foreground_msgcount = 0;
+
+        ok(GetForegroundWindow() == windows[1], "GetForegroundWindow() returned %p\n", GetForegroundWindow());
+        ok(GetActiveWindow() == windows[1], "GetActiveWindow() returned %p\n", GetActiveWindow());
+        ok(GetFocus() == windows[1], "GetFocus() returned %p\n", GetFocus());
+
+        while (PeekMessageA(&msg, 0, 0, 0, PM_REMOVE)) DispatchMessageA(&msg);
+
+        todo_wine_if(test->todo_msgcount_after_peek_message)
+        ok(set_foreground_msgcount == test->msgcount_after_peek_message,
+           "%d: Unexpected number of messages received after PeekMessageA: %d\n", i, set_foreground_msgcount);
+        set_foreground_msgcount = 0;
+
+        todo_wine_if(test->todo_expected_window)
+        ok(GetForegroundWindow() == expected_window, "%d: GetForegroundWindow() returned %p\n", i, GetForegroundWindow());
+        todo_wine_if(test->todo_expected_window)
+        ok(GetActiveWindow() == expected_window, "%d: GetActiveWindow() returned %p\n", i, GetActiveWindow());
+        todo_wine_if(test->todo_expected_window)
+        ok(GetFocus() == expected_window, "%d: GetFocus() returned %p\n", i, GetFocus());
+
+        res = WaitForSingleObject(p->test_done, INFINITE);
+        ok(res == WAIT_OBJECT_0, "Wait failed (%#x), last error %#x.\n", res, GetLastError());
+        ret = ResetEvent(p->test_done);
+        ok(ret, "ResetEvent failed, last error %#x.\n", GetLastError());
+    }
+
+    for (i = 0; i < ARRAY_SIZE(windows); ++i)
+        SetWindowLongPtrA(windows[i], GWLP_WNDPROC, (LONG_PTR)wndprocs[i]);
+
+
+    for (i = 0; i < ARRAY_SIZE(windows); ++i)
+        DestroyWindow(windows[i]);
 
-    DestroyWindow(p->window);
     return 0;
 }
 
@@ -3274,6 +3414,7 @@ static void test_SetForegroundWindow(HWND hwnd)
     HWND hwnd2;
     MSG msg;
     LONG style;
+    int i;
 
     flush_events( TRUE );
     ShowWindow(hwnd, SW_HIDE);
@@ -3350,14 +3491,18 @@ static void test_SetForegroundWindow(HWND hwnd)
     hwnd2 = CreateWindowA("static", NULL, WS_POPUP | WS_VISIBLE, 0, 0, 0, 0, 0, 0, 0, 0);
     check_wnd_state(hwnd2, hwnd2, hwnd2, 0);
 
-    thread_params.window_created = CreateEventW(NULL, FALSE, FALSE, NULL);
-    ok(!!thread_params.window_created, "CreateEvent failed, last error %#x.\n", GetLastError());
-    thread_params.test_finished = CreateEventW(NULL, FALSE, FALSE, NULL);
-    ok(!!thread_params.test_finished, "CreateEvent failed, last error %#x.\n", GetLastError());
+    thread_params.test_ready = CreateEventW(NULL, FALSE, FALSE, NULL);
+    ok(!!thread_params.test_ready, "CreateEvent failed, last error %#x.\n", GetLastError());
+    thread_params.set_foreground = CreateEventW(NULL, FALSE, FALSE, NULL);
+    ok(!!thread_params.set_foreground, "CreateEvent failed, last error %#x.\n", GetLastError());
+    thread_params.test_done = CreateEventW(NULL, FALSE, FALSE, NULL);
+    ok(!!thread_params.test_done, "CreateEvent failed, last error %#x.\n", GetLastError());
     thread = CreateThread(NULL, 0, create_window_thread, &thread_params, 0, &tid);
     ok(!!thread, "Failed to create thread, last error %#x.\n", GetLastError());
-    res = WaitForSingleObject(thread_params.window_created, INFINITE);
+    res = WaitForSingleObject(thread_params.test_ready, INFINITE);
     ok(res == WAIT_OBJECT_0, "Wait failed (%#x), last error %#x.\n", res, GetLastError());
+    ret = ResetEvent(thread_params.test_ready);
+    ok(ret, "ResetEvent failed, last error %#x.\n", GetLastError());
     check_wnd_state(hwnd2, thread_params.window, hwnd2, 0);
 
     SetForegroundWindow(hwnd2);
@@ -3385,12 +3530,43 @@ static void test_SetForegroundWindow(HWND hwnd)
     ok(!SetForegroundWindow(hwnd2), "SetForegroundWindow failed\n");
     check_wnd_state(hwnd, hwnd, hwnd, 0);
 
-    SetEvent(thread_params.test_finished);
+    res = SetEvent(thread_params.test_done);
+    ok(res, "SetEvent failed, last error %#x.\n", GetLastError());
+
+
+    for (i = 0; i < ARRAY_SIZE(set_foreground_tests); ++i)
+    {
+        struct set_foreground_test_desc *test = set_foreground_tests + i;
+
+        while (MsgWaitForMultipleObjects(1, &thread_params.test_ready, FALSE, INFINITE, QS_SENDMESSAGE) != WAIT_OBJECT_0)
+        {
+            while (PeekMessageA(&msg, 0, 0, 0, PM_REMOVE | PM_QS_SENDMESSAGE))
+                DispatchMessageA(&msg);
+        }
+
+        ret = ResetEvent(thread_params.test_ready);
+        ok(ret, "ResetEvent failed, last error %#x.\n", GetLastError());
+
+        if (test->steal_foreground) SetForegroundWindow(hwnd);
+        SetForegroundWindow(thread_params.window);
+        if (test->steal_foreground) SetForegroundWindow(hwnd);
+
+        res = SetEvent(thread_params.set_foreground);
+        ok(res, "SetEvent failed, last error %#x.\n", GetLastError());
+
+        ret = SetEvent(thread_params.test_done);
+        ok(res, "SetEvent failed, last error %#x.\n", GetLastError());
+    }
+
+
     WaitForSingleObject(thread, INFINITE);
-    CloseHandle(thread_params.test_finished);
-    CloseHandle(thread_params.window_created);
+    CloseHandle(thread_params.set_foreground);
+    CloseHandle(thread_params.test_done);
+    CloseHandle(thread_params.test_ready);
     CloseHandle(thread);
     DestroyWindow(hwnd2);
+
+    flush_events(TRUE);
 }
 
 static WNDPROC old_button_proc;
-- 
2.26.1

    

[PATCH 1/6] user32/tests: Add concurrency tests for SetForegroundWindow.

Rémi Bernon

Rémi Bernon

Marvin

Rémi Bernon

Rémi Bernon

Marvin

Rémi Bernon

Rémi Bernon

Marvin

Rémi Bernon

Marvin

Marvin

Rémi Bernon

tags

participants (2)