Re: [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  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
>
> 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