Leak in wxWebView? Topic is solved
Leak in wxWebView?
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.
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.
Re: Leak in wxWebView?
Well, code for the method looks simple enough:
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...
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 "";
}
}
Re: Leak in wxWebView?
Or you can check MSDN...
Or even better - check the MSVC log for memory leaks.
Thank you.
Or even better - check the MSVC log for memory leaks.
Thank you.
Re: Leak in wxWebView?
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
OTOH, it seems odd that such a thing would be overlooked in the IE webview code...
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;
}
Re: Leak in wxWebView?
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.
@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.
Re: Leak in wxWebView?
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:
I noticed that wxConvertStringFromOle did leave behind a 'footprint', the code below does not:
Code: Select all
source = std::wstring(bstr);
SysFreeString(bstr);
Re: Leak in wxWebView?
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.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..
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.
Re: Leak in wxWebView?
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:
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.
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"));
Re: Leak in wxWebView?
You can submit a ticket with report (and patch) either on wxTrac or create a PR on wxWidgets GitHub repo.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?
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.RP__ wrote:I would use wxConvertStringFromOle and free the bstr after that by calling SysFreeString.
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...
Re: Leak in wxWebView?
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.
I have created a ticket in Trac: "Memory leaks in wxWebView IE backend". It's awaiting approval now.
Re: Leak in wxWebView?
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__ 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.
Re: Leak in wxWebView?
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:
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);
https://msdn.microsoft.com/en-us/library/ms891285.aspxMSDN wrote:You can free strings created with SysAllocString using SysFreeString.
https://blogs.msdn.microsoft.com/ericli ... semantics/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.
Re: Leak in wxWebView?
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.
Re: Leak in wxWebView?
The issue has been fixed in the master branch:
https://github.com/wxWidgets/wxWidgets/ ... e253b7c10b
https://github.com/wxWidgets/wxWidgets/ ... e253b7c10b