[PATCH] windowscodecs/tests: Fix a memory leak (Valgrind).

[Sven Baars]

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=43387
Signed-off-by: Sven Baars 
---
 dlls/windowscodecs/tests/metadata.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/dlls/windowscodecs/tests/metadata.c b/dlls/windowscodecs/tests/metadata.c
index b434648fe5..a955702c82 100644
--- a/dlls/windowscodecs/tests/metadata.c
+++ b/dlls/windowscodecs/tests/metadata.c
@@ -3029,6 +3029,7 @@ static void test_queryreader(void)
                     ok(hr == E_INVALIDARG, "got %#x\n", hr);

IWICMetadataQueryReader_Release(new_reader);
+                    PropVariantClear(&value);
                 }
                 else if (value.vt == VT_LPSTR)
                     ok(!lstrcmpA(U(value).pszVal, test_data[i].str_value), "%u: expected %s, got %s\n",

[Marvin]

Hi,

While running your changed tests, I think I found new failures.
Being a bot and all I'm not very good at pattern recognition, so I might be
wrong, but could you please double-check?

Full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=55582

Your paranoid android.

=== wvistau64_he (task log) ===

Task errors:
The previous 1 run(s) terminated abnormally

[Dmitry Timoshkov]

Sven Baars  wrote:
> diff --git a/dlls/windowscodecs/tests/metadata.c b/dlls/windowscodecs/tests/metadata.c
> index b434648fe5..a955702c82 100644
> --- a/dlls/windowscodecs/tests/metadata.c
> +++ b/dlls/windowscodecs/tests/metadata.c
> @@ -3029,6 +3029,7 @@ static void test_queryreader(void)
>                      ok(hr == E_INVALIDARG, "got %#x\n", hr);
>                  IWICMetadataQueryReader_Release(new_reader);
> -
>                PropVariantClear(&value);
>            }
>            else if (value.vt == VT_LPSTR)
>                ok(!lstrcmpA(U(value).pszVal, test_data[i].str_value), "%u: expected %s, got %s\n",

Did you see a comment 7 lines lower? Calling PropVariantClear() for fake
types will lead to crashes.

[Sven Baars]

On 19-08-19 04:51, Dmitry Timoshkov wrote:
> Sven Baars  wrote:
> > diff --git a/dlls/windowscodecs/tests/metadata.c b/dlls/windowscodecs/tests/metadata.c
> > index b434648fe5..a955702c82 100644
> > --- a/dlls/windowscodecs/tests/metadata.c
> > +++ b/dlls/windowscodecs/tests/metadata.c
> > @@ -3029,6 +3029,7 @@ static void test_queryreader(void)
> >                      ok(hr == E_INVALIDARG, "got %#x\n", hr);
> >                  IWICMetadataQueryReader_Release(new_reader);
> > -
> >                PropVariantClear(&value);
> >            }
> >            else if (value.vt == VT_LPSTR)
> >                ok(!lstrcmpA(U(value).pszVal, test_data[i].str_value), "%u: expected %s, got %s\n",
>
> Did you see a comment 7 lines lower? Calling PropVariantClear() for fake
> types will lead to crashes.

Yes, I did. That's why I do not call it for the other types.

[Dmitry Timoshkov]

Sven Baars  wrote:
> > > diff --git a/dlls/windowscodecs/tests/metadata.c b/dlls/windowscodecs/tests/metadata.c
> > > index b434648fe5..a955702c82 100644
> > > --- a/dlls/windowscodecs/tests/metadata.c
> > > +++ b/dlls/windowscodecs/tests/metadata.c
> > > @@ -3029,6 +3029,7 @@ static void test_queryreader(void)
> > >                      ok(hr == E_INVALIDARG, "got %#x\n", hr);
> > >                  IWICMetadataQueryReader_Release(new_reader);
> > > -
> > >                PropVariantClear(&value);
> > >            }
> > >            else if (value.vt == VT_LPSTR)
> > >                ok(!lstrcmpA(U(value).pszVal, test_data[i].str_value), "%u: expected %s, got %s\n",
> >
> > Did you see a comment 7 lines lower? Calling PropVariantClear() for fake
> > types will lead to crashes.
>
> Yes, I did. That's why I do not call it for the other types.

Thanks for the clarification. If that works then I don't object to the patch.

[Vincent Povirk (they/them)]

I don't understand why we need this for VT_UNKNOWN but not VT_LPSTR.

[Sven Baars]

On 19-08-19 17:27, Vincent Povirk (they/them) wrote:
> I don't understand why we need this for VT_UNKNOWN but not VT_LPSTR.

Because the VT_LPSTR ones are actually the "the_worst" string constant
instead of an allocated string.

[Nikolay Sivov]

On 8/19/19 6:35 PM, Sven Baars wrote:
> On 19-08-19 17:27, Vincent Povirk (they/them) wrote:
> > I don't understand why we need this for VT_UNKNOWN but not VT_LPSTR.
>
> Because the VT_LPSTR ones are actually the "the_worst" string constant
> instead of an allocated string.

Can we make it always allocated to get rid of this special path?

[Sven Baars]

On 19-08-19 17:40, Nikolay Sivov wrote:
> On 8/19/19 6:35 PM, Sven Baars wrote:
> > On 19-08-19 17:27, Vincent Povirk (they/them) wrote:
> > > I don't understand why we need this for VT_UNKNOWN but not VT_LPSTR.
> >
> > Because the VT_LPSTR ones are actually the "the_worst" string constant
> > instead of an allocated string.
>
> Can we make it always allocated to get rid of this special path?

Sure, I sent a new patch and removed the comment.

[Vincent Povirk (they/them)]

Signed-off-by: Vincent Povirk