Returning BSTR to ActiveX crashes IE7

Hello,

I'm encountering an issue where calling a simple one method ATL ActiveX
control is crashing IE7. Here's a copy of the Javascript and the C++. The
component is build using VS 2005. The control also leverages the SiteLock
template.

My suspicious is that I'm not building up the BSTR correctly and a memory
leak is occurring.

function GetInfo()
{
var strInfo = "";
var detObj = Detector1;
if(detObj != null)
{
try
{
strInfo = detObj.GetInfo();

var frmOffer = document.forms[0];
if(frmOffer != null)
{
var hdnInfo =
document.getElementById('<%=hdnInfo.ClientID%>');
if(hdnInfo != null)
{
hdnInfo.value = strInfo;
frmOffer.submit();
}
}
}
catch(err)
{
// ActiveX was not installed (i.e., 1st page load before install)
}
}
else
{
strInfo = "Still checking. This may take a few minutes.";
}
}

</script>


STDMETHODIMP CDetector::GetInfo(BSTR* output)
{

try
{
CComBSTR strInfo;

strInfo += L"~@model@~";
strInfo += _bstr_t(m_Model, true); //m_Model is a BSTR
strInfo += L"~@/model@~";

strInfo.CopyTo(output);

SysFreeString(strInfo);
}

catch( char *str )
{
cout << "Exception caught : " << str << "\n";
}

return S_OK;
}

Igor
Wed Jul 16 13:13:08 PDT 2008

Frank <Frank@discussions.microsoft.com> wrote:
> CComBSTR strInfo;
> strInfo += L"~@model@~";
> SysFreeString(strInfo);

Drop SysFreeString call. You are destroying the same string twice - once
explicitly with SysFreeString, and again in ~CComBSTR destructor when
strInfo goes out of scope.
--
With best wishes,
Igor Tandetnik

With sufficient thrust, pigs fly just fine. However, this is not
necessarily a good idea. It is hard to be sure where they are going to
land, and it could be dangerous sitting under them as they fly
overhead. -- RFC 1925

Brian
Wed Jul 16 13:19:42 PDT 2008

inline....

"Frank" <Frank@discussions.microsoft.com> wrote in message news:D852ED34-C14B-4592-8CFC-B91D985A02FE@microsoft.com...
> Hello,
>
> I'm encountering an issue where calling a simple one method ATL ActiveX
> control is crashing IE7. Here's a copy of the Javascript and the C++. The
> component is build using VS 2005. The control also leverages the SiteLock
> template.
>
> My suspicious is that I'm not building up the BSTR correctly and a memory
> leak is occurring.
>
> function GetInfo()
> {
> var strInfo = "";
> var detObj = Detector1;
> if(detObj != null)
> {
> try
> {
> strInfo = detObj.GetInfo();
>
> var frmOffer = document.forms[0];
> if(frmOffer != null)
> {
> var hdnInfo =
> document.getElementById('<%=hdnInfo.ClientID%>');
> if(hdnInfo != null)
> {
> hdnInfo.value = strInfo;
> frmOffer.submit();
> }
> }
> }
> catch(err)
> {
> // ActiveX was not installed (i.e., 1st page load before install)
> }
> }
> else
> {
> strInfo = "Still checking. This may take a few minutes.";
> }
> }
>
> </script>
>
>
> STDMETHODIMP CDetector::GetInfo(BSTR* output)
> {
>
> try
> {
> CComBSTR strInfo;
>
> strInfo += L"~@model@~";
> strInfo += _bstr_t(m_Model, true); //m_Model is a BSTR
> strInfo += L"~@/model@~";
>
> strInfo.CopyTo(output);
>
> SysFreeString(strInfo);

Remove the above line. strInfo is a smart pointer, and will free the BSTR when it goes out of scope. Because you are freeing the
BSTR a second time you are causing memory corruption.

Also, it's a bad habit to be mixing CComBSTR() and _bstr_t(). In the above example, you don't need to use _bstr_t() at all. Just
append m_Modal.

Typically you enclose _bstr_t usage inside a try...catch construct, since it can throw exceptions.


> }
>
> catch( char *str )
> {
> cout << "Exception caught : " << str << "\n";
> }
>
> return S_OK;
> }

Frank
Wed Jul 16 16:51:01 PDT 2008

Thank you Igor. That did it!

"Igor Tandetnik" wrote:

> Frank <Frank@discussions.microsoft.com> wrote:
> > CComBSTR strInfo;
> > strInfo += L"~@model@~";
> > SysFreeString(strInfo);
>
> Drop SysFreeString call. You are destroying the same string twice - once
> explicitly with SysFreeString, and again in ~CComBSTR destructor when
> strInfo goes out of scope.
> --
> With best wishes,
> Igor Tandetnik
>
> With sufficient thrust, pigs fly just fine. However, this is not
> necessarily a good idea. It is hard to be sure where they are going to
> land, and it could be dangerous sitting under them as they fly
> overhead. -- RFC 1925
>
>
>

Frank
Wed Jul 16 16:56:00 PDT 2008

Thank you Brian. I was actually using .Append() in an earlier version but got
too focused on trying fixes for the mem leak issue and hadn't changed it
back. Nice catch!

"Brian Muth" wrote:

> inline....
>
> "Frank" <Frank@discussions.microsoft.com> wrote in message news:D852ED34-C14B-4592-8CFC-B91D985A02FE@microsoft.com...
> > Hello,
> >
> > I'm encountering an issue where calling a simple one method ATL ActiveX
> > control is crashing IE7. Here's a copy of the Javascript and the C++. The
> > component is build using VS 2005. The control also leverages the SiteLock
> > template.
> >
> > My suspicious is that I'm not building up the BSTR correctly and a memory
> > leak is occurring.
> >
> > function GetInfo()
> > {
> > var strInfo = "";
> > var detObj = Detector1;
> > if(detObj != null)
> > {
> > try
> > {
> > strInfo = detObj.GetInfo();
> >
> > var frmOffer = document.forms[0];
> > if(frmOffer != null)
> > {
> > var hdnInfo =
> > document.getElementById('<%=hdnInfo.ClientID%>');
> > if(hdnInfo != null)
> > {
> > hdnInfo.value = strInfo;
> > frmOffer.submit();
> > }
> > }
> > }
> > catch(err)
> > {
> > // ActiveX was not installed (i.e., 1st page load before install)
> > }
> > }
> > else
> > {
> > strInfo = "Still checking. This may take a few minutes.";
> > }
> > }
> >
> > </script>
> >
> >
> > STDMETHODIMP CDetector::GetInfo(BSTR* output)
> > {
> >
> > try
> > {
> > CComBSTR strInfo;
> >
> > strInfo += L"~@model@~";
> > strInfo += _bstr_t(m_Model, true); //m_Model is a BSTR
> > strInfo += L"~@/model@~";
> >
> > strInfo.CopyTo(output);
> >
> > SysFreeString(strInfo);
>
> Remove the above line. strInfo is a smart pointer, and will free the BSTR when it goes out of scope. Because you are freeing the
> BSTR a second time you are causing memory corruption.
>
> Also, it's a bad habit to be mixing CComBSTR() and _bstr_t(). In the above example, you don't need to use _bstr_t() at all. Just
> append m_Modal.
>
> Typically you enclose _bstr_t usage inside a try...catch construct, since it can throw exceptions.
>
>
> > }
> >
> > catch( char *str )
> > {
> > cout << "Exception caught : " << str << "\n";
> > }
> >
> > return S_OK;
> > }
>
>