[PATCH v4 1/4] windows.media.speech/tests: Add test to check if IAsyncInfo_Close non is blocking.
Bernhard Kölbl
besentv at gmail.com
Tue Apr 19 04:57:03 CDT 2022
Hi Rémi,
Rémi Bernon <rbernon at codeweavers.com> schrieb am Di., 19. Apr. 2022, 11:13:
> Hi Bernhard,
>
> On 4/18/22 23:42, Bernhard Kölbl wrote:
> > Signed-off-by: Bernhard Kölbl <besentv at 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 at codeweavers.com>
>
Bernhard
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20220419/488c3298/attachment.htm>
More information about the wine-devel
mailing list