[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<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.

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
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20220815/ed24254b/attachment.htm>