This sequence of patches fixes numerous corner cases in CoMsgWaitForMultipleHandles, particularly around the exact sequence in which signaled handles, windows messages, and APCs are considered and which "win" if more than one thing has happened at a time.
This was previously submitted quite some time ago, and rejected as "Needs splitting": https://www.winehq.org/pipermail/wine-devel/2020-November/177529.html Sorry it has taken me so long to get back to this one, but here's an attempt to tease it apart into a number of smaller commits that each make smaller changes and add some of the new tests. Hopefully that makes it easier to follow which change fixes what. Apologies if the splitting is still a little tortured in places, since it re-thought all at once and then split apart, not developed in this order and then squashed. But hopefully this is more readable.
Major fixes are: - closes a race condition with short/zero timeouts, in which it could reports RPC_S_PENDING without ever even checking the handles - cancellation via IMessageFilter::PendingMesage -> `PENDINGMSG_CANCELCALL` now actually exits the wait - makes sure not to pump additional messages after the handles have been signaled - don't use a MsgWait until after seeing that handles were not already signaled. This is mostly important to COWAIT_WAITALL. It's still not advisable to use COWAIT_WAITALL in an STA (for reasons all our favorite Microsoft Gurus have discussed: https://learn.microsoft.com/en-us/archive/blogs/larryosterman/things-you-sho... https://devblogs.microsoft.com/oldnewthing/20060127-17/?p=32493). But it gets fixed in passing as a consequence of the previous changes, and so there are some new conformance tests to show that the new behavior matches windows.
There's also a some new `todo_wine` tests for spurious calls to IMessageFilter_MessagePending. This behavior has *not* changed, but I expanded the test coverage (unsuccessfully) hoping it would give me a hint about what should change. Since I still don't know what the rules should be, I left the new tests `todo_wine`. But maybe they'll be a useful hint someday.
From: Kevin Puetz PuetzKevinA@JohnDeere.com
There is a potential race condition for very small timeouts, as GetTickCount() could increment between `start_time` and the first update of `now`, causing the loop to immediately return timeout (RPC_S_CALLPENDING) without having even checked the handles.
The !message_loop else has no reason to do this; it only runs once, and then always exits via the `break` statement. The only `continue` that can cause the loop to iterate again is in the messages available branch In an the one-and-only call to WaitForMultipleObjectsEx should simply get the entire specified timeout.
The message_loop (STA) case has the same race condition, but it will be fixed in a later commit (after some preparatory refactoring). --- dlls/combase/combase.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/dlls/combase/combase.c b/dlls/combase/combase.c index 0695bb77405..08a648451f0 100644 --- a/dlls/combase/combase.c +++ b/dlls/combase/combase.c @@ -2086,16 +2086,16 @@ HRESULT WINAPI CoWaitForMultipleHandles(DWORD flags, DWORD timeout, ULONG handle
while (TRUE) { - DWORD now = GetTickCount(), res; - - if (now - start_time > timeout) - { - hr = RPC_S_CALLPENDING; - break; - } - if (message_loop) { + DWORD now = GetTickCount(), res; + + if (now - start_time > timeout) + { + hr = RPC_S_CALLPENDING; + break; + } + TRACE("waiting for rpc completion or window message\n");
res = WAIT_TIMEOUT; @@ -2174,7 +2174,7 @@ HRESULT WINAPI CoWaitForMultipleHandles(DWORD flags, DWORD timeout, ULONG handle TRACE("Waiting for rpc completion\n");
res = WaitForMultipleObjectsEx(handle_count, handles, !!(flags & COWAIT_WAITALL), - (timeout == INFINITE) ? INFINITE : start_time + timeout - now, !!(flags & COWAIT_ALERTABLE)); + timeout, !!(flags & COWAIT_ALERTABLE)); }
switch (res)
Hi,
It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated.
The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=124576
Your paranoid android.
=== debian11 (build log) ===
../wine/dlls/combase/combase.c:2176:13: error: ���res��� undeclared (first use in this function) Task: The win32 Wine build failed
=== debian11 (build log) ===
../wine/dlls/combase/combase.c:2176:13: error: ���res��� undeclared (first use in this function) Task: The wow64 Wine build failed
From: Kevin Puetz PuetzKevinA@JohnDeere.com
If a message arrives for which IMessageFilter::MessagePending returns PENDINGMSG_CANCELCALL, CoMsgWaitForMultipleHandles should immediately return RPC_E_CALL_CANCELED, without waiting any longer for its handles, or pumping any messages (including the one which caused the MessagePending)
Previously the code set the desired HRESULT, but then simply looped again and overwrote it. --- dlls/combase/combase.c | 2 ++ dlls/ole32/tests/compobj.c | 45 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+)
diff --git a/dlls/combase/combase.c b/dlls/combase/combase.c index 08a648451f0..5a9dbcab605 100644 --- a/dlls/combase/combase.c +++ b/dlls/combase/combase.c @@ -2130,6 +2130,7 @@ HRESULT WINAPI CoWaitForMultipleHandles(DWORD flags, DWORD timeout, ULONG handle case PENDINGMSG_CANCELCALL: WARN("call canceled\n"); hr = RPC_E_CALL_CANCELED; + goto done; break; case PENDINGMSG_WAITNOPROCESS: case PENDINGMSG_WAITDEFPROCESS: @@ -2191,6 +2192,7 @@ HRESULT WINAPI CoWaitForMultipleHandles(DWORD flags, DWORD timeout, ULONG handle } break; } +done: if (post_quit) PostQuitMessage(exit_code);
TRACE("-- %#lx\n", hr); diff --git a/dlls/ole32/tests/compobj.c b/dlls/ole32/tests/compobj.c index 8467e86a66f..ad8c61085ec 100644 --- a/dlls/ole32/tests/compobj.c +++ b/dlls/ole32/tests/compobj.c @@ -975,6 +975,16 @@ static DWORD WINAPI MessageFilter_MessagePending( return PENDINGMSG_WAITNOPROCESS; }
+static DWORD WINAPI MessageFilter_MessagePending_cancel( + IMessageFilter *iface, + HTASK threadIDCallee, + DWORD dwTickCount, + DWORD dwPendingType) +{ + trace("MessagePending(cancel)\n"); + return PENDINGMSG_CANCELCALL; +} + static const IMessageFilterVtbl MessageFilter_Vtbl = { MessageFilter_QueryInterface, @@ -987,6 +997,18 @@ static const IMessageFilterVtbl MessageFilter_Vtbl =
static IMessageFilter MessageFilter = { &MessageFilter_Vtbl };
+static const IMessageFilterVtbl MessageFilter_Vtbl_cancel = +{ + MessageFilter_QueryInterface, + MessageFilter_AddRef, + MessageFilter_Release, + MessageFilter_HandleInComingCall, + MessageFilter_RetryRejectedCall, + MessageFilter_MessagePending_cancel +}; + +static IMessageFilter MessageFilter_cancel = { &MessageFilter_Vtbl_cancel }; + static void test_CoRegisterMessageFilter(void) { HRESULT hr; @@ -2611,6 +2633,14 @@ static DWORD CALLBACK post_message_thread(LPVOID arg) return 0; }
+static DWORD CALLBACK post_input_later_thread(LPVOID arg) +{ + HWND hWnd = arg; + Sleep(50); + PostMessageA(hWnd, WM_CHAR, VK_ESCAPE, 0); + return 0; +} + static const char cls_name[] = "cowait_test_class";
static UINT cowait_msgs[100], cowait_msgs_first, cowait_msgs_last; @@ -2749,6 +2779,21 @@ static DWORD CALLBACK test_CoWaitForMultipleHandles_thread(LPVOID arg) success = PeekMessageA(&msg, NULL, uMSG, uMSG, PM_REMOVE); ok(success, "CoWaitForMultipleHandles unexpectedly pumped messages\n");
+ hr = CoRegisterMessageFilter(&MessageFilter_cancel, NULL); + ok(hr == S_OK, "CoRegisterMessageFilter failed: %08lx\n", hr); + + /* a message which arrives during the wait calls IMessageFilter::PendingMessage, + * which can cancel the wait (without pumping the message) */ + thread = CreateThread(NULL, 0, post_input_later_thread, hWnd, 0, &tid); + hr = CoWaitForMultipleHandles(0, 200, 2, handles, &index); + ok(hr == RPC_E_CALL_CANCELED, "expected RPC_E_CALL_CANCELED, got 0x%08lx\n", hr); + success = PeekMessageA(&msg, hWnd, WM_CHAR, WM_CHAR, PM_REMOVE); + ok(success, "CoWaitForMultipleHandles unexpectedly pumped messages\n"); + CloseHandle(thread); + + hr = CoRegisterMessageFilter(NULL, NULL); + ok(hr == S_OK, "CoRegisterMessageFilter failed: %08lx\n", hr); + DestroyWindow(hWnd); CoUninitialize();
Hi,
It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated.
The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details:
The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=124577
Your paranoid android.
=== debian11 (build log) ===
../wine/dlls/combase/combase.c:2177:13: error: ���res��� undeclared (first use in this function) Task: The win32 Wine build failed
=== debian11 (build log) ===
../wine/dlls/combase/combase.c:2177:13: error: ���res��� undeclared (first use in this function) Task: The wow64 Wine build failed
From: Kevin Puetz PuetzKevinA@JohnDeere.com
This wasn't actually uninitialized before; it was the non-FAILED return value of com_get_tlsdata. Which was presumably S_OK, but that's kind of subtle. --- dlls/combase/combase.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/dlls/combase/combase.c b/dlls/combase/combase.c index 5a9dbcab605..1acf66c8f94 100644 --- a/dlls/combase/combase.c +++ b/dlls/combase/combase.c @@ -2187,6 +2187,7 @@ HRESULT WINAPI CoWaitForMultipleHandles(DWORD flags, DWORD timeout, ULONG handle hr = HRESULT_FROM_WIN32(GetLastError()); break; default: + hr = S_OK; *index = res; break; }
Hi,
It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated.
The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=124578
Your paranoid android.
=== debian11 (build log) ===
../wine/dlls/combase/combase.c:2177:13: error: ���res��� undeclared (first use in this function) Task: The win32 Wine build failed
=== debian11 (build log) ===
../wine/dlls/combase/combase.c:2177:13: error: ���res��� undeclared (first use in this function) Task: The wow64 Wine build failed
From: Kevin Puetz PuetzKevinA@JohnDeere.com
- Transpose the if(message_loop) test and while(TRUE); if there is no message loop, there is no need for a loop at all. - Since the decoding of WAIT_* into *index/HRESULT is now outside the loop (not skipped by `break`) the early exit RPC_S_CALLPENDING can be handled the same as any other WAIT_TIMEOUT. - Several local variables can be moved inside the if(message_loop) branch
This commit should cause no functional change; it was separated because the changes to indentation/nesting would make the others harder to read. --- dlls/combase/combase.c | 73 +++++++++++++++++++++--------------------- 1 file changed, 36 insertions(+), 37 deletions(-)
diff --git a/dlls/combase/combase.c b/dlls/combase/combase.c index 1acf66c8f94..e5311632305 100644 --- a/dlls/combase/combase.c +++ b/dlls/combase/combase.c @@ -2052,10 +2052,9 @@ HRESULT WINAPI CoWaitForMultipleHandles(DWORD flags, DWORD timeout, ULONG handle DWORD *index) { BOOL check_apc = !!(flags & COWAIT_ALERTABLE), post_quit = FALSE, message_loop; - DWORD start_time, wait_flags = 0; - struct tlsdata *tlsdata; struct apartment *apt; UINT exit_code; + DWORD res; HRESULT hr;
TRACE("%#lx, %#lx, %lu, %p, %p\n", flags, timeout, handle_count, handles, index); @@ -2071,35 +2070,35 @@ HRESULT WINAPI CoWaitForMultipleHandles(DWORD flags, DWORD timeout, ULONG handle if (!handle_count) return RPC_E_NO_SYNC;
- if (FAILED(hr = com_get_tlsdata(&tlsdata))) - return hr; - apt = com_get_current_apt(); message_loop = apt && !apt->multi_threaded;
- if (flags & COWAIT_WAITALL) - wait_flags |= MWMO_WAITALL; - if (flags & COWAIT_ALERTABLE) - wait_flags |= MWMO_ALERTABLE; + if (message_loop) + { + DWORD start_time, wait_flags = 0; + struct tlsdata *tlsdata;
- start_time = GetTickCount(); + if (FAILED(hr = com_get_tlsdata(&tlsdata))) + return hr;
- while (TRUE) - { - if (message_loop) - { - DWORD now = GetTickCount(), res; + if (flags & COWAIT_WAITALL) + wait_flags |= MWMO_WAITALL; + if (flags & COWAIT_ALERTABLE) + wait_flags |= MWMO_ALERTABLE; + + start_time = GetTickCount();
+ while (TRUE) + { + DWORD now = GetTickCount(); + res = WAIT_TIMEOUT; if (now - start_time > timeout) { - hr = RPC_S_CALLPENDING; break; }
TRACE("waiting for rpc completion or window message\n");
- res = WAIT_TIMEOUT; - if (check_apc) { res = WaitForMultipleObjectsEx(handle_count, handles, !!(flags & COWAIT_WAITALL), 0, TRUE); @@ -2169,28 +2168,28 @@ HRESULT WINAPI CoWaitForMultipleHandles(DWORD flags, DWORD timeout, ULONG handle } continue; } + break; } - else - { - TRACE("Waiting for rpc completion\n"); + } + else + { + TRACE("Waiting for rpc completion\n");
- res = WaitForMultipleObjectsEx(handle_count, handles, !!(flags & COWAIT_WAITALL), - timeout, !!(flags & COWAIT_ALERTABLE)); - } + res = WaitForMultipleObjectsEx(handle_count, handles, !!(flags & COWAIT_WAITALL), + timeout, !!(flags & COWAIT_ALERTABLE)); + }
- switch (res) - { - case WAIT_TIMEOUT: - hr = RPC_S_CALLPENDING; - break; - case WAIT_FAILED: - hr = HRESULT_FROM_WIN32(GetLastError()); - break; - default: - hr = S_OK; - *index = res; - break; - } + switch (res) + { + case WAIT_TIMEOUT: + hr = RPC_S_CALLPENDING; + break; + case WAIT_FAILED: + hr = HRESULT_FROM_WIN32(GetLastError()); + break; + default: + hr = S_OK; + *index = res; break; } done:
From: Kevin Puetz PuetzKevinA@JohnDeere.com
The order of operations here is quite sensitive, in order to keep the desired priority of various message types and queue state in sync. Splitting it betwen two functions makes the dependencies less obvious. Combining them into a single if/else makes it clearer that both cases do /* peek at messages so that it will not trigger MsgWaitForMultipleObjects next time. */ with an apt->win the message was removed and dispatched; without one only the side-effect (clearing QS_ALLPOSTMESSAGE) was used. --- dlls/combase/combase.c | 40 ++++++++++++++++++++++++++++------------ 1 file changed, 28 insertions(+), 12 deletions(-)
diff --git a/dlls/combase/combase.c b/dlls/combase/combase.c index e5311632305..9d6a16b54c0 100644 --- a/dlls/combase/combase.c +++ b/dlls/combase/combase.c @@ -2038,11 +2038,34 @@ HRESULT WINAPI CoRevokeInitializeSpy(ULARGE_INTEGER cookie)
static BOOL com_peek_message(struct apartment *apt, MSG *msg) { - /* First try to retrieve messages for incoming COM calls to the apartment window */ - return (apt->win && PeekMessageW(msg, apt->win, 0, 0, PM_REMOVE | PM_NOYIELD)) || - /* Next retrieve other messages necessary for the app to remain responsive */ - PeekMessageW(msg, NULL, WM_DDE_FIRST, WM_DDE_LAST, PM_REMOVE | PM_NOYIELD) || - PeekMessageW(msg, NULL, 0, 0, PM_QS_PAINT | PM_QS_SENDMESSAGE | PM_REMOVE | PM_NOYIELD); + BOOL PeekMessage_RPC; + /* This first peek (and no other) must be "without filtering messages" in order to reset QS_ALLPOSTMESSAGE */ + if (apt->win) + { + /* Try to retrieve messages for incoming COM calls to the apartment window. + * peeking at a specific hWnd doesn't count as filtering, so this clears QS_ALLPOSTMESSAGE. */ + PeekMessage_RPC = PeekMessageW(msg, apt->win, 0, 0, PM_REMOVE | PM_NOYIELD); + } + else + { + /* Reset QS_ALLPOSTMESSAGE without removing anything from the queue. */ + PeekMessageW(NULL, NULL, 0, 0, PM_QS_POSTMESSAGE | PM_NOREMOVE | PM_NOYIELD); + PeekMessage_RPC = FALSE; + } + + return PeekMessage_RPC || + /* Next retrieve other messages necessary for the app to remain responsive. + * These subsequent peeks must all be filtered; clearing the queue status again could + * prevent newly-posted messages that match earlier PeekMessage calls from waking MsgWait. */ + PeekMessageW(msg, NULL, WM_DDE_FIRST, WM_DDE_LAST, PM_REMOVE | PM_NOYIELD) || /* filters by WM_DDE_* */ + PeekMessageW(msg, NULL, 0, 0, PM_QS_PAINT | PM_QS_SENDMESSAGE | PM_REMOVE | PM_NOYIELD); /* filters by PM_QS_*, excluding POSTMESSAGE */ + + /* When com_peek_message returns FALSE, it cleared QS_ALLPOSTMESSAGE and subsequently found nothing interesting + * Caller may use MsgWaitForMultipleObjects(PM_QS_ALLPOSTMESSAGE) to wait for new activity. + * + * When it returns TRUE, *msg contains the next message that should be dispatched, + * The queue contains un-examined messages, so com_peek_message should be called again + * (to finish draining it) before using MsgWaitForMultipleObjects */ }
/*********************************************************************** @@ -2142,13 +2165,6 @@ HRESULT WINAPI CoWaitForMultipleHandles(DWORD flags, DWORD timeout, ULONG handle } }
- if (!apt->win) - { - /* If window is NULL on apartment, peek at messages so that it will not trigger - * MsgWaitForMultipleObjects next time. */ - PeekMessageW(NULL, NULL, 0, 0, PM_QS_POSTMESSAGE | PM_NOREMOVE | PM_NOYIELD); - } - /* Some apps (e.g. Visio 2010) don't handle WM_PAINT properly and loop forever, * so after processing 100 messages we go back to checking the wait handles */ while (msg_count++ < 100 && com_peek_message(apt, &msg))
From: Kevin Puetz PuetzKevinA@JohnDeere.com
windows does not call MessagePending for DDE/RPC messages, but wine does. I'm not sure how this is supposed to be distinguished, since MessagePending call is also supposed to happen before the message is pumped, so we don't actually even know what message woke up MsgWaitForMultipleObjectsEx.
This same behavior is already seen in the existing test which uses cowait_unmarshal_thread (also todo_wine, in MessageFilter_MessagePending) This additional test just shows that the (mis-)behavior also affects DDE. --- dlls/ole32/tests/compobj.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
diff --git a/dlls/ole32/tests/compobj.c b/dlls/ole32/tests/compobj.c index ad8c61085ec..c9e337b858e 100644 --- a/dlls/ole32/tests/compobj.c +++ b/dlls/ole32/tests/compobj.c @@ -2641,6 +2641,14 @@ static DWORD CALLBACK post_input_later_thread(LPVOID arg) return 0; }
+static DWORD CALLBACK post_dde_later_thread(LPVOID arg) +{ + HWND hWnd = arg; + Sleep(50); + PostMessageA(hWnd, WM_DDE_FIRST, 0, 0); + return 0; +} + static const char cls_name[] = "cowait_test_class";
static UINT cowait_msgs[100], cowait_msgs_first, cowait_msgs_last; @@ -2791,6 +2799,13 @@ static DWORD CALLBACK test_CoWaitForMultipleHandles_thread(LPVOID arg) ok(success, "CoWaitForMultipleHandles unexpectedly pumped messages\n"); CloseHandle(thread);
+ /* DDE/RPC messages shouldn't go to IMessageFilter::PendingMessage */ + thread = CreateThread(NULL, 0, post_dde_later_thread, hWnd, 0, &tid); + hr = CoWaitForMultipleHandles(0, 200, 1, &thread, &index); + todo_wine ok(hr == S_OK, "expected S_OK, got 0x%08lx\n", hr); /* wine gets RPC_E_CALL_CANCELED, because of MessageFilter_cancel */ + ok(index == WAIT_OBJECT_0, "post_dde_later_thread didn't finish\n"); + CloseHandle(thread); + hr = CoRegisterMessageFilter(NULL, NULL); ok(hr == S_OK, "CoRegisterMessageFilter failed: %08lx\n", hr);
From: Kevin Puetz PuetzKevinA@JohnDeere.com
These will be used to test the sequence in which CoWaitForMultipleHandles interleaves com_peek_message/DispatchMessage vs sychronizing on handles.
They are really mocking interactions with some RPC-marshaled interface, but it's considerably simpler to make PostMessage calls than to set up an RPC channel, whose internal use of CoWaitForMultipleHandles would further complicate any test traces/debugging. --- dlls/ole32/tests/compobj.c | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+)
diff --git a/dlls/ole32/tests/compobj.c b/dlls/ole32/tests/compobj.c index c9e337b858e..94df7195e3c 100644 --- a/dlls/ole32/tests/compobj.c +++ b/dlls/ole32/tests/compobj.c @@ -2706,6 +2706,18 @@ static LRESULT CALLBACK cowait_window_proc(HWND hwnd, UINT msg, WPARAM wparam, L cowait_msgs[cowait_msgs_last++] = msg; if(msg == WM_DDE_FIRST) return 6; + if(msg == WM_DDE_EXECUTE && lparam) + { + const char* command = (const char *)GlobalLock((HGLOBAL)lparam); + if(strcmp(command,"[apc]") == 0) + QueueUserAPC(apc_test_proc, GetCurrentThread(), 0); + else if(strcmp(command,"[postmessage]") == 0) + PostMessageA(hwnd,msg,wparam,lparam); /* post the same message again (trigges livelock) */ + else if(strcmp(command,"[semaphore]") == 0) + ReleaseSemaphore(GetPropA(hwnd,"semaphore"), 1, NULL); + GlobalUnlock((HGLOBAL)lparam); + return 0; + } return DefWindowProcA(hwnd, msg, wparam, lparam); }
@@ -2848,6 +2860,15 @@ static DWORD CALLBACK test_CoWaitForMultipleHandles_thread(LPVOID arg) return 0; }
+static HGLOBAL globalalloc_string(const char *s) { + UINT len = strlen(s); + HGLOBAL ret = GlobalAlloc(GMEM_FIXED,len+1); + void *ptr = GlobalLock(ret); + strcpy(ptr,s); + GlobalUnlock(ret); + return ret; +} + static void test_CoWaitForMultipleHandles(void) { HANDLE handles[2], thread; @@ -2857,6 +2878,9 @@ static void test_CoWaitForMultipleHandles(void) HRESULT hr; HWND hWnd; MSG msg; + HGLOBAL execute_apc = globalalloc_string("[apc]"); + HGLOBAL execute_postmessage = globalalloc_string("[postmessage]"); + HGLOBAL execute_semaphore = globalalloc_string("[semaphore]");
hr = CoInitializeEx(NULL, COINIT_APARTMENTTHREADED); ok(hr == S_OK, "CoInitializeEx failed with error 0x%08lx\n", hr); @@ -2879,6 +2903,8 @@ static void test_CoWaitForMultipleHandles(void) handles[1] = CreateSemaphoreA(NULL, 1, 1, NULL); ok(handles[1] != 0, "CreateSemaphoreA failed %lu\n", GetLastError());
+ SetPropA(hWnd,"semaphore",handles[0]); + /* test without flags */
PostMessageA(hWnd, WM_DDE_FIRST, 0, 0); @@ -3225,6 +3251,10 @@ static void test_CoWaitForMultipleHandles(void)
CoUninitialize();
+ RemovePropA(hWnd,"semaphore"); + GlobalFree(execute_apc); + GlobalFree(execute_postmessage); + GlobalFree(execute_semaphore); CloseHandle(handles[0]); CloseHandle(handles[1]); DestroyWindow(hWnd);
From: Kevin Puetz PuetzKevinA@JohnDeere.com
Only one message at a time should be dispatched; if that message results in the handles becoming completed (or an APC queued, if COWAIT_ALERTABLE), the loop should immediately stop and return that outcome.
This replaces the arbitrary "100 message" limit; RPC/DDE messages get priority over WM_PAINT, and as long as they cause the handles to signal there's no need to reach an empty message queue. This also fixes a potential deadlock where MsgWaitForMultipleObjectsEx would wait for new messages when there were actually still un-examined messages left in the queue because the 100 message limit had been previously hit. --- dlls/combase/combase.c | 12 ++++++------ dlls/ole32/tests/compobj.c | 21 +++++++++++++++++++++ 2 files changed, 27 insertions(+), 6 deletions(-)
diff --git a/dlls/combase/combase.c b/dlls/combase/combase.c index 9d6a16b54c0..fc8414b52d3 100644 --- a/dlls/combase/combase.c +++ b/dlls/combase/combase.c @@ -2098,7 +2098,7 @@ HRESULT WINAPI CoWaitForMultipleHandles(DWORD flags, DWORD timeout, ULONG handle
if (message_loop) { - DWORD start_time, wait_flags = 0; + DWORD start_time, wait_flags = MWMO_INPUTAVAILABLE; struct tlsdata *tlsdata;
if (FAILED(hr = com_get_tlsdata(&tlsdata))) @@ -2125,7 +2125,6 @@ HRESULT WINAPI CoWaitForMultipleHandles(DWORD flags, DWORD timeout, ULONG handle if (check_apc) { res = WaitForMultipleObjectsEx(handle_count, handles, !!(flags & COWAIT_WAITALL), 0, TRUE); - check_apc = FALSE; }
if (res == WAIT_TIMEOUT) @@ -2135,7 +2134,6 @@ HRESULT WINAPI CoWaitForMultipleHandles(DWORD flags, DWORD timeout, ULONG handle
if (res == WAIT_OBJECT_0 + handle_count) /* messages available */ { - int msg_count = 0; MSG msg;
/* call message filter */ @@ -2165,10 +2163,9 @@ HRESULT WINAPI CoWaitForMultipleHandles(DWORD flags, DWORD timeout, ULONG handle } }
- /* Some apps (e.g. Visio 2010) don't handle WM_PAINT properly and loop forever, - * so after processing 100 messages we go back to checking the wait handles */ - while (msg_count++ < 100 && com_peek_message(apt, &msg)) + if(com_peek_message(apt, &msg)) { + wait_flags |= MWMO_INPUTAVAILABLE; if (msg.message == WM_QUIT) { TRACE("Received WM_QUIT message\n"); @@ -2181,6 +2178,9 @@ HRESULT WINAPI CoWaitForMultipleHandles(DWORD flags, DWORD timeout, ULONG handle TranslateMessage(&msg); DispatchMessageW(&msg); } + } else { + /* no interesting input remains in the queue, so block until *new* messages are posted */ + wait_flags &= ~MWMO_INPUTAVAILABLE; } continue; } diff --git a/dlls/ole32/tests/compobj.c b/dlls/ole32/tests/compobj.c index 94df7195e3c..66680ba5952 100644 --- a/dlls/ole32/tests/compobj.c +++ b/dlls/ole32/tests/compobj.c @@ -2953,6 +2953,17 @@ static void test_CoWaitForMultipleHandles(void) success = PeekMessageA(&msg, hWnd, WM_DDE_FIRST, WM_DDE_FIRST, PM_REMOVE); ok(!success, "CoWaitForMultipleHandles didn't pump any messages\n");
+ /* test CoWaitForMultipleHandles stops pumping messages as soon as its handles are signaled */ + index = 0xdeadbeef; + PostMessageA(hWnd, WM_DDE_EXECUTE, 0, (LPARAM)execute_semaphore); + PostMessageA(hWnd, WM_DDE_FIRST, 0, 0); + hr = CoWaitForMultipleHandles(0, 50, 1, handles, &index); + ok(hr == S_OK, "expected S_OK, got 0x%08lx\n", hr); + ok(index == 0, "expected index 0, got %lu\n", index); + cowait_msgs_expect_queued(hWnd,WM_DDE_FIRST); /* WM_DDE_EXECUTE already pumped*/ + success = PeekMessageA(&msg, hWnd, WM_DDE_FIRST, WM_DDE_LAST, PM_REMOVE); + ok(!success, "CoWaitForMultipleHandles didn't pump enough messages\n"); + /* test PostMessageA/SendMessageA from a different thread */
index = 0xdeadbeef; @@ -3039,6 +3050,16 @@ static void test_CoWaitForMultipleHandles(void) success = PeekMessageA(&msg, hWnd, WM_DDE_FIRST, WM_DDE_FIRST, PM_REMOVE); ok(success, "CoWaitForMultipleHandles unexpectedly pumped messages\n");
+ index = 0xdeadbeef; + PostMessageA(hWnd, WM_DDE_EXECUTE, 0, (LPARAM)execute_apc); + PostMessageA(hWnd, WM_DDE_FIRST, 0, 0); + hr = CoWaitForMultipleHandles(COWAIT_ALERTABLE, 50, 1, handles, &index); + ok(hr == S_OK, "expected S_OK, got 0x%08lx\n", hr); + ok(index == WAIT_IO_COMPLETION, "expected index WAIT_IO_COMPLETION, got %lu\n", index); + cowait_msgs_expect_queued(hWnd,WM_DDE_FIRST); /* WM_DDE_EXECUTE already pumped*/ + success = PeekMessageA(&msg, hWnd, WM_DDE_FIRST, WM_DDE_LAST, PM_REMOVE); + ok(!success, "CoWaitForMultipleHandles didn't pump enough messages\n"); + /* test with COWAIT_INPUTAVAILABLE (semaphores are still locked) */
index = 0xdeadbeef;
From: Kevin Puetz PuetzKevinA@JohnDeere.com
All cases, not just COWAIT_ALTERTABLE (APCs) should check for success before blocking in MsgWaitForMultipleObjectsEx. Using COWAIT_WAITALL in an STA has the same well-known drawback as MWMO_WAITALL; the queue status then counts as part of the "all handles" return.
But this test shows that that should apply only if there is a need to wait for messages at all - if all the specified handles are signaled, CoWaitForMultipleHandles shouldn't check messages at all. --- dlls/combase/combase.c | 7 ++----- dlls/ole32/tests/compobj.c | 15 +++++++++++++++ 2 files changed, 17 insertions(+), 5 deletions(-)
diff --git a/dlls/combase/combase.c b/dlls/combase/combase.c index fc8414b52d3..42e9fbc2468 100644 --- a/dlls/combase/combase.c +++ b/dlls/combase/combase.c @@ -2074,7 +2074,7 @@ static BOOL com_peek_message(struct apartment *apt, MSG *msg) HRESULT WINAPI CoWaitForMultipleHandles(DWORD flags, DWORD timeout, ULONG handle_count, HANDLE *handles, DWORD *index) { - BOOL check_apc = !!(flags & COWAIT_ALERTABLE), post_quit = FALSE, message_loop; + BOOL post_quit = FALSE, message_loop; struct apartment *apt; UINT exit_code; DWORD res; @@ -2122,10 +2122,7 @@ HRESULT WINAPI CoWaitForMultipleHandles(DWORD flags, DWORD timeout, ULONG handle
TRACE("waiting for rpc completion or window message\n");
- if (check_apc) - { - res = WaitForMultipleObjectsEx(handle_count, handles, !!(flags & COWAIT_WAITALL), 0, TRUE); - } + res = WaitForMultipleObjectsEx(handle_count, handles, !!(flags & COWAIT_WAITALL), 0, !!(flags & COWAIT_ALERTABLE));
if (res == WAIT_TIMEOUT) res = MsgWaitForMultipleObjectsEx(handle_count, handles, diff --git a/dlls/ole32/tests/compobj.c b/dlls/ole32/tests/compobj.c index 66680ba5952..1e011e1354b 100644 --- a/dlls/ole32/tests/compobj.c +++ b/dlls/ole32/tests/compobj.c @@ -3011,6 +3011,21 @@ static void test_CoWaitForMultipleHandles(void) success = PeekMessageA(&msg, hWnd, WM_DDE_FIRST, WM_DDE_FIRST, PM_REMOVE); ok(!success, "CoWaitForMultipleHandles didn't pump any messages\n");
+ ReleaseSemaphore(handles[0], 1, NULL); + + /* COWAIT_ALL will get time out even if the handles became signaled while it waits + * in MsgWaitForMultipleObjects(...,MWIO_WAITALL), as it demands a posted message too */ + index = 0xdeadbeef; + thread = CreateThread(NULL, 0, release_semaphore_thread, handles[1], 0, &tid); + hr = CoWaitForMultipleHandles(COWAIT_WAITALL, 500, 2, handles, &index); + ok(hr == RPC_S_CALLPENDING, "expected RPC_S_CALLPENDING, got 0x%08lx\n", hr); + + /* but will succeed immediately if the handles are already available + * i.e. it checks the handles first before calling MsgWaitForMultipleObjects */ + hr = CoWaitForMultipleHandles(COWAIT_WAITALL, 0, 2, handles, &index); + ok(hr == S_OK, "expected S_OK, got 0x%08lx\n", hr); + ok(index == 0, "expected index 0, got %lu\n", index); + ReleaseSemaphore(handles[0], 1, NULL); ReleaseSemaphore(handles[1], 1, NULL);
From: Kevin Puetz PuetzKevinA@JohnDeere.com
Timeout errors should only occur before/during sleep. If the thread calling CoWaitForMultipleHandles experienced a large scheduling delay (or is simply running slow) it should finish pumping messages/checking handles (and maybe succeed) before declaring WAIT_TIMEOUT/RPC_S_CALLPENDING.
Notably, the previous code had a narrow race (particularly for timeout=0) if GetTickCount increments between start_time and `now`, it could return WAIT_TIMEOUT even when the handles were already signaled before the call. This could also manifest even with longer timeouts, if it dispatched a message whose WNDPROC took quite a while to return control.
The order of operations in CoWaitForMultipleHandles should be:
1. check for completion, and return success if so 2. pump a message that might cause progress toward completion if anything was dispatched, return to step 1 and re-check 3. If there is still timeout remaining, wait for something to change
NOTE: this means CoWaitForMultipleHandles does not time out while there are still relevant messages, and will livelock (and never time out) if dispatching of messages continuously posts additional work. It will exit (successfully) if the handles eventually become signaled.
--
That "overdue success" case is tested, and proves that the CoWait will keep going past the specified timeout. The "indefinitely livelock" when the handles are never signaled has been verified on windows 10 (omit release_semaphore_thread to see it), but I couldn't think of a way to express that as an automated test. --- dlls/combase/combase.c | 108 +++++++++++++++++++------------------ dlls/ole32/tests/compobj.c | 43 +++++++++++++++ 2 files changed, 98 insertions(+), 53 deletions(-)
diff --git a/dlls/combase/combase.c b/dlls/combase/combase.c index 42e9fbc2468..fe366b2ba8a 100644 --- a/dlls/combase/combase.c +++ b/dlls/combase/combase.c @@ -2098,7 +2098,7 @@ HRESULT WINAPI CoWaitForMultipleHandles(DWORD flags, DWORD timeout, ULONG handle
if (message_loop) { - DWORD start_time, wait_flags = MWMO_INPUTAVAILABLE; + DWORD start_time, wait_flags = 0; struct tlsdata *tlsdata;
if (FAILED(hr = com_get_tlsdata(&tlsdata))) @@ -2114,74 +2114,76 @@ HRESULT WINAPI CoWaitForMultipleHandles(DWORD flags, DWORD timeout, ULONG handle while (TRUE) { DWORD now = GetTickCount(); - res = WAIT_TIMEOUT; - if (now - start_time > timeout) - { - break; - } + MSG msg;
TRACE("waiting for rpc completion or window message\n");
res = WaitForMultipleObjectsEx(handle_count, handles, !!(flags & COWAIT_WAITALL), 0, !!(flags & COWAIT_ALERTABLE));
- if (res == WAIT_TIMEOUT) - res = MsgWaitForMultipleObjectsEx(handle_count, handles, + if (res != WAIT_TIMEOUT) + { + break; + } + else if (com_peek_message(apt, &msg)) + { + if (msg.message == WM_QUIT) + { + TRACE("Received WM_QUIT message\n"); + post_quit = TRUE; + exit_code = msg.wParam; + } + else + { + TRACE("Received message whilst waiting for RPC: 0x%04x\n", msg.message); + TranslateMessage(&msg); + DispatchMessageW(&msg); + } + } + else + { + if (now - start_time <= timeout) + { + /* nothing to return, no messages to pump, time remaining: + * sleep for the remaining timeout (or until something happens) */ + res = MsgWaitForMultipleObjectsEx(handle_count, handles, timeout == INFINITE ? INFINITE : start_time + timeout - now, QS_SENDMESSAGE | QS_ALLPOSTMESSAGE | QS_PAINT, wait_flags); + }
- if (res == WAIT_OBJECT_0 + handle_count) /* messages available */ - { - MSG msg; - - /* call message filter */ - - if (apt->filter) + if (res == WAIT_OBJECT_0 + handle_count) /* messages available */ { - PENDINGTYPE pendingtype = tlsdata->pending_call_count_server ? PENDINGTYPE_NESTED : PENDINGTYPE_TOPLEVEL; - DWORD be_handled = IMessageFilter_MessagePending(apt->filter, 0 /* FIXME */, now - start_time, pendingtype); - - TRACE("IMessageFilter_MessagePending returned %ld\n", be_handled); + /* call message filter */
- switch (be_handled) + if (apt->filter) { - case PENDINGMSG_CANCELCALL: - WARN("call canceled\n"); - hr = RPC_E_CALL_CANCELED; - goto done; - break; - case PENDINGMSG_WAITNOPROCESS: - case PENDINGMSG_WAITDEFPROCESS: - default: - /* FIXME: MSDN is very vague about the difference - * between WAITNOPROCESS and WAITDEFPROCESS - there - * appears to be none, so it is possibly a left-over - * from the 16-bit world. */ - break; + PENDINGTYPE pendingtype = tlsdata->pending_call_count_server ? PENDINGTYPE_NESTED : PENDINGTYPE_TOPLEVEL; + DWORD be_handled = IMessageFilter_MessagePending(apt->filter, 0 /* FIXME */, now - start_time, pendingtype); + + TRACE("IMessageFilter_MessagePending returned %ld\n", be_handled); + + switch (be_handled) + { + case PENDINGMSG_CANCELCALL: + WARN("call canceled\n"); + hr = RPC_E_CALL_CANCELED; + goto done; + break; + case PENDINGMSG_WAITNOPROCESS: + case PENDINGMSG_WAITDEFPROCESS: + default: + /* FIXME: MSDN is very vague about the difference + * between WAITNOPROCESS and WAITDEFPROCESS - there + * appears to be none, so it is possibly a left-over + * from the 16-bit world. */ + break; + } } } - - if(com_peek_message(apt, &msg)) + else { - wait_flags |= MWMO_INPUTAVAILABLE; - if (msg.message == WM_QUIT) - { - TRACE("Received WM_QUIT message\n"); - post_quit = TRUE; - exit_code = msg.wParam; - } - else - { - TRACE("Received message whilst waiting for RPC: 0x%04x\n", msg.message); - TranslateMessage(&msg); - DispatchMessageW(&msg); - } - } else { - /* no interesting input remains in the queue, so block until *new* messages are posted */ - wait_flags &= ~MWMO_INPUTAVAILABLE; + break; } - continue; } - break; } } else diff --git a/dlls/ole32/tests/compobj.c b/dlls/ole32/tests/compobj.c index 1e011e1354b..869ab99122c 100644 --- a/dlls/ole32/tests/compobj.c +++ b/dlls/ole32/tests/compobj.c @@ -2881,6 +2881,7 @@ static void test_CoWaitForMultipleHandles(void) HGLOBAL execute_apc = globalalloc_string("[apc]"); HGLOBAL execute_postmessage = globalalloc_string("[postmessage]"); HGLOBAL execute_semaphore = globalalloc_string("[semaphore]"); + DWORD start_time;
hr = CoInitializeEx(NULL, COINIT_APARTMENTTHREADED); ok(hr == S_OK, "CoInitializeEx failed with error 0x%08lx\n", hr); @@ -2964,6 +2965,23 @@ static void test_CoWaitForMultipleHandles(void) success = PeekMessageA(&msg, hWnd, WM_DDE_FIRST, WM_DDE_LAST, PM_REMOVE); ok(!success, "CoWaitForMultipleHandles didn't pump enough messages\n");
+ /* test CoWaitForMultipleHandles will keep working past the timeout if the queue + * still has messages (this is ensured by posting a message that, when dispatched, + * just posts another identical message, so the queue can *never* be drained). + * but it will still exit with success once the handles become signaled */ + index = 0xdeadbeef; + PostMessageA(hWnd, WM_DDE_EXECUTE, 0, (LPARAM)execute_postmessage); + start_time = GetTickCount(); + /* if this thread is disabled, the call to CoWaitForMultipleHandles will livelock forever, on both windows and wine */ + thread = CreateThread(NULL, 0, release_semaphore_thread, handles[0], 0, &tid); + hr = CoWaitForMultipleHandles(0, 50, 1, handles, &index); + ok(GetTickCount() - start_time >= 200, "CoWaitForMultipleHandles exited too soon\n"); + ok(hr == S_OK, "expected S_OK, got 0x%08lx\n", hr); + cowait_msgs_expect_queued(hWnd,WM_DDE_EXECUTE); /* each pumped execute_postmessage added one more back */ + success = PeekMessageA(&msg, hWnd, WM_DDE_FIRST, WM_DDE_LAST, PM_REMOVE); + ok(!success, "CoWaitForMultipleHandles didn't pump enough messages\n"); + CloseHandle(thread); + /* test PostMessageA/SendMessageA from a different thread */
index = 0xdeadbeef; @@ -3026,6 +3044,31 @@ static void test_CoWaitForMultipleHandles(void) ok(hr == S_OK, "expected S_OK, got 0x%08lx\n", hr); ok(index == 0, "expected index 0, got %lu\n", index);
+ ReleaseSemaphore(handles[1], 1, NULL); + + /* COWAIT_ALL will pump message which are already in the queue, + * (but no longer QS_ALLPOSTMESSAGE), before blocking in MsgWaitForMultipleObjectsEx */ + index = 0xdeadbeef; + PostMessageA(hWnd, WM_DDE_EXECUTE, 0, (LPARAM)execute_semaphore); + PeekMessageA(&msg, hWnd, 0, 0, PM_NOREMOVE); // clear QS_ALLPOSTMESSAGE + hr = CoWaitForMultipleHandles(COWAIT_WAITALL, 50, 1, handles, &index); + ok(hr == S_OK, "expected S_OK, got 0x%08lx\n", hr); + ok(index == 0, "expected index 0, got %lu\n", index); + success = PeekMessageA(&msg, hWnd, WM_DDE_FIRST, WM_DDE_FIRST, PM_REMOVE); + ok(!success, "CoWaitForMultipleHandles didn't pump any messages\n"); + + ReleaseSemaphore(handles[1], 1, NULL); + + /* test early completion (rather than blocking in MsgWaitForMultipleObjectsEx again) + * if pumping a message results in all handles becoming signaled) */ + index = 0xdeadbeef; + PostMessageA(hWnd, WM_DDE_EXECUTE, 0, (LPARAM)execute_semaphore); + hr = CoWaitForMultipleHandles(COWAIT_WAITALL, 50, 2, handles, &index); + ok(hr == S_OK, "expected S_OK, got 0x%08lx\n", hr); + ok(index == 0, "expected index 0, got %lu\n", index); + success = PeekMessageA(&msg, hWnd, WM_DDE_FIRST, WM_DDE_FIRST, PM_REMOVE); + ok(!success, "CoWaitForMultipleHandles didn't pump any messages\n"); + ReleaseSemaphore(handles[0], 1, NULL); ReleaseSemaphore(handles[1], 1, NULL);
-
Kevin Puetz -
Kevin Puetz (＠puetzk)
-
Marvin