Leak in wxWebView? Topic is solved

If you are using the main C++ distribution of wxWidgets, Feel free to ask any question related to wxWidgets development here. This means questions regarding to C++ and wxWidgets, not compile problems.
Post Reply
RP__
Earned some good credits
Earned some good credits
Posts: 124
Joined: Tue Jan 20, 2015 5:53 pm

Leak in wxWebView?

Post by RP__ »

Hello everyone,

I'm starting to get more and more convinced that there is a memory leak in the GetPageSource function.
Every time I request the page source, my heap increases and it does not decrease the same when the variable I put the source in, is out of scope.
When I do not call the function at all, the memory increases as expected, but when I use the function, the heap increases abnormally.
I do convert the wxString to a wide string, but I expect this not to be the issue.

I am running my application on Windows 7 and Windows 10, using the IE back-end.

Any suggestions how to fix it or what causes it would really help.
The web view is active for a full day, so this is undesired behaviour.
PB
Part Of The Furniture
Part Of The Furniture
Posts: 4193
Joined: Sun Jan 03, 2010 5:45 pm

Re: Leak in wxWebView?

Post by PB »

Well, code for the method looks simple enough:

Code: Select all

wxString wxWebViewIE::GetPageSource() const
{
    wxCOMPtr<IHTMLDocument2> document(GetDocument());

    if(document)
    {
        wxCOMPtr<IHTMLElement> bodyTag;
        wxCOMPtr<IHTMLElement> htmlTag;
        wxString source;
        HRESULT hr = document->get_body(&bodyTag);
        if(SUCCEEDED(hr))
        {
            hr = bodyTag->get_parentElement(&htmlTag);
            if(SUCCEEDED(hr))
            {
                BSTR bstr;
                if ( htmlTag->get_outerHTML(&bstr) == S_OK )
                    source = wxString(bstr);
            }
        }
        return source;
    }
    else
    {
        return "";
    }
}
The question is, should bstr be released via SysFreeString() after being converted to wxString? I am not sure, it seems quite likely to me but perhaps wxString takes its ownership. You could patch the code with that, rebuild wxWidgets and see if it works or goes down in flames. Or you can ask in wxdev group...
ONEEYEMAN
Part Of The Furniture
Part Of The Furniture
Posts: 7459
Joined: Sat Apr 16, 2005 7:22 am
Location: USA, Ukraine

Re: Leak in wxWebView?

Post by ONEEYEMAN »

Or you can check MSDN...
Or even better - check the MSVC log for memory leaks.

Thank you.
PB
Part Of The Furniture
Part Of The Furniture
Posts: 4193
Joined: Sun Jan 03, 2010 5:45 pm

Re: Leak in wxWebView?

Post by PB »

Actually, that wxString constructor is just wxString(const wchar_t *pwz), so that BSTR is left alone.I skimmed through webview_ie.cpp and that is not the only place like that.

Possible memory leak aside, I think that is might not even be the best/safe way of creating a wxString from BSTR, e.g. oleutils.cpp has this

Code: Select all

WXDLLEXPORT wxString wxConvertStringFromOle(BSTR bStr)
{
    // NULL BSTR is equivalent to an empty string (this is the convention used
    // by VB and hence we must follow it)
    if ( !bStr )
        return wxString();

    const int len = SysStringLen(bStr);

#if wxUSE_UNICODE
    wxString str(bStr, len);
#else
    wxString str;
    if (len)
    {
        wxStringBufferLength buf(str, len); // asserts if len == 0
        buf.SetLength(WideCharToMultiByte(CP_ACP, 0 /* no flags */,
                                  bStr, len /* not necessarily NUL-terminated */,
                                  buf, len,
                                  NULL, NULL /* no default char */));
    }
#endif

    return str;
}
OTOH, it seems odd that such a thing would be overlooked in the IE webview code...
RP__
Earned some good credits
Earned some good credits
Posts: 124
Joined: Tue Jan 20, 2015 5:53 pm

Re: Leak in wxWebView?

Post by RP__ »

I can confirm that when I first create a copy of the bstr and then SysFreeString the leak in GetPageSource is gone.

@PB: would using wxConvertStringFromOle instead of the default constructor take care of the issue safely?
And if so, would I still require to call SysFreeString on the original bstr? From what I can see, wxConvertStringFromOle just copies the string but does not free any memory.

Using wxStringBufferLength could solve the issue as well.
RP__
Earned some good credits
Earned some good credits
Posts: 124
Joined: Tue Jan 20, 2015 5:53 pm

Re: Leak in wxWebView?

Post by RP__ »

When I use the following fix, I get no leaks at all.
I noticed that wxConvertStringFromOle did leave behind a 'footprint', the code below does not:

Code: Select all

source = std::wstring(bstr);
SysFreeString(bstr);
PB
Part Of The Furniture
Part Of The Furniture
Posts: 4193
Joined: Sun Jan 03, 2010 5:45 pm

Re: Leak in wxWebView?

Post by PB »

RP__ wrote:@PB: would using wxConvertStringFromOle instead of the default constructor take care of the issue safely?
And if so, would I still require to call SysFreeString on the original bstr? From what I can see, wxConvertStringFromOle just copies the string but does not free any memory..
No, the leak would still be there. The BSTR would still need to be released with SysFreeString(). I meant was that BSTRs can have embedded 0s in it, so generally one should not rely on their length being determined by the first 0 byte encountered: The afore-mentioned wxString constructor treats BSTR as it was just a regular 0-terminated wchar* while wxConvertStringFromOle() asks for the BSTR length (and also converts the string encoding when in ANSI build). But it is not much likely for such strings to be there when using the IE interface.

I think the issue should be reported on wxTrac as a suspected memory leak - I skimmed IHTMLElement docs on MSDN but I did not find there whether the string should be disposed. I am no expert but in such scenarios the programmer is expected to free the returned memory, best using some kind of smart pointer (e.g. MSVC specific _bstr_t).

As I wrote above, GetPageSource() may not be the only place where the leak is, but it may be most noticeable due to the string length. If you do not want to report the issue, please tell me and I will.
RP__
Earned some good credits
Earned some good credits
Posts: 124
Joined: Tue Jan 20, 2015 5:53 pm

Re: Leak in wxWebView?

Post by RP__ »

Thanks for the information regarding the leak.
I would like to report it myself and would also like to fix the issues in the IE webview source.

Where can I find what I need to do in order to report the issue and provide a fix?
I would use wxConvertStringFromOle and free the bstr after that by calling SysFreeString.

EDIT:
While reviewing the code, I see actions like this:

Code: Select all

document->put_designMode(SysAllocString(L"On"));
I cannot find whether designMode takes ownership of the string, so I'm wondering if this string should also be freed after usage as it allocs but does not free in the wx functions.
PB
Part Of The Furniture
Part Of The Furniture
Posts: 4193
Joined: Sun Jan 03, 2010 5:45 pm

Re: Leak in wxWebView?

Post by PB »

RP__ wrote:I would like to report it myself and would also like to fix the issues in the IE webview source.
Where can I find what I need to do in order to report the issue and provide a fix?
You can submit a ticket with report (and patch) either on wxTrac or create a PR on wxWidgets GitHub repo.
RP__ wrote:I would use wxConvertStringFromOle and free the bstr after that by calling SysFreeString.
Actually, I have been already thinking about that. I believe it would be best to introduce a new RAII class - a barebone smart ptr wrapping BSTR, i.e., something like a simplified _b_str_t. It would also have wxString() GetwxString() const method which would basically call wxOleToString(). This class could be placed in a newly created msw/ole/bstrutil.{h|cpp} where wxBasicString would also be moved to. Perhaps it would be best to discuss this first in the ticket about suspected BSTR memory leaks.

Few years back I submitted wxCOMPtr to solve similar issues with COM interface pointers, as we all know, to deal with pointers manually is rather error-prone...
RP__
Earned some good credits
Earned some good credits
Posts: 124
Joined: Tue Jan 20, 2015 5:53 pm

Re: Leak in wxWebView?

Post by RP__ »

If you want to create a new RAII class for it, then that's fine and I will let you handle this if you'd like.
I have created a ticket in Trac: "Memory leaks in wxWebView IE backend". It's awaiting approval now.
PB
Part Of The Furniture
Part Of The Furniture
Posts: 4193
Joined: Sun Jan 03, 2010 5:45 pm

Re: Leak in wxWebView?

Post by PB »

RP__ wrote:If you want to create a new RAII class for it, then that's fine and I will let you handle this if you'd like.
I am sorry if I was not clear: It was just a suggestion, I certainly did not mean to tell you what to do.
RP__
Earned some good credits
Earned some good credits
Posts: 124
Joined: Tue Jan 20, 2015 5:53 pm

Re: Leak in wxWebView?

Post by RP__ »

I think that the best possible solution should be used and if it is either me implementing wxConvertStringFromOle and SysFreeString, or you implementing a new RAII class, I'd prefer your solution as it benefits wxWidgets better as a whole compared to my 'local' solution.

EDIT:
Just confirmed that the functions using BSTR like this also need to be freed and thus also create a leak at this point:

Code: Select all

document->execCommand(SysAllocString(command.wc_str()), VARIANT_FALSE, VARIANT(), NULL);
MSDN wrote:You can free strings created with SysAllocString using SysFreeString.
https://msdn.microsoft.com/en-us/library/ms891285.aspx
Eric’s Complete Guide To BSTR Semantics wrote:2) A BSTR must be allocated and freed with the SysAlloc* family of functions. A PWSZ can be an automatic-storage buffer from the stack or allocated with malloc, new, LocalAlloc or any other memory allocator.
https://blogs.msdn.microsoft.com/ericli ... semantics/
PB
Part Of The Furniture
Part Of The Furniture
Posts: 4193
Joined: Sun Jan 03, 2010 5:45 pm

Re: Leak in wxWebView?

Post by PB »

I have submitted PR #502: Improve BSTR handling in MSW, so we'll see what wxWidgets maintainers will say. They (he?) are extremely busy so it may take some time.
PB
Part Of The Furniture
Part Of The Furniture
Posts: 4193
Joined: Sun Jan 03, 2010 5:45 pm

Re: Leak in wxWebView?

Post by PB »

The issue has been fixed in the master branch:
https://github.com/wxWidgets/wxWidgets/ ... e253b7c10b
Post Reply