[PATCH 1/2] quartz/tests: Test IGraphBuilder::RenderFile cleanup on failure

[Zebediah Figura (she/her)]

On 6/28/21 5:09 PM, Tim Clem wrote:
> June 28, 2021 12:46 PM, "Zebediah Figura (she/her)" <zfigura at codeweavers.com> wrote:
> 
>> On 6/28/21 1:46 PM, Tim Clem wrote:
>>
>>> Testing on Windows confirmed that IGraphBuilder::RenderFile removes the source filter it adds
>>> if the file cannot be played. This was resulting in leaked file handles and threads if a
>>> program tried to play media without a proper set of filters installed.
>>> Signed-off-by: Tim Clem <tclem at codeweavers.com>
>>> ---
>>> dlls/quartz/tests/filtergraph.c | 54 +++++++++++++++++++++++++++++++++
>>> 1 file changed, 54 insertions(+)
>>> diff --git a/dlls/quartz/tests/filtergraph.c b/dlls/quartz/tests/filtergraph.c
>>> index fdcfcead0be..a509a07872c 100644
>>> --- a/dlls/quartz/tests/filtergraph.c
>>> +++ b/dlls/quartz/tests/filtergraph.c
>>> @@ -4186,6 +4186,59 @@ static void test_ec_complete(void)
>>> ok(filter3.ref == 1, "Got outstanding refcount %d.\n", filter3.ref);
>>> }
>>>> +static void test_renderfile_failure(void)
>>> +{
>>> + static const char bogus_data[20] = {0xde, 0xad, 0xbe, 0xef};
>>> +
>>> + struct testfilter testfilter;
>>> + IFilterGraph2 *graph;
>>> + IEnumFilters *filterenum;
>>> + IBaseFilter *filter;
>>> + HRESULT hr;
>>> + BOOL saw_testfilter = FALSE;
>>> + FILTER_INFO filter_info;
>>> + const WCHAR *filename;
>>> +
>>> + /* Windows removes the source filter from the graph if a RenderFile
>>> + * call fails. It leaves the rest of the graph intact. */
>>> +
>>> + graph = create_graph();
>>> + testfilter_init(&testfilter, NULL, 0);
>>> + hr = IFilterGraph2_AddFilter(graph, &testfilter.IBaseFilter_iface, L"dummy");
>>> + ok(hr == S_OK, "Got hr %#x.\n", hr);
>>> +
>>> + filename = create_file(L"test.nonsense", bogus_data, sizeof(bogus_data));
>>> + hr = IFilterGraph2_RenderFile(graph, filename, NULL);
>>> + if (SUCCEEDED(hr)) {
>>> + skip("Expected RenderFile to fail on invalid file.\n");
>>> + goto out;
>>> + }
>>
>> Any reason why this isn't "ok(hr == VFW_E_CANNOT_RENDER, ...)"?
> 
> Windows reports VFW_E_UNSUPPORTED_STREAM in this case. The documentation on the various return values is really vague, so I thought it was best to be non-specific here.

Vague and not always accurate, especially when it comes to error codes ;-)

In any case I'd prefer to test the actual return value here, with 
todo_wine if necessary.

FWIW, I suspect that RenderFile() should be doing an unconditional 
translation of VFW_E_CANNOT_RENDER -> VFW_E_UNSUPPORTED_STREAM. Just 
from quick testing I see that happening both when there's no source 
filter registered, and when an intermediate filter fails to decode. Not 
sure why there's other error codes listed there, but I can't immediately 
think of ways to provoke them.

>>> +
>>> + hr = IFilterGraph2_EnumFilters(graph, &filterenum);
>>> + ok(hr == S_OK, "Got hr %#x.\n", hr);
>>> +
>>> + while (IEnumFilters_Next(filterenum, 1, &filter, NULL) == S_OK)
>>> + {
>>> + if (!saw_testfilter && filter == &testfilter.IBaseFilter_iface)
>>> + saw_testfilter = TRUE;
>>> + else {
>>> + hr = IBaseFilter_QueryFilterInfo(filter, &filter_info);
>>> + ok(hr == S_OK, "Got hr %#x.\n", hr);
>>> + todo_wine ok(0, "Unexpected filter %p (%s) left in graph.\n",
>>> + filter, wine_dbgstr_w(filter_info.achName));
>>> + }
>>> +
>>> + IBaseFilter_Release(filter);
>>> + }
>>> + IEnumFilters_Release(filterenum);
>>> +
>>> + ok(saw_testfilter, "Expected testfilter to remain in graph.\n");
>>
>> Or, even easier:
>>
>> ok(testfilter.graph == (IFilterGraph *)graph, "Got graph %p.\n", testfilter.graph);
> 
> I updated the test as per your other email. Much simpler. Thanks for the notes.
> 
>>
>>> +
>>> +out:
>>> + IFilterGraph2_Release(graph);
>>> + DeleteFileW(filename);
>>
>> Can you please check for failure from DeleteFile() here? I try to be extra paranoid that we aren't
>> leaking handles to the file anywhere; we've done so in the past.
> 
> Good idea, especially since this *is* leaking a file handle here before the patch - the source made by RenderFile will not be closed.
> 
>>
>>> +}
>>> +
>>> /* Remove and re-add the filter, to flush the graph's internal
>>> * IMediaSeeking cache. Don't expose IMediaSeeking when adding, to show
>>> * that it's only queried when needed. */
>>> @@ -5645,6 +5698,7 @@ START_TEST(filtergraph)
>>> test_sync_source();
>>> test_filter_state();
>>> test_ec_complete();
>>> + test_renderfile_failure();
>>> test_graph_seeking();
>>> test_default_sync_source();
>>> test_add_source_filter();