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

[Brendan McGrath]

On 15/7/22 05:02, David Kahurani wrote:
> 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.

I've created a wine bug for this:
https://bugs.winehq.org/show_bug.cgi?id=53556

I've added a lot more detail there, and an experimental fix. I'd be curious to know if the segfault you are seeing is the same thing.
> > 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