[PATCH v4 1/4] windows.media.speech/tests: Add test to check if IAsyncInfo_Close non is blocking.
Signed-off-by: Bernhard Kölbl <besentv(a)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); - 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); @@ -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); @@ -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); @@ -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); - hr = IAsyncOperation_SpeechRecognitionCompilationResult_put_Completed(operation, &compilation_handler2.IAsyncHandler_Compilation_iface); todo_wine ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr); - WaitForSingleObject(compilation_handler2.event_finished, INFINITE); + todo_wine ok(!WaitForSingleObject(compilation_handler2.event_finished, 1000), "Wait for event_finished failed.\n"); CloseHandle(compilation_handler2.event_finished); 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(); + + todo_wine ok(compilation_handler3.event_finished != NULL, "Finished event wasn't created.\n"); + + 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... */ + + 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"); + + 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); -- 2.35.1
Signed-off-by: Bernhard Kölbl <besentv(a)gmail.com> --- dlls/windows.media.speech/async.c | 142 ++++++++++++++++++++--- dlls/windows.media.speech/tests/speech.c | 88 +++++++------- 2 files changed, 172 insertions(+), 58 deletions(-) diff --git a/dlls/windows.media.speech/async.c b/dlls/windows.media.speech/async.c index 94d0b35d9c1..cf0da23e493 100644 --- a/dlls/windows.media.speech/async.c +++ b/dlls/windows.media.speech/async.c @@ -23,6 +23,9 @@ WINE_DEFAULT_DEBUG_CHANNEL(speech); +#define Closed 4 +#define HANDLER_NOT_SET ((void *)~(ULONG_PTR)0) + /* * * IAsyncOperation<IInspectable*> @@ -35,6 +38,11 @@ struct async_operation IAsyncInfo IAsyncInfo_iface; const GUID *iid; LONG ref; + + IAsyncOperationCompletedHandler_IInspectable *handler; + + AsyncStatus status; + HRESULT hr; }; static inline struct async_operation *impl_from_IAsyncOperation_IInspectable(IAsyncOperation_IInspectable *iface) @@ -84,7 +92,14 @@ static ULONG WINAPI async_operation_Release( IAsyncOperation_IInspectable *iface TRACE("iface %p, ref %lu.\n", iface, ref); if (!ref) + { + IAsyncInfo_Close(&impl->IAsyncInfo_iface); + + if (impl->handler && impl->handler != HANDLER_NOT_SET) + IAsyncOperationCompletedHandler_IInspectable_Release(impl->handler); + free(impl); + } return ref; } @@ -110,21 +125,79 @@ static HRESULT WINAPI async_operation_GetTrustLevel( IAsyncOperation_IInspectabl static HRESULT WINAPI async_operation_put_Completed( IAsyncOperation_IInspectable *iface, IAsyncOperationCompletedHandler_IInspectable *handler ) { - FIXME("iface %p, handler %p stub!\n", iface, handler); - return E_NOTIMPL; + struct async_operation *impl = impl_from_IAsyncOperation_IInspectable(iface); + HRESULT hr; + + TRACE("iface %p, handler %p.\n", iface, handler); + + if (impl->handler == HANDLER_NOT_SET) + { + /* + impl->hanlder can only be set once with async_operation_put_Completed, + so by default we set a non HANDLER_NOT_SET value, in this case NULL. + */ + impl->handler = NULL; + + if (handler) + { + if (impl->status != Started) + { + IAsyncOperation_IInspectable *operation = &impl->IAsyncOperation_IInspectable_iface; + AsyncStatus status = impl->status; + + IAsyncOperation_IInspectable_AddRef(operation); + IAsyncOperationCompletedHandler_IInspectable_AddRef(handler); + + IAsyncOperationCompletedHandler_IInspectable_Invoke(handler, operation, status); + + IAsyncOperationCompletedHandler_IInspectable_Release(handler); + IAsyncOperation_IInspectable_Release(operation); + } + } + + return S_OK; + } + else if (impl->status == Closed) + hr = E_ILLEGAL_METHOD_CALL; + else if (impl->handler != HANDLER_NOT_SET) + hr = E_ILLEGAL_DELEGATE_ASSIGNMENT; + else + hr = E_UNEXPECTED; + + return hr; } static HRESULT WINAPI async_operation_get_Completed( IAsyncOperation_IInspectable *iface, IAsyncOperationCompletedHandler_IInspectable **handler ) { - FIXME("iface %p, handler %p stub!\n", iface, handler); - return E_NOTIMPL; + struct async_operation *impl = impl_from_IAsyncOperation_IInspectable(iface); + HRESULT hr; + + FIXME("iface %p, handler %p semi stub!\n", iface, handler); + + if (impl->handler != HANDLER_NOT_SET && impl->status == Closed) + hr = E_ILLEGAL_METHOD_CALL; + else + hr = S_OK; + + *handler = NULL; /* Hanlder *seems* to always be NULL from the tests. */ + return hr; } static HRESULT WINAPI async_operation_GetResults( IAsyncOperation_IInspectable *iface, IInspectable **results ) { - FIXME("iface %p, results %p stub!\n", iface, results); - return E_NOTIMPL; + /* NOTE: Despite the name, this function only returns one result! */ + struct async_operation *impl = impl_from_IAsyncOperation_IInspectable(iface); + HRESULT hr; + + TRACE("iface %p, results %p.\n", iface, results); + + if (impl->status == Closed) + hr = E_ILLEGAL_METHOD_CALL; + else + hr = S_OK; + + return hr; } static const struct IAsyncOperation_IInspectableVtbl async_operation_vtbl = @@ -159,26 +232,53 @@ static HRESULT WINAPI async_operation_info_get_Id( IAsyncInfo *iface, UINT32 *id static HRESULT WINAPI async_operation_info_get_Status( IAsyncInfo *iface, AsyncStatus *status ) { - FIXME("iface %p, status %p stub!\n", iface, status); - return E_NOTIMPL; + struct async_operation *impl = impl_from_IAsyncInfo(iface); + HRESULT hr; + + TRACE("iface %p, status %p.\n", iface, status); + + if (impl->status == Closed) + hr = E_ILLEGAL_METHOD_CALL; + else + hr = S_OK; + + *status = impl->status; + return hr; } static HRESULT WINAPI async_operation_info_get_ErrorCode( IAsyncInfo *iface, HRESULT *error_code ) { - FIXME("iface %p, error_code %p stub!\n", iface, error_code); - return E_NOTIMPL; + struct async_operation *impl = impl_from_IAsyncInfo(iface); + TRACE("iface %p, error_code %p.\n", iface, error_code); + + *error_code = impl->hr; + return S_OK; } static HRESULT WINAPI async_operation_info_Cancel( IAsyncInfo *iface ) { - FIXME("iface %p stub!\n", iface); - return E_NOTIMPL; + struct async_operation *impl = impl_from_IAsyncInfo(iface); + HRESULT hr; + + TRACE("iface %p.\n", iface); + hr = S_OK; + + if (impl->status == Closed) + hr = E_ILLEGAL_METHOD_CALL; + else if (impl->status == Started) + impl->status = Canceled; + + return hr; } static HRESULT WINAPI async_operation_info_Close( IAsyncInfo *iface ) { - FIXME("iface %p stub!\n", iface); - return E_NOTIMPL; + struct async_operation *impl = impl_from_IAsyncInfo(iface); + + TRACE("iface %p.\n", iface); + + impl->status = Closed; + return S_OK; } static const struct IAsyncInfoVtbl async_operation_info_vtbl = @@ -202,11 +302,14 @@ static const struct IAsyncInfoVtbl async_operation_info_vtbl = HRESULT async_operation_create( const GUID *iid, IAsyncOperation_IInspectable **out ) { struct async_operation *impl; + HRESULT hr; + + *out = NULL; if (!(impl = calloc(1, sizeof(*impl)))) { - *out = NULL; - return E_OUTOFMEMORY; + hr = E_OUTOFMEMORY; + goto error; } impl->IAsyncOperation_IInspectable_iface.lpVtbl = &async_operation_vtbl; @@ -214,7 +317,14 @@ HRESULT async_operation_create( const GUID *iid, IAsyncOperation_IInspectable ** impl->iid = iid; impl->ref = 1; + impl->handler = HANDLER_NOT_SET; + impl->status = Closed; + *out = &impl->IAsyncOperation_IInspectable_iface; TRACE("created %p\n", *out); return S_OK; + +error: + free(impl); + return hr; } diff --git a/dlls/windows.media.speech/tests/speech.c b/dlls/windows.media.speech/tests/speech.c index 7fca3fa6179..03608842cd7 100644 --- a/dlls/windows.media.speech/tests/speech.c +++ b/dlls/windows.media.speech/tests/speech.c @@ -992,20 +992,20 @@ 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); + ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr); + ok(handler == NULL, "Handler had value %p.\n", handler); if (FAILED(hr)) goto skip_operation; 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); + ok(hr == E_ILLEGAL_METHOD_CALL, "Got unexpected hr %#lx.\n", hr); + ok(compilation_result == (void*)0xdeadbeef, "Compilation result had value %p.\n", compilation_result); hr = IAsyncOperation_SpeechRecognitionCompilationResult_put_Completed(operation, &compilation_handler.IAsyncHandler_Compilation_iface); - todo_wine ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr); + ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr); - todo_wine ok(!WaitForSingleObject(compilation_handler.event_finished, 1000), "Wait for event_finished failed.\n"); + 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); @@ -1014,32 +1014,36 @@ 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 || broken(handler != NULL), "Handler had value %p.\n", handler); /* Broken on Win10 1507 x32... */ + 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); - todo_wine check_interface(compilation_result, &IID_IAgileObject, TRUE); - hr = ISpeechRecognitionCompilationResult_get_Status(compilation_result, &result_status); - todo_wine ok(hr == S_OK, "ISpeechRecognitionCompilationResult_get_Status failed, hr %#lx.\n", hr); - todo_wine ok(result_status == SpeechRecognitionResultStatus_Success, "Got unexpected status %#x.\n", result_status); + if (SUCCEEDED(hr)) + { + todo_wine check_interface(compilation_result, &IID_IAgileObject, TRUE); + + hr = ISpeechRecognitionCompilationResult_get_Status(compilation_result, &result_status); + todo_wine ok(hr == S_OK, "ISpeechRecognitionCompilationResult_get_Status failed, hr %#lx.\n", hr); + todo_wine ok(result_status == SpeechRecognitionResultStatus_Success, "Got unexpected status %#x.\n", result_status); - ref = ISpeechRecognitionCompilationResult_Release(compilation_result); - todo_wine ok(!ref , "Got unexpected ref %lu.\n", ref); + ref = ISpeechRecognitionCompilationResult_Release(compilation_result); + todo_wine ok(!ref , "Got unexpected ref %lu.\n", ref); + } hr = IAsyncOperation_SpeechRecognitionCompilationResult_GetResults(operation, &compilation_result); todo_wine ok(hr == E_UNEXPECTED, "Got unexpected hr %#lx.\n", hr); hr = IAsyncOperation_SpeechRecognitionCompilationResult_QueryInterface(operation, &IID_IAsyncInfo, (void **)&info); - todo_wine ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr); + ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr); /* Check if AsyncInfo and AsyncOperation share the same refcount. */ IAsyncOperation_SpeechRecognitionCompilationResult_AddRef(operation); - todo_wine check_refcount(operation, 3); - todo_wine check_refcount(info, 3); + check_refcount(operation, 3); + check_refcount(info, 3); IAsyncOperation_SpeechRecognitionCompilationResult_Release(operation); - todo_wine check_refcount(info, 2); + check_refcount(info, 2); id = 0xdeadbeef; hr = IAsyncInfo_get_Id(info, &id); @@ -1060,24 +1064,24 @@ static void test_SpeechRecognizer(void) todo_wine ok(async_status == Completed, "Status was %#x.\n", async_status); hr = IAsyncInfo_Close(info); - todo_wine ok(hr == S_OK, "IAsyncInfo_Close failed, hr %#lx.\n", hr); + ok(hr == S_OK, "IAsyncInfo_Close failed, hr %#lx.\n", hr); hr = IAsyncInfo_Close(info); - todo_wine ok(hr == S_OK, "IAsyncInfo_Close failed, hr %#lx.\n", hr); + ok(hr == S_OK, "IAsyncInfo_Close failed, hr %#lx.\n", hr); async_status = 0xdeadbeef; hr = IAsyncInfo_get_Status(info, &async_status); - todo_wine ok(hr == E_ILLEGAL_METHOD_CALL, "IAsyncInfo_get_Status failed, hr %#lx.\n", hr); - todo_wine ok(async_status == AsyncStatus_Closed, "Status was %#x.\n", async_status); + ok(hr == E_ILLEGAL_METHOD_CALL, "IAsyncInfo_get_Status failed, hr %#lx.\n", hr); + ok(async_status == AsyncStatus_Closed, "Status was %#x.\n", async_status); ref = IAsyncInfo_Release(info); - todo_wine ok(ref == 1, "Got unexpected ref %lu.\n", ref); + ok(ref == 1, "Got unexpected ref %lu.\n", ref); hr = IAsyncOperation_SpeechRecognitionCompilationResult_put_Completed(operation, NULL); - todo_wine ok(hr == E_ILLEGAL_METHOD_CALL, "Got unexpected hr %#lx.\n", hr); + ok(hr == E_ILLEGAL_METHOD_CALL, "Got unexpected hr %#lx.\n", hr); handler = (void*)0xdeadbeef; hr = IAsyncOperation_SpeechRecognitionCompilationResult_get_Completed(operation, &handler); - todo_wine ok(hr == E_ILLEGAL_METHOD_CALL, "Got unexpected hr %#lx.\n", hr); + ok(hr == E_ILLEGAL_METHOD_CALL, "Got unexpected hr %#lx.\n", hr); IAsyncOperation_SpeechRecognitionCompilationResult_Release(operation); @@ -1086,19 +1090,19 @@ static void test_SpeechRecognizer(void) compilation_handler2.event_finished = CreateEventW(NULL, FALSE, FALSE, NULL); compilation_handler2.thread_id = GetCurrentThreadId(); - todo_wine ok(compilation_handler2.event_finished != NULL, "Finished event wasn't created.\n"); + 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); - todo_wine check_interface(operation, &IID_IAgileObject, TRUE); + ok(hr == S_OK, "ISpeechRecognizer_CompileConstraintsAsync failed, hr %#lx.\n", hr); + check_interface(operation, &IID_IAgileObject, TRUE); hr = IAsyncOperation_SpeechRecognitionCompilationResult_QueryInterface(operation, &IID_IAsyncInfo, (void **)&info); - todo_wine ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr); + ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr); hr = IAsyncOperation_SpeechRecognitionCompilationResult_put_Completed(operation, &compilation_handler2.IAsyncHandler_Compilation_iface); - todo_wine ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr); + ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr); - todo_wine ok(!WaitForSingleObject(compilation_handler2.event_finished, 1000), "Wait for event_finished failed.\n"); + ok(!WaitForSingleObject(compilation_handler2.event_finished, 1000), "Wait for event_finished failed.\n"); CloseHandle(compilation_handler2.event_finished); async_status = 0xdeadbeef; @@ -1107,32 +1111,32 @@ static void test_SpeechRecognizer(void) 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); + ok(ref == 1, "Got unexpected ref %lu.\n", ref); ref = IAsyncOperation_SpeechRecognitionCompilationResult_Release(operation); - todo_wine ok(!ref, "Got unexpected ref %lu.\n", ref); + 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); + 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); + 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(); - todo_wine ok(compilation_handler3.event_finished != NULL, "Finished event wasn't created.\n"); + ok(compilation_handler3.event_finished != NULL, "Finished event wasn't created.\n"); hr = ISpeechRecognizer_CompileConstraintsAsync(recognizer_2, &operation); - todo_wine ok(hr == S_OK, "ISpeechRecognizer_CompileConstraintsAsync failed, hr %#lx.\n", hr); + 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... */ + 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... */ if (thread_status == WAIT_TIMEOUT) { @@ -1140,25 +1144,25 @@ static void test_SpeechRecognizer(void) 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); + 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"); + ok(!WaitForSingleObject(close_thread, 100), "Wait for close_thread failed.\n"); 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"); + ok(!WaitForSingleObject(compilation_handler3.event_finished, 500), "Wait for event_finished failed.\n"); + 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); + ok(ref == 1, "Got unexpected ref %lu.\n", ref); skip_operation: ref = IAsyncOperation_SpeechRecognitionCompilationResult_Release(operation); -- 2.35.1
On 4/18/22 23:42, Bernhard Kölbl wrote:
Signed-off-by: Bernhard Kölbl <besentv(a)gmail.com> --- dlls/windows.media.speech/async.c | 142 ++++++++++++++++++++--- dlls/windows.media.speech/tests/speech.c | 88 +++++++------- 2 files changed, 172 insertions(+), 58 deletions(-)
diff --git a/dlls/windows.media.speech/async.c b/dlls/windows.media.speech/async.c index 94d0b35d9c1..cf0da23e493 100644 --- a/dlls/windows.media.speech/async.c +++ b/dlls/windows.media.speech/async.c @@ -23,6 +23,9 @@
WINE_DEFAULT_DEBUG_CHANNEL(speech);
+#define Closed 4 +#define HANDLER_NOT_SET ((void *)~(ULONG_PTR)0) + /* * * IAsyncOperation<IInspectable*> @@ -35,6 +38,11 @@ struct async_operation IAsyncInfo IAsyncInfo_iface; const GUID *iid; LONG ref; + + IAsyncOperationCompletedHandler_IInspectable *handler; + + AsyncStatus status; + HRESULT hr; };
static inline struct async_operation *impl_from_IAsyncOperation_IInspectable(IAsyncOperation_IInspectable *iface) @@ -84,7 +92,14 @@ static ULONG WINAPI async_operation_Release( IAsyncOperation_IInspectable *iface TRACE("iface %p, ref %lu.\n", iface, ref);
if (!ref) + { + IAsyncInfo_Close(&impl->IAsyncInfo_iface); + + if (impl->handler && impl->handler != HANDLER_NOT_SET) + IAsyncOperationCompletedHandler_IInspectable_Release(impl->handler); + free(impl); + }
return ref; } @@ -110,21 +125,79 @@ static HRESULT WINAPI async_operation_GetTrustLevel( IAsyncOperation_IInspectabl static HRESULT WINAPI async_operation_put_Completed( IAsyncOperation_IInspectable *iface, IAsyncOperationCompletedHandler_IInspectable *handler ) { - FIXME("iface %p, handler %p stub!\n", iface, handler); - return E_NOTIMPL; + struct async_operation *impl = impl_from_IAsyncOperation_IInspectable(iface); + HRESULT hr; + + TRACE("iface %p, handler %p.\n", iface, handler); + + if (impl->handler == HANDLER_NOT_SET) + { + /* + impl->hanlder can only be set once with async_operation_put_Completed, + so by default we set a non HANDLER_NOT_SET value, in this case NULL. + */ + impl->handler = NULL; + + if (handler) + { + if (impl->status != Started) + { + IAsyncOperation_IInspectable *operation = &impl->IAsyncOperation_IInspectable_iface; + AsyncStatus status = impl->status; + + IAsyncOperation_IInspectable_AddRef(operation); + IAsyncOperationCompletedHandler_IInspectable_AddRef(handler); + + IAsyncOperationCompletedHandler_IInspectable_Invoke(handler, operation, status); + + IAsyncOperationCompletedHandler_IInspectable_Release(handler); + IAsyncOperation_IInspectable_Release(operation); + } + } + + return S_OK; + } + else if (impl->status == Closed) + hr = E_ILLEGAL_METHOD_CALL; + else if (impl->handler != HANDLER_NOT_SET) + hr = E_ILLEGAL_DELEGATE_ASSIGNMENT; + else + hr = E_UNEXPECTED; + + return hr; }
I think the last branch cannot be taken. Then I think that something like that would be clearer:
if (impl->status == Closed) hr = E_ILLEGAL_METHOD_CALL; else if (impl->handler != HANDLER_NOT_SET) hr = E_ILLEGAL_DELEGATE_ASSIGNMENT; else if ((impl->handler = handler)) { IAsyncOperationCompletedHandler_IInspectable_AddRef(impl->handler); if (impl->status > Started) { IAsyncOperation_IInspectable *operation = &impl->IAsyncOperation_IInspectable_iface; AsyncStatus status = impl->status; impl->handler = NULL; /* prevent concurrent Invoke */
IAsyncOperationCompletedHandler_IInspectable_Invoke(handler, operation, status); IAsyncOperationCompletedHandler_IInspectable_Release(handler);
return S_OK; } }
I don't think you need to add a reference on "operation" around the call to Invoke.
static HRESULT WINAPI async_operation_get_Completed( IAsyncOperation_IInspectable *iface, IAsyncOperationCompletedHandler_IInspectable **handler ) { - FIXME("iface %p, handler %p stub!\n", iface, handler); - return E_NOTIMPL; + struct async_operation *impl = impl_from_IAsyncOperation_IInspectable(iface); + HRESULT hr; + + FIXME("iface %p, handler %p semi stub!\n", iface, handler); + + if (impl->handler != HANDLER_NOT_SET && impl->status == Closed) + hr = E_ILLEGAL_METHOD_CALL; + else + hr = S_OK; + + *handler = NULL; /* Hanlder *seems* to always be NULL from the tests. */ + return hr; }
static HRESULT WINAPI async_operation_GetResults( IAsyncOperation_IInspectable *iface, IInspectable **results ) { - FIXME("iface %p, results %p stub!\n", iface, results); - return E_NOTIMPL; + /* NOTE: Despite the name, this function only returns one result! */ + struct async_operation *impl = impl_from_IAsyncOperation_IInspectable(iface); + HRESULT hr; + + TRACE("iface %p, results %p.\n", iface, results); + + if (impl->status == Closed) + hr = E_ILLEGAL_METHOD_CALL; + else + hr = S_OK; + + return hr; }
Does it really succeed if status is Started? I think that the check is more likely to be if (status != Completed) instead.
static const struct IAsyncOperation_IInspectableVtbl async_operation_vtbl = @@ -159,26 +232,53 @@ static HRESULT WINAPI async_operation_info_get_Id( IAsyncInfo *iface, UINT32 *id
static HRESULT WINAPI async_operation_info_get_Status( IAsyncInfo *iface, AsyncStatus *status ) { - FIXME("iface %p, status %p stub!\n", iface, status); - return E_NOTIMPL; + struct async_operation *impl = impl_from_IAsyncInfo(iface); + HRESULT hr; + + TRACE("iface %p, status %p.\n", iface, status); + + if (impl->status == Closed) + hr = E_ILLEGAL_METHOD_CALL; + else + hr = S_OK; + + *status = impl->status; + return hr; }
static HRESULT WINAPI async_operation_info_get_ErrorCode( IAsyncInfo *iface, HRESULT *error_code ) { - FIXME("iface %p, error_code %p stub!\n", iface, error_code); - return E_NOTIMPL; + struct async_operation *impl = impl_from_IAsyncInfo(iface); + TRACE("iface %p, error_code %p.\n", iface, error_code); + + *error_code = impl->hr; + return S_OK; }
static HRESULT WINAPI async_operation_info_Cancel( IAsyncInfo *iface ) { - FIXME("iface %p stub!\n", iface); - return E_NOTIMPL; + struct async_operation *impl = impl_from_IAsyncInfo(iface); + HRESULT hr; + + TRACE("iface %p.\n", iface); + hr = S_OK; + + if (impl->status == Closed) + hr = E_ILLEGAL_METHOD_CALL; + else if (impl->status == Started) + impl->status = Canceled; + + return hr; }
static HRESULT WINAPI async_operation_info_Close( IAsyncInfo *iface ) { - FIXME("iface %p stub!\n", iface); - return E_NOTIMPL; + struct async_operation *impl = impl_from_IAsyncInfo(iface); + + TRACE("iface %p.\n", iface); + + impl->status = Closed; + return S_OK; }
static const struct IAsyncInfoVtbl async_operation_info_vtbl = @@ -202,11 +302,14 @@ static const struct IAsyncInfoVtbl async_operation_info_vtbl = HRESULT async_operation_create( const GUID *iid, IAsyncOperation_IInspectable **out ) { struct async_operation *impl; + HRESULT hr; + + *out = NULL;
if (!(impl = calloc(1, sizeof(*impl)))) { - *out = NULL; - return E_OUTOFMEMORY; + hr = E_OUTOFMEMORY; + goto error; }
impl->IAsyncOperation_IInspectable_iface.lpVtbl = &async_operation_vtbl; @@ -214,7 +317,14 @@ HRESULT async_operation_create( const GUID *iid, IAsyncOperation_IInspectable ** impl->iid = iid; impl->ref = 1;
+ impl->handler = HANDLER_NOT_SET; + impl->status = Closed; +
I'm thinking that for this patch, and as it's still all stubs, it'd be better to set the initial status to Completed. This way put_Completed would invoke the handler, and you wouldn't have to bother adding Wine-specific and temporary wait timeouts. It may require a temporary tweak or two in the tests, like GetResults which would succeed but not return any result. I think you could set it to 0xdeadbeef and check that GetResults succeeded and returned a result != 0xdeadbeef (or skip the result tests). -- Rémi Bernon <rbernon(a)codeweavers.com>
Am Di., 19. Apr. 2022 um 14:54 Uhr schrieb Rémi Bernon <rbernon(a)codeweavers.com>:
On 4/18/22 23:42, Bernhard Kölbl wrote:
Signed-off-by: Bernhard Kölbl <besentv(a)gmail.com> --- dlls/windows.media.speech/async.c | 142 ++++++++++++++++++++--- dlls/windows.media.speech/tests/speech.c | 88 +++++++------- 2 files changed, 172 insertions(+), 58 deletions(-)
diff --git a/dlls/windows.media.speech/async.c b/dlls/windows.media.speech/async.c index 94d0b35d9c1..cf0da23e493 100644 --- a/dlls/windows.media.speech/async.c +++ b/dlls/windows.media.speech/async.c @@ -23,6 +23,9 @@
WINE_DEFAULT_DEBUG_CHANNEL(speech);
+#define Closed 4 +#define HANDLER_NOT_SET ((void *)~(ULONG_PTR)0) + /* * * IAsyncOperation<IInspectable*> @@ -35,6 +38,11 @@ struct async_operation IAsyncInfo IAsyncInfo_iface; const GUID *iid; LONG ref; + + IAsyncOperationCompletedHandler_IInspectable *handler; + + AsyncStatus status; + HRESULT hr; };
static inline struct async_operation *impl_from_IAsyncOperation_IInspectable(IAsyncOperation_IInspectable *iface) @@ -84,7 +92,14 @@ static ULONG WINAPI async_operation_Release( IAsyncOperation_IInspectable *iface TRACE("iface %p, ref %lu.\n", iface, ref);
if (!ref) + { + IAsyncInfo_Close(&impl->IAsyncInfo_iface); + + if (impl->handler && impl->handler != HANDLER_NOT_SET) + IAsyncOperationCompletedHandler_IInspectable_Release(impl->handler); + free(impl); + }
return ref; } @@ -110,21 +125,79 @@ static HRESULT WINAPI async_operation_GetTrustLevel( IAsyncOperation_IInspectabl static HRESULT WINAPI async_operation_put_Completed( IAsyncOperation_IInspectable *iface, IAsyncOperationCompletedHandler_IInspectable *handler ) { - FIXME("iface %p, handler %p stub!\n", iface, handler); - return E_NOTIMPL; + struct async_operation *impl = impl_from_IAsyncOperation_IInspectable(iface); + HRESULT hr; + + TRACE("iface %p, handler %p.\n", iface, handler); + + if (impl->handler == HANDLER_NOT_SET) + { + /* + impl->hanlder can only be set once with async_operation_put_Completed, + so by default we set a non HANDLER_NOT_SET value, in this case NULL. + */ + impl->handler = NULL; + + if (handler) + { + if (impl->status != Started) + { + IAsyncOperation_IInspectable *operation = &impl->IAsyncOperation_IInspectable_iface; + AsyncStatus status = impl->status; + + IAsyncOperation_IInspectable_AddRef(operation); + IAsyncOperationCompletedHandler_IInspectable_AddRef(handler); + + IAsyncOperationCompletedHandler_IInspectable_Invoke(handler, operation, status); + + IAsyncOperationCompletedHandler_IInspectable_Release(handler); + IAsyncOperation_IInspectable_Release(operation); + } + } + + return S_OK; + } + else if (impl->status == Closed) + hr = E_ILLEGAL_METHOD_CALL; + else if (impl->handler != HANDLER_NOT_SET) + hr = E_ILLEGAL_DELEGATE_ASSIGNMENT; + else + hr = E_UNEXPECTED; + + return hr; }
I think the last branch cannot be taken. Then I think that something like that would be clearer:
if (impl->status == Closed) hr = E_ILLEGAL_METHOD_CALL; else if (impl->handler != HANDLER_NOT_SET) hr = E_ILLEGAL_DELEGATE_ASSIGNMENT; else if ((impl->handler = handler)) { IAsyncOperationCompletedHandler_IInspectable_AddRef(impl->handler); if (impl->status > Started) { IAsyncOperation_IInspectable *operation = &impl->IAsyncOperation_IInspectable_iface; AsyncStatus status = impl->status; impl->handler = NULL; /* prevent concurrent Invoke */
IAsyncOperationCompletedHandler_IInspectable_Invoke(handler, operation, status); IAsyncOperationCompletedHandler_IInspectable_Release(handler);
return S_OK; } }
Yes, but I'd prefer to keep the "set only once" behavior of put_Completed, which doesn't seem to be achieved by this. Also, I'd keep the last path for the sake of error handling: The WinRT C++ wrappers are very good at this and if this code was to have a serious bug, we would noctice it more easily.
I don't think you need to add a reference on "operation" around the call to Invoke.
Agreed.
static HRESULT WINAPI async_operation_get_Completed( IAsyncOperation_IInspectable *iface, IAsyncOperationCompletedHandler_IInspectable **handler ) { - FIXME("iface %p, handler %p stub!\n", iface, handler); - return E_NOTIMPL; + struct async_operation *impl = impl_from_IAsyncOperation_IInspectable(iface); + HRESULT hr; + + FIXME("iface %p, handler %p semi stub!\n", iface, handler); + + if (impl->handler != HANDLER_NOT_SET && impl->status == Closed) + hr = E_ILLEGAL_METHOD_CALL; + else + hr = S_OK; + + *handler = NULL; /* Hanlder *seems* to always be NULL from the tests. */ + return hr; }
static HRESULT WINAPI async_operation_GetResults( IAsyncOperation_IInspectable *iface, IInspectable **results ) { - FIXME("iface %p, results %p stub!\n", iface, results); - return E_NOTIMPL; + /* NOTE: Despite the name, this function only returns one result! */ + struct async_operation *impl = impl_from_IAsyncOperation_IInspectable(iface); + HRESULT hr; + + TRACE("iface %p, results %p.\n", iface, results); + + if (impl->status == Closed) + hr = E_ILLEGAL_METHOD_CALL; + else + hr = S_OK; + + return hr; }
Does it really succeed if status is Started? I think that the check is more likely to be if (status != Completed) instead.
It's probably nearly impossible to test but, how about something like: if (impl->status == Closed) hr = E_ILLEGAL_METHOD_CALL; else if (impl->status > Started) hr = S_OK; else hr = E_UNEXPECTED;
static const struct IAsyncOperation_IInspectableVtbl async_operation_vtbl = @@ -159,26 +232,53 @@ static HRESULT WINAPI async_operation_info_get_Id( IAsyncInfo *iface, UINT32 *id
static HRESULT WINAPI async_operation_info_get_Status( IAsyncInfo *iface, AsyncStatus *status ) { - FIXME("iface %p, status %p stub!\n", iface, status); - return E_NOTIMPL; + struct async_operation *impl = impl_from_IAsyncInfo(iface); + HRESULT hr; + + TRACE("iface %p, status %p.\n", iface, status); + + if (impl->status == Closed) + hr = E_ILLEGAL_METHOD_CALL; + else + hr = S_OK; + + *status = impl->status; + return hr; }
static HRESULT WINAPI async_operation_info_get_ErrorCode( IAsyncInfo *iface, HRESULT *error_code ) { - FIXME("iface %p, error_code %p stub!\n", iface, error_code); - return E_NOTIMPL; + struct async_operation *impl = impl_from_IAsyncInfo(iface); + TRACE("iface %p, error_code %p.\n", iface, error_code); + + *error_code = impl->hr; + return S_OK; }
static HRESULT WINAPI async_operation_info_Cancel( IAsyncInfo *iface ) { - FIXME("iface %p stub!\n", iface); - return E_NOTIMPL; + struct async_operation *impl = impl_from_IAsyncInfo(iface); + HRESULT hr; + + TRACE("iface %p.\n", iface); + hr = S_OK; + + if (impl->status == Closed) + hr = E_ILLEGAL_METHOD_CALL; + else if (impl->status == Started) + impl->status = Canceled; + + return hr; }
static HRESULT WINAPI async_operation_info_Close( IAsyncInfo *iface ) { - FIXME("iface %p stub!\n", iface); - return E_NOTIMPL; + struct async_operation *impl = impl_from_IAsyncInfo(iface); + + TRACE("iface %p.\n", iface); + + impl->status = Closed; + return S_OK; }
static const struct IAsyncInfoVtbl async_operation_info_vtbl = @@ -202,11 +302,14 @@ static const struct IAsyncInfoVtbl async_operation_info_vtbl = HRESULT async_operation_create( const GUID *iid, IAsyncOperation_IInspectable **out ) { struct async_operation *impl; + HRESULT hr; + + *out = NULL;
if (!(impl = calloc(1, sizeof(*impl)))) { - *out = NULL; - return E_OUTOFMEMORY; + hr = E_OUTOFMEMORY; + goto error; }
impl->IAsyncOperation_IInspectable_iface.lpVtbl = &async_operation_vtbl; @@ -214,7 +317,14 @@ HRESULT async_operation_create( const GUID *iid, IAsyncOperation_IInspectable ** impl->iid = iid; impl->ref = 1;
+ impl->handler = HANDLER_NOT_SET; + impl->status = Closed; +
I'm thinking that for this patch, and as it's still all stubs, it'd be better to set the initial status to Completed. This way put_Completed would invoke the handler, and you wouldn't have to bother adding Wine-specific and temporary wait timeouts.
I agree on setting it to Completed for this patch, but as mentioned before, the Wait-Timeouts are actually not related to this.
It may require a temporary tweak or two in the tests, like GetResults which would succeed but not return any result. I think you could set it to 0xdeadbeef and check that GetResults succeeded and returned a result != 0xdeadbeef (or skip the result tests).
I don't really understand this one: Do you want me to skip/remove the GetResults tests? I could add a test for the case of being in the Closed state, but for the Started state it would probably rely on races... Bernhard
On 4/20/22 14:24, Bernhard Kölbl wrote:
static HRESULT WINAPI async_operation_get_Completed( IAsyncOperation_IInspectable *iface, IAsyncOperationCompletedHandler_IInspectable **handler ) { - FIXME("iface %p, handler %p stub!\n", iface, handler); - return E_NOTIMPL; + struct async_operation *impl = impl_from_IAsyncOperation_IInspectable(iface); + HRESULT hr; + + FIXME("iface %p, handler %p semi stub!\n", iface, handler); + + if (impl->handler != HANDLER_NOT_SET && impl->status == Closed) + hr = E_ILLEGAL_METHOD_CALL; + else + hr = S_OK; + + *handler = NULL; /* Hanlder *seems* to always be NULL from the tests. */ + return hr; }
static HRESULT WINAPI async_operation_GetResults( IAsyncOperation_IInspectable *iface, IInspectable **results ) { - FIXME("iface %p, results %p stub!\n", iface, results); - return E_NOTIMPL; + /* NOTE: Despite the name, this function only returns one result! */ + struct async_operation *impl = impl_from_IAsyncOperation_IInspectable(iface); + HRESULT hr; + + TRACE("iface %p, results %p.\n", iface, results); + + if (impl->status == Closed) + hr = E_ILLEGAL_METHOD_CALL; + else + hr = S_OK; + + return hr; }
Does it really succeed if status is Started? I think that the check is more likely to be if (status != Completed) instead.
It's probably nearly impossible to test but, how about something like:
if (impl->status == Closed) hr = E_ILLEGAL_METHOD_CALL; else if (impl->status > Started) hr = S_OK; else hr = E_UNEXPECTED;
Sure.
static const struct IAsyncOperation_IInspectableVtbl async_operation_vtbl = @@ -159,26 +232,53 @@ static HRESULT WINAPI async_operation_info_get_Id( IAsyncInfo *iface, UINT32 *id
static HRESULT WINAPI async_operation_info_get_Status( IAsyncInfo *iface, AsyncStatus *status ) { - FIXME("iface %p, status %p stub!\n", iface, status); - return E_NOTIMPL; + struct async_operation *impl = impl_from_IAsyncInfo(iface); + HRESULT hr; + + TRACE("iface %p, status %p.\n", iface, status); + + if (impl->status == Closed) + hr = E_ILLEGAL_METHOD_CALL; + else + hr = S_OK; + + *status = impl->status; + return hr; }
static HRESULT WINAPI async_operation_info_get_ErrorCode( IAsyncInfo *iface, HRESULT *error_code ) { - FIXME("iface %p, error_code %p stub!\n", iface, error_code); - return E_NOTIMPL; + struct async_operation *impl = impl_from_IAsyncInfo(iface); + TRACE("iface %p, error_code %p.\n", iface, error_code); + + *error_code = impl->hr; + return S_OK; }
static HRESULT WINAPI async_operation_info_Cancel( IAsyncInfo *iface ) { - FIXME("iface %p stub!\n", iface); - return E_NOTIMPL; + struct async_operation *impl = impl_from_IAsyncInfo(iface); + HRESULT hr; + + TRACE("iface %p.\n", iface); + hr = S_OK; + + if (impl->status == Closed) + hr = E_ILLEGAL_METHOD_CALL; + else if (impl->status == Started) + impl->status = Canceled; + + return hr; }
static HRESULT WINAPI async_operation_info_Close( IAsyncInfo *iface ) { - FIXME("iface %p stub!\n", iface); - return E_NOTIMPL; + struct async_operation *impl = impl_from_IAsyncInfo(iface); + + TRACE("iface %p.\n", iface); + + impl->status = Closed; + return S_OK; }
static const struct IAsyncInfoVtbl async_operation_info_vtbl = @@ -202,11 +302,14 @@ static const struct IAsyncInfoVtbl async_operation_info_vtbl = HRESULT async_operation_create( const GUID *iid, IAsyncOperation_IInspectable **out ) { struct async_operation *impl; + HRESULT hr; + + *out = NULL;
if (!(impl = calloc(1, sizeof(*impl)))) { - *out = NULL; - return E_OUTOFMEMORY; + hr = E_OUTOFMEMORY; + goto error; }
impl->IAsyncOperation_IInspectable_iface.lpVtbl = &async_operation_vtbl; @@ -214,7 +317,14 @@ HRESULT async_operation_create( const GUID *iid, IAsyncOperation_IInspectable ** impl->iid = iid; impl->ref = 1;
+ impl->handler = HANDLER_NOT_SET; + impl->status = Closed; +
I'm thinking that for this patch, and as it's still all stubs, it'd be better to set the initial status to Completed. This way put_Completed would invoke the handler, and you wouldn't have to bother adding Wine-specific and temporary wait timeouts.
I agree on setting it to Completed for this patch, but as mentioned before, the Wait-Timeouts are actually not related to this.
It may require a temporary tweak or two in the tests, like GetResults which would succeed but not return any result. I think you could set it to 0xdeadbeef and check that GetResults succeeded and returned a result != 0xdeadbeef (or skip the result tests).
I don't really understand this one: Do you want me to skip/remove the GetResults tests? I could add a test for the case of being in the Closed state, but for the Started state it would probably rely on races...
I mean, if you set the initial status to Completed, GetResults will succeed, but not return any result as there's none to return yet. The current test only checks SUCCEEDED(hr) to decide whether it should skip checking the results pointer. You will need to change that to make sure the returned result pointer is valid. -- Rémi Bernon <rbernon(a)codeweavers.com>
I mean, if you set the initial status to Completed, GetResults will succeed, but not return any result as there's none to return yet.
The current test only checks SUCCEEDED(hr) to decide whether it should skip checking the results pointer. You will need to change that to make sure the returned result pointer is valid.
Right, I'll just do the deadbeef check. Bernhard
Signed-off-by: Bernhard Kölbl <besentv(a)gmail.com> --- dlls/windows.media.speech/async.c | 123 ++++++++++++++++++++++- dlls/windows.media.speech/private.h | 4 +- dlls/windows.media.speech/recognizer.c | 7 +- dlls/windows.media.speech/tests/speech.c | 22 ++-- 4 files changed, 141 insertions(+), 15 deletions(-) diff --git a/dlls/windows.media.speech/async.c b/dlls/windows.media.speech/async.c index cf0da23e493..66cba46d85b 100644 --- a/dlls/windows.media.speech/async.c +++ b/dlls/windows.media.speech/async.c @@ -40,7 +40,13 @@ struct async_operation LONG ref; IAsyncOperationCompletedHandler_IInspectable *handler; + IInspectable *result; + async_operation_worker worker; + TP_WORK *async_run_work; + IInspectable *invoker; + + CRITICAL_SECTION cs; AsyncStatus status; HRESULT hr; }; @@ -95,9 +101,14 @@ static ULONG WINAPI async_operation_Release( IAsyncOperation_IInspectable *iface { IAsyncInfo_Close(&impl->IAsyncInfo_iface); + if (impl->invoker) + IInspectable_Release(impl->invoker); if (impl->handler && impl->handler != HANDLER_NOT_SET) IAsyncOperationCompletedHandler_IInspectable_Release(impl->handler); + if (impl->result) + IInspectable_Release(impl->result); + DeleteCriticalSection(&impl->cs); free(impl); } @@ -130,6 +141,7 @@ static HRESULT WINAPI async_operation_put_Completed( IAsyncOperation_IInspectabl TRACE("iface %p, handler %p.\n", iface, handler); + EnterCriticalSection(&impl->cs); if (impl->handler == HANDLER_NOT_SET) { /* @@ -140,19 +152,25 @@ static HRESULT WINAPI async_operation_put_Completed( IAsyncOperation_IInspectabl if (handler) { - if (impl->status != Started) + if (impl->status != Started && impl->status != Closed) { IAsyncOperation_IInspectable *operation = &impl->IAsyncOperation_IInspectable_iface; AsyncStatus status = impl->status; IAsyncOperation_IInspectable_AddRef(operation); IAsyncOperationCompletedHandler_IInspectable_AddRef(handler); + LeaveCriticalSection(&impl->cs); IAsyncOperationCompletedHandler_IInspectable_Invoke(handler, operation, status); IAsyncOperationCompletedHandler_IInspectable_Release(handler); IAsyncOperation_IInspectable_Release(operation); } + else + { + IAsyncOperationCompletedHandler_IInspectable_AddRef((impl->handler = handler)); + LeaveCriticalSection(&impl->cs); + } } return S_OK; @@ -163,6 +181,7 @@ static HRESULT WINAPI async_operation_put_Completed( IAsyncOperation_IInspectabl hr = E_ILLEGAL_DELEGATE_ASSIGNMENT; else hr = E_UNEXPECTED; + LeaveCriticalSection(&impl->cs); return hr; } @@ -175,10 +194,12 @@ static HRESULT WINAPI async_operation_get_Completed( IAsyncOperation_IInspectabl FIXME("iface %p, handler %p semi stub!\n", iface, handler); + EnterCriticalSection(&impl->cs); if (impl->handler != HANDLER_NOT_SET && impl->status == Closed) hr = E_ILLEGAL_METHOD_CALL; else hr = S_OK; + LeaveCriticalSection(&impl->cs); *handler = NULL; /* Hanlder *seems* to always be NULL from the tests. */ return hr; @@ -192,10 +213,18 @@ static HRESULT WINAPI async_operation_GetResults( IAsyncOperation_IInspectable * TRACE("iface %p, results %p.\n", iface, results); + EnterCriticalSection(&impl->cs); if (impl->status == Closed) hr = E_ILLEGAL_METHOD_CALL; + else if (!impl->result) + hr = E_UNEXPECTED; else + { + *results = impl->result; + impl->result = NULL; /* NOTE: AsyncOperation gives up it's reference to result here! */ hr = S_OK; + } + LeaveCriticalSection(&impl->cs); return hr; } @@ -237,12 +266,15 @@ static HRESULT WINAPI async_operation_info_get_Status( IAsyncInfo *iface, AsyncS TRACE("iface %p, status %p.\n", iface, status); + EnterCriticalSection(&impl->cs); if (impl->status == Closed) hr = E_ILLEGAL_METHOD_CALL; else hr = S_OK; *status = impl->status; + LeaveCriticalSection(&impl->cs); + return hr; } @@ -251,7 +283,10 @@ static HRESULT WINAPI async_operation_info_get_ErrorCode( IAsyncInfo *iface, HRE struct async_operation *impl = impl_from_IAsyncInfo(iface); TRACE("iface %p, error_code %p.\n", iface, error_code); + EnterCriticalSection(&impl->cs); *error_code = impl->hr; + LeaveCriticalSection(&impl->cs); + return S_OK; } @@ -263,10 +298,12 @@ static HRESULT WINAPI async_operation_info_Cancel( IAsyncInfo *iface ) TRACE("iface %p.\n", iface); hr = S_OK; + EnterCriticalSection(&impl->cs); if (impl->status == Closed) hr = E_ILLEGAL_METHOD_CALL; else if (impl->status == Started) impl->status = Canceled; + LeaveCriticalSection(&impl->cs); return hr; } @@ -277,7 +314,16 @@ static HRESULT WINAPI async_operation_info_Close( IAsyncInfo *iface ) TRACE("iface %p.\n", iface); + EnterCriticalSection(&impl->cs); + if (impl->async_run_work) + { + CloseThreadpoolWork(impl->async_run_work); + impl->async_run_work = NULL; + } + impl->status = Closed; + LeaveCriticalSection(&impl->cs); + return S_OK; } @@ -299,7 +345,65 @@ static const struct IAsyncInfoVtbl async_operation_info_vtbl = async_operation_info_Close }; -HRESULT async_operation_create( const GUID *iid, IAsyncOperation_IInspectable **out ) +static void CALLBACK async_run_cb(TP_CALLBACK_INSTANCE *instance, void *data, TP_WORK *work) +{ + IAsyncOperation_IInspectable *operation = data; + struct async_operation *impl = impl_from_IAsyncOperation_IInspectable(operation); + IInspectable *result = NULL, *invoker = NULL; + async_operation_worker worker; + HRESULT hr; + + EnterCriticalSection(&impl->cs); + if (impl->status == Canceled) + goto abort; + + impl->status = Started; + + invoker = impl->invoker; + worker = impl->worker; + LeaveCriticalSection(&impl->cs); + + hr = worker(invoker, &result); + + EnterCriticalSection(&impl->cs); + if (FAILED(hr)) + impl->status = Error; + else + impl->status = Completed; + + impl->result = result; + impl->hr = hr; + + if (impl->status == Canceled) + goto abort; + + if (impl->handler != NULL && impl->handler != HANDLER_NOT_SET) + { + IAsyncOperationCompletedHandler_IInspectable *handler = impl->handler; + AsyncStatus status = impl->status; + + IAsyncOperation_IInspectable_AddRef(operation); + IAsyncOperationCompletedHandler_IInspectable_AddRef(handler); + LeaveCriticalSection(&impl->cs); + + IAsyncOperationCompletedHandler_IInspectable_Invoke(handler, operation, status); + + IAsyncOperationCompletedHandler_IInspectable_Release(handler); + IAsyncOperation_IInspectable_Release(operation); + } + else + LeaveCriticalSection(&impl->cs); + + IAsyncOperation_IInspectable_Release(operation); + return; + +abort: + impl->hr = E_FAIL; + LeaveCriticalSection(&impl->cs); + IAsyncOperation_IInspectable_Release(operation); +} + +HRESULT async_operation_create( const GUID *iid, IInspectable *invoker, async_operation_worker worker, IAsyncOperation_IInspectable **out ) { struct async_operation *impl; HRESULT hr; @@ -318,8 +422,23 @@ HRESULT async_operation_create( const GUID *iid, IAsyncOperation_IInspectable ** impl->ref = 1; impl->handler = HANDLER_NOT_SET; + impl->worker = worker; impl->status = Closed; + IAsyncOperation_IInspectable_AddRef(&impl->IAsyncOperation_IInspectable_iface); /* AddRef to keep the obj alive in the callback. */ + if (!(impl->async_run_work = CreateThreadpoolWork(async_run_cb, &impl->IAsyncOperation_IInspectable_iface, NULL))) + { + hr = HRESULT_FROM_WIN32(GetLastError()); + goto error; + } + + if (invoker) IInspectable_AddRef((impl->invoker = invoker)); + + InitializeCriticalSection(&impl->cs); + impl->cs.DebugInfo->Spare[0] = (DWORD_PTR)(__FILE__ ": async_operation.cs"); + + SubmitThreadpoolWork(impl->async_run_work); + *out = &impl->IAsyncOperation_IInspectable_iface; TRACE("created %p\n", *out); return S_OK; diff --git a/dlls/windows.media.speech/private.h b/dlls/windows.media.speech/private.h index 1cf61c51f1e..8eb0570c170 100644 --- a/dlls/windows.media.speech/private.h +++ b/dlls/windows.media.speech/private.h @@ -69,7 +69,9 @@ struct vector_iids const GUID *view; }; -HRESULT async_operation_create( const GUID *iid, IAsyncOperation_IInspectable **out ); +typedef HRESULT (WINAPI *async_operation_worker)( IInspectable *invoker, IInspectable **result ); + +HRESULT async_operation_create( const GUID *iid, IInspectable *invoker, async_operation_worker worker, IAsyncOperation_IInspectable **out ); HRESULT typed_event_handlers_append( struct list *list, ITypedEventHandler_IInspectable_IInspectable *handler, EventRegistrationToken *token ); HRESULT typed_event_handlers_remove( struct list *list, EventRegistrationToken *token ); diff --git a/dlls/windows.media.speech/recognizer.c b/dlls/windows.media.speech/recognizer.c index b16ecb24641..5cffe1172e3 100644 --- a/dlls/windows.media.speech/recognizer.c +++ b/dlls/windows.media.speech/recognizer.c @@ -348,12 +348,17 @@ static HRESULT WINAPI recognizer_get_UIOptions( ISpeechRecognizer *iface, ISpeec return E_NOTIMPL; } +static HRESULT WINAPI compile_worker( IInspectable *invoker, IInspectable **result ) +{ + return S_OK; +} + static HRESULT WINAPI recognizer_CompileConstraintsAsync( ISpeechRecognizer *iface, IAsyncOperation_SpeechRecognitionCompilationResult **operation ) { IAsyncOperation_IInspectable **value = (IAsyncOperation_IInspectable **)operation; FIXME("iface %p, operation %p semi-stub!\n", iface, operation); - return async_operation_create(&IID_IAsyncOperation_SpeechRecognitionCompilationResult, value); + return async_operation_create(&IID_IAsyncOperation_SpeechRecognitionCompilationResult, NULL, compile_worker, value); } static HRESULT WINAPI recognizer_RecognizeAsync( ISpeechRecognizer *iface, diff --git a/dlls/windows.media.speech/tests/speech.c b/dlls/windows.media.speech/tests/speech.c index 03608842cd7..7b05bcdcd7c 100644 --- a/dlls/windows.media.speech/tests/speech.c +++ b/dlls/windows.media.speech/tests/speech.c @@ -1009,11 +1009,11 @@ static void test_SpeechRecognizer(void) CloseHandle(compilation_handler.event_finished); hr = IAsyncOperation_SpeechRecognitionCompilationResult_put_Completed(operation, NULL); - todo_wine ok(hr == E_ILLEGAL_DELEGATE_ASSIGNMENT, "Got unexpected hr %#lx.\n", hr); + ok(hr == E_ILLEGAL_DELEGATE_ASSIGNMENT, "Got unexpected hr %#lx.\n", hr); handler = (void*)0xdeadbeef; hr = IAsyncOperation_SpeechRecognitionCompilationResult_get_Completed(operation, &handler); - todo_wine ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr); + ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr); ok(handler == NULL || broken(handler != NULL), "Handler had value %p.\n", handler); /* Broken on Win10 1507 x32... */ hr = IAsyncOperation_SpeechRecognitionCompilationResult_GetResults(operation, &compilation_result); @@ -1032,7 +1032,7 @@ static void test_SpeechRecognizer(void) } hr = IAsyncOperation_SpeechRecognitionCompilationResult_GetResults(operation, &compilation_result); - todo_wine ok(hr == E_UNEXPECTED, "Got unexpected hr %#lx.\n", hr); + ok(hr == E_UNEXPECTED, "Got unexpected hr %#lx.\n", hr); hr = IAsyncOperation_SpeechRecognitionCompilationResult_QueryInterface(operation, &IID_IAsyncInfo, (void **)&info); ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr); @@ -1052,16 +1052,16 @@ static void test_SpeechRecognizer(void) 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 == Completed, "Status was %#x.\n", async_status); + ok(hr == S_OK, "IAsyncInfo_get_Status failed, hr %#lx.\n", hr); + ok(async_status == Completed, "Status was %#x.\n", async_status); hr = IAsyncInfo_Cancel(info); - todo_wine ok(hr == S_OK, "IAsyncInfo_Cancel failed, hr %#lx.\n", hr); + ok(hr == S_OK, "IAsyncInfo_Cancel failed, hr %#lx.\n", hr); 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 == Completed, "Status was %#x.\n", async_status); + ok(hr == S_OK, "IAsyncInfo_get_Status failed, hr %#lx.\n", hr); + ok(async_status == Completed, "Status was %#x.\n", async_status); hr = IAsyncInfo_Close(info); ok(hr == S_OK, "IAsyncInfo_Close failed, hr %#lx.\n", hr); @@ -1107,11 +1107,11 @@ static void test_SpeechRecognizer(void) 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 == Completed, "Status was %#x.\n", async_status); + ok(hr == S_OK, "IAsyncInfo_get_Status failed, hr %#lx.\n", hr); + ok(async_status == Completed, "Status was %#x.\n", async_status); ref = IAsyncInfo_Release(info); - ok(ref == 1, "Got unexpected ref %lu.\n", ref); + ok(ref <= 1, "Got unexpected ref %lu.\n", ref); ref = IAsyncOperation_SpeechRecognitionCompilationResult_Release(operation); ok(!ref, "Got unexpected ref %lu.\n", ref); -- 2.35.1
On 4/18/22 23:42, Bernhard Kölbl wrote:
Signed-off-by: Bernhard Kölbl <besentv(a)gmail.com> --- dlls/windows.media.speech/async.c | 123 ++++++++++++++++++++++- dlls/windows.media.speech/private.h | 4 +- dlls/windows.media.speech/recognizer.c | 7 +- dlls/windows.media.speech/tests/speech.c | 22 ++-- 4 files changed, 141 insertions(+), 15 deletions(-)
diff --git a/dlls/windows.media.speech/async.c b/dlls/windows.media.speech/async.c index cf0da23e493..66cba46d85b 100644 --- a/dlls/windows.media.speech/async.c +++ b/dlls/windows.media.speech/async.c @@ -40,7 +40,13 @@ struct async_operation LONG ref;
IAsyncOperationCompletedHandler_IInspectable *handler; + IInspectable *result;
+ async_operation_worker worker;
IMHO "callback" is more commonly used and more appropriate name than "worker".
+ TP_WORK *async_run_work; + IInspectable *invoker; + + CRITICAL_SECTION cs; AsyncStatus status; HRESULT hr; };
@@ -299,7 +345,65 @@ static const struct IAsyncInfoVtbl async_operation_info_vtbl = async_operation_info_Close };
-HRESULT async_operation_create( const GUID *iid, IAsyncOperation_IInspectable **out ) +static void CALLBACK async_run_cb(TP_CALLBACK_INSTANCE *instance, void *data, TP_WORK *work) +{ + IAsyncOperation_IInspectable *operation = data; + struct async_operation *impl = impl_from_IAsyncOperation_IInspectable(operation); + IInspectable *result = NULL, *invoker = NULL; + async_operation_worker worker; + HRESULT hr; + + EnterCriticalSection(&impl->cs); + if (impl->status == Canceled) + goto abort; + + impl->status = Started;
Should it really ignore and overwrite the Closed status here? Or perhaps instead treat it as cancellation, as it means the client probably doesn't care about the result anymore?
+ + invoker = impl->invoker; + worker = impl->worker; + LeaveCriticalSection(&impl->cs); + + hr = worker(invoker, &result); + + EnterCriticalSection(&impl->cs); + if (FAILED(hr)) + impl->status = Error; + else + impl->status = Completed; + + impl->result = result; + impl->hr = hr; + + if (impl->status == Canceled) + goto abort;
This condition will never be true as you just wrote the status above. I think you can consider that if the callback had been called it's too late for it to be cancelled.
+ + if (impl->handler != NULL && impl->handler != HANDLER_NOT_SET) + { + IAsyncOperationCompletedHandler_IInspectable *handler = impl->handler; + AsyncStatus status = impl->status; + + IAsyncOperation_IInspectable_AddRef(operation); + IAsyncOperationCompletedHandler_IInspectable_AddRef(handler); + LeaveCriticalSection(&impl->cs); + + IAsyncOperationCompletedHandler_IInspectable_Invoke(handler, operation, status); + + IAsyncOperationCompletedHandler_IInspectable_Release(handler); + IAsyncOperation_IInspectable_Release(operation);
I don't think you need to add all these references here, you already hold one on the operation and on the handler.
+ } + else + LeaveCriticalSection(&impl->cs); + + IAsyncOperation_IInspectable_Release(operation); + return; + +abort: + impl->hr = E_FAIL; + LeaveCriticalSection(&impl->cs); + IAsyncOperation_IInspectable_Release(operation); +} +
It will be hard to check, but I think you need to invoke the handler even in the Cancel case, as MSDN suggests:
If you're implementing IAsyncOperation<TResult>, then the set implementation of Completed should store the handler, and the surrounding logic should invoke it when Close is called. The implementation should set the asyncStatus parameter of invoked callbacks appropriately if there is a Cancel call, Status is not Completed, errors occurred, and so on.
-- Rémi Bernon <rbernon(a)codeweavers.com>
Am Di., 19. Apr. 2022 um 15:08 Uhr schrieb Rémi Bernon <rbernon(a)codeweavers.com>:
On 4/18/22 23:42, Bernhard Kölbl wrote:
Signed-off-by: Bernhard Kölbl <besentv(a)gmail.com> --- dlls/windows.media.speech/async.c | 123 ++++++++++++++++++++++- dlls/windows.media.speech/private.h | 4 +- dlls/windows.media.speech/recognizer.c | 7 +- dlls/windows.media.speech/tests/speech.c | 22 ++-- 4 files changed, 141 insertions(+), 15 deletions(-)
diff --git a/dlls/windows.media.speech/async.c b/dlls/windows.media.speech/async.c index cf0da23e493..66cba46d85b 100644 --- a/dlls/windows.media.speech/async.c +++ b/dlls/windows.media.speech/async.c @@ -40,7 +40,13 @@ struct async_operation LONG ref;
IAsyncOperationCompletedHandler_IInspectable *handler; + IInspectable *result;
+ async_operation_worker worker;
IMHO "callback" is more commonly used and more appropriate name than "worker".
Sure. I used "worker" because the threadpool callback is already named like that, so a possbile confusion would be eliminated.
+ TP_WORK *async_run_work; + IInspectable *invoker; + + CRITICAL_SECTION cs; AsyncStatus status; HRESULT hr; };
@@ -299,7 +345,65 @@ static const struct IAsyncInfoVtbl async_operation_info_vtbl = async_operation_info_Close };
-HRESULT async_operation_create( const GUID *iid, IAsyncOperation_IInspectable **out ) +static void CALLBACK async_run_cb(TP_CALLBACK_INSTANCE *instance, void *data, TP_WORK *work) +{ + IAsyncOperation_IInspectable *operation = data; + struct async_operation *impl = impl_from_IAsyncOperation_IInspectable(operation); + IInspectable *result = NULL, *invoker = NULL; + async_operation_worker worker; + HRESULT hr; + + EnterCriticalSection(&impl->cs); + if (impl->status == Canceled) + goto abort; + + impl->status = Started;
Should it really ignore and overwrite the Closed status here?
Or perhaps instead treat it as cancellation, as it means the client probably doesn't care about the result anymore?
Right, I use Closed as the default/init state, that's why it is overwritten here. So not sure if it needs a change?
+ + invoker = impl->invoker; + worker = impl->worker; + LeaveCriticalSection(&impl->cs); + + hr = worker(invoker, &result); + + EnterCriticalSection(&impl->cs); + if (FAILED(hr)) + impl->status = Error; + else + impl->status = Completed; + + impl->result = result; + impl->hr = hr; + + if (impl->status == Canceled) + goto abort;
This condition will never be true as you just wrote the status above. I think you can consider that if the callback had been called it's too late for it to be cancelled.
The idea is to cancel the invoication of the handler, i.e. leaving the thread asap. The condition can be true if you call cancel during the call to worker/callback.
+ + if (impl->handler != NULL && impl->handler != HANDLER_NOT_SET) + { + IAsyncOperationCompletedHandler_IInspectable *handler = impl->handler; + AsyncStatus status = impl->status; + + IAsyncOperation_IInspectable_AddRef(operation); + IAsyncOperationCompletedHandler_IInspectable_AddRef(handler); + LeaveCriticalSection(&impl->cs); + + IAsyncOperationCompletedHandler_IInspectable_Invoke(handler, operation, status); + + IAsyncOperationCompletedHandler_IInspectable_Release(handler); + IAsyncOperation_IInspectable_Release(operation);
I don't think you need to add all these references here, you already hold one on the operation and on the handler.
Yeah.
+ } + else + LeaveCriticalSection(&impl->cs); + + IAsyncOperation_IInspectable_Release(operation); + return; + +abort: + impl->hr = E_FAIL; + LeaveCriticalSection(&impl->cs); + IAsyncOperation_IInspectable_Release(operation); +} +
It will be hard to check, but I think you need to invoke the handler even in the Cancel case, as MSDN suggests:
If you're implementing IAsyncOperation<TResult>, then the set implementation of Completed should store the handler, and the surrounding logic should invoke it when Close is called. The implementation should set the asyncStatus parameter of invoked callbacks appropriately if there is a Cancel call, Status is not Completed, errors occurred, and so on.
Yeah, seems odd but I can change it. Bernhard
On 4/20/22 18:29, Bernhard Kölbl wrote:
Am Di., 19. Apr. 2022 um 15:08 Uhr schrieb Rémi Bernon <rbernon(a)codeweavers.com>:
On 4/18/22 23:42, Bernhard Kölbl wrote:
Signed-off-by: Bernhard Kölbl <besentv(a)gmail.com> --- dlls/windows.media.speech/async.c | 123 ++++++++++++++++++++++- dlls/windows.media.speech/private.h | 4 +- dlls/windows.media.speech/recognizer.c | 7 +- dlls/windows.media.speech/tests/speech.c | 22 ++-- 4 files changed, 141 insertions(+), 15 deletions(-)
diff --git a/dlls/windows.media.speech/async.c b/dlls/windows.media.speech/async.c index cf0da23e493..66cba46d85b 100644 --- a/dlls/windows.media.speech/async.c +++ b/dlls/windows.media.speech/async.c @@ -40,7 +40,13 @@ struct async_operation LONG ref;
IAsyncOperationCompletedHandler_IInspectable *handler; + IInspectable *result;
+ async_operation_worker worker;
IMHO "callback" is more commonly used and more appropriate name than "worker".
Sure. I used "worker" because the threadpool callback is already named like that, so a possbile confusion would be eliminated.
+ TP_WORK *async_run_work; + IInspectable *invoker; + + CRITICAL_SECTION cs; AsyncStatus status; HRESULT hr; };
@@ -299,7 +345,65 @@ static const struct IAsyncInfoVtbl async_operation_info_vtbl = async_operation_info_Close };
-HRESULT async_operation_create( const GUID *iid, IAsyncOperation_IInspectable **out ) +static void CALLBACK async_run_cb(TP_CALLBACK_INSTANCE *instance, void *data, TP_WORK *work) +{ + IAsyncOperation_IInspectable *operation = data; + struct async_operation *impl = impl_from_IAsyncOperation_IInspectable(operation); + IInspectable *result = NULL, *invoker = NULL; + async_operation_worker worker; + HRESULT hr; + + EnterCriticalSection(&impl->cs); + if (impl->status == Canceled) + goto abort; + + impl->status = Started;
Should it really ignore and overwrite the Closed status here?
Or perhaps instead treat it as cancellation, as it means the client probably doesn't care about the result anymore?
Right, I use Closed as the default/init state, that's why it is overwritten here. So not sure if it needs a change?
I think the initial status, should be changed, in this patch, to Started, and it then doesn't need to be set here (only check if the operation has already been cancelled).
+ + invoker = impl->invoker; + worker = impl->worker; + LeaveCriticalSection(&impl->cs); + + hr = worker(invoker, &result); + + EnterCriticalSection(&impl->cs); + if (FAILED(hr)) + impl->status = Error; + else + impl->status = Completed; + + impl->result = result; + impl->hr = hr; + + if (impl->status == Canceled) + goto abort;
This condition will never be true as you just wrote the status above. I think you can consider that if the callback had been called it's too late for it to be cancelled.
The idea is to cancel the invoication of the handler, i.e. leaving the thread asap. The condition can be true if you call cancel during the call to worker/callback.
As stated below I think you still need to invoke the handler, even in the Canceled state, you don't need to care about whether the handler will take long or not. In any case, this specific condition here will never be true: you just wrote either Error or Completed to impl->status, inside the critical section, so it cannot be Canceled there. If you want to keep the Canceled state here, after the callback has executed, you need to check it and keep it before setting Error / Completed.
+ + if (impl->handler != NULL && impl->handler != HANDLER_NOT_SET) + { + IAsyncOperationCompletedHandler_IInspectable *handler = impl->handler; + AsyncStatus status = impl->status; + + IAsyncOperation_IInspectable_AddRef(operation); + IAsyncOperationCompletedHandler_IInspectable_AddRef(handler); + LeaveCriticalSection(&impl->cs); + + IAsyncOperationCompletedHandler_IInspectable_Invoke(handler, operation, status); + + IAsyncOperationCompletedHandler_IInspectable_Release(handler); + IAsyncOperation_IInspectable_Release(operation);
I don't think you need to add all these references here, you already hold one on the operation and on the handler.
Yeah.
+ } + else + LeaveCriticalSection(&impl->cs); + + IAsyncOperation_IInspectable_Release(operation); + return; + +abort: + impl->hr = E_FAIL; + LeaveCriticalSection(&impl->cs); + IAsyncOperation_IInspectable_Release(operation); +} +
It will be hard to check, but I think you need to invoke the handler even in the Cancel case, as MSDN suggests:
If you're implementing IAsyncOperation<TResult>, then the set implementation of Completed should store the handler, and the surrounding logic should invoke it when Close is called. The implementation should set the asyncStatus parameter of invoked callbacks appropriately if there is a Cancel call, Status is not Completed, errors occurred, and so on.
Yeah, seems odd but I can change it.
Bernhard-- Rémi Bernon <rbernon(a)codeweavers.com>
Am Mi., 20. Apr. 2022 um 18:42 Uhr schrieb Rémi Bernon <rbernon(a)codeweavers.com>:
On 4/20/22 18:29, Bernhard Kölbl wrote:
Am Di., 19. Apr. 2022 um 15:08 Uhr schrieb Rémi Bernon <rbernon(a)codeweavers.com>:
On 4/18/22 23:42, Bernhard Kölbl wrote:
Signed-off-by: Bernhard Kölbl <besentv(a)gmail.com> --- dlls/windows.media.speech/async.c | 123 ++++++++++++++++++++++- dlls/windows.media.speech/private.h | 4 +- dlls/windows.media.speech/recognizer.c | 7 +- dlls/windows.media.speech/tests/speech.c | 22 ++-- 4 files changed, 141 insertions(+), 15 deletions(-)
diff --git a/dlls/windows.media.speech/async.c b/dlls/windows.media.speech/async.c index cf0da23e493..66cba46d85b 100644 --- a/dlls/windows.media.speech/async.c +++ b/dlls/windows.media.speech/async.c @@ -40,7 +40,13 @@ struct async_operation LONG ref;
IAsyncOperationCompletedHandler_IInspectable *handler; + IInspectable *result;
+ async_operation_worker worker;
IMHO "callback" is more commonly used and more appropriate name than "worker".
Sure. I used "worker" because the threadpool callback is already named like that, so a possbile confusion would be eliminated.
+ TP_WORK *async_run_work; + IInspectable *invoker; + + CRITICAL_SECTION cs; AsyncStatus status; HRESULT hr; };
@@ -299,7 +345,65 @@ static const struct IAsyncInfoVtbl async_operation_info_vtbl = async_operation_info_Close };
-HRESULT async_operation_create( const GUID *iid, IAsyncOperation_IInspectable **out ) +static void CALLBACK async_run_cb(TP_CALLBACK_INSTANCE *instance, void *data, TP_WORK *work) +{ + IAsyncOperation_IInspectable *operation = data; + struct async_operation *impl = impl_from_IAsyncOperation_IInspectable(operation); + IInspectable *result = NULL, *invoker = NULL; + async_operation_worker worker; + HRESULT hr; + + EnterCriticalSection(&impl->cs); + if (impl->status == Canceled) + goto abort; + + impl->status = Started;
Should it really ignore and overwrite the Closed status here?
Or perhaps instead treat it as cancellation, as it means the client probably doesn't care about the result anymore?
Right, I use Closed as the default/init state, that's why it is overwritten here. So not sure if it needs a change?
I think the initial status, should be changed, in this patch, to Started, and it then doesn't need to be set here (only check if the operation has already been cancelled).
+ + invoker = impl->invoker; + worker = impl->worker; + LeaveCriticalSection(&impl->cs); + + hr = worker(invoker, &result); + + EnterCriticalSection(&impl->cs); + if (FAILED(hr)) + impl->status = Error; + else + impl->status = Completed; + + impl->result = result; + impl->hr = hr; + + if (impl->status == Canceled) + goto abort;
This condition will never be true as you just wrote the status above. I think you can consider that if the callback had been called it's too late for it to be cancelled.
The idea is to cancel the invoication of the handler, i.e. leaving the thread asap. The condition can be true if you call cancel during the call to worker/callback.
As stated below I think you still need to invoke the handler, even in the Canceled state, you don't need to care about whether the handler will take long or not.
In any case, this specific condition here will never be true: you just wrote either Error or Completed to impl->status, inside the critical section, so it cannot be Canceled there.
If you want to keep the Canceled state here, after the callback has executed, you need to check it and keep it before setting Error / Completed.
My bad, I mistook the assignments for compares. I'll make it so Canceled will be kept and the handler is always called.
+ + if (impl->handler != NULL && impl->handler != HANDLER_NOT_SET) + { + IAsyncOperationCompletedHandler_IInspectable *handler = impl->handler; + AsyncStatus status = impl->status; + + IAsyncOperation_IInspectable_AddRef(operation); + IAsyncOperationCompletedHandler_IInspectable_AddRef(handler); + LeaveCriticalSection(&impl->cs); + + IAsyncOperationCompletedHandler_IInspectable_Invoke(handler, operation, status); + + IAsyncOperationCompletedHandler_IInspectable_Release(handler); + IAsyncOperation_IInspectable_Release(operation);
I don't think you need to add all these references here, you already hold one on the operation and on the handler.
Yeah.
+ } + else + LeaveCriticalSection(&impl->cs); + + IAsyncOperation_IInspectable_Release(operation); + return; + +abort: + impl->hr = E_FAIL; + LeaveCriticalSection(&impl->cs); + IAsyncOperation_IInspectable_Release(operation); +} +
It will be hard to check, but I think you need to invoke the handler even in the Cancel case, as MSDN suggests:
If you're implementing IAsyncOperation<TResult>, then the set implementation of Completed should store the handler, and the surrounding logic should invoke it when Close is called. The implementation should set the asyncStatus parameter of invoked callbacks appropriately if there is a Cancel call, Status is not Completed, errors occurred, and so on.
Yeah, seems odd but I can change it.
Bernhard-- Rémi Bernon <rbernon(a)codeweavers.com>
Signed-off-by: Bernhard Kölbl <besentv(a)gmail.com> --- dlls/windows.media.speech/recognizer.c | 124 ++++++++++++++++++++++- dlls/windows.media.speech/tests/speech.c | 10 +- 2 files changed, 128 insertions(+), 6 deletions(-) diff --git a/dlls/windows.media.speech/recognizer.c b/dlls/windows.media.speech/recognizer.c index 5cffe1172e3..1e9469c9dae 100644 --- a/dlls/windows.media.speech/recognizer.c +++ b/dlls/windows.media.speech/recognizer.c @@ -23,6 +23,128 @@ WINE_DEFAULT_DEBUG_CHANNEL(speech); +/* + * + * ISpeechRecognitionCompilationResult + * + */ + +struct compilation_result +{ + ISpeechRecognitionCompilationResult ISpeechRecognitionCompilationResult_iface; + LONG ref; + + SpeechRecognitionResultStatus status; +}; + +static inline struct compilation_result *impl_from_ISpeechRecognitionCompilationResult( ISpeechRecognitionCompilationResult *iface ) +{ + return CONTAINING_RECORD(iface, struct compilation_result, ISpeechRecognitionCompilationResult_iface); +} + +static HRESULT WINAPI compilation_result_QueryInterface( ISpeechRecognitionCompilationResult *iface, REFIID iid, void **out ) +{ + struct compilation_result *impl = impl_from_ISpeechRecognitionCompilationResult(iface); + + TRACE("iface %p, iid %s, out %p.\n", iface, debugstr_guid(iid), out); + + if (IsEqualGUID(iid, &IID_IUnknown) || + IsEqualGUID(iid, &IID_IInspectable) || + IsEqualGUID(iid, &IID_IAgileObject) || + IsEqualGUID(iid, &IID_ISpeechRecognitionCompilationResult)) + { + IInspectable_AddRef((*out = &impl->ISpeechRecognitionCompilationResult_iface)); + return S_OK; + } + + WARN("%s not implemented, returning E_NOINTERFACE.\n", debugstr_guid(iid)); + *out = NULL; + return E_NOINTERFACE; +} + +static ULONG WINAPI compilation_result_AddRef( ISpeechRecognitionCompilationResult *iface ) +{ + struct compilation_result *impl = impl_from_ISpeechRecognitionCompilationResult(iface); + ULONG ref = InterlockedIncrement(&impl->ref); + TRACE("iface %p, ref %lu.\n", iface, ref); + return ref; +} + +static ULONG WINAPI compilation_result_Release( ISpeechRecognitionCompilationResult *iface ) +{ + struct compilation_result *impl = impl_from_ISpeechRecognitionCompilationResult(iface); + + ULONG ref = InterlockedDecrement(&impl->ref); + TRACE("iface %p, ref %lu.\n", iface, ref); + + if (!ref) + free(impl); + + return ref; +} + +static HRESULT WINAPI compilation_result_GetIids( ISpeechRecognitionCompilationResult *iface, ULONG *iid_count, IID **iids ) +{ + FIXME("iface %p, iid_count %p, iids %p stub!\n", iface, iid_count, iids); + return E_NOTIMPL; +} + +static HRESULT WINAPI compilation_result_GetRuntimeClassName( ISpeechRecognitionCompilationResult *iface, HSTRING *class_name ) +{ + FIXME("iface %p, class_name %p stub!\n", iface, class_name); + return E_NOTIMPL; +} + +static HRESULT WINAPI compilation_result_GetTrustLevel( ISpeechRecognitionCompilationResult *iface, TrustLevel *trust_level ) +{ + FIXME("iface %p, trust_level %p stub!\n", iface, trust_level); + return E_NOTIMPL; +} + +static HRESULT WINAPI compilation_result_get_Status( ISpeechRecognitionCompilationResult *iface, SpeechRecognitionResultStatus *value ) +{ + struct compilation_result *impl = impl_from_ISpeechRecognitionCompilationResult(iface); + TRACE("iface %p, value %p.\n", iface, value); + *value = impl->status; + return S_OK; +} + +static const struct ISpeechRecognitionCompilationResultVtbl compilation_result_vtbl = +{ + /* IUnknown methods */ + compilation_result_QueryInterface, + compilation_result_AddRef, + compilation_result_Release, + /* IInspectable methods */ + compilation_result_GetIids, + compilation_result_GetRuntimeClassName, + compilation_result_GetTrustLevel, + /* ISpeechRecognitionCompilationResult methods */ + compilation_result_get_Status +}; + + +static HRESULT WINAPI compilation_result_create( SpeechRecognitionResultStatus status, ISpeechRecognitionCompilationResult **out ) +{ + struct compilation_result *impl; + + TRACE("out %p.\n", out); + + if (!(impl = calloc(1, sizeof(*impl)))) + { + *out = NULL; + return E_OUTOFMEMORY; + } + + impl->ISpeechRecognitionCompilationResult_iface.lpVtbl = &compilation_result_vtbl; + impl->ref = 1; + impl->status = status; + + *out = &impl->ISpeechRecognitionCompilationResult_iface; + TRACE("created %p\n", *out); + return S_OK; +} + /* * * SpeechContinuousRecognitionSession @@ -350,7 +472,7 @@ static HRESULT WINAPI recognizer_get_UIOptions( ISpeechRecognizer *iface, ISpeec static HRESULT WINAPI compile_worker( IInspectable *invoker, IInspectable **result ) { - return S_OK; + return compilation_result_create(SpeechRecognitionResultStatus_Success, (ISpeechRecognitionCompilationResult **) result); } static HRESULT WINAPI recognizer_CompileConstraintsAsync( ISpeechRecognizer *iface, diff --git a/dlls/windows.media.speech/tests/speech.c b/dlls/windows.media.speech/tests/speech.c index 7b05bcdcd7c..13913a1ec96 100644 --- a/dlls/windows.media.speech/tests/speech.c +++ b/dlls/windows.media.speech/tests/speech.c @@ -1017,18 +1017,18 @@ static void test_SpeechRecognizer(void) 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); + ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr); if (SUCCEEDED(hr)) { - todo_wine check_interface(compilation_result, &IID_IAgileObject, TRUE); + check_interface(compilation_result, &IID_IAgileObject, TRUE); hr = ISpeechRecognitionCompilationResult_get_Status(compilation_result, &result_status); - todo_wine ok(hr == S_OK, "ISpeechRecognitionCompilationResult_get_Status failed, hr %#lx.\n", hr); - todo_wine ok(result_status == SpeechRecognitionResultStatus_Success, "Got unexpected status %#x.\n", result_status); + ok(hr == S_OK, "ISpeechRecognitionCompilationResult_get_Status failed, hr %#lx.\n", hr); + ok(result_status == SpeechRecognitionResultStatus_Success, "Got unexpected status %#x.\n", result_status); ref = ISpeechRecognitionCompilationResult_Release(compilation_result); - todo_wine ok(!ref , "Got unexpected ref %lu.\n", ref); + ok(!ref , "Got unexpected ref %lu.\n", ref); } hr = IAsyncOperation_SpeechRecognitionCompilationResult_GetResults(operation, &compilation_result); -- 2.35.1
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=112780 Your paranoid android. === w10pro64 (32 bit report) === windows.media.speech: 1650:speech: unhandled exception c0000002 at 76DDB5B2
Hi Bernhard, On 4/18/22 23:42, Bernhard Kölbl wrote:
Signed-off-by: Bernhard Kölbl <besentv(a)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.
@@ -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?
@@ -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?
hr = IAsyncOperation_SpeechRecognitionCompilationResult_put_Completed(operation, &compilation_handler2.IAsyncHandler_Compilation_iface); todo_wine ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr);
- WaitForSingleObject(compilation_handler2.event_finished, INFINITE); + todo_wine ok(!WaitForSingleObject(compilation_handler2.event_finished, 1000), "Wait for event_finished failed.\n"); CloseHandle(compilation_handler2.event_finished);
Same as above.
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.
+ 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.
+ 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?
+ 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(a)codeweavers.com>
Hi Rémi, Rémi Bernon <rbernon(a)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(a)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(a)codeweavers.com>
Bernhard
On 4/19/22 11:57, Bernhard Kölbl wrote:
- 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.
I think having a non-INFINITE timeout makes the intent less clear. If the wait is never supposed to timeout, INFINITE makes it obvious imho. It also makes sure you will never get spurious failures even if for some reason a run gets particularly slow. If for some reason the test breaks and the wait starts blocking, the testbot timeout is there to catch it, although yes, it's a bit longer.
@@ -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.
Okay, a dedicated patch would make it clearer (or with the broken result changes).
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.
The handlers and recognizer too, no? I don't think you use the other variables after this except to clean them up, which you could do there, before re-creating them.
+ 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.
Sure, though you can think the same for every function call. It's currently unnecessary so I think you shouldn't be forward thinking too much, and change it later iff it becomes necessary (probably never). Cheers, -- Rémi Bernon <rbernon(a)codeweavers.com>
Rémi Bernon <rbernon(a)codeweavers.com> writes:
On 4/19/22 11:57, Bernhard Kölbl wrote:
- 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.
I think having a non-INFINITE timeout makes the intent less clear. If the wait is never supposed to timeout, INFINITE makes it obvious imho. It also makes sure you will never get spurious failures even if for some reason a run gets particularly slow.
If for some reason the test breaks and the wait starts blocking, the testbot timeout is there to catch it, although yes, it's a bit longer.
When I'm running make test before pushing commits, I don't get the testbot timeout, so it's nice if it doesn't get stuck forever. -- Alexandre Julliard julliard(a)winehq.org
On 4/19/22 17:28, Alexandre Julliard wrote:
Rémi Bernon <rbernon(a)codeweavers.com> writes:
On 4/19/22 11:57, Bernhard Kölbl wrote:
- 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.
I think having a non-INFINITE timeout makes the intent less clear. If the wait is never supposed to timeout, INFINITE makes it obvious imho. It also makes sure you will never get spurious failures even if for some reason a run gets particularly slow.
If for some reason the test breaks and the wait starts blocking, the testbot timeout is there to catch it, although yes, it's a bit longer.
When I'm running make test before pushing commits, I don't get the testbot timeout, so it's nice if it doesn't get stuck forever.
Okay, nevermind what I said then. -- Rémi Bernon <rbernon(a)codeweavers.com>
participants (4)
-
Alexandre Julliard -
Bernhard Kölbl -
Marvin -
Rémi Bernon