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 42 matching lines @@
 CLOSETHEMEDATA pfnClose = NULL;
 DRAWTHEMEBACKGROUND pfnDrawThemeBackground = NULL;
 OPENTHEMEDATA pfnOpenThemeData = NULL;
 
 ATOM CPluginClass::s_atomPaneClass = NULL;
 HINSTANCE CPluginClass::s_hUxtheme = NULL;
 std::set<CPluginClass*> CPluginClass::s_instances;
 std::map<DWORD, CPluginClass*> CPluginClass::s_threadInstances;
 
 CComAutoCriticalSection CPluginClass::s_criticalSectionLocal;
-CComAutoCriticalSection CPluginClass::s_criticalSectionBrowser;
 CComAutoCriticalSection CPluginClass::s_criticalSectionWindow;
 
 CComQIPtr<IWebBrowser2> CPluginClass::s_asyncWebBrowser2;
 
 /*
  * Without namespace declaration, the identifier "Rectangle" is ambiguous
  * See http://msdn.microsoft.com/en-us/library/windows/desktop/dd162898(v=vs.85).aspx
  */
 namespace AdblockPlus
 {
@@ skipping 10 matching lines @@
     }
 
     int Width() const
     {
       return right - left;
     }
   };
 }
 
 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();
 
   m_isAdvised = false;
   m_hTabWnd = NULL;
   m_hStatusBarWnd = NULL;
   m_hPaneWnd = NULL;
   m_nPaneWidth = 0;
   m_pWndProcStatus = NULL;
   m_hTheme = NULL;
   m_isInitializedOk = false;
 
 
   m_tab = new CPluginTab(this);
 
   Dictionary::Create(GetBrowserLanguage());
 }
 
 CPluginClass::~CPluginClass()
 {
   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;
 
   s_criticalSectionLocal.Lock();
   {
     browser = s_asyncWebBrowser2;
   }
   s_criticalSectionLocal.Unlock();
 
   return browser;
 }
 
 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);
     }
   }
   else
   {
+    DEBUG_ERROR_LOG(0, 0, 0, "CPluginClass::GetBrowserUrl - Reached with m_webBrowser2 == nullptr");
     url = m_tab->GetDocumentUrl();
   }
   return url;
 }
 
 DWORD WINAPI CPluginClass::StartInitObject(LPVOID thisPtr)
 {
   if (thisPtr == NULL)
     return 0;
   if (!((CPluginClass*)thisPtr)->InitObject())
@@ skipping 22 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");
       }
 
-      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
         s_mimeFilter = CPluginClientFactory::GetMimeFilterClientInstance();
         s_asyncWebBrowser2 = unknownSite;
         s_instances.insert(this);
       }
       s_criticalSectionLocal.Unlock();
 
       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)
       {
         DEBUG_EXCEPTION(ex);
         Unadvise();
       }
     }
     else
     {
       Unadvise();
@@ skipping 29 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();
 
-      // 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;
 }
 
 bool CPluginClass::IsStatusBarEnabled()
 {
   DEBUG_GENERAL("IsStatusBarEnabled start");
   HKEY pHkey;
   HKEY pHkeySub;
   RegOpenCurrentUser(KEY_QUERY_VALUE, &pHkey);
   DWORD truth = 1;
@@ skipping 126 matching lines @@
     }
     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 (GetBrowser().IsEqualObject(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
       UpdateStatusBar();
     }
     else
     {
       DEBUG_NAVI(L"Navi::Begin navigation url:" + url)
       m_tab->CacheFrame(url);
     }
   }
   catch (...)
   {
   }
 }
 
 // Entry point
 void STDMETHODCALLTYPE CPluginClass::OnDownloadComplete()
 {
   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 (...)
   {
   }
 }
 
 // Entry point
 void STDMETHODCALLTYPE CPluginClass::OnDocumentComplete(IDispatch* frameBrowserDisp, VARIANT* /*urlOrPidl*/)
 {
   try
   {
     DEBUG_NAVI(L"Navi::Document Complete");
     ATL::CComQIPtr<IWebBrowser2> webBrowser2 = frameBrowserDisp;
     if (!webBrowser2)
     {
       return;
     }
     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 (...)
   {
   }
 }
 
 // Entry point
 void STDMETHODCALLTYPE CPluginClass::OnWindowStateChanged(unsigned long flags, unsigned long validFlagsMask)
 {
   try
@@ skipping 58 matching lines @@
   {
     Unadvise();
   }
   catch (...)
   {
   }
 }
 
 bool CPluginClass::InitObject()
 {
-  DEBUG_GENERAL("InitObject");
+  DEBUG_GENERAL("InitObject - begin");
   CPluginSettings* settings = CPluginSettings::GetInstance();
 
   if (!settings->GetPluginEnabled())
   {
     s_mimeFilter->Unregister();
   }
 
   // Load theme module
   s_criticalSectionLocal.Lock();
   {
@@ skipping 92 matching lines @@
     if (((m_hPaneWnd == NULL) || !IsStatusBarEnabled()) && isFirstRun)
     {
       ShowStatusBar();
     }
 
     // Enable acceptable ads by default
     std::wstring aaUrl = CPluginClient::GetInstance()->GetPref(L"subscriptions_exceptionsurl", L"");
     CPluginClient::GetInstance()->AddSubscription(aaUrl);
   }
   s_criticalSectionLocal.Unlock();
+
+  DEBUG_GENERAL("InitObject - end");
   return true;
 }
 
 bool CPluginClass::CreateStatusBarPane()
 {
   CriticalSection::Lock lock(m_csStatusBar);
 
   CPluginClient* client = CPluginClient::GetInstance();
 
   std::array<wchar_t, MAX_PATH> className;
@@ skipping 869 matching lines @@
   }
   if ((m_hPaneWnd != NULL) && !::InvalidateRect(m_hPaneWnd, NULL, FALSE))
   {
       DEBUG_ERROR_LOG(::GetLastError(), PLUGIN_ERROR_UI, PLUGIN_ERROR_UI_INVALIDATE_STATUSBAR, "Class::Invalidate statusbar");
   }
 }
 
 
 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");
       }
       m_isAdvised = false;
     }
   }
   s_criticalSectionLocal.Unlock();
 }
 
@@ skipping 19 matching lines @@
   s_criticalSectionLocal.Unlock();
 
   return icon;
 }
 
 ATOM CPluginClass::GetAtomPaneClass()
 {
   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;
-}