From bc88e9d7be9dbc039071cef0c7f80b9b1017f804 Mon Sep 17 00:00:00 2001 From: "ananta@chromium.org" Date: Fri, 25 Sep 2009 16:04:43 +0000 Subject: This fixes a crash in IE8 with ChromeFrame when a new tab was created. ChromeFrame VTable patches the IInternetProtocol interface for the CLSID_HttpProtocol and CLSID_HttpSProtocol handlers. However we were using the same VTable information to patch both the handlers essentially overwriting the first one. While this all worked purely by chance, it exposed a bug in IE8 where every new tab initially goes into a new process and if the chromeframe is unloaded we would leave behind an IInternetProtocol interface in urlmon patched, which would crash when dereferenced. Added a check in the VTable patching code for this case. This fixes bug http://code.google.com/p/chromium/issues/detail?id=22768 Bug=22768 Review URL: http://codereview.chromium.org/244002 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@27191 0039d316-1c4b-4281-b951-d872f2087c98 --- chrome_frame/protocol_sink_wrap.cc | 104 ++++++++++++++++++++++++++++++------- 1 file changed, 85 insertions(+), 19 deletions(-) (limited to 'chrome_frame/protocol_sink_wrap.cc') diff --git a/chrome_frame/protocol_sink_wrap.cc b/chrome_frame/protocol_sink_wrap.cc index a567e9f..5559e61 100644 --- a/chrome_frame/protocol_sink_wrap.cc +++ b/chrome_frame/protocol_sink_wrap.cc @@ -14,9 +14,8 @@ #include "base/string_util.h" #include "chrome_frame/utils.h" -#include "chrome_frame/vtable_patch_manager.h" -// BINDSTATUS_SERVER_MIMETYPEAVAILABLE == 54. Introduced in IE 8, so +// BINDSTATUS_SERVER_MIMETYPEAVAILABLE == 54. Introduced in IE 8, so // not in everyone's headers yet. See: // http://msdn.microsoft.com/en-us/library/ms775133(VS.85,loband).aspx #ifndef BINDSTATUS_SERVER_MIMETYPEAVAILABLE @@ -25,22 +24,36 @@ static const wchar_t* kChromeMimeType = L"application/chromepage"; static const char kTextHtmlMimeType[] = "text/html"; +const wchar_t kUrlMonDllName[] = L"urlmon.dll"; static const int kInternetProtocolStartIndex = 3; static const int kInternetProtocolReadIndex = 9; static const int kInternetProtocolStartExIndex = 13; +// TODO(ananta) +// We should avoid duplicate VTable declarations. BEGIN_VTABLE_PATCHES(IInternetProtocol) VTABLE_PATCH_ENTRY(kInternetProtocolStartIndex, ProtocolSinkWrap::OnStart) VTABLE_PATCH_ENTRY(kInternetProtocolReadIndex, ProtocolSinkWrap::OnRead) END_VTABLE_PATCHES() +BEGIN_VTABLE_PATCHES(IInternetProtocolSecure) + VTABLE_PATCH_ENTRY(kInternetProtocolStartIndex, ProtocolSinkWrap::OnStart) + VTABLE_PATCH_ENTRY(kInternetProtocolReadIndex, ProtocolSinkWrap::OnRead) +END_VTABLE_PATCHES() + BEGIN_VTABLE_PATCHES(IInternetProtocolEx) VTABLE_PATCH_ENTRY(kInternetProtocolStartIndex, ProtocolSinkWrap::OnStart) VTABLE_PATCH_ENTRY(kInternetProtocolReadIndex, ProtocolSinkWrap::OnRead) VTABLE_PATCH_ENTRY(kInternetProtocolStartExIndex, ProtocolSinkWrap::OnStartEx) END_VTABLE_PATCHES() +BEGIN_VTABLE_PATCHES(IInternetProtocolExSecure) + VTABLE_PATCH_ENTRY(kInternetProtocolStartIndex, ProtocolSinkWrap::OnStart) + VTABLE_PATCH_ENTRY(kInternetProtocolReadIndex, ProtocolSinkWrap::OnRead) + VTABLE_PATCH_ENTRY(kInternetProtocolStartExIndex, ProtocolSinkWrap::OnStartEx) +END_VTABLE_PATCHES() + // // ProtocolSinkWrap implementation // @@ -64,12 +77,45 @@ ProtocolSinkWrap::~ProtocolSinkWrap() { DLOG(INFO) << "ProtocolSinkWrap: active sinks: " << sink_map_.size(); } -bool ProtocolSinkWrap::PatchProtocolHandler(const wchar_t* dll, - const CLSID& handler_clsid) { - HMODULE module = ::GetModuleHandle(dll); +bool ProtocolSinkWrap::PatchProtocolHandlers() { + HRESULT hr = PatchProtocolMethods(CLSID_HttpProtocol, + IInternetProtocol_PatchInfo, + IInternetProtocolEx_PatchInfo); + if (FAILED(hr)) { + NOTREACHED() << "Failed to patch IInternetProtocol interface." + << " Error: " << hr; + return false; + } + + hr = PatchProtocolMethods(CLSID_HttpSProtocol, + IInternetProtocolSecure_PatchInfo, + IInternetProtocolExSecure_PatchInfo); + if (FAILED(hr)) { + NOTREACHED() << "Failed to patch IInternetProtocol secure interface." + << " Error: " << hr; + return false; + } + + return true; +} + +void ProtocolSinkWrap::UnpatchProtocolHandlers() { + vtable_patch::UnpatchInterfaceMethods(IInternetProtocol_PatchInfo); + vtable_patch::UnpatchInterfaceMethods(IInternetProtocolEx_PatchInfo); + vtable_patch::UnpatchInterfaceMethods(IInternetProtocolSecure_PatchInfo); + vtable_patch::UnpatchInterfaceMethods(IInternetProtocolExSecure_PatchInfo); +} + +HRESULT ProtocolSinkWrap::CreateProtocolHandlerInstance( + const CLSID& clsid, IInternetProtocol** protocol) { + if (!protocol) { + return E_INVALIDARG; + } + + HMODULE module = ::GetModuleHandle(kUrlMonDllName); if (!module) { NOTREACHED() << "urlmon is not yet loaded. Error: " << GetLastError(); - return false; + return E_FAIL; } typedef HRESULT (WINAPI* DllGetClassObject_Fn)(REFCLSID, REFIID, LPVOID*); @@ -77,15 +123,15 @@ bool ProtocolSinkWrap::PatchProtocolHandler(const wchar_t* dll, ::GetProcAddress(module, "DllGetClassObject")); if (!fn) { NOTREACHED() << "DllGetClassObject not found in urlmon.dll"; - return false; + return E_FAIL; } ScopedComPtr protocol_class_factory; - HRESULT hr = fn(handler_clsid, IID_IClassFactory, + HRESULT hr = fn(clsid, IID_IClassFactory, reinterpret_cast(protocol_class_factory.Receive())); if (FAILED(hr)) { NOTREACHED() << "DllGetclassObject failed. Error: " << hr; - return false; + return hr; } ScopedComPtr handler_instance; @@ -94,19 +140,39 @@ bool ProtocolSinkWrap::PatchProtocolHandler(const wchar_t* dll, if (FAILED(hr)) { NOTREACHED() << "ClassFactory::CreateInstance failed for InternetProtocol." << " Error: " << hr; + } else { + *protocol = handler_instance.Detach(); + } + + return hr; +} + +HRESULT ProtocolSinkWrap::PatchProtocolMethods( + const CLSID& clsid_protocol, + vtable_patch::MethodPatchInfo* protocol_patch_info, + vtable_patch::MethodPatchInfo* protocol_ex_patch_info) { + if (!protocol_patch_info || !protocol_ex_patch_info) { + return E_INVALIDARG; + } + + ScopedComPtr http_protocol; + HRESULT hr = CreateProtocolHandlerInstance(clsid_protocol, + http_protocol.Receive()); + if (FAILED(hr)) { + NOTREACHED() << "ClassFactory::CreateInstance failed for InternetProtocol." + << " Error: " << hr; return false; } ScopedComPtr ipex; - ipex.QueryFrom(handler_instance); + ipex.QueryFrom(http_protocol); if (ipex) { - vtable_patch::PatchInterfaceMethods(ipex, IInternetProtocolEx_PatchInfo); + hr = vtable_patch::PatchInterfaceMethods(ipex, protocol_ex_patch_info); } else { - vtable_patch::PatchInterfaceMethods(handler_instance, - IInternetProtocol_PatchInfo); + hr = vtable_patch::PatchInterfaceMethods(http_protocol, + protocol_patch_info); } - - return true; + return hr; } // IInternetProtocol/Ex method implementation. @@ -264,10 +330,10 @@ STDMETHODIMP ProtocolSinkWrap::ReportData(DWORD flags, ULONG progress, // determination logic. // NOTE: We use the report_data_recursiveness_ variable to detect situations - // in which calls to ReportData are re-entrant (such as when the entire - // contents of a page fit inside a single packet). In these cases, we - // don't care about re-entrant calls beyond the second, and so we compare - // report_data_recursiveness_ inside the while loop, making sure we skip + // in which calls to ReportData are re-entrant (such as when the entire + // contents of a page fit inside a single packet). In these cases, we + // don't care about re-entrant calls beyond the second, and so we compare + // report_data_recursiveness_ inside the while loop, making sure we skip // what would otherwise be spurious calls to ReportProgress(). report_data_recursiveness_++; -- cgit v1.1