[PATCH v3 1/2] oleacc: Check proper UI thread for window focus.
From: Connor McAdams <conmanx360(a)gmail.com> When RPC is done in a multi-threaded apartment environment, this method may be called in a thread other than the one that the object was created in. To account for this, make sure to check the UI thread the HWND belongs to rather than the one belonging to the current thread. Signed-off-by: Connor McAdams <conmanx360(a)gmail.com> --- dlls/oleacc/client.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/dlls/oleacc/client.c b/dlls/oleacc/client.c index 3b33be55a21..a726c607109 100644 --- a/dlls/oleacc/client.c +++ b/dlls/oleacc/client.c @@ -227,6 +227,7 @@ static HRESULT WINAPI Client_get_accRole(IAccessible *iface, VARIANT varID, VARI static HRESULT WINAPI Client_get_accState(IAccessible *iface, VARIANT varID, VARIANT *pvarState) { Client *This = impl_from_Client(iface); + GUITHREADINFO info; LONG style; TRACE("(%p)->(%s %p)\n", This, debugstr_variant(&varID), pvarState); @@ -244,7 +245,10 @@ static HRESULT WINAPI Client_get_accState(IAccessible *iface, VARIANT varID, VAR V_I4(pvarState) |= STATE_SYSTEM_UNAVAILABLE; else if(IsWindow(This->hwnd)) V_I4(pvarState) |= STATE_SYSTEM_FOCUSABLE; - if(GetFocus() == This->hwnd) + + info.cbSize = sizeof(info); + if(GetGUIThreadInfo(GetWindowThreadProcessId(This->hwnd, NULL), &info) && + info.hwndFocus == This->hwnd) V_I4(pvarState) |= STATE_SYSTEM_FOCUSED; if(!(style & WS_VISIBLE)) V_I4(pvarState) |= STATE_SYSTEM_INVISIBLE; -- 2.25.1
Signed-off-by: Connor McAdams <cmcadams(a)codeweavers.com> --- v3: Remove unnecessary whitespace deletion done by accident in v2. dlls/oleacc/tests/main.c | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/dlls/oleacc/tests/main.c b/dlls/oleacc/tests/main.c index 5823b4336ba..b5765a56733 100644 --- a/dlls/oleacc/tests/main.c +++ b/dlls/oleacc/tests/main.c @@ -1698,6 +1698,23 @@ static void test_AccessibleChildren(IAccessible *acc) ok(V_VT(children+2) == VT_EMPTY, "V_VT(children+2) = %d\n", V_VT(children+2)); } +static DWORD WINAPI acc_focus_test_thread(LPVOID lpParameter) +{ + IAccessible *acc = (IAccessible *)lpParameter; + VARIANT vid, v; + HRESULT hr; + + V_VT(&vid) = VT_I4; + V_I4(&vid) = CHILDID_SELF; + hr = IAccessible_get_accState(acc, vid, &v); + ok(hr == S_OK, "got %x\n", hr); + ok(V_VT(&v) == VT_I4, "V_VT(&v) = %d\n", V_VT(&v)); + ok(V_I4(&v) == (STATE_SYSTEM_FOCUSABLE|STATE_SYSTEM_INVISIBLE|STATE_SYSTEM_FOCUSED), + "V_I4(&v) = %x\n", V_I4(&v)); + + return 0; +} + static void test_default_client_accessible_object(void) { IAccessible *acc; @@ -1712,6 +1729,7 @@ static void test_default_client_accessible_object(void) RECT rect; LONG l, left, top, width, height; ULONG fetched; + HANDLE hthread; hwnd = CreateWindowA("oleacc_test", "wnd &t &junk", WS_OVERLAPPEDWINDOW, 0, 0, 100, 100, NULL, NULL, NULL, NULL); @@ -1870,6 +1888,21 @@ static void test_default_client_accessible_object(void) ok(V_I4(&v) == (STATE_SYSTEM_FOCUSABLE|STATE_SYSTEM_INVISIBLE), "V_I4(&v) = %x\n", V_I4(&v)); + /* + * Test that STATE_SYSTEM_FOCUSED works, and is specific to the UI Thread + * of the HWND the IAccessible represents. + */ + SetFocus(hwnd); + hr = IAccessible_get_accState(acc, vid, &v); + ok(hr == S_OK, "got %x\n", hr); + ok(V_VT(&v) == VT_I4, "V_VT(&v) = %d\n", V_VT(&v)); + ok(V_I4(&v) == (STATE_SYSTEM_FOCUSABLE|STATE_SYSTEM_INVISIBLE|STATE_SYSTEM_FOCUSED), + "V_I4(&v) = %x\n", V_I4(&v)); + + hthread = CreateThread(NULL, 0, acc_focus_test_thread, (void *)acc, 0, NULL); + ok(!WaitForSingleObject(hthread, 10000), "Focus thread failed to return!\n"); + CloseHandle(hthread); + str = (void*)0xdeadbeef; hr = IAccessible_get_accHelp(acc, vid, &str); ok(hr == S_FALSE, "got %x\n", hr); -- 2.25.1
Hi Connor, The patch looks good for me. On 8/12/21 9:07 PM, Connor McAdams wrote:
+ info.cbSize = sizeof(info); + if(GetGUIThreadInfo(GetWindowThreadProcessId(This->hwnd, NULL), &info) && + info.hwndFocus == This->hwnd) It doesn't really matter but you can just call GetGUIThreadInfo(0, &info) here. If This->hwnd thread is not foreground the hwnd comparison will always fail.
While you're fixing the function are you also planning to fix the STATE_SYSTEM_FOCUSABLE flag? It should probably look like this: if(GetForegroundWindow() == GetAncestor(This->hwnd, GA_ROOT)) V_I4(pvarState) |= STATE_SYSTEM_FOCUSABLE; Thanks, Piotr
On Sat, Aug 28, 2021 at 03:32:59PM +0200, Piotr Caban wrote:
Hi Connor,
The patch looks good for me.
On 8/12/21 9:07 PM, Connor McAdams wrote:
+ info.cbSize = sizeof(info); + if(GetGUIThreadInfo(GetWindowThreadProcessId(This->hwnd, NULL), &info) && + info.hwndFocus == This->hwnd) It doesn't really matter but you can just call GetGUIThreadInfo(0, &info) here. If This->hwnd thread is not foreground the hwnd comparison will always fail.
While you're fixing the function are you also planning to fix the STATE_SYSTEM_FOCUSABLE flag? It should probably look like this: if(GetForegroundWindow() == GetAncestor(This->hwnd, GA_ROOT)) V_I4(pvarState) |= STATE_SYSTEM_FOCUSABLE;
I tried the code you suggested, but it fails the tests. I'll do a bit more investigation, I'm not sure why it isn't working...
Thanks, Piotr
On 8/30/21 4:55 PM, Connor McAdams wrote:
On Sat, Aug 28, 2021 at 03:32:59PM +0200, Piotr Caban wrote:
On 8/12/21 9:07 PM, Connor McAdams wrote:
+ info.cbSize = sizeof(info); + if(GetGUIThreadInfo(GetWindowThreadProcessId(This->hwnd, NULL), &info) && + info.hwndFocus == This->hwnd) It doesn't really matter but you can just call GetGUIThreadInfo(0, &info) here. If This->hwnd thread is not foreground the hwnd comparison will always fail.
While you're fixing the function are you also planning to fix the STATE_SYSTEM_FOCUSABLE flag? It should probably look like this: if(GetForegroundWindow() == GetAncestor(This->hwnd, GA_ROOT)) V_I4(pvarState) |= STATE_SYSTEM_FOCUSABLE;
I tried the code you suggested, but it fails the tests. I'll do a bit more investigation, I'm not sure why it isn't working... The comment was based on documentation that might be wrong. While the code looks suspicious feel free to leave it as is if it's hard to figure out how it should be done (maybe current code behaves the same as native).
On Sat, Aug 28, 2021 at 03:32:59PM +0200, Piotr Caban wrote:
Hi Connor,
The patch looks good for me.
On 8/12/21 9:07 PM, Connor McAdams wrote:
+ info.cbSize = sizeof(info); + if(GetGUIThreadInfo(GetWindowThreadProcessId(This->hwnd, NULL), &info) && + info.hwndFocus == This->hwnd) It doesn't really matter but you can just call GetGUIThreadInfo(0, &info) here. If This->hwnd thread is not foreground the hwnd comparison will always fail.
Sorry for replying late, I was out last week. Just tested out doing `GetGUIThreadInfo(0, &info)` and it doesn't work properly unless the window is visible, i.e ShowWindow(hwnd, TRUE) is called before GetGUIThreadInfo(0, &info). I also did some more testing on the weird `EVENT_SYSTEM_FOCUSABLE` behavior, and it seems like our current behavior matches Windows, in that any accessible object representing an HWND is always considered `focusable` so long as it isn't disabled, which contradicts the documentation. I can send a newer version of the patch with tests for these, if you think that's necessary. But, I think the current patches work functionally as is, unless there are some stylistic errors.
Hi Connor, On 9/7/21 9:04 PM, Connor McAdams wrote:
On Sat, Aug 28, 2021 at 03:32:59PM +0200, Piotr Caban wrote:
On 8/12/21 9:07 PM, Connor McAdams wrote:
+ info.cbSize = sizeof(info); + if(GetGUIThreadInfo(GetWindowThreadProcessId(This->hwnd, NULL), &info) && + info.hwndFocus == This->hwnd) It doesn't really matter but you can just call GetGUIThreadInfo(0, &info) here. If This->hwnd thread is not foreground the hwnd comparison will always fail.
Sorry for replying late, I was out last week.
Just tested out doing `GetGUIThreadInfo(0, &info)` and it doesn't work properly unless the window is visible, i.e ShowWindow(hwnd, TRUE) is called before GetGUIThreadInfo(0, &info). It looks like a bug in wine. I've done some quick tests and GetForegroundWindow is also returning wrong window in invisible window test.
Here's the test I've tried: GUITHREADINFO info; HWND win, hwnd; win = CreateWindow(L"button", L"button", WS_POPUP, 0, 0, 100, 100, 0, 0, 0, 0); hwnd = SetFocus(win); ok(hwnd == win, "SetFocus\n"); hwnd = GetForegroundWindow(); ok(hwnd == win, "GetForegroundWindow\n"); memset(&info, 0, sizeof(info)); info.cbSize = sizeof(info); GetGUIThreadInfo(0, &info); ok(info.hwndFocus == win, "info.hwndFocus\n"); memset(&info, 0, sizeof(info)); info.cbSize = sizeof(info); GetGUIThreadInfo(GetWindowThreadProcessId(win, NULL), &info); ok(info.hwndFocus == win, "info.hwndFocus\n"); Anyway, while it's better to fix GetGUIThreadInfo, I guess it's OK to workaround it here. It would be good to add a comment though (especially if changing it to the call I have suggested doesn't cause any test failures).
I also did some more testing on the weird `EVENT_SYSTEM_FOCUSABLE` behavior, and it seems like our current behavior matches Windows, in that any accessible object representing an HWND is always considered `focusable` so long as it isn't disabled, which contradicts the documentation.
I've done some tests and it looks like in some cases disabled windows may also be focusable. It's not related to this patch so you can leave it as is. I don't see any easy patterns in my limited testing.
I can send a newer version of the patch with tests for these, if you think that's necessary. But, I think the current patches work functionally as is, unless there are some stylistic errors. You will probably need to resend the patch since it's already fall of the list (after being marked as superseded).
Thanks, Piotr
participants (2)
-
Connor McAdams -
Piotr Caban