Issue 1619 - Wrong return value of CPluginClass::Invoke

src/plugin/PluginClass.cpp

Index: src/plugin/PluginClass.cpp
===================================================================
--- a/src/plugin/PluginClass.cpp
+++ b/src/plugin/PluginClass.cpp
@@ -588,7 +588,7 @@
     m_tab->CacheFrame(url);
   }
 }
-STDMETHODIMP CPluginClass::OnTabChanged(DISPPARAMS* pDispParams, WORD wFlags)
+void CPluginClass::OnTabChanged(DISPPARAMS* pDispParams, WORD wFlags)
 {
   DEBUG_GENERAL("Tab changed");
   bool newtabshown = pDispParams->rgvarg[1].intVal==3;
@@ -598,8 +598,6 @@
     if (it == s_threadInstances.end())
     {
       s_threadInstances[::GetCurrentThreadId()] = this;
-
-
       if (!m_isInitializedOk)
       {
         m_isInitializedOk = true;
@@ -613,7 +611,6 @@
   }
   notificationMessage.Hide();
   DEBUG_GENERAL("Tab change end");
-  return VARIANT_TRUE;
 }
 
 // This gets called whenever there's a browser event
@@ -624,48 +621,9 @@
   DEBUG_GENERAL(tmp);
   switch (dispidMember)
   {
-
   case DISPID_WINDOWSTATECHANGED:
-    return OnTabChanged(pDispParams, wFlags);
-    break;
-  case DISPID_HTMLDOCUMENTEVENTS2_ONBEFOREUPDATE:
-    return VARIANT_TRUE;
-    break;
-
-  case DISPID_HTMLDOCUMENTEVENTS2_ONCLICK:
-    return VARIANT_TRUE;
-    break;
-
-  case DISPID_EVMETH_ONLOAD:
-    DEBUG_NAVI("Navi::OnLoad")
-      return VARIANT_TRUE;
-    break;
-
-  case DISPID_EVMETH_ONCHANGE:
-    return VARIANT_TRUE;
-
-  case DISPID_EVMETH_ONMOUSEDOWN:
-    return VARIANT_TRUE;
-
-  case DISPID_EVMETH_ONMOUSEENTER:
-    return VARIANT_TRUE;
-
-  case DISPID_IHTMLIMGELEMENT_START:
-    return VARIANT_TRUE;
-
-  case STDDISPID_XOBJ_ERRORUPDATE:
-    return VARIANT_TRUE;
-
-  case STDDISPID_XOBJ_ONPROPERTYCHANGE:
-    return VARIANT_TRUE;
-
-  case DISPID_READYSTATECHANGE:
-    DEBUG_NAVI("Navi::ReadyStateChange")
-      return VARIANT_TRUE;
-
-  case DISPID_BEFORENAVIGATE:
-    DEBUG_NAVI("Navi::BeforeNavigate")
-      return VARIANT_TRUE;
+    OnTabChanged(pDispParams, wFlags);
+    return S_OK;
   case DISPID_COMMANDSTATECHANGE:
     if (m_hPaneWnd == NULL)
     {
@@ -681,80 +639,58 @@
         {
           MoveWindow(m_hPaneWnd, rect.right - 200, 0, m_nPaneWidth, rect.bottom - rect.top, TRUE);
         }
-      }      
+      }
     }
-    break;
-  case DISPID_STATUSTEXTCHANGE:
-    break;
-
+    return S_OK;
   case DISPID_BEFORENAVIGATE2:
     BeforeNavigate2(pDispParams);
-    break;
-
-  case DISPID_DOWNLOADBEGIN:
-    {
-      DEBUG_NAVI("Navi::Download Begin")
-    }
-    break;
-
+    return S_OK;
   case DISPID_DOWNLOADCOMPLETE:
     {
       DEBUG_NAVI("Navi::Download Complete")
-
-        CComQIPtr<IWebBrowser2> browser = GetBrowser();
+      ATL::CComQIPtr<IWebBrowser2> browser = GetBrowser();
       if (browser)
       {
         m_tab->OnDownloadComplete(browser);
       }
     }
-    break;
-
+    return S_OK;
   case DISPID_DOCUMENTCOMPLETE:
     {
       DEBUG_NAVI("Navi::Document Complete")
-
-        CComQIPtr<IWebBrowser2> browser = GetBrowser();
-
+      ATL::CComPtr<IWebBrowser2> browser = GetBrowser();

[Eric, 2014/12/30 14:37:50]

We could declare this as IWebBrowser2* with one small modification below.

[sergei, 2015/01/13 13:24:21]

I find it as a bad practice, `GetBrowser()` returns smart pointer, if we store
merely raw pointer the smart pointer object is destroyed. It works here but it
smells a lot.

       if (browser && pDispParams->cArgs >= 2 && pDispParams->rgvarg[1].vt == VT_DISPATCH)
       {
-        CComQIPtr<IWebBrowser2> pBrowser = pDispParams->rgvarg[1].pdispVal;
+        ATL::CComQIPtr<IWebBrowser2> pBrowser = pDispParams->rgvarg[1].pdispVal;

[Eric, 2014/12/30 14:37:50]

We can't avoid a QI call to initialize pBrowser2 because pdispVal is IDispatch*.

         if (pBrowser)
         {
           CString url;
-          CComBSTR bstrUrl;
+          ATL::CComBSTR bstrUrl;
           if (SUCCEEDED(pBrowser->get_LocationURL(&bstrUrl)) && ::SysStringLen(bstrUrl) > 0)
           {
             url = bstrUrl;
-
             CPluginClient::UnescapeUrl(url);
-
             m_tab->OnDocumentComplete(browser, url, browser.IsEqualObject(pBrowser));

[Eric, 2014/12/30 14:37:50]

Change
    browser.IsEqualObject(pBrowser)
to
    pBrowser.IsEqualObject(browser)

           }
         }
       }
     }
-    break;
+    return S_OK;
 
   case DISPID_ONQUIT:
   case DISPID_QUIT:
     {
       Unadvice();
     }
-    break;
-
+    return S_OK;
   default:
     {
-      CString did;
+      ATL::CString did;
       did.Format(L"DispId:%u", dispidMember);
-
       DEBUG_NAVI(L"Navi::Default " + did)
     }
-
-    // do nothing
-    break;
   }
-
-  return VARIANT_TRUE;
+  return DISP_E_MEMBERNOTFOUND;

[Eric, 2014/12/30 14:37:50]

Does this work on all supported versions of IE?

This return value, in addition to the removal of the trivial case statements
above, changes the semantics from the events we don't care about from "no
operation", which will never be an error, to "method not found", which might be
interpreted differently.

 }
 
 bool CPluginClass::InitObject(bool bBHO)