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

src/plugin/PluginClass.cpp

Index: src/plugin/PluginClass.cpp
===================================================================
--- a/src/plugin/PluginClass.cpp
+++ b/src/plugin/PluginClass.cpp
@@ -60,7 +60,6 @@
 std::map<DWORD, CPluginClass*> CPluginClass::s_threadInstances;
 
 CComAutoCriticalSection CPluginClass::s_criticalSectionLocal;
-CComAutoCriticalSection CPluginClass::s_criticalSectionBrowser;
 CComAutoCriticalSection CPluginClass::s_criticalSectionWindow;
 
 CComQIPtr<IWebBrowser2> CPluginClass::s_asyncWebBrowser2;
@@ -91,6 +90,7 @@
 }
 
 CPluginClass::CPluginClass()
+  : m_webBrowser2(nullptr)
 {
   //Use this line to debug memory leaks
   //	_CrtDumpMemoryLeaks();
@@ -115,56 +115,32 @@
   delete m_tab;
 }
 
-
-/////////////////////////////////////////////////////////////////////////////
-// Initialization
-
-HRESULT CPluginClass::FinalConstruct()
-{
-  return S_OK;
-}
-
-void CPluginClass::FinalRelease()
-{
-  s_criticalSectionBrowser.Lock();
-  {
-    m_webBrowser2.Release();
-  }
-  s_criticalSectionBrowser.Unlock();
-}
-
 HWND CPluginClass::GetBrowserHWND() const
 {
-  SHANDLE_PTR hBrowserWndHandle = NULL;
-
-  CComQIPtr<IWebBrowser2> browser = GetBrowser();
-  if (browser)
+  if (!m_webBrowser2)
   {
-    HRESULT hr = browser->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(0, 0, 0, "CPluginClass::GetBrowserHWND - Reached with m_webBrowser2 == nullptr");
+    return nullptr;
   }
-
+  SHANDLE_PTR hBrowserWndHandle;
+  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")
+  }
   return (HWND)hBrowserWndHandle;
 }
 
-
-CComQIPtr<IWebBrowser2> CPluginClass::GetBrowser() const
+bool CPluginClass::IsRootPageBrowser(IWebBrowser2* otherBrowser)
 {
-  CComQIPtr<IWebBrowser2> browser;
-
-  s_criticalSectionBrowser.Lock();
-  {
-    browser = m_webBrowser2;
-  }
-  s_criticalSectionBrowser.Unlock();
-
-  return browser;
+  /*
+   * 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);
 }
 
-
 CComQIPtr<IWebBrowser2> CPluginClass::GetAsyncBrowser()
 {
   CComQIPtr<IWebBrowser2> browser;
@@ -181,11 +157,10 @@
 std::wstring CPluginClass::GetBrowserUrl() const
 {
   std::wstring url;
-  CComQIPtr<IWebBrowser2> browser = GetBrowser();
-  if (browser)
+  if (m_webBrowser2)
   {
     CComBSTR bstrURL;
-    if (SUCCEEDED(browser->get_LocationURL(&bstrURL)) && bstrURL)
+    if (SUCCEEDED(m_webBrowser2->get_LocationURL(&bstrURL)) && bstrURL)
     {
       url = std::wstring(bstrURL, SysStringLen(bstrURL));
       UnescapeUrl(url);
@@ -193,6 +168,7 @@
   }
   else
   {
+    DEBUG_ERROR_LOG(0, 0, 0, "CPluginClass::GetBrowserUrl - Reached with m_webBrowser2 == nullptr");
     url = m_tab->GetDocumentUrl();
   }
   return url;
@@ -235,11 +211,18 @@
         DEBUG_ERROR_LOG(hr, PLUGIN_ERROR_SET_SITE, PLUGIN_ERROR_SET_SITE_COINIT, "Class::SetSite - CoInitialize");
       }
 
-      s_criticalSectionBrowser.Lock();
+      /*
+       * 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.
+       */

[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 = unknownSite;
+        throw std::logic_error("CPluginClass::SetSite - Unable to convert site pointer to IWebBrowser2*");
       }
-      s_criticalSectionBrowser.Unlock();
 
       //register the mimefilter
       //and only mimefilter
@@ -256,30 +239,26 @@
 
       try
       {
-        auto webBrowser = GetBrowser();
-        if (webBrowser)
+        DEBUG_GENERAL("Loaded as BHO");
+        HRESULT hr = DispEventAdvise(m_webBrowser2);
+        if (SUCCEEDED(hr))
         {
-          DEBUG_GENERAL("Loaded as BHO");
-          HRESULT hr = DispEventAdvise(webBrowser);
-          if (SUCCEEDED(hr))
+          m_isAdvised = true;
+          try
           {
-            m_isAdvised = true;
-            try
-            {
-              std::thread startInitObjectThread(StartInitObject, this);
-              startInitObjectThread.detach(); // TODO: but actually we should wait for the thread in the dtr.
-            }
-            catch (const std::system_error& ex)
-            {
-              DEBUG_SYSTEM_EXCEPTION(ex, PLUGIN_ERROR_THREAD, PLUGIN_ERROR_MAIN_THREAD_CREATE_PROCESS, 
-                "Class::Thread - Failed to create StartInitObject thread");
-            }
+            std::thread startInitObjectThread(StartInitObject, this);
+            startInitObjectThread.detach(); // TODO: but actually we should wait for the thread in the dtr.
           }
-          else
+          catch (const std::system_error& ex)
           {
-            DEBUG_ERROR_LOG(hr, PLUGIN_ERROR_SET_SITE, PLUGIN_ERROR_SET_SITE_ADVICE, "Class::SetSite - Advise");
+            DEBUG_SYSTEM_EXCEPTION(ex, PLUGIN_ERROR_THREAD, PLUGIN_ERROR_MAIN_THREAD_CREATE_PROCESS, 
+              "Class::Thread - Failed to create StartInitObject thread");
           }
         }
+        else
+        {
+          DEBUG_ERROR_LOG(hr, PLUGIN_ERROR_SET_SITE, PLUGIN_ERROR_SET_SITE_ADVICE, "Class::SetSite - Advise");
+        }
       }
       catch (const std::runtime_error& ex)
       {
@@ -329,23 +308,16 @@
       }
       s_criticalSectionLocal.Unlock();
 
-      // Release browser interface
-      s_criticalSectionBrowser.Lock();
-      {
-        m_webBrowser2.Release();

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

Why is this line removed?

-      }
-      s_criticalSectionBrowser.Unlock();
-
       DEBUG_GENERAL("================================================================================\nNEW TAB UI - END\n================================================================================")
 
       ::CoUninitialize();
     }
 
-    IObjectWithSiteImpl<CPluginClass>::SetSite(unknownSite);
   }
   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;
 }
 
@@ -492,7 +464,7 @@
     if (url.find(L"javascript") == 0)
     {
     }
-    else if (GetBrowser().IsEqualObject(webBrowser))
+    else if (IsRootPageBrowser(webBrowser))
     {
       m_tab->OnNavigate(url);
       DEBUG_GENERAL(
@@ -521,12 +493,13 @@
 {
   try
   {
+    if (!m_webBrowser2)
+    {
+      DEBUG_ERROR_LOG(0, 0, 0, "CPluginClass::OnDownloadComplete - Reached with m_webBrowser2 == nullptr");
+      return;
+    }
     DEBUG_NAVI(L"Navi::Download Complete")
-    ATL::CComPtr<IWebBrowser2> browser = GetBrowser();
-    if (browser)
-    {
-      m_tab->OnDownloadComplete(browser);
-    }
+    m_tab->OnDownloadComplete(m_webBrowser2);
   }
   catch (...)
   {
@@ -546,8 +519,7 @@
     }
     std::wstring frameSrc = GetLocationUrl(*webBrowser2);
     UnescapeUrl(frameSrc);
-    bool isRootPageBrowser = GetBrowser().IsEqualObject(webBrowser2);
-    m_tab->OnDocumentComplete(webBrowser2, frameSrc, isRootPageBrowser);
+    m_tab->OnDocumentComplete(webBrowser2, frameSrc, IsRootPageBrowser(webBrowser2));
   }
   catch (...)
   {
@@ -626,7 +598,7 @@
 
 bool CPluginClass::InitObject()
 {
-  DEBUG_GENERAL("InitObject");
+  DEBUG_GENERAL("InitObject - begin");
   CPluginSettings* settings = CPluginSettings::GetInstance();
 
   if (!settings->GetPluginEnabled())
@@ -739,6 +711,8 @@
     CPluginClient::GetInstance()->AddSubscription(aaUrl);
   }
   s_criticalSectionLocal.Unlock();
+
+  DEBUG_GENERAL("InitObject - end");
   return true;
 }
 
@@ -1628,11 +1602,16 @@
 
 void CPluginClass::Unadvise()
 {
+  if (!m_webBrowser2)
+  {
+    DEBUG_ERROR_LOG(0, 0, 0, "CPluginClass::Unadvise - Reached with m_webBrowser2 == nullptr");
+    return;
+  }
   s_criticalSectionLocal.Lock();
   {
     if (m_isAdvised)
     {
-      HRESULT hr = DispEventUnadvise(GetBrowser());
+      HRESULT hr = DispEventUnadvise(m_webBrowser2);
       if (FAILED(hr))
       {
         DEBUG_ERROR_LOG(hr, PLUGIN_ERROR_SET_SITE, PLUGIN_ERROR_SET_SITE_UNADVISE, "Class::Unadvise - Unadvise");
@@ -1672,70 +1651,3 @@
   return s_atomPaneClass;
 }
 
-HWND CPluginClass::GetTabHWND() const
-{
-  std::array<wchar_t, MAX_PATH> className;
-  // Get browser window and url
-  HWND hBrowserWnd = GetBrowserHWND();
-  if (!hBrowserWnd)
-  {
-    DEBUG_ERROR_LOG(0, PLUGIN_ERROR_UI, PLUGIN_ERROR_UI_NO_STATUSBAR_BROWSER, "Class::GetTabWindow - No tab window")
-      s_criticalSectionWindow.Unlock();
-
-    return false;
-  }
-
-  // Looking for a TabWindowClass window in IE7
-
-  HWND hTabWnd = ::GetWindow(hBrowserWnd, GW_CHILD);
-  while (hTabWnd)
-  {
-    className[0] = L'\0';
-    int classNameLength = GetClassNameW(hTabWnd, className.data(), className.size());
-
-    if (classNameLength && (wcscmp(className.data(), L"TabWindowClass") == 0 || wcscmp(className.data(), L"Frame Tab") == 0))
-    {
-      // IE8 support
-      HWND hTabWnd2 = hTabWnd;
-      if (wcscmp(className.data(), L"Frame Tab") == 0)
-      {
-        hTabWnd2 = ::FindWindowEx(hTabWnd2, NULL, L"TabWindowClass", NULL);
-      }
-
-      if (hTabWnd2)
-      {
-        DWORD nProcessId;
-        ::GetWindowThreadProcessId(hTabWnd2, &nProcessId);
-        if (::GetCurrentProcessId() == nProcessId)
-        {
-          bool bExistingTab = false;
-          s_criticalSectionLocal.Lock();
-
-          {
-            for (auto instance : s_instances)
-            {
-              if (instance->m_hTabWnd == hTabWnd2)
-              {
-                bExistingTab = true;
-                break;
-              }
-            }
-          }
-
-          if (!bExistingTab)
-          {
-            hBrowserWnd = hTabWnd2;
-            hTabWnd = hTabWnd2;
-            s_criticalSectionLocal.Unlock();
-            break;
-          }
-          s_criticalSectionLocal.Unlock();
-
-        }
-      }
-    }
-
-    hTabWnd = ::GetWindow(hTabWnd, GW_HWNDNEXT);
-  }
-  return hTabWnd;
-}