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

[Zebediah Figura (she/her)]

On 6/28/21 2:45 PM, Zebediah Figura (she/her) 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
> >
> > 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, ...)"?
> > -
> > - 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);

Actually, I guess I was misreading this test a bit; it (sensibly) does 
two things at once. I'd recommend something like:

hr = IFilterGraph2_EnumFilters(...);
     ok(hr == S_OK, "Got hr %#x.\n", hr);

hr = IEnumFilters_Next(...);
     ok(hr == S_OK, "Got hr %#x.\n", hr);
     ok(filter == &testfilter.IBaseFilter_iface, "Got unexpected filter 
%p.\n", filter);

hr = IEnumFilters_Next(...);
     todo_wine ok(hr == S_FALSE, "Got hr %#x.\n", hr);

IEnumFilters_Release(...);

No need for a loop.
> > -
> >
> > +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.
> > +}
> > - /* 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();