[PATCH v2 0/1] MR423: ieframe fix

[David Kahurani (＠kahurani)]

Signed-off-by: David Kahurani

--
  v2: ieframe: Fix memory leak and possible reference leak

https://gitlab.winehq.org/wine/wine/-/merge_requests/423

[PATCH v2 1/1] ieframe: Fix memory leak and possible reference leak

[David Kahurani]

From: David Kahurani

Signed-off-by: David Kahurani 
---
 dlls/ieframe/iexplore.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/dlls/ieframe/iexplore.c b/dlls/ieframe/iexplore.c
index 1616d4d7f84..f1e1c5f7f60 100644
--- a/dlls/ieframe/iexplore.c
+++ b/dlls/ieframe/iexplore.c
@@ -1028,8 +1028,13 @@ static HDDEDATA open_dde_url(WCHAR *dde_url)

hres = IWebBrowser2_Navigate2(&ie->IWebBrowser2_iface, &urlv, NULL, NULL, NULL, NULL);
     if(FAILED(hres))
+    {
+        SysFreeString(V_BSTR(&urlv));
+        IWebBrowser2_Release(&ie->IWebBrowser2_iface);
         return 0;
+    }

+    SysFreeString(V_BSTR(&urlv));
     IWebBrowser2_Release(&ie->IWebBrowser2_iface);
     return ULongToHandle(DDE_FACK);
 }

[Nikolay Sivov (＠nsivov)]

Nikolay Sivov (@nsivov) commented about dlls/ieframe/iexplore.c:
>  hres = IWebBrowser2_Navigate2(&ie->IWebBrowser2_iface, &urlv, NULL, NULL, NULL, NULL);
>  if(FAILED(hres))
> -
>
> {
> -
>    SysFreeString(V_BSTR(&urlv));
> -
>    IWebBrowser2_Release(&ie->IWebBrowser2_iface);
>    return 0;
> -
>
> }
> -
>
> SysFreeString(V_BSTR(&urlv));
> IWebBrowser2_Release(&ie->IWebBrowser2_iface);
> return ULongToHandle(DDE_FACK);
>
> }

VariantClear() seems more fitting. Also, you don't need this duplication. Just use FAILED(hres) to return 0 or DDE_FACK.

[David Kahurani (＠kahurani)]

On Mon Jul 11 17:59:39 2022 +0000, Nikolay Sivov wrote:
> VariantClear() seems more fitting. Also, you don't need this
> duplication. Just use FAILED(hres) to return 0 or DDE_FACK.

Hello.

Thanks for the review.

Yes, VariatClear() would indeed be more fitting but I used SysFreeString instead for consistency with the rest of the file. Should I ignore that? Or...maybe look into migrating the whole module?

[Jacek Caban (＠jacek)]

On Mon Jul 11 18:30:06 2022 +0000, David Kahurani wrote:
> Hello.
> Thanks for the review.
> Yes, VariatClear() would indeed be more fitting but I used SysFreeString
> instead for consistency with the rest of the file. Should I ignore that?
> Or...maybe look into migrating the whole module?

Both VariantClear and SysFreeString seem fine to me here as long as it's not duplicated.