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