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

src/plugin/PluginClass.cpp

Index: src/plugin/PluginClass.cpp
diff --git a/src/plugin/PluginClass.cpp b/src/plugin/PluginClass.cpp
index 2a54b54223d7fa82bb1ce5635a920bcc3cdec4e0..12f84941be2d78a3b5ac4828a233b6d0d9e459da 100644
--- a/src/plugin/PluginClass.cpp
+++ b/src/plugin/PluginClass.cpp
@@ -75,8 +75,7 @@ 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;
@@ -114,28 +113,6 @@ void CPluginClass::FinalRelease()
   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;
@@ -210,7 +187,7 @@ DWORD WINAPI CPluginClass::StartInitObject(LPVOID thisPtr)
     return 0;
   if (!((CPluginClass*)thisPtr)->InitObject(true))
   {
-    ((CPluginClass*)thisPtr)->Unadvice();
+    ((CPluginClass*)thisPtr)->Unadvise();
   }
 
   return 0;
@@ -262,34 +239,30 @@ STDMETHODIMP CPluginClass::SetSite(IUnknown* unknownSite)
     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
       {
@@ -328,13 +301,13 @@ STDMETHODIMP CPluginClass::SetSite(IUnknown* unknownSite)
     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)
@@ -502,87 +475,100 @@ void CPluginClass::ShowStatusBar()
   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;
   }
-  //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)
+  if (!urlVariant || urlVariant->vt != VT_BSTR)
   {
-    WebBrowser2Ptr = pDispParams->rgvarg[6].pdispVal;
-  }
-  else
-  {
-    // Wrong type, return.
     return;
   }
+  std::wstring url(urlVariant->bstrVal, SysStringLen(urlVariant->bstrVal));
+  UnescapeUrl(url);
 
-  // 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
+  //Register a mime filter if it's not registered yet
+  if (s_mimeFilter == NULL)
   {
-    // Wrong type, return.
-    return;
+    s_mimeFilter = CPluginClientFactory::GetMimeFilterClientInstance();
   }
 
+  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());
@@ -592,166 +578,36 @@ STDMETHODIMP CPluginClass::OnTabChanged(DISPPARAMS* pDispParams, WORD wFlags)
       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)
+  {
+    CreateStatusBarPane();
+  }
+  else
   {
-    WCHAR tmp[256];
-    wsprintf(tmp, L"Invoke: %d\n", dispidMember);
-    DEBUG_GENERAL(tmp);
-    switch (dispidMember)
+    if (AdblockPlus::IE::InstalledMajorVersion() > 6)
     {
-    case DISPID_WINDOWSTATECHANGED:
-      {
-        // #1163 should validate and convert arguments here
-        return OnTabChanged(pDispParams, wFlags);
-      }
-
-    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:
+      RECT rect;
+      BOOL rectRes = GetClientRect(m_hStatusBarWnd, &rect);
+      if (rectRes == TRUE)
       {
-        CString did;
-        did.Format(L"DispId:%u", dispidMember);
-
-        DEBUG_NAVI(L"Navi::Default " + did)
+        MoveWindow(m_hPaneWnd, rect.right - 200, 0, m_nPaneWidth, rect.bottom - rect.top, TRUE);
       }
-      /*
-       * 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)
@@ -1743,23 +1599,18 @@ void CPluginClass::UpdateStatusBar()
 }
 
 
-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();