Issue 1672 - Subscribe and listen COM events using ATL::IDispEventImpl and BEGIN_SINK_MAP

src/plugin/PluginClass.cpp

 #include "PluginStdAfx.h"
 
 #include "PluginClass.h"
 #include "PluginSettings.h"
 #include "PluginSystem.h"
 #include "PluginFilter.h"
 #include "PluginMimeFilterClient.h"
 #include "PluginClient.h"
 #include "PluginClientFactory.h"
 #include "PluginMutex.h"
@@ skipping 57 matching lines @@
       return right - left;
     }
   };
 }
 
 CPluginClass::CPluginClass()
 {
   //Use this line to debug memory leaks
   //    _CrtDumpMemoryLeaks();
 
-  m_isAdviced = false;
-  m_nConnectionID = 0;
+  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);
@@ skipping 17 matching lines @@
 
 void CPluginClass::FinalRelease()
 {
   s_criticalSectionBrowser.Lock();
   {
     m_webBrowser2.Release();
   }
   s_criticalSectionBrowser.Unlock();
 }
 
-
-// This method tries to get a 'connection point' from the stored browser, which can be
-// used to attach or detach from the stream of browser events
-CComPtr<IConnectionPoint> CPluginClass::GetConnectionPoint()
-{
-  CComQIPtr<IConnectionPointContainer, &IID_IConnectionPointContainer> pContainer(GetBrowser());
-  if (!pContainer)
-  {
-    return NULL;
-  }
-
-  CComPtr<IConnectionPoint> pPoint;
-  HRESULT hr = pContainer->FindConnectionPoint(DIID_DWebBrowserEvents2, &pPoint);
-  if (FAILED(hr))
-  {
-    DEBUG_ERROR_LOG(hr, PLUGIN_ERROR_SET_SITE, PLUGIN_ERROR_SET_SITE_FIND_CONNECTION_POINT, "Class::GetConnectionPoint - FindConnectionPoint")
-      return NULL;
-  }
-
-  return pPoint;
-}
-
 HWND CPluginClass::GetBrowserHWND() const
 {
   SHANDLE_PTR hBrowserWndHandle = NULL;
 
   CComQIPtr<IWebBrowser2> browser = GetBrowser();
   if (browser)
   {
     HRESULT hr = browser->get_HWND(&hBrowserWndHandle);
     if (FAILED(hr))
     {
@@ skipping 54 matching lines @@
 
   return url;
 }
 
 DWORD WINAPI CPluginClass::StartInitObject(LPVOID thisPtr)
 {
   if (thisPtr == NULL)
     return 0;
   if (!((CPluginClass*)thisPtr)->InitObject(true))
   {
-    ((CPluginClass*)thisPtr)->Unadvice();
+    ((CPluginClass*)thisPtr)->Unadvise();
   }
 
   return 0;
 }
 
 
 
 // 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.
@@ skipping 31 matching lines @@
       // 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())
+      auto webBrowser = GetBrowser();
+      if (webBrowser)
       {
         DEBUG_GENERAL("Loaded as BHO");
-        CComPtr<IConnectionPoint> pPoint = GetConnectionPoint();
-        if (pPoint)
+        HRESULT hr = DispEventAdvise(webBrowser);

[Eric, 2015/01/12 14:18:07]

The Advise/Unadvise life cycle belong in a constructor/destructor pair.

Right now setting up the advice relationship happens in loose code here and in a
separate function elsewhere. It would be clearer to localize these to the same
place in the code.

[sergei, 2015/01/13 11:49:12]

It does not belong to constructor/destructor pair.
I see, it definitely makes sense to have a pair Advise/Unadvise which works with
m_isAdvised and makes necessary calls. But I would like to do it as another
issue may be even GFB.

We have the method `Unadvise` to avoid code duplication because it's called from
several places but "advising" is called only from one place here, so it should
be fine to have it as is from the simplification point of view. As well as it's
always better to have smaller commits.

[Eric, 2015/01/13 18:44:30]

It absolutely should be paired.

As it's written now, there's a race condition between calling DispEventAdvise()
and setting 'm_isAdvised'. In what should be the destructor, there's a
semaphore-protected block that ensures that DispEventUnadvise() is called
atomically with resetting 'm_isAdvised' (which is defective--see comment--but
whatever). There should be a similar semaphore-protected block for
DispEventAdvise().

So this pair of calls is managing a kind of event-connection resource, and
because it's a resource, it falls under the purview of RAII. Unlike with
connection points, there's no explicit handle or object associated with the
resource, but it's still a resource. The member 'm_isAdvised' acts as the proxy
for that resource.

Such a resource class should have two members:
  -- a browser pointer, to avoid calling the (de facto) global GetBrowser() in
the destructor.
  -- the semaphore used to protect the block. Per #1467, we can use the standard
library for this.

The class does not need 'm_isAdvised', because it would be redundant. Such a
member would always be true during the existence of the instance.

The member variable in CPluginClass can be unique_ptr<>, which will
automatically call the destructor. Presence of the allocated instance represents
presence of the resource.

+        if (SUCCEEDED(hr))
         {
-          HRESULT hr = pPoint->Advise((IDispatch*)this, &m_nConnectionID);
-          if (SUCCEEDED(hr))
+          m_isAdvised = true;
+          try
           {
-            m_isAdviced = 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)
-            {
-              auto errDescription = std::string("Class::Thread - Failed to create StartInitObject thread, ") +
-                ex.code().message() + ex.what();
-              DEBUG_ERROR_LOG(ex.code().value(), PLUGIN_ERROR_THREAD, PLUGIN_ERROR_MAIN_THREAD_CREATE_PROCESS, errDescription.c_str());
-            }
+            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 - Advice");
+            auto errDescription = std::string("Class::Thread - Failed to create StartInitObject thread, ") +
+              ex.code().message() + ex.what();
+            DEBUG_ERROR_LOG(ex.code().value(), PLUGIN_ERROR_THREAD, PLUGIN_ERROR_MAIN_THREAD_CREATE_PROCESS, errDescription.c_str());
           }
         }
+        else
+        {
+          DEBUG_ERROR_LOG(hr, PLUGIN_ERROR_SET_SITE, PLUGIN_ERROR_SET_SITE_ADVICE, "Class::SetSite - Advice");
+        }
       }
       else // Check if loaded as toolbar handler
       {
         DEBUG_GENERAL("Loaded as toolbar handler");
         CComPtr<IServiceProvider> pServiceProvider;
 
         HRESULT hr = unknownSite->QueryInterface(&pServiceProvider);
         if (SUCCEEDED(hr))
         {
           if (pServiceProvider)
@@ skipping 18 matching lines @@
         }
         else
         {
           DEBUG_ERROR_LOG(hr, PLUGIN_ERROR_SET_SITE, PLUGIN_ERROR_SET_SITE_QUERY_SERVICE_PROVIDER, "Class::SetSite - QueryInterface (service provider)");
         }
       }
     }
     catch (std::runtime_error e)
     {
       DEBUG_ERROR(e.what());
-      Unadvice();
+      Unadvise();
     }
   }
   else
   {
-    // Unadvice
-    Unadvice();
+    // Unadvise

[Oleksandr, 2015/01/10 23:27:24]

I guess we can loose the captain obvious comment here.

[sergei, 2015/01/13 11:49:12]

removed

+    Unadvise();
 
     // Destroy window
     if (m_pWndProcStatus)
     {
       ::SetWindowLongPtr(m_hStatusBarWnd, GWLP_WNDPROC, (LPARAM)(WNDPROC)m_pWndProcStatus);
 
       m_pWndProcStatus = NULL;
     }
 
     if (m_hPaneWnd)
@@ skipping 146 matching lines @@
         }
       }
     }
     else
     {
       DEBUG_ERROR_LOG(hr, PLUGIN_ERROR_UI, PLUGIN_ERROR_UI_GET_STATUSBAR, "Class::Get statusbar state");
     }
   }
   DEBUG_GENERAL("ShowStatusBar end");
 }
 

[Eric, 2015/01/12 14:18:07]

// Entry point

We need to annotate all the entry points with a consistent textual marker for
auditing. Unfortunately the function signature isn't consistent. 

-/*
- * #1163 This class is the implementation for method DISPID_BEFORENAVIGATE2 in CPluginClass::Invoke.
- * - It validates and convertes its own arguments, rather than unifying them in the Invoke body.
- * - It's declared void and not HRESULT, so DISPID_BEFORENAVIGATE2 can only return S_OK.
- */
-void CPluginClass::BeforeNavigate2(DISPPARAMS* pDispParams)
+void STDMETHODCALLTYPE CPluginClass::OnBeforeNavigate2(
+                        /* [in] */ IDispatch* frameBrowserDisp,
+                        /* [in] */ VARIANT* urlVariant,
+                        /* [in] */ VARIANT* /*Flags*/,
+                        /* [in] */ VARIANT* /*TargetFrameName*/,
+                        /* [in] */ VARIANT* /*PostData*/,
+                        /* [in] */ VARIANT* /*Headers*/,
+                        /* [in, out] */ VARIANT_BOOL* /*Cancel*/)
 {

[Eric, 2015/01/12 14:18:07]

This patch set changes OnBeforeNavigate2 into an entry point. That means it
needs a 'try' statement and a catch-all block.

-
-  if (pDispParams->cArgs < 7)
+  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);
+
   //Register a mime filter if it's not registered yet
   if (s_mimeFilter == NULL)
   {
     s_mimeFilter = CPluginClientFactory::GetMimeFilterClientInstance();
   }
 
-  // Get the IWebBrowser2 interface
-  CComQIPtr<IWebBrowser2, &IID_IWebBrowser2> WebBrowser2Ptr;
-  VARTYPE vt = pDispParams->rgvarg[6].vt;
-  if (vt == VT_DISPATCH)
-  {
-    WebBrowser2Ptr = pDispParams->rgvarg[6].pdispVal;
-  }
-  else
-  {
-    // Wrong type, return.
-    return;
-  }
-
-  // Get the URL
-  CString url;
-  vt = pDispParams->rgvarg[5].vt;
-  if (vt == VT_BYREF + VT_VARIANT)
-  {
-    url = pDispParams->rgvarg[5].pvarVal->bstrVal;
-
-    CPluginClient::UnescapeUrl(url);
-  }
-  else
-  {
-    // Wrong type, return.
-    return;
-  }
+  CString urlCString = ToCString(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)
+  if (urlCString.Find(L"javascript") == 0)
   {
   }
-  else if (GetBrowser().IsEqualObject(WebBrowser2Ptr))
+  else if (GetBrowser().IsEqualObject(webBrowser))
   {
-    m_tab->OnNavigate(url);
+    m_tab->OnNavigate(urlCString);
 
-    DEBUG_GENERAL(L"================================================================================\nBegin main navigation url:" + url + "\n================================================================================")
+    DEBUG_GENERAL(L"================================================================================\nBegin main navigation url:" + urlCString + "\n================================================================================")
 
 #ifdef ENABLE_DEBUG_RESULT
-      CPluginDebug::DebugResultDomain(url);
+      CPluginDebug::DebugResultDomain(urlCString);
 #endif
 
     UpdateStatusBar();
   }
   else
   {
-    DEBUG_NAVI(L"Navi::Begin navigation url:" + url)
-    m_tab->CacheFrame(url);
+    DEBUG_NAVI(L"Navi::Begin navigation url:" + urlCString)
+    m_tab->CacheFrame(urlCString);
   }
 }
 

[Eric, 2015/01/12 14:18:07]

// Entry point

-/*
- * #1163 implements behavior for method DISPID_WINDOWSTATECHANGED in CPluginClass::Invoke
- * - should validate and convert arguments in Invoke, not here
- * - does not validate number of arguments before indexing into 'rgvarg'
- * - does not validate type of argument before using its value
- */
-STDMETHODIMP CPluginClass::OnTabChanged(DISPPARAMS* pDispParams, WORD wFlags)
+void STDMETHODCALLTYPE CPluginClass::OnDownloadBegin()
+{
+  DEBUG_NAVI("Navi::Download Begin")
+}
+

[Eric, 2015/01/12 14:18:07]

// Entry point

+void STDMETHODCALLTYPE CPluginClass::OnDownloadComplete()
+{

[Eric, 2015/01/12 14:18:07]

try-statement and catch-all

+  DEBUG_NAVI("Navi::Download Complete")
+  ATL::CComPtr<IWebBrowser2> browser = GetBrowser();
+  if (browser)
+  {
+    m_tab->OnDownloadComplete(browser);
+  }
+}
+

[Eric, 2015/01/12 14:18:07]

// Entry point

+void STDMETHODCALLTYPE CPluginClass::OnDocumentComplete(IDispatch* frameBrowserDisp, VARIANT* /*urlOrPidl*/)
+{

[Eric, 2015/01/12 14:18:07]

try-statement and catch-all

+  ATL::CComQIPtr<IWebBrowser2> webBrowser2 = frameBrowserDisp;
+  if (!webBrowser2)
+  {
+    return;
+  }
+  ATL::CString frameSrc;
+  ATL::CComBSTR locationUrl;
+  if (FAILED(webBrowser2->get_LocationURL(&locationUrl)) && !!locationUrl)
+  {
+    return;
+  }
+  frameSrc = locationUrl;
+  CPluginClient::UnescapeUrl(frameSrc);
+  bool isRootPageBrowser = GetBrowser().IsEqualObject(webBrowser2);
+  m_tab->OnDocumentComplete(webBrowser2, frameSrc, isRootPageBrowser);
+}
+

[Eric, 2015/01/12 14:18:07]

// Entry point

+void STDMETHODCALLTYPE CPluginClass::OnWindowStateChanged(unsigned long flags, unsigned long validFlagsMask)
 {

[Eric, 2015/01/12 14:18:07]

try-statement and catch-all

   DEBUG_GENERAL("Tab changed");
-  bool newtabshown = pDispParams->rgvarg[1].intVal==3;
+  bool newtabshown = validFlagsMask == (OLECMDIDF_WINDOWSTATE_USERVISIBLE | OLECMDIDF_WINDOWSTATE_ENABLED)
+                  && flags == (OLECMDIDF_WINDOWSTATE_USERVISIBLE | OLECMDIDF_WINDOWSTATE_ENABLED);
   if (newtabshown)
   {
     std::map<DWORD,CPluginClass*>::const_iterator it = s_threadInstances.find(GetCurrentThreadId());
     if (it == s_threadInstances.end())
     {
       s_threadInstances[::GetCurrentThreadId()] = this;
       if (!m_isInitializedOk)
       {
         m_isInitializedOk = true;
-        InitObject(true);
+        if (!InitObject(true))
+        {
+          //                                    Unadvise();
+        }

[Oleksandr, 2015/01/10 23:27:24]

This doesn't seem to be rebased to the current tip. (rev. 746)

         UpdateStatusBar();
       }
     }
   }
   notificationMessage.Hide();
   DEBUG_GENERAL("Tab change end");
-  return S_OK;
 }
 

[Eric, 2015/01/12 14:18:07]

// Entry point

-// This gets called whenever there's a browser event
-// ENTRY POINT
-STDMETHODIMP CPluginClass::Invoke(DISPID dispidMember, REFIID riid, LCID lcid, WORD wFlags, DISPPARAMS* pDispParams, VARIANT* pvarResult, EXCEPINFO* pExcepInfo, UINT* puArgErr)
+void STDMETHODCALLTYPE CPluginClass::OnCommandStateChange(long /*command*/, VARIANT_BOOL /*enable*/)
 {

[Eric, 2015/01/12 14:18:07]

try-statement and catch-all

-  try
+  if (m_hPaneWnd == NULL)
   {
-    WCHAR tmp[256];
-    wsprintf(tmp, L"Invoke: %d\n", dispidMember);
-    DEBUG_GENERAL(tmp);
-    switch (dispidMember)
+    CreateStatusBarPane();
+  }
+  else
+  {
+    if (AdblockPlus::IE::InstalledMajorVersion() > 6)
     {
-    case DISPID_WINDOWSTATECHANGED:
+      RECT rect;
+      BOOL rectRes = GetClientRect(m_hStatusBarWnd, &rect);
+      if (rectRes == TRUE)
       {
-        // #1163 should validate and convert arguments here
-        return OnTabChanged(pDispParams, wFlags);
+        MoveWindow(m_hPaneWnd, rect.right - 200, 0, m_nPaneWidth, rect.bottom - rect.top, TRUE);
       }
-
-    case DISPID_HTMLDOCUMENTEVENTS2_ONBEFOREUPDATE:
-      break;
-
-    case DISPID_HTMLDOCUMENTEVENTS2_ONCLICK:
-      break;
-
-    case DISPID_EVMETH_ONLOAD:
-      DEBUG_NAVI("Navi::OnLoad")
-      break;
-
-    case DISPID_EVMETH_ONCHANGE:
-      break;
-
-    case DISPID_EVMETH_ONMOUSEDOWN:
-      break;
-
-    case DISPID_EVMETH_ONMOUSEENTER:
-      break;
-
-    case DISPID_IHTMLIMGELEMENT_START:
-      break;
-
-    case STDDISPID_XOBJ_ERRORUPDATE:
-      break;
-
-    case STDDISPID_XOBJ_ONPROPERTYCHANGE:
-      break;
-
-    case DISPID_READYSTATECHANGE:
-      DEBUG_NAVI("Navi::ReadyStateChange");
-      break;
-
-    case DISPID_BEFORENAVIGATE:
-      DEBUG_NAVI("Navi::BeforeNavigate");
-      break;
-
-    case DISPID_COMMANDSTATECHANGE:
-      if (m_hPaneWnd == NULL)
-      {
-        CreateStatusBarPane();
-      }
-      else
-      {
-      if (AdblockPlus::IE::InstalledMajorVersion() > 6)
-        {
-          RECT rect;
-          BOOL rectRes = GetClientRect(m_hStatusBarWnd, &rect);
-          if (rectRes == TRUE)
-          {
-            MoveWindow(m_hPaneWnd, rect.right - 200, 0, m_nPaneWidth, rect.bottom - rect.top, TRUE);
-          }
-        }      
-      }
-      break;
-
-    case DISPID_STATUSTEXTCHANGE:
-      break;
-
-    case DISPID_BEFORENAVIGATE2:
-      {
-        // #1163 should validate and convert parameters here
-        BeforeNavigate2(pDispParams);
-      }
-      break;
-
-    case DISPID_DOWNLOADBEGIN:
-      {
-        DEBUG_NAVI("Navi::Download Begin")
-      }
-      break;
-
-    case DISPID_DOWNLOADCOMPLETE:
-      {
-        DEBUG_NAVI("Navi::Download Complete");
-        CComQIPtr<IWebBrowser2> browser = GetBrowser();
-        if (browser)
-        {
-          m_tab->OnDownloadComplete(browser);
-        }
-      }
-      break;
-
-    case DISPID_DOCUMENTCOMPLETE:
-      {
-        DEBUG_NAVI("Navi::Document Complete");
-        CComQIPtr<IWebBrowser2> browser = GetBrowser();
-        if (browser && pDispParams->cArgs >= 2 && pDispParams->rgvarg[1].vt == VT_DISPATCH)
-        {
-          CComQIPtr<IWebBrowser2> pBrowser = pDispParams->rgvarg[1].pdispVal;
-          if (pBrowser)
-          {
-            CString url;
-            CComBSTR bstrUrl;
-            if (SUCCEEDED(pBrowser->get_LocationURL(&bstrUrl)) && ::SysStringLen(bstrUrl) > 0)
-            {
-              url = bstrUrl;
-              CPluginClient::UnescapeUrl(url);
-              m_tab->OnDocumentComplete(browser, url, browser.IsEqualObject(pBrowser));
-            }
-          }
-        }
-      }
-      break;
-
-    case DISPID_ONQUIT:
-    case DISPID_QUIT:
-      {
-        Unadvice();
-      }
-      break;
-
-    default:
-      {
-        CString did;
-        did.Format(L"DispId:%u", dispidMember);
-
-        DEBUG_NAVI(L"Navi::Default " + did)
-      }
-      /*
-       * Ordinarily a method not dispatched should return DISP_E_MEMBERNOTFOUND.
-       * As a conservative initial change, we leave it behaving as before,
-       *   which is to do nothing and return S_OK.
-       */
-      // do nothing
-      break;
     }
   }
-  catch(...)
-  {
-    DEBUG_GENERAL( "Caught unknown exception in CPluginClass::Invoke" );
-    return E_FAIL;
-  }
-  return S_OK;
 }
 
 bool CPluginClass::InitObject(bool bBHO)
 {
   DEBUG_GENERAL("InitObject");
   CPluginSettings* settings = CPluginSettings::GetInstance();
 
   if (!settings->GetPluginEnabled())
   {
     s_mimeFilter->Unregister();
@@ skipping 971 matching lines @@
   {
     CreateStatusBarPane();
   }
   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::Unadvice()
+void STDMETHODCALLTYPE CPluginClass::Unadvise()

[Eric, 2015/01/12 14:18:07]

If no OnQuit function is provided, this is an entry point.

 {
   s_criticalSectionLocal.Lock();
   {
-    if (m_isAdviced)
+    if (m_isAdvised)
     {
-      CComPtr<IConnectionPoint> pPoint = GetConnectionPoint();
-      if (pPoint)
+      HRESULT hr = DispEventUnadvise(GetBrowser());
+      if (FAILED(hr))
       {
-        HRESULT hr = pPoint->Unadvise(m_nConnectionID);
-        if (FAILED(hr))
-        {
-          DEBUG_ERROR_LOG(hr, PLUGIN_ERROR_SET_SITE, PLUGIN_ERROR_SET_SITE_UNADVICE, "Class::Unadvice - Unadvise");
-        }
+        DEBUG_ERROR_LOG(hr, PLUGIN_ERROR_SET_SITE, PLUGIN_ERROR_SET_SITE_UNADVISE, "Class::Unadvise - Unadvise");
       }
-
-      m_isAdviced = false;
+      m_isAdvised = false;
     }
   }
   s_criticalSectionLocal.Unlock();
 }
 
 HICON CPluginClass::GetIcon(int type)
 {
   HICON icon = NULL;
 
   s_criticalSectionLocal.Lock();
@@ skipping 81 matching lines @@
         }
       }
     }
 
     hTabWnd = ::GetWindow(hTabWnd, GW_HWNDNEXT);
   }
 
   return hTabWnd;
 
 }