Noissue - Remove dead code in CPluginClass::SetSite

src/plugin/PluginClass.cpp

Index: src/plugin/PluginClass.cpp
===================================================================
--- a/src/plugin/PluginClass.cpp
+++ b/src/plugin/PluginClass.cpp
@@ -233,175 +233,149 @@
 }
 
 
-
-// This gets called when a new browser window is created (which also triggers the
-// creation of this object). The pointer passed in should be to a IWebBrowser2
-// interface that represents the browser for the window.
-// it is also called when a tab is closed, this unknownSite will be null
-// so we should handle that it is called this way several times during a session
+/*
+ * IE calls this when it creates a new browser window or tab, immediately after it also 
+ * creates the object. The argument 'unknownSite' in is the OLE "site" of the object, 
+ * which is an IWebBrowser2 interface associated with the window/tab. 
+ *
+ * IE also ordinarily calls this again when its window/tab is closed, in which case
+ * 'unknownSite' will be null. Extraordinarily, this is sometimes _not_ called when IE
+ * is shutting down. Thus 'SetSite(nullptr)' has some similarities with a destructor,
+ * but it is not a proper substitute for one.

[sergei, 2015/03/09 10:23:50]

I'm not sure that we need this comment except the case when SetSite is not
called. Are you sure that 'SetSite(nullptr)' not called if the shutting down is
graceful, thus it's not a case of the crash or something like this? Would be
great if you can provide a link to the documentation.

[Eric, 2015/03/09 13:31:29]

I was just rewriting the comment, mostly, with the one change you picked up on;
see below. Given that the lifecycle of IE extensions is not well documented,
having some kind of comment here is certainly warranted.
Yes, mostly. I verified it last week. I haven't seen anything about this topic
in any MS documentation. Of particular interest, I haven't seen anything about a
requirement to call 'SetSite(nullptr)' before destroying a control instance. It
seems best to assume that it need not be.

I wrote some more detailed trace messages for this function that included values
of 'this' and the site pointer. Most of them were in matched pairs, but not all.
I was very reliably getting unmatched pairs. There was no obvious crashing
behavior, although I can't be completely sure there wasn't any internally; this
is why I said "mostly" above.

The consistency in unmatched pairs seems to be something related to never having
loaded a page or interrupting a page load by shutting down IE; I don't have
anything definitive there. I'm going to be looking into this issue further.

[Oleksandr, 2015/03/10 08:38:26]

I am pretty sure it _is_ called every time when IE is shutting down. It's that
sometimes the IE shutdown from the end user perspective doesn't really shutdown
the IE and the process keeps hanging. In that case it doesn't call
SetSite(nullptr). Do you have any test cases when it is not called?
Also, I think this comment merely reflects the documentation. I'd vote for
removing it or at least making it much shorter

[Oleksandr, 2015/03/10 08:42:04]

https://msdn.microsoft.com/en-us/library/bb250489(v=vs.85).aspx#basics
Could it have been the fact that the IE process did not exit?

+ */
 STDMETHODIMP CPluginClass::SetSite(IUnknown* unknownSite)
 {
-  CPluginSettings* settings = CPluginSettings::GetInstance();
+  try
+  {
+    if (unknownSite)
+    {
 
-  MULTIPLE_VERSIONS_CHECK();
+      DEBUG_GENERAL(L"================================================================================\nNEW TAB UI\n================================================================================")
 
-  if (unknownSite)
-  {
+      HRESULT hr = ::CoInitialize(NULL);
+      if (FAILED(hr))
+      {
+        DEBUG_ERROR_LOG(hr, PLUGIN_ERROR_SET_SITE, PLUGIN_ERROR_SET_SITE_COINIT, "Class::SetSite - CoInitialize");
+      }
 
-    DEBUG_GENERAL(L"================================================================================\nNEW TAB UI\n================================================================================")
+      s_criticalSectionBrowser.Lock();
+      {
+        m_webBrowser2 = unknownSite;

[Eric, 2015/03/06 15:58:04]

Assigned to m_webBrowser2 here, declared as CComQIPtr<IWebBrowser2>

[sergei, 2015/03/06 16:07:19]

Here `AddRef` is called.

+      }
+      s_criticalSectionBrowser.Unlock();
 
-    HRESULT hr = ::CoInitialize(NULL);
-    if (FAILED(hr))
-    {
-      DEBUG_ERROR_LOG(hr, PLUGIN_ERROR_SET_SITE, PLUGIN_ERROR_SET_SITE_COINIT, "Class::SetSite - CoInitialize");
-    }
+      //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_criticalSectionBrowser.Lock();
-    {
-      m_webBrowser2 = unknownSite;
-    }
-    s_criticalSectionBrowser.Unlock();
+      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();
 
-    //register the mimefilter
-    //and only mimefilter
-    //on some few computers the mimefilter does not get properly registered when it is done on another thread
+      try
+      {
+        // Check if loaded as BHO
+        if (GetBrowser())

[sergei, 2015/03/09 10:23:50]

We don't need this comment now.

[Eric, 2015/03/09 13:31:29]

We don't need this particular comment, but we'll need another one in the next
patch set. We still need to check that the QI call on site pointer to
'IWebBrowser2' is not null. If it's not, we likely have some severe IE defect,
but regardless we need to handle that case gracefully.

[Eric, 2015/03/17 10:41:07]

Done.

+        {
+          DEBUG_GENERAL("Loaded as BHO");
+          CComPtr<IConnectionPoint> pPoint = GetConnectionPoint();
+          if (pPoint)
+          {
+            HRESULT hr = pPoint->Advise((IDispatch*)this, &m_nConnectionID);
+            if (SUCCEEDED(hr))
+            {
+              m_isAdviced = true;
 
-    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
-    {
-      // Check if loaded as BHO
-      if (GetBrowser())
-      {
-        DEBUG_GENERAL("Loaded as BHO");
-        CComPtr<IConnectionPoint> pPoint = GetConnectionPoint();
-        if (pPoint)
-        {
-          HRESULT hr = pPoint->Advise((IDispatch*)this, &m_nConnectionID);
-          if (SUCCEEDED(hr))
-          {
-            m_isAdviced = true;
-
-            try
+              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");
+              }
+            }
+            else
             {
-              std::thread startInitObjectThread(StartInitObject, this);
-              startInitObjectThread.detach(); // TODO: but actually we should wait for the thread in the dtr.
+              DEBUG_ERROR_LOG(hr, PLUGIN_ERROR_SET_SITE, PLUGIN_ERROR_SET_SITE_ADVICE, "Class::SetSite - Advice");
             }
-            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");
-            }
-          }
-          else
-          {
-            DEBUG_ERROR_LOG(hr, PLUGIN_ERROR_SET_SITE, PLUGIN_ERROR_SET_SITE_ADVICE, "Class::SetSite - Advice");
           }
         }
       }
-      else // Check if loaded as toolbar handler
+      catch (const std::runtime_error& ex)
       {
-        DEBUG_GENERAL("Loaded as toolbar handler");
-        CComPtr<IServiceProvider> pServiceProvider;
+        DEBUG_EXCEPTION(ex);
+        Unadvice();
+      }
+    }
+    else
+    {
+      // Unadvice
+      Unadvice();
 
-        HRESULT hr = unknownSite->QueryInterface(&pServiceProvider);
-        if (SUCCEEDED(hr))
+      // Destroy window
+      if (m_pWndProcStatus)
+      {
+        ::SetWindowLongPtr(m_hStatusBarWnd, GWLP_WNDPROC, (LPARAM)(WNDPROC)m_pWndProcStatus);
+
+        m_pWndProcStatus = NULL;
+      }
+
+      if (m_hPaneWnd)
+      {
+        DestroyWindow(m_hPaneWnd);
+        m_hPaneWnd = NULL;
+      }
+
+      m_hTabWnd = NULL;
+      m_hStatusBarWnd = NULL;
+
+      // Remove instance from the list, shutdown threads
+      HANDLE hMainThread = NULL;
+      HANDLE hTabThread = NULL;
+
+      s_criticalSectionLocal.Lock();
+      {
+        s_instances.erase(this);
+
+        std::map<DWORD,CPluginClass*>::iterator it = s_threadInstances.find(::GetCurrentThreadId());
+        if (it != s_threadInstances.end())
         {
-          if (pServiceProvider)
-          {
-            s_criticalSectionBrowser.Lock();
-            {
-              HRESULT hr = pServiceProvider->QueryService(IID_IWebBrowserApp, &m_webBrowser2);
-              if (SUCCEEDED(hr))
-              {
-                if (m_webBrowser2)
-                {
-                  InitObject(false);
-                }
-              }
-              else
-              {
-                DEBUG_ERROR_LOG(hr, PLUGIN_ERROR_SET_SITE, PLUGIN_ERROR_SET_SITE_QUERY_BROWSER, "Class::SetSite - QueryService (IID_IWebBrowserApp)");
-              }
-            }
-            s_criticalSectionBrowser.Unlock();
-          }
+          s_threadInstances.erase(it);
         }
-        else
+        if (s_instances.empty())
         {
-          DEBUG_ERROR_LOG(hr, PLUGIN_ERROR_SET_SITE, PLUGIN_ERROR_SET_SITE_QUERY_SERVICE_PROVIDER, "Class::SetSite - QueryInterface (service provider)");
+          // TODO: Explicitly releasing a resource when a container becomes empty looks like a job better suited for shared_ptr
+          CPluginClientFactory::ReleaseMimeFilterClientInstance();
         }
       }
-    }
-    catch (const std::runtime_error& ex)
-    {
-      DEBUG_EXCEPTION(ex);
-      Unadvice();
-    }
-  }
-  else
-  {
-    // Unadvice
-    Unadvice();
+      s_criticalSectionLocal.Unlock();
 
-    // Destroy window
-    if (m_pWndProcStatus)
-    {
-      ::SetWindowLongPtr(m_hStatusBarWnd, GWLP_WNDPROC, (LPARAM)(WNDPROC)m_pWndProcStatus);
+      // Release browser interface
+      s_criticalSectionBrowser.Lock();
+      {
+        m_webBrowser2.Release();

[Eric, 2015/03/06 15:58:04]

Explicit release here.

[sergei, 2015/03/06 16:07:19]

So, since `AddRef` is called and `m_webBrowser2` is a smart pointer, there is no
inconsistency. After `m_webBrowser2.Release();` the following calls
`m_webBrowser2.Release();` are safe, they do nothing.

[Eric, 2015/03/06 16:27:22]

Calling 'Release' through 'CComPtr' apparently also zeros out the pointer as
well, contrary to all good design sense. I had to read the source code to see
this; it's certainly not an obvious behavior.

I am coming to despise ATL even more than I did before, which is quite saying
something.

+      }
+      s_criticalSectionBrowser.Unlock();
 
-      m_pWndProcStatus = NULL;
+      DEBUG_GENERAL("================================================================================\nNEW TAB UI - END\n================================================================================")
+
+      ::CoUninitialize();
     }
 
-    if (m_hPaneWnd)
-    {
-      DestroyWindow(m_hPaneWnd);
-      m_hPaneWnd = NULL;
-    }
-
-    m_hTabWnd = NULL;
-    m_hStatusBarWnd = NULL;
-
-    // Remove instance from the list, shutdown threads
-    HANDLE hMainThread = NULL;
-    HANDLE hTabThread = NULL;
-
-    s_criticalSectionLocal.Lock();
-    {
-      s_instances.erase(this);
-
-      std::map<DWORD,CPluginClass*>::iterator it = s_threadInstances.find(::GetCurrentThreadId());
-      if (it != s_threadInstances.end())
-      {
-        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();
-    }
-    s_criticalSectionBrowser.Unlock();
-
-    DEBUG_GENERAL("================================================================================\nNEW TAB UI - END\n================================================================================")
-
-    ::CoUninitialize();
+    IObjectWithSiteImpl<CPluginClass>::SetSite(unknownSite);
   }
-
-  return IObjectWithSiteImpl<CPluginClass>::SetSite(unknownSite);
+  catch (...)
+  {
+  }
+  return S_OK;
 }
 
 bool CPluginClass::IsStatusBarEnabled()