Some preliminary cleanups (split off from !969)
- extend an existing `wine_todo` test to show that DDE messages cause spurious calls to IMessageFilter::MessagePending (the same as RPC ones) I have no current ideas for fixing this, just demonstrating the behavior (before refactoring the code in ways that won't change this) - extract a helper `com_filter_messagepending` for the interactions between `CoWaitForMultipleHandles` and IMessageFilter::MessagePending - Pull the remaining use of `PeekMessageW` into the existing `com_peek_message` helper - one behavior fix, to make `RPC_E_CALL_CANCELED` actually work
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 shows that the (mis-)behavior also affects DDE. --- dlls/ole32/tests/compobj.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
diff --git a/dlls/ole32/tests/compobj.c b/dlls/ole32/tests/compobj.c index 8467e86a66f..5aeddbd809e 100644 --- a/dlls/ole32/tests/compobj.c +++ b/dlls/ole32/tests/compobj.c @@ -2611,6 +2611,14 @@ static DWORD CALLBACK post_message_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; @@ -2769,6 +2777,7 @@ static DWORD CALLBACK test_CoWaitForMultipleHandles_thread(LPVOID arg) hr = CoRegisterMessageFilter(&MessageFilter, NULL); ok(hr == S_OK, "CoRegisterMessageFilter failed: %08lx\n", hr);
+ /* See todo_wine in MessageFilter_MessagePending; RPC/DDE messages shouldn't be triggering it */ thread = CreateThread(NULL, 0, cowait_unmarshal_thread, stream, 0, &tid); ok(thread != NULL, "CreateThread failed, error %lu\n", GetLastError()); hr = CoWaitForMultipleHandles(0, 50, 1, &event, &index); @@ -2778,6 +2787,14 @@ static DWORD CALLBACK test_CoWaitForMultipleHandles_thread(LPVOID arg) ok(index == WAIT_OBJECT_0, "cowait_unmarshal_thread didn't finish\n"); CloseHandle(thread);
+ thread = CreateThread(NULL, 0, post_dde_later_thread, hWnd, 0, &tid); + hr = CoWaitForMultipleHandles(0, 50, 1, &thread, &index); + ok(hr == RPC_S_CALLPENDING, "expected RPC_S_CALLPENDING, got 0x%08lx\n", hr); + hr = CoWaitForMultipleHandles(0, 200, 1, &thread, &index); + ok(hr == S_OK, "expected S_OK, got 0x%08lx\n", hr); + 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
Rather than rely on the leftover (non-FAILED) return value of com_get_tlsdata --- dlls/combase/combase.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/dlls/combase/combase.c b/dlls/combase/combase.c index 0695bb77405..92114c31aed 100644 --- a/dlls/combase/combase.c +++ b/dlls/combase/combase.c @@ -2186,6 +2186,7 @@ HRESULT WINAPI CoWaitForMultipleHandles(DWORD flags, DWORD timeout, ULONG handle hr = HRESULT_FROM_WIN32(GetLastError()); break; default: + hr = S_OK; *index = res; break; }
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 are ways to peek that will clear QS_ALLPOSTMESSAGE so MsgWaitForMultipleObjects waits. When there is an apt->win, the message is removed and pumped; without one only the side-effect (clearing QS_ALLPOSTMESSAGE) is wanted. --- 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 92114c31aed..643aff9d45f 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
--- dlls/combase/combase.c | 58 +++++++++++++++++++++++------------------- 1 file changed, 32 insertions(+), 26 deletions(-)
diff --git a/dlls/combase/combase.c b/dlls/combase/combase.c index 643aff9d45f..5f2cbc70263 100644 --- a/dlls/combase/combase.c +++ b/dlls/combase/combase.c @@ -2068,6 +2068,36 @@ static BOOL com_peek_message(struct apartment *apt, MSG *msg) * (to finish draining it) before using MsgWaitForMultipleObjects */ }
+static HRESULT com_filter_messagepending(struct tlsdata *tlsdata, DWORD dwTickCount) +{ + struct apartment *apt = tlsdata->apt; + + if (apt->filter) + { + PENDINGTYPE pendingtype = tlsdata->pending_call_count_server ? PENDINGTYPE_NESTED : PENDINGTYPE_TOPLEVEL; + DWORD be_handled = IMessageFilter_MessagePending(apt->filter, 0 /* FIXME */, dwTickCount, pendingtype); + + TRACE("IMessageFilter_MessagePending returned %ld\n", be_handled); + + switch (be_handled) + { + case PENDINGMSG_CANCELCALL: + WARN("call canceled\n"); + return RPC_E_CALL_CANCELED; + 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; + } + } + return S_OK; +} + /*********************************************************************** * CoWaitForMultipleHandles (combase.@) */ @@ -2097,7 +2127,7 @@ HRESULT WINAPI CoWaitForMultipleHandles(DWORD flags, DWORD timeout, ULONG handle if (FAILED(hr = com_get_tlsdata(&tlsdata))) return hr;
- apt = com_get_current_apt(); + apt = tlsdata->apt; message_loop = apt && !apt->multi_threaded;
if (flags & COWAIT_WAITALL) @@ -2139,31 +2169,7 @@ HRESULT WINAPI CoWaitForMultipleHandles(DWORD flags, DWORD timeout, ULONG handle int msg_count = 0; MSG msg;
- /* call message filter */ - - if (apt->filter) - { - 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; - 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; - } - } + hr = com_filter_messagepending(tlsdata, now - start_time);
/* 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 */
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 | 1 + dlls/ole32/tests/compobj.c | 45 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+)
diff --git a/dlls/combase/combase.c b/dlls/combase/combase.c index 5f2cbc70263..74734462693 100644 --- a/dlls/combase/combase.c +++ b/dlls/combase/combase.c @@ -2170,6 +2170,7 @@ HRESULT WINAPI CoWaitForMultipleHandles(DWORD flags, DWORD timeout, ULONG handle MSG msg;
hr = com_filter_messagepending(tlsdata, now - start_time); + if(FAILED(hr)) break;
/* 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 */ diff --git a/dlls/ole32/tests/compobj.c b/dlls/ole32/tests/compobj.c index 5aeddbd809e..03c0f5c6517 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; @@ -2619,6 +2641,14 @@ static DWORD CALLBACK post_dde_later_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; @@ -2757,6 +2787,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=124996
Your paranoid android.
=== debian11 (32 bit report) ===
wmvcore: wmvcore.c:1620: Test failed: Stream 0: Format 3: Got hr 0x8007000e. Unhandled exception: page fault on read access to 0x00000000 in 32-bit code (0x00410cbd).
=== debian11 (build log) ===
Use of uninitialized value $Flaky in addition (+) at /home/testbot/lib/WineTestBot/LogUtils.pm line 720, <$LogFile> line 25070. Use of uninitialized value $Flaky in addition (+) at /home/testbot/lib/WineTestBot/LogUtils.pm line 720, <$LogFile> line 25070. Use of uninitialized value $Flaky in addition (+) at /home/testbot/lib/WineTestBot/LogUtils.pm line 720, <$LogFile> line 25070.
-
Kevin Puetz -
Kevin Puetz (＠puetzk)
-
Marvin