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)

[sergei, 2015/11/30 15:52:08]

We don't need it because ATL::CComPtr default constructor initializes internal
pointer to nullptr.

[Eric, 2015/11/30 16:31:51]

I'd rather have it explicit as a lightweight form of documentation. The lifetime
of that object is inherently problematic if you're not deeply familiar with how
IE uses SetSite.

[sergei, 2015/12/07 10:49:08]

ATL::CComPtr initializes it to nullptr in the constructor, what does this
initialization document here in addition and how is it related to the lifetime
of some object? I'm also not sure which object is referred to above.

[Eric, 2015/12/07 14:04:12]

The very-lightweight documentation is that every value that 'm_webBrowser2'
takes appears explicitly in the code, including the initial value of zero.
It's related to the life cycle (not really the life span) of 'CPluginClass'.

FYI, this is how I think about this issue. Knowing what I know now, if I were
implementing this from scratch, I would move much of what's currently in
'CPluginClass' to a new class that would be created/destroyed in 'SetSite()'.
(I'm not planning on introducing such a change.) The way I see it, there is,
effectively, a second class with a slightly narrower lifetime whose constructor
has one argument of type 'IWebBrowser2'. But there's a disconnect between (1) a
C++ constructor with one argument and (2) the 'SetSite' implementation pattern.
These could be bridged explicitly, but doing so at this point would be quite an
extensive change.

[sergei, 2015/12/08 09:41:59]

I think it will go away in a future and I don't understand the reason to
introduce it to get rid of it later.

[sergei, 2015/12/08 09:41:59]

It sounds not convincing. Why are we not doing it everywhere?
Sounds reasonable but it's not related to initialization of class members with
the same values which they get from the default constructor.

 {
   //Use this line to debug memory leaks
   //	_CrtDumpMemoryLeaks();
@@ -115,56 +115,27 @@
   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;

[sergei, 2015/11/30 15:52:09]

it would be good to initialize it to NULL.

[Eric, 2015/11/30 16:31:51]

Welcome to MicrosoftLand. SHANDLE_PTR is not a pointer type; it's 'long'.

We're not testing its value. Since we're not returning 'nullptr' on error (see
below), we're only returning its value if 'get_HWND()' succeeds, and in that
case we can rely upon its value.

I'd say we don't need to bother to initialize it, particularly since
initializing it with 'nullptr', what looks on the surface like it would be the
obvious choice, doesn't compile.

[Oleksandr, 2015/12/03 11:51:58]

We don't check its value now, but we might in future. So I don't see why we
would remove the initialization here.

[Eric, 2015/12/03 14:24:54]

Initialized it to zero, which matches its type.

As a rule, though, if we're going to do something in the future, we can do its
prerequisites in the future also.

[sergei, 2015/12/07 10:49:08]

Agree, except following some good practices, like here.

Beside the fact that it's always good to initialize variables, I would like to
say it's a good practice to always initialize the value which is returned
through a pointer, like here or like e.g. void** in QueryInterface, because it
allows to find a bug during the development cycle. Such pointers are often held
by some smart-pointer wrappers, so the callee can in addition verify that the
value stored at the pointer value is not a valid value to avoid resource leakage
by overriding it.
Although one can argue that it's not our case because we are calling Microsoft's
method (which in particular does not fail or trigger any assert depending on the
argument value) and we are not going to implement tests for our plugin, I still
think that it's worth doing because at least it simplifies the readability
because one don't have to think and look into whether it's our implementation or
can somehow fail later and of course it does not cause any questions during the
codereview.

[sergei, 2015/12/07 10:49:08]

Although zero is OK, NULL looks better here.

[Eric, 2015/12/07 14:04:12]

I disagree. Sometimes NULL is defined as '(void*)0'. I don't want to incur any
cognitive overhead thinking about what it ought to be. It's Microsoft's
interface defect; we have to live with it.

[Eric, 2015/12/07 14:04:12]

Almost always.  But even saying that is saying too much here, because there's no
remaining issue of initialization.

+  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")

[sergei, 2015/11/30 15:52:08]

Actually, I think we should return nullptr in case of error, shouldn't we?

[Eric, 2015/11/30 16:31:51]

Done.

That's a good idea.

+  }
   return (HWND)hBrowserWndHandle;
 }
 
-
-CComQIPtr<IWebBrowser2> CPluginClass::GetBrowser() const
+bool CPluginClass::IsRootBrowser(IWebBrowser2* otherBrowser)
 {
-  CComQIPtr<IWebBrowser2> browser;
-
-  s_criticalSectionBrowser.Lock();
-  {
-    browser = m_webBrowser2;
-  }
-  s_criticalSectionBrowser.Unlock();
-
-  return browser;
+  return m_webBrowser2.IsEqualObject(otherBrowser);
 }
 
-
 CComQIPtr<IWebBrowser2> CPluginClass::GetAsyncBrowser()
 {
   CComQIPtr<IWebBrowser2> browser;
@@ -181,11 +152,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 +163,7 @@
   }
   else
   {
+    DEBUG_ERROR_LOG(0, 0, 0, "CPluginClass::GetBrowserUrl - Reached with m_webBrowser2 == nullptr");
     url = m_tab->GetDocumentUrl();
   }
   return url;
@@ -235,16 +206,18 @@
         DEBUG_ERROR_LOG(hr, PLUGIN_ERROR_SET_SITE, PLUGIN_ERROR_SET_SITE_COINIT, "Class::SetSite - CoInitialize");
       }
 
-      s_criticalSectionBrowser.Lock();
+      /*
+       * We were instantiated as a BHO, so our site is always of type IWebBrowser2.
+       */
+      m_webBrowser2 = ATL::CComQIPtr<IWebBrowser2>(unknownSite);
+      if (!m_webBrowser2)
       {
-        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
       //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
@@ -256,30 +229,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 +298,16 @@
       }
       s_criticalSectionLocal.Unlock();
 
-      // Release browser interface
-      s_criticalSectionBrowser.Lock();
-      {
-        m_webBrowser2.Release();

[sergei, 2015/11/30 15:52:08]

It's still good to call `m_webBrowser2.Release();` here at this point.

[Eric, 2015/11/30 16:31:51]

Basically right. The correct thing to do is to nullify the smart pointer. That
will trigger a call to 'Release()' on the interface in the destructor of
'm_webBrowser2'. No need to break abstraction here.

For the record, I've verified that IE always calls the destructor for
'CPluginClass' immediately after calling 'SetSite(0)' on it (except sometimes on
program termination). If we did not nullify the pointer here, it would be
destroyed anyway shortly after in the destructor for 'CPluginClass'.
Nevertheless, it's good to make everything about this variable as explicit as
possible.

-      }
-      s_criticalSectionBrowser.Unlock();
-
       DEBUG_GENERAL("================================================================================\nNEW TAB UI - END\n================================================================================")
 
       ::CoUninitialize();
     }
 
-    IObjectWithSiteImpl<CPluginClass>::SetSite(unknownSite);
   }
   catch (...)
   {
   }
+  IObjectWithSiteImpl<CPluginClass>::SetSite(unknownSite);
   return S_OK;
 }
 
@@ -492,7 +454,7 @@
     if (url.find(L"javascript") == 0)
     {
     }
-    else if (GetBrowser().IsEqualObject(webBrowser))
+    else if (IsRootBrowser(webBrowser))
     {
       m_tab->OnNavigate(url);
       DEBUG_GENERAL(
@@ -521,12 +483,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 +509,7 @@
     }
     std::wstring frameSrc = GetLocationUrl(*webBrowser2);
     UnescapeUrl(frameSrc);
-    bool isRootPageBrowser = GetBrowser().IsEqualObject(webBrowser2);
-    m_tab->OnDocumentComplete(webBrowser2, frameSrc, isRootPageBrowser);
+    m_tab->OnDocumentComplete(webBrowser2, frameSrc, IsRootBrowser(webBrowser2));
   }
   catch (...)
   {
@@ -626,7 +588,7 @@
 
 bool CPluginClass::InitObject()
 {
-  DEBUG_GENERAL("InitObject");
+  DEBUG_GENERAL("InitObject - begin");
   CPluginSettings* settings = CPluginSettings::GetInstance();
 
   if (!settings->GetPluginEnabled())
@@ -739,6 +701,8 @@
     CPluginClient::GetInstance()->AddSubscription(aaUrl);
   }
   s_criticalSectionLocal.Unlock();
+
+  DEBUG_GENERAL("InitObject - end");
   return true;
 }
 
@@ -1628,11 +1592,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 +1641,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;
-}