[PATCH v6 0/3] MR2496: Fix hang when a program calls IMFMediaSession_Start with an invalid source.
This fixes a bug when the session topology contains an invalid source, which makes the session thread to hang and stop executing commands. -- v6: mf/session: Handle error when a source fails to start. mf/session: Handle errors when subscribing to source's events. mf/tests: Test media session error handling. https://gitlab.winehq.org/wine/wine/-/merge_requests/2496
From: Santino Mazza <smazza(a)codeweavers.com> Test error handling for mfsession_Start when a source fails at different stages. --- dlls/mf/tests/mf.c | 183 ++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 179 insertions(+), 4 deletions(-) diff --git a/dlls/mf/tests/mf.c b/dlls/mf/tests/mf.c index 399f983983f..d4a956b5459 100644 --- a/dlls/mf/tests/mf.c +++ b/dlls/mf/tests/mf.c @@ -41,6 +41,39 @@ #include "initguid.h" #include "evr9.h" +#define DEFINE_EXPECT(func) \ + static BOOL expect_ ## func = FALSE, called_ ## func = FALSE + +#define SET_EXPECT(func) \ + expect_ ## func = TRUE + +#define CHECK_EXPECT2(func) \ + do { \ + ok(expect_ ##func, "unexpected call " #func "\n"); \ + called_ ## func = TRUE; \ + }while(0) + +#define CHECK_EXPECT(func) \ + do { \ + CHECK_EXPECT2(func); \ + expect_ ## func = FALSE; \ + }while(0) + +#define CHECK_CALLED(func) \ + do { \ + ok(called_ ## func, "expected " #func "\n"); \ + expect_ ## func = called_ ## func = FALSE; \ + }while(0) + +#define CHECK_NOT_CALLED(func) \ + do { \ + ok(!called_ ## func, "unexpected " #func "\n"); \ + expect_ ## func = called_ ## func = FALSE; \ + }while(0) + +#define CLEAR_CALLED(func) \ + expect_ ## func = called_ ## func = FALSE + extern GUID DMOVideoFormat_RGB32; HRESULT (WINAPI *pMFCreateSampleCopierMFT)(IMFTransform **copier); @@ -236,10 +269,15 @@ static void init_sink_node(IMFStreamSink *stream_sink, MF_CONNECT_METHOD method, } } +DEFINE_EXPECT(test_source_BeginGetEvent); +DEFINE_EXPECT(test_source_QueueEvent); +DEFINE_EXPECT(test_source_Start); + struct test_source { IMFMediaSource IMFMediaSource_iface; LONG refcount; + HRESULT begin_get_event_res; IMFPresentationDescriptor *pd; }; @@ -294,8 +332,9 @@ static HRESULT WINAPI test_source_GetEvent(IMFMediaSource *iface, DWORD flags, I static HRESULT WINAPI test_source_BeginGetEvent(IMFMediaSource *iface, IMFAsyncCallback *callback, IUnknown *state) { - ok(0, "Unexpected call.\n"); - return E_NOTIMPL; + struct test_source *source = impl_from_IMFMediaSource(iface); + CHECK_EXPECT(test_source_BeginGetEvent); + return source->begin_get_event_res; } static HRESULT WINAPI test_source_EndGetEvent(IMFMediaSource *iface, IMFAsyncResult *result, IMFMediaEvent **event) @@ -307,7 +346,7 @@ static HRESULT WINAPI test_source_EndGetEvent(IMFMediaSource *iface, IMFAsyncRes static HRESULT WINAPI test_source_QueueEvent(IMFMediaSource *iface, MediaEventType event_type, REFGUID ext_type, HRESULT hr, const PROPVARIANT *value) { - ok(0, "Unexpected call.\n"); + CHECK_EXPECT(test_source_QueueEvent); return E_NOTIMPL; } @@ -326,7 +365,7 @@ static HRESULT WINAPI test_source_CreatePresentationDescriptor(IMFMediaSource *i static HRESULT WINAPI test_source_Start(IMFMediaSource *iface, IMFPresentationDescriptor *pd, const GUID *time_format, const PROPVARIANT *start_position) { - ok(0, "Unexpected call.\n"); + CHECK_EXPECT(test_source_Start); return E_NOTIMPL; } @@ -372,6 +411,7 @@ static IMFMediaSource *create_test_source(IMFPresentationDescriptor *pd) source = calloc(1, sizeof(*source)); source->IMFMediaSource_iface.lpVtbl = &test_source_vtbl; source->refcount = 1; + source->begin_get_event_res = E_NOTIMPL; IMFPresentationDescriptor_AddRef((source->pd = pd)); return &source->IMFMediaSource_iface; @@ -1914,6 +1954,41 @@ static HRESULT wait_media_event_(int line, IMFMediaSession *session, IMFAsyncCal return status; } +#define wait_media_event_until_blocking(a, b, c, d, e) wait_media_event_until_blocking_(__LINE__, a, b, c, d, e) +static HRESULT wait_media_event_until_blocking_(int line, IMFMediaSession *session, IMFAsyncCallback *callback, + MediaEventType expect_type, DWORD timeout, PROPVARIANT *value) +{ + struct test_callback *impl = impl_from_IMFAsyncCallback(callback); + MediaEventType type; + HRESULT hr, status; + DWORD ret; + GUID guid; + + do + { + hr = IMFMediaSession_BeginGetEvent(session, &impl->IMFAsyncCallback_iface, (IUnknown *)session); + ok_(__FILE__, line)(hr == S_OK, "Unexpected hr %#lx.\n", hr); + ret = WaitForSingleObject(impl->event, timeout); + if (ret == WAIT_TIMEOUT) return WAIT_TIMEOUT; + hr = IMFMediaEvent_GetType(impl->media_event, &type); + ok_(__FILE__, line)(hr == S_OK, "Unexpected hr %#lx.\n", hr); + } while (type != expect_type); + + ok_(__FILE__, line)(type == expect_type, "got type %lu\n", type); + + hr = IMFMediaEvent_GetExtendedType(impl->media_event, &guid); + ok_(__FILE__, line)(hr == S_OK, "Unexpected hr %#lx.\n", hr); + ok_(__FILE__, line)(IsEqualGUID(&guid, &GUID_NULL), "got extended type %s\n", debugstr_guid(&guid)); + + hr = IMFMediaEvent_GetValue(impl->media_event, value); + ok_(__FILE__, line)(hr == S_OK, "Unexpected hr %#lx.\n", hr); + + hr = IMFMediaEvent_GetStatus(impl->media_event, &status); + ok_(__FILE__, line)(hr == S_OK, "Unexpected hr %#lx.\n", hr); + + return status; +} + static IMFMediaSource *create_media_source(const WCHAR *name, const WCHAR *mime) { IMFSourceResolver *resolver; @@ -1985,6 +2060,7 @@ static void test_media_session_events(void) struct test_stream_sink stream_sink = test_stream_sink; struct test_media_sink media_sink = test_media_sink; struct test_handler handler = test_handler; + struct test_source *source_impl; IMFAsyncCallback *callback, *callback2; IMFMediaType *input_type, *output_type; IMFTopologyNode *src_node, *sink_node; @@ -2363,6 +2439,105 @@ static void test_media_session_events(void) /* sometimes briefly leaking */ IMFMediaSession_Release(session); + + /* test IMFMediaSession_Start with source returning an error in BeginGetEvent */ + source_impl = impl_from_IMFMediaSource(source); + + hr = MFCreateMediaSession(NULL, &session); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + + hr = IMFMediaSession_SetTopology(session, 0, topology); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + hr = wait_media_event(session, callback, MESessionTopologySet, 1000, &propvar); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + ok(propvar.vt == VT_UNKNOWN, "got vt %u\n", propvar.vt); + ok(propvar.punkVal != (IUnknown *)topology, "got punkVal %p\n", propvar.punkVal); + PropVariantClear(&propvar); + + source_impl->begin_get_event_res = 0x80001234; + + SET_EXPECT(test_source_BeginGetEvent); + SET_EXPECT(test_source_QueueEvent); + SET_EXPECT(test_source_Start); + + propvar.vt = VT_EMPTY; + hr = IMFMediaSession_Start(session, &GUID_NULL, &propvar); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + hr = wait_media_event_until_blocking(session, callback, MESessionStarted, 1000, &propvar); + todo_wine ok(hr == 0x80001234, "Unexpected hr %#lx.\n", hr); + ok(propvar.vt == VT_EMPTY, "got vt %u\n", propvar.vt); + ok(propvar.punkVal != (IUnknown *)topology, "got punkVal %p\n", propvar.punkVal); + PropVariantClear(&propvar); + + CHECK_CALLED(test_source_BeginGetEvent); + todo_wine { CHECK_NOT_CALLED(test_source_Start); } + + hr = IMFMediaSession_ClearTopologies(session); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + + hr = IMFMediaSession_Shutdown(session); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + ok(media_sink.shutdown, "media sink didn't shutdown.\n"); + media_sink.shutdown = FALSE; + + source_impl->begin_get_event_res = E_NOTIMPL; + + CLEAR_CALLED(test_source_BeginGetEvent); + CLEAR_CALLED(test_source_QueueEvent); + CLEAR_CALLED(test_source_Start); + + /* sometimes briefly leaking */ + IMFMediaSession_Release(session); + + + /* test IMFMediaSession_Start when test source BeginGetEvent returns S_OK */ + hr = MFCreateMediaSession(NULL, &session); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + + hr = IMFMediaSession_SetTopology(session, 0, topology); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + hr = wait_media_event(session, callback, MESessionTopologySet, 1000, &propvar); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + ok(propvar.vt == VT_UNKNOWN, "got vt %u\n", propvar.vt); + ok(propvar.punkVal != (IUnknown *)topology, "got punkVal %p\n", propvar.punkVal); + PropVariantClear(&propvar); + + source_impl = impl_from_IMFMediaSource(source); + source_impl->begin_get_event_res = S_OK; + + SET_EXPECT(test_source_BeginGetEvent); + SET_EXPECT(test_source_QueueEvent); + SET_EXPECT(test_source_Start); + + propvar.vt = VT_EMPTY; + hr = IMFMediaSession_Start(session, &GUID_NULL, &propvar); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + hr = wait_media_event_until_blocking(session, callback, MESessionStarted, 1000, &propvar); + todo_wine ok(hr == E_NOTIMPL, "Unexpected hr %#lx.\n", hr); + ok(propvar.vt == VT_EMPTY, "got vt %u\n", propvar.vt); + ok(propvar.punkVal != (IUnknown *)topology, "got punkVal %p\n", propvar.punkVal); + PropVariantClear(&propvar); + + CHECK_CALLED(test_source_BeginGetEvent); + CHECK_CALLED(test_source_Start); + + hr = IMFMediaSession_ClearTopologies(session); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + + hr = IMFMediaSession_Shutdown(session); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + ok(media_sink.shutdown, "media sink didn't shutdown.\n"); + media_sink.shutdown = FALSE; + + source_impl->begin_get_event_res = E_NOTIMPL; + + CLEAR_CALLED(test_source_BeginGetEvent); + CLEAR_CALLED(test_source_QueueEvent); + CLEAR_CALLED(test_source_Start); + + /* sometimes briefly leaking */ + IMFMediaSession_Release(session); + IMFAsyncCallback_Release(callback); if (handler.current_type) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2496
From: Santino Mazza <smazza(a)codeweavers.com> --- dlls/mf/session.c | 18 +++++++++++------- dlls/mf/tests/mf.c | 4 ++-- 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/dlls/mf/session.c b/dlls/mf/session.c index 85baf07d05a..bac4f23b9c6 100644 --- a/dlls/mf/session.c +++ b/dlls/mf/session.c @@ -866,13 +866,13 @@ static void session_command_complete_with_event(struct media_session *session, M session_command_complete(session); } -static void session_subscribe_sources(struct media_session *session) +static HRESULT session_subscribe_sources(struct media_session *session) { struct media_source *source; - HRESULT hr; + HRESULT hr = S_OK; if (session->presentation.flags & SESSION_FLAG_SOURCES_SUBSCRIBED) - return; + return hr; LIST_FOR_EACH_ENTRY(source, &session->presentation.sources, struct media_source, entry) { @@ -880,10 +880,12 @@ static void session_subscribe_sources(struct media_session *session) source->object))) { WARN("Failed to subscribe to source events, hr %#lx.\n", hr); + return hr; } } session->presentation.flags |= SESSION_FLAG_SOURCES_SUBSCRIBED; + return hr; } static void session_start(struct media_session *session, const GUID *time_format, const PROPVARIANT *start_position) @@ -909,7 +911,11 @@ static void session_start(struct media_session *session, const GUID *time_format session->presentation.start_position.vt = VT_EMPTY; PropVariantCopy(&session->presentation.start_position, start_position); - session_subscribe_sources(session); + if (FAILED(hr = session_subscribe_sources(session))) + { + session_command_complete_with_event(session, MESessionStarted, hr, NULL); + return; + } LIST_FOR_EACH_ENTRY(source, &session->presentation.sources, struct media_source, entry) { @@ -1308,10 +1314,8 @@ static void session_set_rate(struct media_session *session, BOOL thin, float rat if (SUCCEEDED(hr)) hr = IMFRateControl_GetRate(session->clock_rate_control, NULL, &clock_rate); - if (SUCCEEDED(hr) && (rate != clock_rate)) + if (SUCCEEDED(hr) && (rate != clock_rate) && SUCCEEDED(hr = session_subscribe_sources(session))) { - session_subscribe_sources(session); - LIST_FOR_EACH_ENTRY(source, &session->presentation.sources, struct media_source, entry) { if (SUCCEEDED(hr = MFGetService(source->object, &MF_RATE_CONTROL_SERVICE, &IID_IMFRateControl, diff --git a/dlls/mf/tests/mf.c b/dlls/mf/tests/mf.c index d4a956b5459..d2cb0ff792b 100644 --- a/dlls/mf/tests/mf.c +++ b/dlls/mf/tests/mf.c @@ -2464,13 +2464,13 @@ static void test_media_session_events(void) hr = IMFMediaSession_Start(session, &GUID_NULL, &propvar); ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); hr = wait_media_event_until_blocking(session, callback, MESessionStarted, 1000, &propvar); - todo_wine ok(hr == 0x80001234, "Unexpected hr %#lx.\n", hr); + ok(hr == 0x80001234, "Unexpected hr %#lx.\n", hr); ok(propvar.vt == VT_EMPTY, "got vt %u\n", propvar.vt); ok(propvar.punkVal != (IUnknown *)topology, "got punkVal %p\n", propvar.punkVal); PropVariantClear(&propvar); CHECK_CALLED(test_source_BeginGetEvent); - todo_wine { CHECK_NOT_CALLED(test_source_Start); } + CHECK_NOT_CALLED(test_source_Start); hr = IMFMediaSession_ClearTopologies(session); ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2496
From: Santino Mazza <smazza(a)codeweavers.com> --- dlls/mf/session.c | 4 ++++ dlls/mf/tests/mf.c | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/dlls/mf/session.c b/dlls/mf/session.c index bac4f23b9c6..7d6a65d3f55 100644 --- a/dlls/mf/session.c +++ b/dlls/mf/session.c @@ -920,7 +920,11 @@ static void session_start(struct media_session *session, const GUID *time_format LIST_FOR_EACH_ENTRY(source, &session->presentation.sources, struct media_source, entry) { if (FAILED(hr = IMFMediaSource_Start(source->source, source->pd, &GUID_NULL, start_position))) + { WARN("Failed to start media source %p, hr %#lx.\n", source->source, hr); + session_command_complete_with_event(session, MESessionStarted, hr, NULL); + return; + } } session->state = SESSION_STATE_STARTING_SOURCES; diff --git a/dlls/mf/tests/mf.c b/dlls/mf/tests/mf.c index d2cb0ff792b..cc28786cf65 100644 --- a/dlls/mf/tests/mf.c +++ b/dlls/mf/tests/mf.c @@ -2513,7 +2513,7 @@ static void test_media_session_events(void) hr = IMFMediaSession_Start(session, &GUID_NULL, &propvar); ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); hr = wait_media_event_until_blocking(session, callback, MESessionStarted, 1000, &propvar); - todo_wine ok(hr == E_NOTIMPL, "Unexpected hr %#lx.\n", hr); + ok(hr == E_NOTIMPL, "Unexpected hr %#lx.\n", hr); ok(propvar.vt == VT_EMPTY, "got vt %u\n", propvar.vt); ok(propvar.punkVal != (IUnknown *)topology, "got punkVal %p\n", propvar.punkVal); PropVariantClear(&propvar); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2496
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=131780 Your paranoid android. === w1064_2qxl (64 bit report) === mf: mf.c:2558: Test failed: Release returned 2 mf.c:2560: Test failed: Release returned 3
This merge request was approved by Nikolay Sivov. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2496
On Tue Apr 11 13:59:29 2023 +0000, **** wrote:
Marvin replied on the mailing list: ``` Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=131780 Your paranoid android. === w1064_2qxl (64 bit report) === mf: mf.c:2558: Test failed: Release returned 2 mf.c:2560: Test failed: Release returned 3 ``` mmm I tested this in the testbot but the tests don't fail, with the same machine w1064_2qxl
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2496#note_29605
participants (4)
-
Marvin -
Nikolay Sivov (＠nsivov) -
Santino Mazza
-
Santino Mazza (＠tati1454)