Issue #3383 - Rewrite and simplify browser-site handling in CPluginClass

src/plugin/PluginClass.cpp

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2015 Eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 104 matching lines @@
   delete m_tab;
 }
 
 HWND CPluginClass::GetBrowserHWND() const
 {
   if (!m_webBrowser2)
   {
     DEBUG_ERROR_LOG(0, 0, 0, "CPluginClass::GetBrowserHWND - Reached with m_webBrowser2 == nullptr");
     return nullptr;
   }
-  SHANDLE_PTR hBrowserWndHandle;
+  SHANDLE_PTR hBrowserWndHandle = 0;
   HRESULT hr = m_webBrowser2->get_HWND(&hBrowserWndHandle);
   if (FAILED(hr))
   {
-    DEBUG_ERROR_LOG(hr, PLUGIN_ERROR_UI, PLUGIN_ERROR_UI_GET_BROWSER_WINDOW, "Class::GetBrowserHWND - failed")
+    DEBUG_ERROR_LOG(hr, PLUGIN_ERROR_UI, PLUGIN_ERROR_UI_GET_BROWSER_WINDOW, "Class::GetBrowserHWND - failed");
+    return nullptr;
   }
   return (HWND)hBrowserWndHandle;
 }
 
-bool CPluginClass::IsRootPageBrowser(IWebBrowser2* otherBrowser)
-{
-  /*
-   * The ownership overhead of 'CComPtr', which we don't need, is what we
-   *   pay for the convenience of using its member function 'IsEqualObject'.
-   */
-  CComPtr<IWebBrowser> thisBrowser = m_webBrowser2;

[sergei, 2015/10/01 16:15:51]

NIT: why is `IWebBrowser` and not `IWebBrowser2`?

[Eric, 2015/11/18 13:57:31]

Typo. Fixed.

-  return thisBrowser.IsEqualObject(otherBrowser);
+bool CPluginClass::IsRootBrowser(IWebBrowser2* otherBrowser)
+{
+  return m_webBrowser2.IsEqualObject(otherBrowser);
 }
 
 CComQIPtr<IWebBrowser2> CPluginClass::GetAsyncBrowser()
 {
   CComQIPtr<IWebBrowser2> browser;
 
   s_criticalSectionLocal.Lock();
   {
     browser = s_asyncWebBrowser2;
   }
   s_criticalSectionLocal.Unlock();
 
   return browser;
 }
 
 std::wstring CPluginClass::GetBrowserUrl() const
 {
   std::wstring url;
   if (m_webBrowser2)
   {
     CComBSTR bstrURL;
     if (SUCCEEDED(m_webBrowser2->get_LocationURL(&bstrURL)) && bstrURL)
     {
       url = std::wstring(bstrURL, SysStringLen(bstrURL));
-      UnescapeUrl(url);
     }
   }
   else
   {
     DEBUG_ERROR_LOG(0, 0, 0, "CPluginClass::GetBrowserUrl - Reached with m_webBrowser2 == nullptr");
     url = m_tab->GetDocumentUrl();
   }
   return url;
 }
 
@@ skipping 28 matching lines @@
 
       DEBUG_GENERAL(L"================================================================================\nNEW TAB UI\n================================================================================")
 
       HRESULT hr = ::CoInitialize(NULL);
       if (FAILED(hr))
       {
         DEBUG_ERROR_LOG(hr, PLUGIN_ERROR_SET_SITE, PLUGIN_ERROR_SET_SITE_COINIT, "Class::SetSite - CoInitialize");
       }
 
       /*
-       * We have a responsibility to call 'AdRef' on 'unknownSite' here.
-       * We discharge that responsibility by calling our base class function 
-       *   'SetSite' at the end of this function.
-       * Since the base class is taking care of that, 'm_webBrowser2' can be
-       *   defined as a plain pointer.
+       * We were instantiated as a BHO, so our site is always of type IWebBrowser2.
        */

[sergei, 2015/10/01 16:15:51]

I'm sorry, but this comment seems just wrong.

[Oleksandr, 2015/10/05 10:44:47]

Whatever the base class SetSite does it doesn't know that we have called
QueryInterface and stored a pointer here. So we still need to AddRef here. Which
is why I agree that we should still use CComPtr for m_webBrowser2.

[sergei, 2015/10/05 11:00:56]

The base class calls `AddRef` and corresponding `Release` and does not know that
we have called `QueryInterface` here, right, but `QueryInterface` also calls
`AddRef`, so we need to have corresponding call `Release`. We still can call
`Release` manually (as it was before) but smart pointer ensures that it will be
called when the class is being destroyed, so it is the first reason we need to
use smart pointer. The second one is that in particular the implementation of
`ATL::CComPtr` has a couple of good caveats useful during in debug build
(dereference operators and `CComPtr::operator&`) and in runtime
(`CComPtr::constructor` and `CComPtr::Release`).

[Eric, 2015/11/18 13:57:30]

I've done this in the new patch set.

[Eric, 2015/11/18 13:57:31]

The base class SetSite() assigns its argument 'unknownSite' to an internal
member of type 'CComPtr<IUnknown>'. So if there's any lingering concern that
we're not properly managing lifetime, the ATL base class implementation is
taking care of that silently.

I've rewritten the comment anyway, because that issue isn't particularly
relevant.

-      hr = unknownSite->QueryInterface(IID_IWebBrowser2, (void**)(&m_webBrowser2));

[sergei, 2015/10/01 16:15:51]

I cannot find the call of `Release` on `m_webBrowser2`.

[Eric, 2015/11/18 13:57:30]

Done.

-      if (FAILED(hr))
+      m_webBrowser2 = ATL::CComQIPtr<IWebBrowser2>(unknownSite);
+      if (!m_webBrowser2)
       {
         throw std::logic_error("CPluginClass::SetSite - Unable to convert site pointer to IWebBrowser2*");
       }
 
       //register the mimefilter
       //and only mimefilter
       //on some few computers the mimefilter does not get properly registered when it is done on another thread
-
       s_criticalSectionLocal.Lock();
       {
         // Always register on startup, then check if we need to unregister in a separate thread
         s_mimeFilter = CPluginClientFactory::GetMimeFilterClientInstance();
         s_asyncWebBrowser2 = unknownSite;
         s_instances.insert(this);
       }
       s_criticalSectionLocal.Unlock();
 
       try
@@ skipping 59 matching lines @@
         {
           s_threadInstances.erase(it);
         }
         if (s_instances.empty())
         {
           // TODO: Explicitly releasing a resource when a container becomes empty looks like a job better suited for shared_ptr
           CPluginClientFactory::ReleaseMimeFilterClientInstance();
         }
       }
       s_criticalSectionLocal.Unlock();
+
+      m_webBrowser2 = nullptr;
 
       DEBUG_GENERAL("================================================================================\nNEW TAB UI - END\n================================================================================")
 
       ::CoUninitialize();
     }
 
   }
   catch (...)
   {
   }
   IObjectWithSiteImpl<CPluginClass>::SetSite(unknownSite);

[Eric, 2015/08/13 16:58:41]

Moving this statement outside the try-block ensures it's always called, which
matters more now since the new code here contains an explicit exception throw.

   return S_OK;
 }
 
 bool CPluginClass::IsStatusBarEnabled()
 {
   DEBUG_GENERAL("IsStatusBarEnabled start");
   HKEY pHkey;
   HKEY pHkeySub;
   RegOpenCurrentUser(KEY_QUERY_VALUE, &pHkey);
   DWORD truth = 1;
@@ skipping 118 matching lines @@
     ATL::CComQIPtr<IWebBrowser2> webBrowser = frameBrowserDisp;
     if (!webBrowser)
     {
       return;
     }
     if (!urlVariant || urlVariant->vt != VT_BSTR)
     {
       return;
     }
     std::wstring url(urlVariant->bstrVal, SysStringLen(urlVariant->bstrVal));
-    UnescapeUrl(url);
 
     // If webbrowser2 is equal to top level browser (as set in SetSite), we are
     // navigating new page
     CPluginClient* client = CPluginClient::GetInstance();
     if (url.find(L"javascript") == 0)
     {
     }
-    else if (IsRootPageBrowser(webBrowser))
+    else if (IsRootBrowser(webBrowser))
     {
       m_tab->OnNavigate(url);
       DEBUG_GENERAL(
       L"================================================================================\n"
       L"Begin main navigation url:" + url + L"\n"
       L"================================================================================")
 
 #ifdef ENABLE_DEBUG_RESULT
       CPluginDebug::DebugResultDomain(url);
 #endif
@@ skipping 33 matching lines @@
 {
   try
   {
     DEBUG_NAVI(L"Navi::Document Complete");
     ATL::CComQIPtr<IWebBrowser2> webBrowser2 = frameBrowserDisp;
     if (!webBrowser2)
     {
       return;
     }
     std::wstring frameSrc = GetLocationUrl(*webBrowser2);
-    UnescapeUrl(frameSrc);
-    m_tab->OnDocumentComplete(webBrowser2, frameSrc, IsRootPageBrowser(webBrowser2));
+    m_tab->OnDocumentComplete(webBrowser2, frameSrc, IsRootBrowser(webBrowser2));
   }
   catch (...)
   {
   }
 }
 
 // Entry point
 void STDMETHODCALLTYPE CPluginClass::OnWindowStateChanged(unsigned long flags, unsigned long validFlagsMask)
 {
   try
@@ skipping 559 matching lines @@
           if (FAILED(hr))
           {
             DEBUG_ERROR_LOG(hr, PLUGIN_ERROR_NAVIGATION, PLUGIN_ERROR_NAVIGATION_SETTINGS, "Navigation::Failed")
           }
         }
       }
       break;
     }
   case ID_MENU_DISABLE_ON_SITE:
     {
-      CPluginSettings* settings = CPluginSettings::GetInstance();
       std::wstring urlString = GetTab()->GetDocumentUrl();
       std::string filterText = client->GetWhitelistingFilter(urlString);
       if (!filterText.empty())
       {
         client->RemoveFilter(filterText);
       }
       else
       {
-        settings->AddWhiteListedDomain(ToCString(client->GetHostFromUrl(urlString)));
+        CPluginSettings::GetInstance()->AddWhiteListedDomain(client->GetHostFromUrl(urlString));
       }
     }
   default:
     break;
   }
 
   // Invalidate and redraw the control
   UpdateStatusBar();
 }
 
@@ skipping 522 matching lines @@
   s_criticalSectionLocal.Unlock();
 
   return icon;
 }
 
 ATOM CPluginClass::GetAtomPaneClass()
 {
   return s_atomPaneClass;
 }