[PATCH v3 0/2] MR2496: Draft: Fix hang when a program calls IMFMediaSession_Start with an invalid source.

[Santino Mazza (＠tati1454)]

This fixes a bug when the session topology contains an invalid
source, which makes the session thread to hang and stop executing
commands.

--
  v3: mf: Handle errors when subscribing to events.
      mf/tests: Test media session error handling.

https://gitlab.winehq.org/wine/wine/-/merge_requests/2496

[PATCH v3 1/2] mf/tests: Test media session error handling.

[Santino Mazza]

From: Santino Mazza

---
 dlls/mf/tests/mf.c | 197 +++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 189 insertions(+), 8 deletions(-)

diff --git a/dlls/mf/tests/mf.c b/dlls/mf/tests/mf.c
index 399f983983f..15b77013a1b 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,111 @@ 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 = wait_media_event(session, callback, MESessionTopologiesCleared, 1000, &propvar);
+    ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
+    ok(propvar.vt == VT_EMPTY, "got vt %u\n", propvar.vt);
+
+    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 = wait_media_event(session, callback, MESessionTopologiesCleared, 1000, &propvar);
+    ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
+    ok(propvar.vt == VT_EMPTY, "got vt %u\n", propvar.vt);
+
+    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)
@@ -2379,13 +2560,13 @@ static void test_media_session_events(void)
     ok(ref == 0, "Release returned %ld\n", ref);

ref = IMFMediaSource_Release(source);
-    ok(ref == 0, "Release returned %ld\n", ref);
+    todo_wine ok(ref == 0, "Release returned %ld\n", ref);
     ref = IMFPresentationDescriptor_Release(pd);
-    ok(ref == 0, "Release returned %ld\n", ref);
+    todo_wine ok(ref == 0, "Release returned %ld\n", ref);
     ref = IMFStreamDescriptor_Release(sd);
-    ok(ref == 0, "Release returned %ld\n", ref);
+    todo_wine ok(ref == 0, "Release returned %ld\n", ref);
     ref = IMFMediaType_Release(input_type);
-    ok(ref == 0, "Release returned %ld\n", ref);
+    todo_wine ok(ref == 0, "Release returned %ld\n", ref);

hr = MFShutdown();

[PATCH v3 2/2] mf: Handle errors when subscribing to events.

[Santino Mazza]

From: Santino Mazza

---
 dlls/mf/session.c  | 21 ++++++++++++++-------
 dlls/mf/tests/mf.c | 14 +++++++-------
 2 files changed, 21 insertions(+), 14 deletions(-)

diff --git a/dlls/mf/session.c b/dlls/mf/session.c
index 85baf07d05a..f6656ec45c0 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,12 +911,19 @@ 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)
             {
                 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;
@@ -1308,10 +1317,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 15b77013a1b..9c1e333ff14 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);
@@ -2516,7 +2516,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);
@@ -2560,13 +2560,13 @@ static void test_media_session_events(void)
     ok(ref == 0, "Release returned %ld\n", ref);

ref = IMFMediaSource_Release(source);
-    todo_wine ok(ref == 0, "Release returned %ld\n", ref);
+    ok(ref == 0, "Release returned %ld\n", ref);
     ref = IMFPresentationDescriptor_Release(pd);
-    todo_wine ok(ref == 0, "Release returned %ld\n", ref);
+    ok(ref == 0, "Release returned %ld\n", ref);
     ref = IMFStreamDescriptor_Release(sd);
-    todo_wine ok(ref == 0, "Release returned %ld\n", ref);
+    ok(ref == 0, "Release returned %ld\n", ref);
     ref = IMFMediaType_Release(input_type);
-    todo_wine ok(ref == 0, "Release returned %ld\n", ref);
+    ok(ref == 0, "Release returned %ld\n", ref);

hr = MFShutdown();

[PATCH v3 0/2] MR2496: Fix hang when a program calls IMFMediaSession_Start with an invalid source.

[Nikolay Sivov (＠nsivov)]

Nikolay Sivov (@nsivov) commented about dlls/mf/session.c:
>          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)
>        {
>            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;
> -
>            }

This should be two separate changes.

[PATCH v3 0/2] MR2496: Fix hang when a program calls IMFMediaSession_Start with an invalid source.

[Nikolay Sivov (＠nsivov)]

Nikolay Sivov (@nsivov) commented about dlls/mf/session.c:
>          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;
> -
>        }

Please use opening brace on a separate line, like the rest of this file.

[PATCH v3 0/2] MR2496: Fix hang when a program calls IMFMediaSession_Start with an invalid source.

[Nikolay Sivov (＠nsivov)]

Nikolay Sivov (@nsivov) commented about dlls/mf/session.c:
>  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)))

Why !SUCCEEDED()?