<div dir="auto"><div>Hi Rémi,<br><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">Rémi Bernon <<a href="mailto:rbernon@codeweavers.com" rel="noreferrer noreferrer noreferrer noreferrer noreferrer noreferrer" target="_blank">rbernon@codeweavers.com</a>> schrieb am Di., 19. Apr. 2022, 11:13:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Bernhard,<br>
<br>
On 4/18/22 23:42, Bernhard Kölbl wrote:<br>
> Signed-off-by: Bernhard Kölbl <<a href="mailto:besentv@gmail.com" rel="noreferrer noreferrer noreferrer noreferrer noreferrer noreferrer noreferrer" target="_blank">besentv@gmail.com</a>><br>
> ---<br>
> v4: Fix races in tests, leaks and implement stubs. + Some rework.<br>
> ---<br>
> dlls/windows.media.speech/tests/speech.c | 136 +++++++++++++++++++----<br>
> 1 file changed, 112 insertions(+), 24 deletions(-)<br>
> <br>
> diff --git a/dlls/windows.media.speech/tests/speech.c b/dlls/windows.media.speech/tests/speech.c<br>
> index ef75178f8c1..7fca3fa6179 100644<br>
> --- a/dlls/windows.media.speech/tests/speech.c<br>
> +++ b/dlls/windows.media.speech/tests/speech.c<br>
> @@ -239,8 +239,8 @@ struct compilation_handler<br>
> IAsyncHandler_Compilation IAsyncHandler_Compilation_iface;<br>
> LONG ref;<br>
> <br>
> + HANDLE event_block;<br>
> HANDLE event_finished;<br>
> - BOOLEAN sleeping;<br>
> DWORD thread_id;<br>
> };<br>
> <br>
> @@ -290,7 +290,9 @@ HRESULT WINAPI compilation_handler_Invoke( IAsyncHandler_Compilation *iface,<br>
> trace("Iface %p, info %p, status %d.\n", iface, info, status);<br>
> trace("Caller thread id %lu callback thread id %lu.\n", impl->thread_id, id);<br>
> <br>
> - ok(status != Started, "Got unexpected status %#x.\n", status);<br>
> + /* Block handler until event is set. */<br>
> + if (impl->event_block) WaitForSingleObject(impl->event_block, INFINITE);<br>
> + /* Signal finishing of the handler. */<br>
> if (impl->event_finished) SetEvent(impl->event_finished);<br>
> <br>
> return S_OK;<br>
> @@ -813,6 +815,39 @@ static void test_VoiceInformation(void)<br>
> RoUninitialize();<br>
> }<br>
> <br>
> +struct async_operation_block_param<br>
> +{<br>
> + IAsyncOperationCompletedHandler_SpeechRecognitionCompilationResult *handler;<br>
> + IAsyncOperation_SpeechRecognitionCompilationResult *operation;<br>
> +};<br>
> +<br>
> +static DWORD WINAPI async_operation_block_thread(void *arg)<br>
> +{<br>
> + struct async_operation_block_param *param = arg;<br>
> + HRESULT hr;<br>
> +<br>
> + hr = IAsyncOperation_SpeechRecognitionCompilationResult_put_Completed(param->operation, param->handler);<br>
> + ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr);<br>
> +<br>
> + return 0;<br>
> +}<br>
> +<br>
> +struct async_operation_close_param<br>
> +{<br>
> + IAsyncInfo *info;<br>
> +};<br>
> +<br>
> +static DWORD WINAPI async_operation_close_thread(void *arg)<br>
> +{<br>
> + struct async_operation_close_param *param = arg;<br>
> + HRESULT hr;<br>
> +<br>
> + hr = IAsyncInfo_Close(param->info);<br>
> + ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr);<br>
> +<br>
> + return 0;<br>
> +}<br>
> +<br>
> static void test_SpeechRecognizer(void)<br>
> {<br>
> static const WCHAR *speech_recognition_name = L"Windows.Media.SpeechRecognition.SpeechRecognizer";<br>
> @@ -824,21 +859,24 @@ static void test_SpeechRecognizer(void)<br>
> ISpeechRecognizerFactory *sr_factory = NULL;<br>
> ISpeechRecognizerStatics *sr_statics = NULL;<br>
> ISpeechRecognizerStatics2 *sr_statics2 = NULL;<br>
> - ISpeechRecognizer *recognizer = NULL;<br>
> + ISpeechRecognizer *recognizer = NULL, *recognizer_2 = NULL;<br>
> ISpeechRecognizer2 *recognizer2 = NULL;<br>
> IActivationFactory *factory = NULL;<br>
> - IInspectable *inspectable = NULL;<br>
> + IInspectable *inspectable = NULL, *inspectable_2 = NULL;<br>
> IClosable *closable = NULL;<br>
> ILanguage *language = NULL;<br>
> IAsyncInfo *info = NULL;<br>
> + struct compilation_handler compilation_handler, compilation_handler2, compilation_handler3;<br>
> struct completed_event_handler completed_handler;<br>
> struct recognition_result_handler result_handler;<br>
> - struct compilation_handler compilation_handler;<br>
> - struct compilation_handler compilation_handler2;<br>
> + struct async_operation_block_param block_param;<br>
> + struct async_operation_close_param close_param;<br>
> SpeechRecognitionResultStatus result_status;<br>
> EventRegistrationToken token = { .value = 0 };<br>
> + HANDLE close_thread, blocked_thread;<br>
> AsyncStatus async_status;<br>
> HSTRING hstr, hstr_lang;<br>
> + DWORD thread_status;<br>
> HRESULT hr;<br>
> UINT32 id;<br>
> LONG ref;<br>
> @@ -945,6 +983,7 @@ static void test_SpeechRecognizer(void)<br>
> ok(ref == 1, "Got unexpected ref %lu.\n", ref);<br>
> <br>
> compilation_handler_create_static(&compilation_handler);<br>
> + compilation_handler.event_block = NULL;<br>
> compilation_handler.event_finished = CreateEventW(NULL, FALSE, FALSE, NULL);<br>
> compilation_handler.thread_id = GetCurrentThreadId();<br>
> <br>
> @@ -965,9 +1004,8 @@ static void test_SpeechRecognizer(void)<br>
> <br>
> hr = IAsyncOperation_SpeechRecognitionCompilationResult_put_Completed(operation, &compilation_handler.IAsyncHandler_Compilation_iface);<br>
> todo_wine ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr);<br>
> - todo_wine check_refcount(&compilation_handler.IAsyncHandler_Compilation_iface, 1);<br>
> <br>
<br>
<br>
See below, if you remove the test because it's broken on some windows <br>
version it'd be better to do it in a dedicated patch to make it clearer.<br>
<br>
<br>
> - WaitForSingleObject(compilation_handler.event_finished, INFINITE);<br>
> + todo_wine ok(!WaitForSingleObject(compilation_handler.event_finished, 1000), "Wait for event_finished failed.\n");<br>
> CloseHandle(compilation_handler.event_finished);<br>
> <br>
> hr = IAsyncOperation_SpeechRecognitionCompilationResult_put_Completed(operation, NULL);<br>
<br>
<br>
I don't understand why you need to change this now? If it is going to <br>
fail and timeout after you stubbed some method, maybe it'd be better to <br>
make the change with the stub.<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">The reason for this change is so the tests can't be blocked forever and you get a message if it failed. 1000ms is long enough for the finished event to fire. Makes debugging the tests easier imho.</div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
> @@ -976,7 +1014,7 @@ static void test_SpeechRecognizer(void)<br>
> handler = (void*)0xdeadbeef;<br>
> hr = IAsyncOperation_SpeechRecognitionCompilationResult_get_Completed(operation, &handler);<br>
> todo_wine ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr);<br>
> - todo_wine ok(handler == NULL, "Handler had value %p.\n", handler);<br>
> + todo_wine ok(handler == NULL || broken(handler != NULL), "Handler had value %p.\n", handler); /* Broken on Win10 1507 x32... */<br>
> <br>
> hr = IAsyncOperation_SpeechRecognitionCompilationResult_GetResults(operation, &compilation_result);<br>
> todo_wine ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr);<br>
<br>
<br>
Maybe this could check that handler value is the same as the handler you <br>
passed? The condition looks a bit too lax otherwise. It would also <br>
deserve to be in dedicated patch adding/changing broken results imho.<br>
<br>
<br>
> @@ -1048,7 +1086,7 @@ static void test_SpeechRecognizer(void)<br>
> compilation_handler2.event_finished = CreateEventW(NULL, FALSE, FALSE, NULL);<br>
> compilation_handler2.thread_id = GetCurrentThreadId();<br>
> <br>
> - ok(compilation_handler2.event_finished != NULL, "Finished event wasn't created.\n");<br>
> + todo_wine ok(compilation_handler2.event_finished != NULL, "Finished event wasn't created.\n");<br>
> <br>
> hr = ISpeechRecognizer_CompileConstraintsAsync(recognizer, &operation);<br>
> todo_wine ok(hr == S_OK, "ISpeechRecognizer_CompileConstraintsAsync failed, hr %#lx.\n", hr);<br>
<br>
<br>
The todo_wine here looks wrong?<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">Doesn't matter, the test is skipped anyway, but I can change it. </div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
> @@ -1057,23 +1095,10 @@ static void test_SpeechRecognizer(void)<br>
> hr = IAsyncOperation_SpeechRecognitionCompilationResult_QueryInterface(operation, &IID_IAsyncInfo, (void **)&info);<br>
> todo_wine ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr);<br>
> <br>
> - /* This one can fail, if the async operation had already finished */<br>
> - compilation_result = (void*)0xdeadbeef;<br>
> - hr = IAsyncOperation_SpeechRecognitionCompilationResult_GetResults(operation, &compilation_result);<br>
> - todo_wine ok(hr == E_ILLEGAL_METHOD_CALL, "Got unexpected hr %#lx.\n", hr);<br>
> - todo_wine ok(compilation_result == (void*)0xdeadbeef, "Compilation result had value %p.\n", compilation_result);<br>
> -<br>
> - async_status = 0xdeadbeef;<br>
> - hr = IAsyncInfo_get_Status(info, &async_status);<br>
> - todo_wine ok(hr == S_OK, "IAsyncInfo_get_Status failed, hr %#lx.\n", hr);<br>
> - todo_wine ok(async_status == Started || async_status == Completed, "Status was %#x.\n", async_status);<br>
> -<br>
> - IAsyncInfo_Release(info);<br>
> -<br>
<br>
<br>
Why remove these tests?<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">The tests were basically testing races and only caused timing issues.</div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br></blockquote></div></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> async_status = 0xdeadbeef;<br>
> @@ -1081,10 +1106,73 @@ static void test_SpeechRecognizer(void)<br>
> todo_wine ok(hr == S_OK, "IAsyncInfo_get_Status failed, hr %#lx.\n", hr);<br>
> todo_wine ok(async_status == Completed, "Status was %#x.\n", async_status);<br>
> <br>
> + ref = IAsyncInfo_Release(info);<br>
> + todo_wine ok(ref == 1, "Got unexpected ref %lu.\n", ref);<br>
> + ref = IAsyncOperation_SpeechRecognitionCompilationResult_Release(operation);<br>
> + todo_wine ok(!ref, "Got unexpected ref %lu.\n", ref);<br>
> +<br>
> + /* Test if AsyncInfo_Close waits for the handler to finish. */<br>
> + hr = RoActivateInstance(hstr, &inspectable_2);<br>
> + todo_wine ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr);<br>
> +<br>
> + hr = IInspectable_QueryInterface(inspectable_2, &IID_ISpeechRecognizer, (void **)&recognizer_2);<br>
> + todo_wine ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr);<br>
> +<br>
> + compilation_handler_create_static(&compilation_handler3);<br>
> + compilation_handler3.event_block = CreateEventW(NULL, FALSE, FALSE, NULL);<br>
> + compilation_handler3.event_finished = CreateEventW(NULL, FALSE, FALSE, NULL);<br>
> + compilation_handler3.thread_id = GetCurrentThreadId();<br>
> +<br>
<br>
<br>
Do you really need new variables here? I think you should just re-use <br>
the existing ones, eventually cleaning them up and re-creating if really <br>
necessary.<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto"><br></div><div dir="auto">I guess you mean the inspectables. Yeah I can remove them.</div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
> + todo_wine ok(compilation_handler3.event_finished != NULL, "Finished event wasn't created.\n");<br>
> +<br>
<br>
<br>
The todo_wine looks wrong here too.<br>
<br>
<br>
> + hr = ISpeechRecognizer_CompileConstraintsAsync(recognizer_2, &operation);<br>
> + todo_wine ok(hr == S_OK, "ISpeechRecognizer_CompileConstraintsAsync failed, hr %#lx.\n", hr);<br>
> +<br>
> + block_param.handler = &compilation_handler3.IAsyncHandler_Compilation_iface;<br>
> + block_param.operation = operation;<br>
> + blocked_thread = CreateThread(NULL, 0, async_operation_block_thread, &block_param, 0, NULL);<br>
> + thread_status = WaitForSingleObject(blocked_thread, 1000);<br>
> + todo_wine ok(thread_status == WAIT_TIMEOUT || broken(thread_status == WAIT_OBJECT_0), "Wait for block_thread didn't time out.\n"); /* Broken on Win10 1507 x32... */<br>
> +<br>
<br>
<br>
I think the problem you have here is that sometimes the operation isn't <br>
completed when you set the handler, and put_Completed doesn't call it <br>
immediately, and your thread doesn't block in that case.<br>
<br>
You probably shouldn't wait for the thread here, and instead only wait <br>
for it after you've waited on event_finished.<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">Right.</div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
> + if (thread_status == WAIT_TIMEOUT)<br>
> + {<br>
> + todo_wine ok(compilation_handler3.ref == 3, "Got unexpected ref %lu.\n", compilation_handler3.ref);<br>
> + todo_wine check_refcount(operation, 3);<br>
> +<br>
> + hr = IAsyncOperation_SpeechRecognitionCompilationResult_QueryInterface(operation, &IID_IAsyncInfo, (void **)&info);<br>
> + todo_wine ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr);<br>
> +<br>
> + <a href="http://close_param.info" rel="noreferrer noreferrer noreferrer noreferrer noreferrer noreferrer noreferrer noreferrer" target="_blank">close_param.info</a> = info;<br>
> + close_thread = CreateThread(NULL, 0, async_operation_close_thread, &close_param, 0, NULL);<br>
> + todo_wine ok(!WaitForSingleObject(close_thread, 100), "Wait for close_thread failed.\n");<br>
> +<br>
<br>
<br>
If IAsyncInfo_Close doesn't actually block then you don't need a thread <br>
to call it, right?<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">The idea is to have WaitForSingleObject fail if it ever was to block, either if someone changes the code or sth on Windows changes.</div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
> + CloseHandle(close_thread);<br>
> + }<br>
> +<br>
> + SetEvent(compilation_handler3.event_block);<br>
> + todo_wine ok(!WaitForSingleObject(compilation_handler3.event_finished, 500), "Wait for event_finished failed.\n");<br>
> + todo_wine ok(!WaitForSingleObject(blocked_thread, 1000), "Wait for block_thread failed.\n");<br>
> +<br>
> + CloseHandle(blocked_thread);<br>
> + CloseHandle(compilation_handler3.event_block);<br>
> + CloseHandle(compilation_handler3.event_finished);<br>
> +<br>
> + ref = IAsyncInfo_Release(info);<br>
> + todo_wine ok(ref == 1, "Got unexpected ref %lu.\n", ref);<br>
> +<br>
> skip_operation:<br>
> ref = IAsyncOperation_SpeechRecognitionCompilationResult_Release(operation);<br>
> ok(!ref, "Got unexpected ref %lu.\n", ref);<br>
> <br>
> + if (recognizer_2)<br>
> + {<br>
> + ref = ISpeechRecognizer_Release(recognizer_2);<br>
> + ok(ref == 1, "Got unexpected ref %lu.\n", ref);<br>
> +<br>
> + ref = IInspectable_Release(inspectable_2);<br>
> + ok(!ref, "Got unexpected ref %lu.\n", ref);<br>
> + }<br>
> +<br>
> ref = ISpeechContinuousRecognitionSession_Release(session);<br>
> ok(ref == 1, "Got unexpected ref %lu.\n", ref);<br>
> <br>
<br>
<br>
Cheers,<br>
-- <br>
Rémi Bernon <<a href="mailto:rbernon@codeweavers.com" rel="noreferrer noreferrer noreferrer noreferrer noreferrer noreferrer noreferrer" target="_blank">rbernon@codeweavers.com</a>><br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">Bernhard</div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"></blockquote></div></div></div>