Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Unified Diff: src/plugin/PluginClass.cpp

Issue 6495401389588480: Issue #1173 - add exception handler to CPluginClass::Invoke (Closed)
Patch Set: Created Aug. 6, 2014, 9:32 p.m.
Use n/p to move between diff chunks; N/P to move between comments.
Jump to:
View side-by-side diff with in-line comments
Download patch
« no previous file with comments | « no previous file | no next file » | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Index: src/plugin/PluginClass.cpp
===================================================================
--- a/src/plugin/PluginClass.cpp
+++ b/src/plugin/PluginClass.cpp
@@ -394,6 +394,7 @@
}
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
Eric 2014/09/29 19:16:44 Well, I don't know what the issue would be yet; I
Oleksandr 2014/10/02 20:44:16 How about TODO: then? On 2014/09/29 19:16:44, Eri
Eric 2014/10/02 23:11:02 OK.
CPluginClientFactory::ReleaseMimeFilterClientInstance();
}
}
@@ -527,6 +528,11 @@
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 #116
Eric 2014/09/29 19:16:44 These comments are intended to exist only while #1
Oleksandr 2014/10/02 20:44:16 ok, as long as you'll be able to keep track of the
Eric 2014/10/02 23:11:02 No problem. That's what grep is for.
void CPluginClass::BeforeNavigate2(DISPPARAMS* pDispParams)
{
@@ -595,6 +601,13 @@
#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");
@@ -605,163 +618,168 @@
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 t
Eric 2014/10/02 23:11:02 Per a suggestion in another review, I'm changing t
+ 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
Oleksandr 2014/10/02 20:44:16 Yes, I think it would be sufficient. We could in t
Eric 2014/10/02 23:11:02 OK. I just used DEBUG_GENERAL, lacking anything sp
+ return E_FAIL;
+ }
+ return S_OK;
}
bool CPluginClass::InitObject(bool bBHO)
« no previous file with comments | « no previous file | no next file » | no next file with comments »

Powered by Google App Engine
This is Rietveld