[PATCH v2 1/2] qedit: Add test for adding a reference during callback

[David Kahurani]

On Sat, Jul 9, 2022 at 9:52 AM Brendan McGrath <brendan at redmandi.com> wrote:
>
> Unravel Two adds a reference to the IMediaSample during its callback.
>
> This patch adds a test that checks if an application does do this, that it can
> free it later and the reference count will finish at zero.
>
> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=51616
> Signed-off-by: Brendan McGrath <brendan at redmandi.com>
> ---
>
> Thanks Zeb for taking a look. Agree with your feedback. I guess we need
> to prove the workaround is incorrect.
>
> This test I think replicates what Unravel Two does, and it does demonstrate
> that the workaround will cause an issue in this scenario.
>
> But it would be good to hear back from the original author; although 12
> years is a long time!
>
> Unrelated to this patchset - but I noticed I get a seg fault when running the
> mediadet tests (the others seem ok). It happens right at the end:
> Segmentation fault (core dumped)
> make: *** [Makefile:120413: dlls/qedit/tests/mediadet.ok] Error 139
>
> I didn't see it on the Wine test servers, but I'm wondering if you get
> the same when you run locally?

Yes, I also get that.

>
> Anyway, thanks again. Appreciate your time.
>
>
>  dlls/qedit/tests/mediadet.c | 27 ++++++++++++++++++++++-----
>  1 file changed, 22 insertions(+), 5 deletions(-)
>
> diff --git a/dlls/qedit/tests/mediadet.c b/dlls/qedit/tests/mediadet.c
> index 3202baca9244..3ea752176618 100644
> --- a/dlls/qedit/tests/mediadet.c
> +++ b/dlls/qedit/tests/mediadet.c
> @@ -866,6 +866,11 @@ static void test_put_filter(void)
>      ok(!ref, "Got outstanding refcount %ld.\n", ref);
>  }
>
> +struct MySample {
> +  IMediaSample sample;
> +  LONG refcount;
> +};
> +
>  static HRESULT WINAPI ms_QueryInterface(IMediaSample *iface, REFIID riid,
>          void **ppvObject)
>  {
> @@ -874,12 +879,18 @@ static HRESULT WINAPI ms_QueryInterface(IMediaSample *iface, REFIID riid,
>
>  static ULONG WINAPI ms_AddRef(IMediaSample *iface)
>  {
> -    return 2;
> +    struct MySample *my_sample = (struct MySample*) iface;
> +    ULONG refcount = InterlockedIncrement(&my_sample->refcount);
> +
> +    return refcount;
>  }
>
>  static ULONG WINAPI ms_Release(IMediaSample *iface)
>  {
> -    return 1;
> +    struct MySample *my_sample = (struct MySample*) iface;
> +    ULONG refcount = InterlockedDecrement(&my_sample->refcount);
> +
> +    return refcount;
>  }
>
>  static HRESULT WINAPI ms_GetPointer(IMediaSample *iface, BYTE **ppBuffer)
> @@ -989,7 +1000,7 @@ static const IMediaSampleVtbl my_sample_vt = {
>      ms_SetMediaTime
>  };
>
> -static IMediaSample my_sample = { &my_sample_vt };
> +static struct MySample my_sample = { {&my_sample_vt}, 0 };
>
>  static BOOL samplecb_called = FALSE;
>
> @@ -1012,8 +1023,9 @@ static ULONG WINAPI sgcb_Release(ISampleGrabberCB *iface)
>  static HRESULT WINAPI sgcb_SampleCB(ISampleGrabberCB *iface, double SampleTime,
>          IMediaSample *pSample)
>  {
> -    ok(pSample == &my_sample, "Got wrong IMediaSample: %p, expected %p\n", pSample, &my_sample);
> +    ok(pSample == &my_sample.sample, "Got wrong IMediaSample: %p, expected %p\n", pSample, &my_sample);
>      samplecb_called = TRUE;
> +    IMediaSample_AddRef(pSample);
>      return E_NOTIMPL;
>  }
>
> @@ -1043,6 +1055,7 @@ static void test_samplegrabber(void)
>      IEnumPins *pins;
>      HRESULT hr;
>      FILTER_STATE fstate;
> +    ULONG refcount;
>
>      /* Invalid RIID */
>      hr = CoCreateInstance(&CLSID_SampleGrabber, NULL, CLSCTX_INPROC_SERVER, &IID_IClassFactory,
> @@ -1074,10 +1087,14 @@ static void test_samplegrabber(void)
>      hr = IPin_QueryInterface(pin, &IID_IMemInputPin, (void**)&inpin);
>      ok(hr == S_OK, "Got hr %#lx.\n", hr);
>
> -    hr = IMemInputPin_Receive(inpin, &my_sample);
> +    hr = IMemInputPin_Receive(inpin, &my_sample.sample);
>      ok(hr == S_OK, "Got hr %#lx.\n", hr);
>      ok(samplecb_called == TRUE, "SampleCB should have been called\n");
>
> +    // Release the Ref we added in the callback
> +    refcount = IUnknown_Release(&my_sample.sample);
> +    todo_wine ok(refcount == 0, "refcount should be zero\n");
> +
>      IMemInputPin_Release(inpin);
>      IPin_Release(pin);
>
> --
> 2.34.1
>
>