Improve wxBasicString and fix memory leaks when using it

Improve and extend wxBasicString API and fix multiple BSTR leaks in due to
confusingly named Get() method in the old API.

Closes #17889.

include/wx/msw/ole/oleutils.h

@@ -65,20 +65,48 @@ inline void wxOleUninitialize()
 
 class WXDLLIMPEXP_CORE wxBasicString
 {
-public:
+public:    
-    // ctors & dtor
+    // Takes over the ownership of bstr
+    explicit wxBasicString(BSTR bstr = NULL);
+
+    // Constructs the BSTR from wxString
     wxBasicString(const wxString& str);
+
+    // Creates a copy of the BSTR owned by bstr
     wxBasicString(const wxBasicString& bstr);
+
+    // Frees the owned BSTR
     ~wxBasicString();
 
+    // Returns the owned BSTR and gives up its ownership
+    BSTR Detach();
+
+    // Frees the owned BSTR
+    void Free();
+
+    // Returns a copy of the owned BSTR,
+    // the caller is responsible for freeing it
+    BSTR Copy() const { return SysAllocString(m_bstrBuf); }
+
+    // Returns the address of the owned BSTR, not to be called
+    // when wxBasicString already contains a non-NULL BSTR
+    BSTR* ByRef();
+
+    // Sets its BSTR to a copy of the BSTR owned by bstr
     wxBasicString& operator=(const wxBasicString& bstr);
 
-    // accessors
+    // Creates its BSTR from wxString
-        // just get the string
+    wxBasicString& operator=(const wxString& str);
-    operator BSTR() const { return m_bstrBuf; }
+
-        // retrieve a copy of our string - caller must SysFreeString() it later!
+    // Takes over the ownership of bstr
-    BSTR Get() const { return SysAllocString(m_bstrBuf); }
+    wxBasicString& operator=(BSTR bstr);
 
+    /// Returns the owned BSTR while keeping its ownership    
+    operator BSTR() const { return m_bstrBuf; }
+
+    // retrieve a copy of our string - caller must SysFreeString() it later!
+    wxDEPRECATED_MSG("use Copy() instead") 
+    BSTR Get() const { return Copy(); }
 private:
     // actual string
     BSTR m_bstrBuf;

src/msw/mediactrl_am.cpp

@@ -1127,7 +1127,7 @@ bool wxAMMediaBackend::Load(const wxURI& location, const wxURI& proxy)
     if(pPlay)
     {
         pPlay->put_UseHTTPProxy(VARIANT_TRUE);
-        pPlay->put_HTTPProxyHost(wxBasicString(proxy.GetServer()).Get());
+        pPlay->put_HTTPProxyHost(wxBasicString(proxy.GetServer()));
         pPlay->put_HTTPProxyPort(wxAtoi(proxy.GetPort()));
         pPlay->Release();
     }
@@ -1150,9 +1150,9 @@ bool wxAMMediaBackend::DoLoad(const wxString& location)
     // the docs say its async and put_FileName is not -
     // but in practice they both seem to be async anyway
     if(GetMP())
-        hr = GetMP()->Open( wxBasicString(location).Get() );
+        hr = GetMP()->Open( wxBasicString(location) );
     else
-        hr = GetAM()->put_FileName( wxBasicString(location).Get() );
+        hr = GetAM()->put_FileName( wxBasicString(location) );
 
     if(FAILED(hr))
     {

src/msw/mediactrl_wmp10.cpp

@@ -905,21 +905,21 @@ bool wxWMP10MediaBackend::Load(const wxURI& location,
     {
         long lOldSetting;
         if( pWMPNetwork->getProxySettings(
-                    wxBasicString(location.GetScheme()).Get(), &lOldSetting
+                    wxBasicString(location.GetScheme()), &lOldSetting
                                         ) == 0 &&
 
             pWMPNetwork->setProxySettings(
-                    wxBasicString(location.GetScheme()).Get(), // protocol
+                    wxBasicString(location.GetScheme()), // protocol
                                 2) == 0) // 2 == manually specify
         {
-            BSTR bsOldName = NULL;
+            wxBasicString bsOldName;
             long lOldPort = 0;
 
             pWMPNetwork->getProxyName(
-                        wxBasicString(location.GetScheme()).Get(),
+                        wxBasicString(location.GetScheme()),
-                        &bsOldName);
+                        bsOldName.ByRef());
             pWMPNetwork->getProxyPort(
-                        wxBasicString(location.GetScheme()).Get(),
+                        wxBasicString(location.GetScheme()),
                         &lOldPort);
 
             long lPort;
@@ -936,11 +936,11 @@ bool wxWMP10MediaBackend::Load(const wxURI& location,
             }
 
             if( pWMPNetwork->setProxyName(
-                        wxBasicString(location.GetScheme()).Get(), // proto
+                        wxBasicString(location.GetScheme()), // proto
-                        wxBasicString(server).Get() ) == 0  &&
+                        wxBasicString(server) ) == 0  &&
 
                 pWMPNetwork->setProxyPort(
-                        wxBasicString(location.GetScheme()).Get(), // proto
+                        wxBasicString(location.GetScheme()), // proto
                         lPort
                                          ) == 0
               )
@@ -948,16 +948,16 @@ bool wxWMP10MediaBackend::Load(const wxURI& location,
                 bOK = DoLoad(location.BuildURI());
 
                 pWMPNetwork->setProxySettings(
-                    wxBasicString(location.GetScheme()).Get(), // protocol
+                    wxBasicString(location.GetScheme()), // protocol
                                 lOldSetting);
                 if(bsOldName)
                     pWMPNetwork->setProxyName(
-                        wxBasicString(location.GetScheme()).Get(), // protocol
+                        wxBasicString(location.GetScheme()), // protocol
                                     bsOldName);
 
                 if(lOldPort)
                     pWMPNetwork->setProxyPort(
-                        wxBasicString(location.GetScheme()).Get(), // protocol
+                        wxBasicString(location.GetScheme()), // protocol
                                 lOldPort);
 
                 pWMPNetwork->Release();
@@ -997,7 +997,7 @@ bool wxWMP10MediaBackend::DoLoad(const wxString& location)
     {
         IWMPMedia* pWMPMedia;
 
-        if( (hr = pWMPCore3->newMedia(wxBasicString(location).Get(),
+        if( (hr = pWMPCore3->newMedia(wxBasicString(location),
                                &pWMPMedia)) == 0)
         {
             // this (get_duration) will actually FAIL, but it will work.
@@ -1012,7 +1012,7 @@ bool wxWMP10MediaBackend::DoLoad(const wxString& location)
 #endif
     {
         // just load it the "normal" way
-        hr = m_pWMPPlayer->put_URL( wxBasicString(location).Get() );
+        hr = m_pWMPPlayer->put_URL( wxBasicString(location) );
     }
 
     if(FAILED(hr))
@@ -1061,12 +1061,12 @@ bool wxWMP10MediaBackend::ShowPlayerControls(wxMediaCtrlPlayerControls flags)
     if(!flags)
     {
         m_pWMPPlayer->put_enabled(VARIANT_FALSE);
-        m_pWMPPlayer->put_uiMode(wxBasicString(wxT("none")).Get());
+        m_pWMPPlayer->put_uiMode(wxBasicString(wxT("none")));
     }
     else
     {
         // TODO: use "custom"? (note that CE only supports none/full)
-        m_pWMPPlayer->put_uiMode(wxBasicString(wxT("full")).Get());
+        m_pWMPPlayer->put_uiMode(wxBasicString(wxT("full")));
         m_pWMPPlayer->put_enabled(VARIANT_TRUE);
     }
 
@@ -1358,7 +1358,7 @@ wxLongLong wxWMP10MediaBackend::GetDownloadTotal()
     if(m_pWMPPlayer->get_currentMedia(&pWMPMedia) == 0)
     {
         BSTR bsOut;
-        pWMPMedia->getItemInfo(wxBasicString(wxT("FileSize")).Get(),
+        pWMPMedia->getItemInfo(wxBasicString(wxT("FileSize")),
                                &bsOut);
 
         wxString sFileSize = wxConvertStringFromOle(bsOut);

src/msw/ole/access.cpp

@@ -938,8 +938,7 @@ STDMETHODIMP wxIAccessible::get_accDefaultAction ( VARIANT varID, BSTR* pszDefau
         }
         else
         {
-            wxBasicString basicString(defaultAction);
+            * pszDefaultAction = wxBasicString(defaultAction).Detach();
-            * pszDefaultAction = basicString.Get();
             return S_OK;
         }
     }
@@ -999,8 +998,7 @@ STDMETHODIMP wxIAccessible::get_accDescription ( VARIANT varID, BSTR* pszDescrip
         }
         else
         {
-            wxBasicString basicString(description);
+            * pszDescription = wxBasicString(description).Detach();
-            * pszDescription = basicString.Get();
             return S_OK;
         }
     }
@@ -1060,8 +1058,7 @@ STDMETHODIMP wxIAccessible::get_accHelp ( VARIANT varID, BSTR* pszHelp)
         }
         else
         {
-            wxBasicString basicString(helpString);
+            * pszHelp = wxBasicString(helpString).Detach();
-            * pszHelp = basicString.Get();
             return S_OK;
         }
     }
@@ -1171,8 +1168,7 @@ STDMETHODIMP wxIAccessible::get_accKeyboardShortcut ( VARIANT varID, BSTR* pszKe
         }
         else
         {
-            wxBasicString basicString(keyboardShortcut);
+            * pszKeyboardShortcut = wxBasicString(keyboardShortcut).Detach();
-            * pszKeyboardShortcut = basicString.Get();
             return S_OK;
         }
     }
@@ -1235,8 +1231,7 @@ STDMETHODIMP wxIAccessible::get_accName ( VARIANT varID, BSTR* pszName)
         }
         else
         {
-            wxBasicString basicString(name);
+            *pszName = wxBasicString(name).Detach();
-            *pszName = basicString.Get();
         }
         return S_OK;
     }
@@ -1416,8 +1411,7 @@ STDMETHODIMP wxIAccessible::get_accValue ( VARIANT varID, BSTR* pszValue)
         }
         else
         {
-            wxBasicString basicString(strValue);
+            * pszValue = wxBasicString(strValue).Detach();
-            * pszValue = basicString.Get();
             return S_OK;
         }
     }

src/msw/ole/oleutils.cpp

@@ -38,7 +38,7 @@
 
 WXDLLEXPORT BSTR wxConvertStringToOle(const wxString& str)
 {
-    return wxBasicString(str).Get();
+    return wxBasicString(str).Detach();
 }
 
 WXDLLEXPORT wxString wxConvertStringFromOle(BSTR bStr)
@@ -71,20 +71,19 @@ WXDLLEXPORT wxString wxConvertStringFromOle(BSTR bStr)
 // wxBasicString
 // ----------------------------------------------------------------------------
 
+wxBasicString::wxBasicString(BSTR bstr)
+{
+    m_bstrBuf = bstr;
+}
+
 wxBasicString::wxBasicString(const wxString& str)
 {
     m_bstrBuf = SysAllocString(str.wc_str(*wxConvCurrent));
 }
 
-wxBasicString::wxBasicString(const wxBasicString& src)
+wxBasicString::wxBasicString(const wxBasicString& bstr)
 {
-    m_bstrBuf = src.Get();
+    m_bstrBuf = bstr.Copy();
-}
-
-wxBasicString& wxBasicString::operator=(const wxBasicString& src)
-{
-    SysReAllocString(&m_bstrBuf, src);
-    return *this;
 }
 
 wxBasicString::~wxBasicString()
@@ -92,6 +91,59 @@ wxBasicString::~wxBasicString()
     SysFreeString(m_bstrBuf);
 }
 
+BSTR wxBasicString::Detach()
+{
+    BSTR bstr = m_bstrBuf;
+
+    m_bstrBuf = NULL;
+
+    return bstr;
+}
+
+void wxBasicString::Free()
+{
+    SysFreeString(m_bstrBuf);
+    m_bstrBuf = NULL;
+}
+
+BSTR* wxBasicString::ByRef()
+{
+    wxASSERT_MSG(!m_bstrBuf,
+        wxS("Can't get direct access to initialized BSTR"));
+    return &m_bstrBuf;
+}
+
+wxBasicString& wxBasicString::operator=(const wxBasicString& src)
+{
+    if ( this != &src )
+    {
+        wxCHECK_MSG(m_bstrBuf == NULL || m_bstrBuf != src.m_bstrBuf, 
+            *this, wxS("Attempting to assign already owned BSTR"));
+        SysFreeString(m_bstrBuf);
+        m_bstrBuf = src.Copy();
+    }
+
+    return *this;
+}
+
+wxBasicString& wxBasicString::operator=(const wxString& str)
+{
+    SysFreeString(m_bstrBuf);
+    m_bstrBuf = SysAllocString(str.wc_str(*wxConvCurrent));
+
+    return *this;
+}
+
+wxBasicString& wxBasicString::operator=(BSTR bstr)
+{
+    wxCHECK_MSG(m_bstrBuf == NULL || m_bstrBuf != bstr, 
+        *this, wxS("Attempting to assign already owned BSTR"));
+
+    SysFreeString(m_bstrBuf);
+    m_bstrBuf = bstr;
+
+    return *this;
+}
 
 // ----------------------------------------------------------------------------
 // Convert variants

src/msw/webview_ie.cpp

@@ -231,9 +231,9 @@ wxString wxWebViewIE::GetPageSource() const
             hr = bodyTag->get_parentElement(&htmlTag);
             if(SUCCEEDED(hr))
             {
-                BSTR bstr;
+                wxBasicString bstr;
-                if ( htmlTag->get_outerHTML(&bstr) == S_OK )
+                if ( htmlTag->get_outerHTML(bstr.ByRef()) == S_OK )
-                    source = wxString(bstr);
+                    source = bstr;
             }
         }
         return source;
@@ -576,8 +576,8 @@ wxString wxWebViewIE::GetCurrentTitle() const
     wxString s;
     if(document)
     {
-        BSTR title;
+        wxBasicString title;
-        if ( document->get_nameProp(&title) == S_OK )
+        if ( document->get_nameProp(title.ByRef()) == S_OK )
             s = title;
     }
 
@@ -705,8 +705,8 @@ bool wxWebViewIE::IsEditable() const
 
     if(document)
     {
-        BSTR mode;
+        wxBasicString mode;
-        if ( document->get_designMode(&mode) == S_OK )
+        if ( document->get_designMode(mode.ByRef()) == S_OK )
         {
             if ( wxString(mode) == "On" )
                 return true;
@@ -731,9 +731,9 @@ bool wxWebViewIE::HasSelection() const
         HRESULT hr = document->get_selection(&selection);
         if(SUCCEEDED(hr))
         {
-            BSTR type;
+            wxBasicString type;
-            if ( selection->get_type(&type) == S_OK )
+            if ( selection->get_type(type.ByRef()) == S_OK )
-                sel = wxString(type);
+                sel = type;
         }
         return sel != "None";
     }
@@ -767,9 +767,9 @@ wxString wxWebViewIE::GetSelectedText() const
                 hr = disrange->QueryInterface(IID_IHTMLTxtRange, (void**)&range);
                 if(SUCCEEDED(hr))
                 {
-                    BSTR text;
+                    wxBasicString text;
-                    if ( range->get_text(&text) == S_OK )
+                    if ( range->get_text(text.ByRef()) == S_OK )
-                        selected = wxString(text);
+                        selected = text;
                 }
             }
         }
@@ -800,9 +800,9 @@ wxString wxWebViewIE::GetSelectedSource() const
                 hr = disrange->QueryInterface(IID_IHTMLTxtRange, (void**)&range);
                 if(SUCCEEDED(hr))
                 {
-                    BSTR text;
+                    wxBasicString text;
-                    if ( range->get_htmlText(&text) == S_OK )
+                    if ( range->get_htmlText(text.ByRef()) == S_OK )
-                        selected = wxString(text);
+                        selected = text;
                 }
             }
         }
@@ -841,9 +841,9 @@ wxString wxWebViewIE::GetPageText() const
         HRESULT hr = document->get_body(&body);
         if(SUCCEEDED(hr))
         {
-            BSTR out;
+            wxBasicString out;
-            if ( body->get_innerText(&out) == S_OK )
+            if ( body->get_innerText(out.ByRef()) == S_OK )
-                text = wxString(out);
+                text = out;
         }
         return text;
     }
@@ -949,8 +949,7 @@ wxCOMPtr<IHTMLDocument2> wxWebViewIE::GetDocument() const
 
 bool wxWebViewIE::IsElementVisible(wxCOMPtr<IHTMLElement> elm)
 {
-    wxCOMPtr<IHTMLElement> elm1 = elm;
+    wxCOMPtr<IHTMLElement> elm1 = elm;   
-    BSTR tmp_bstr;
     bool is_visible = true;
     //This method is not perfect but it does discover most of the hidden elements.
     //so if a better solution is found, then please do improve.
@@ -962,15 +961,18 @@ bool wxWebViewIE::IsElementVisible(wxCOMPtr<IHTMLElement> elm)
             wxCOMPtr<wxIHTMLCurrentStyle> style;
             if(SUCCEEDED(elm2->get_currentStyle(&style)))
             {
+                wxBasicString display_bstr;
+                wxBasicString visibility_bstr;
+
                 //Check if the object has the style display:none.
-                if((style->get_display(&tmp_bstr) != S_OK) || 
+                if((style->get_display(display_bstr.ByRef()) != S_OK) || 
-                   (tmp_bstr != NULL && (wxCRT_StricmpW(tmp_bstr, L"none") == 0)))
+                    wxString(display_bstr).IsSameAs(wxS("none"), false))
                 {
                     is_visible = false;
                 }
                 //Check if the object has the style visibility:hidden.
-                if((is_visible && (style->get_visibility(&tmp_bstr) != S_OK)) ||
+                if((is_visible && (style->get_visibility(visibility_bstr.ByRef()) != S_OK)) ||
-                  (tmp_bstr != NULL && wxCRT_StricmpW(tmp_bstr, L"hidden") == 0))
+                    wxString(visibility_bstr).IsSameAs(wxS("hidden"), false))
                 {
                     is_visible = false;
                 }
@@ -1007,8 +1009,9 @@ void wxWebViewIE::FindInternal(const wxString& text, int flags, int internal_fla
         if(SUCCEEDED(document->QueryInterface(wxIID_IMarkupContainer, (void **)&pIMC)))
         {
             wxCOMPtr<wxIMarkupPointer> ptrBegin, ptrEnd;
-            BSTR attr_bstr = SysAllocString(L"style=\"background-color:#ffff00\"");
+            wxBasicString attr_bstr(wxString("style=\"background-color:#ffff00\""));
-            BSTR text_bstr = SysAllocString(text.wc_str());
+            wxBasicString text_bstr(text.wc_str());
+            
             pIMS->CreateMarkupPointer(&ptrBegin);
             pIMS->CreateMarkupPointer(&ptrEnd);
 
@@ -1064,9 +1067,6 @@ void wxWebViewIE::FindInternal(const wxString& text, int flags, int internal_fla
                 }
                 ptrBegin->MoveToPointer(ptrEnd);
             }
-            //Clean up.
-            SysFreeString(text_bstr);
-            SysFreeString(attr_bstr);
         }
     }
 }