Issue #1173 - add exception handler to CPluginClass::Invoke

src/plugin/PluginClass.cpp

 #include "PluginStdAfx.h"
 
 #include "PluginClass.h"
 #include "PluginSettings.h"
 #include "PluginSystem.h"
 #ifdef SUPPORT_FILTER
 #include "PluginFilter.h"
 #endif
 #include "PluginMimeFilterClient.h"
 #include "PluginClient.h"
@@ skipping 376 matching lines @@
     {
       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();
     }
@@ skipping 113 matching lines @@
       }
     }
     else
     {
       DEBUG_ERROR_LOG(hr, PLUGIN_ERROR_UI, PLUGIN_ERROR_UI_GET_STATUSBAR, "Class::Get statusbar state");
     }
   }
   DEBUG_GENERAL("ShowStatusBar end");
 }
 
+/*
+ * #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)
 {
 
   if (pDispParams->cArgs < 7)
   {
     return;
   }
   //Register a mime filter if it's not registered yet
   if (s_mimeFilter == NULL)
   {
@@ skipping 48 matching lines @@
   }
   else
   {
     DEBUG_NAVI(L"Navi::Begin navigation url:" + url)
 
 #ifdef SUPPORT_FRAME_CACHING
       m_tab->CacheFrame(url);
 #endif
   }
 }
+
+/*
+ * #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)
 {
   DEBUG_GENERAL("Tab changed");
   bool newtabshown = pDispParams->rgvarg[1].intVal==3;
   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;
-        if (!InitObject(true))
-        {
-          //                                    Unadvice();
-        }
+        InitObject(true);
         UpdateStatusBar();
       }
     }
   }
   notificationMessage.Hide();
   DEBUG_GENERAL("Tab change end");
-  return VARIANT_TRUE;
+  return S_OK;
 }
 
 // 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)
 {
-  WCHAR tmp[256];
-  wsprintf(tmp, L"Invoke: %d\n", dispidMember);
-  DEBUG_GENERAL(tmp);
-  switch (dispidMember)
+  try
   {
+    WCHAR tmp[256];
+    wsprintf(tmp, L"Invoke: %d\n", dispidMember);
+    DEBUG_GENERAL(tmp);
+    switch (dispidMember)
+    {
+    case DISPID_WINDOWSTATECHANGED:
+      {
+        // #1163 should validate and convert arguments here
+        return OnTabChanged(pDispParams, wFlags);
+      }
 
-  case DISPID_WINDOWSTATECHANGED:
-    return OnTabChanged(pDispParams, wFlags);
-    break;
-  case DISPID_HTMLDOCUMENTEVENTS2_ONBEFOREUPDATE:
-    return VARIANT_TRUE;
-    break;
+    case DISPID_HTMLDOCUMENTEVENTS2_ONBEFOREUPDATE:
+      break;
 
-  case DISPID_HTMLDOCUMENTEVENTS2_ONCLICK:
-    return VARIANT_TRUE;
-    break;
+    case DISPID_HTMLDOCUMENTEVENTS2_ONCLICK:
+      break;
 
-  case DISPID_EVMETH_ONLOAD:
-    DEBUG_NAVI("Navi::OnLoad")
-      return VARIANT_TRUE;
-    break;
+    case DISPID_EVMETH_ONLOAD:
+      DEBUG_NAVI("Navi::OnLoad")
+      break;
 
-  case DISPID_EVMETH_ONCHANGE:
-    return VARIANT_TRUE;
+    case DISPID_EVMETH_ONCHANGE:
+      break;
 
-  case DISPID_EVMETH_ONMOUSEDOWN:
-    return VARIANT_TRUE;
+    case DISPID_EVMETH_ONMOUSEDOWN:
+      break;
 
-  case DISPID_EVMETH_ONMOUSEENTER:
-    return VARIANT_TRUE;
+    case DISPID_EVMETH_ONMOUSEENTER:
+      break;
 
-  case DISPID_IHTMLIMGELEMENT_START:
-    return VARIANT_TRUE;
+    case DISPID_IHTMLIMGELEMENT_START:
+      break;
 
-  case STDDISPID_XOBJ_ERRORUPDATE:
-    return VARIANT_TRUE;
+    case STDDISPID_XOBJ_ERRORUPDATE:
+      break;
 
-  case STDDISPID_XOBJ_ONPROPERTYCHANGE:
-    return VARIANT_TRUE;
+    case STDDISPID_XOBJ_ONPROPERTYCHANGE:
+      break;
 
-  case DISPID_READYSTATECHANGE:
-    DEBUG_NAVI("Navi::ReadyStateChange")
-      return VARIANT_TRUE;
+    case DISPID_READYSTATECHANGE:
+      DEBUG_NAVI("Navi::ReadyStateChange");
+      break;
 
-  case DISPID_BEFORENAVIGATE:
-    DEBUG_NAVI("Navi::BeforeNavigate")
-      return VARIANT_TRUE;
-  case DISPID_COMMANDSTATECHANGE:
-    if (m_hPaneWnd == NULL)
-    {
-      CreateStatusBarPane();
-    }
-    else
-    {
-      if (CPluginClient::GetInstance()->GetIEVersion() > 6)
+    case DISPID_BEFORENAVIGATE:
+      DEBUG_NAVI("Navi::BeforeNavigate");
+      break;
+
+    case DISPID_COMMANDSTATECHANGE:
+      if (m_hPaneWnd == NULL)
       {
-        RECT rect;
-        BOOL rectRes = GetClientRect(m_hStatusBarWnd, &rect);
-        if (rectRes == TRUE)
+        CreateStatusBarPane();
+      }
+      else
+      {
+        if (CPluginClient::GetInstance()->GetIEVersion() > 6)
         {
-          MoveWindow(m_hPaneWnd, rect.right - 200, 0, m_nPaneWidth, rect.bottom - rect.top, TRUE);
+          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_STATUSTEXTCHANGE:
-    break;
+      }
+      break;
 
-  case DISPID_BEFORENAVIGATE2:
-    BeforeNavigate2(pDispParams);
-    break;
-
-  case DISPID_DOWNLOADBEGIN:
-    {
-      DEBUG_NAVI("Navi::Download Begin")
-    }
-    break;
-
-  case DISPID_DOWNLOADCOMPLETE:
-    {
-      DEBUG_NAVI("Navi::Download Complete")
-
+    case DISPID_DOCUMENTCOMPLETE:
+      {
+        DEBUG_NAVI("Navi::Document 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)
+        if (browser && pDispParams->cArgs >= 2 && pDispParams->rgvarg[1].vt == VT_DISPATCH)
         {
-          CString url;
-          CComBSTR bstrUrl;
-          if (SUCCEEDED(pBrowser->get_LocationURL(&bstrUrl)) && ::SysStringLen(bstrUrl) > 0)
+          CComQIPtr<IWebBrowser2> pBrowser = pDispParams->rgvarg[1].pdispVal;
+          if (pBrowser)
           {
-            url = bstrUrl;
-
-            CPluginClient::UnescapeUrl(url);
-
-            m_tab->OnDocumentComplete(browser, url, browser.IsEqualObject(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;
     }
-    break;
-
-  case DISPID_ONQUIT:
-  case DISPID_QUIT:
-    {
-      Unadvice();
-    }
-    break;
-
-  default:
-    {
-      CString did;
-      did.Format(L"DispId:%u", dispidMember);
-
-      DEBUG_NAVI(L"Navi::Default " + did)
-    }
-
-    // do nothing
-    break;
   }
-
-  return VARIANT_TRUE;
+  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 78 matching lines @@
   bool isFirstRun = CPluginClient::GetInstance()->IsFirstRun();
   CPluginClient::GetInstance()->SetPref(L"currentVersion", std::wstring(IEPLUGIN_VERSION));
   // This is the first time ABP was installed
   // Or ABP was updated from the version that did not support Acceptable Ads (<1.2)
   if (isFirstRun || versionCompRes < 0)
   {   
     if (!isFirstRun)
     {
       CPluginClient::GetInstance()->SetPref(L"displayUpdatePage", true);
     }
 

[Oleksandr, 2014/10/03 08:31:50]

I understand this was added here inadvertently, since the code was merged
already, right?

[Oleksandr, 2014/10/03 15:19:20]

hm. Trying doing a diff for patchset 1 vs a patchset 2 here. There's a bunch of
code added here in patchset 2.
On 2014/10/03 15:14:13, Eric wrote:

[Eric, 2014/10/03 15:37:52]

Ah. What you're seeing is an artifact of rebasing the code between the time the
first patch set was created and that of the second. 

The changes are from 0892c5dece26064d992adf5f25c8895ed7ca6e3f, which is a change
of yours after we forked a development branch a couple of months ago.

     // IE6 can't be accessed from another thread, execute in current thread
     if (ieVersion < 7)
     {
       FirstRunThread();
     }
     else
     {
       CreateThread(NULL, NULL, (LPTHREAD_START_ROUTINE)CPluginClass::FirstRunThread, NULL, NULL, NULL);
     }
     if (((m_hPaneWnd == NULL) || !IsStatusBarEnabled()) && isFirstRun)
@@ skipping 1127 matching lines @@
         }
       }
     }
 
     hTabWnd = ::GetWindow(hTabWnd, GW_HWNDNEXT);
   }
 
   return hTabWnd;
 
 }