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())
       {
+        // AUDIT: Explicitly releasing a resource when a container becomes empty looks like a job better suited for shared_ptr

[Oleksandr, 2014/08/21 14:49:56]

I think it's better to create an issue then to put this comment into the code.

[Eric, 2014/09/29 19:16:44]

Well, I don't know what the issue would be yet; I haven't spent enough time
analyzing the code to know everything that's involved here. Is it better to have
neither comment nor issue on this?

[Oleksandr, 2014/10/02 20:44:16]

How about TODO: then?

On 2014/09/29 19:16:44, Eric wrote:

[Eric, 2014/10/02 23:11:02]

OK.

         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.
+ */

[Oleksandr, 2014/08/21 14:49:56]

All the comments in this file that start with #1163 are redundant here, IMHO. I
don't any of them adds in any way to readability of the code.

[Eric, 2014/09/29 19:16:44]

These comments are intended to exist only while #1163 is being worked on,
because it's not an issue that can get solved with a single review. These
comments essentially record a work-in-progress.

I don't intend them to be permanent. They'll all go as #1163 gets finished.

[Oleksandr, 2014/10/02 20:44:16]

ok, as long as you'll be able to keep track of them :)

On 2014/09/29 19:16:44, Eric wrote:

[Eric, 2014/10/02 23:11:02]

No problem. That's what grep is for.

 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
 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)
+  // Entry point exception handler

[Oleksandr, 2014/08/21 14:49:56]

Self-evident, IMHO

[Eric, 2014/09/29 19:16:44]

I mentioned this in another review, but I'd like to have some way of using
'grep' to find all the entry points. Suggestions welcome.

[Eric, 2014/10/02 23:11:02]

Per a suggestion in another review, I'm changing this simply to mark this
function as an entry point.

+  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(...)
+  {

[Oleksandr, 2014/08/21 14:49:56]

Maybe let's log at least something first

[Eric, 2014/09/29 19:16:44]

We could. Got a suggestion? Right now we've got no general-purpose log message
for unknown exceptions. There's not a lot to say since we don't have an
exception object. Is just the function name sufficient?

[Oleksandr, 2014/10/02 20:44:16]

Yes, I think it would be sufficient. We could in theory also log thread and
process IDs, but I don't remember that ever being useful before for me, so we
can just skip it.

On 2014/09/29 19:16:44, Eric wrote:

[Eric, 2014/10/02 23:11:02]

OK. I just used DEBUG_GENERAL, lacking anything specifically for this case yet.

+    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 1210 matching lines @@
         }
       }
     }
 
     hTabWnd = ::GetWindow(hTabWnd, GW_HWNDNEXT);
   }
 
   return hTabWnd;
 
 }