Second attempt at trying to land buggy bho avoidance: http://codereview.chromium.org/3031009

A different approach to avoid crashes in buggy 3rd party BHOs.This time we're more preceise and only target the buggy components.Behaviour for components that handle browser events correctly, is unchanged even in the presence of buggy DLLs.The core class here is the BuggyBhoTls class and here's the comment for that class:

// Construct an instance of this class on the stack when firing web browser
// events that can be sent to buggy BHOs. This class will intercept those
// BHOs (see list in cc file) and ignore notifications to those components
// for as long as the BuggyBhoTls instance on the stack lives.

TEST=A better fix for bug 44463 but also fixes bug 49373 and a number of crashes that haven't been associated with bug reports yet.
BUG=44463, 49373

Review URL: http://codereview.chromium.org/3053008

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@53236 0039d316-1c4b-4281-b951-d872f2087c98

8 files changed, 308 insertions, 69 deletions

diff --git a/chrome_frame/bind_context_info.cc b/chrome_frame/bind_context_info.cc
index 1f57573..b1f9565 100644
--- a/chrome_frame/bind_context_info.cc
+++ b/chrome_frame/bind_context_info.cc
@@ -47,9 +47,10 @@ HRESULT BindContextInfo::FromBindContext(IBindCtx* bind_context,
     hr = internal.QueryFrom(context);
     DCHECK(SUCCEEDED(hr));
     if (SUCCEEDED(hr)) {
-      BindContextInfo* ret = NULL;
       hr = internal->GetCppObject(reinterpret_cast<void**>(info));
       DCHECK_EQ(hr, S_OK);
+      DLOG_IF(ERROR, *info != static_cast<BindContextInfo*>(internal.get()))
+          << "marshalling took place!";
     }
   } else {
     DCHECK(FAILED(hr));
diff --git a/chrome_frame/buggy_bho_handling.cc b/chrome_frame/buggy_bho_handling.cc
new file mode 100644
index 0000000..8067ec5
--- /dev/null
+++ b/chrome_frame/buggy_bho_handling.cc
@@ -0,0 +1,200 @@
+// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome_frame/buggy_bho_handling.h"
+
+#include "base/logging.h"
+#include "base/scoped_comptr_win.h"
+
+#include "chrome_frame/function_stub.h"
+#include "chrome_frame/utils.h"
+#include "chrome_frame/vtable_patch_manager.h"
+
+namespace buggy_bho {
+
+base::ThreadLocalPointer<BuggyBhoTls> BuggyBhoTls::s_bad_object_tls_;
+
+struct ModuleAndVersion {
+  const char* module_name_;
+  const uint32 major_version_;
+  const uint32 minor_version_;
+};
+
+const ModuleAndVersion kBuggyModules[] = {
+  { "askbar.dll", 4, 1 },  // troublemaker: 4.1.0.5.
+  { "gbieh.dll", 3, 8 },  // troublemaker: 3.8.14.12
+  { "gbiehcef.dll", 3, 8 },  // troublemaker: 3.8.11.23
+  { "alot.dll", 2, 5 },  // troublemaker: 2.5.12000.509
+  { "srchbxex.dll", 1, 2 },  // troublemaker: 1.2.123.0
+  { "tbabso.dll", 4, 5 },  // troublemaker: 4.5.156.0
+  { "tbabs0.dll.dll", 4, 5 },  // troublemaker: 4.5.156.0
+  { "tbbes0.dll", 4, 5 },  // troublemaker: 4.5.153.0
+  { "ctbr.dll", 5, 1 },  // troublemaker: 5.1.0.95
+
+  // Viruses?
+  { "msgsc2.dll", 0xffff, 0xffff },  // troublemaker: 1.2.3000.1
+  { "essclis.dll", 0xffff, 0xffff },  // troublemaker: 4.3.1800.2
+};
+
+bool IsBuggyBho(HMODULE mod) {
+  DCHECK(mod);
+
+  char path[MAX_PATH * 2] = {0};
+  ::GetModuleFileNameA(mod, path, arraysize(path));
+  const char* file_name = ::PathFindFileNameA(path);
+  for (size_t i = 0; i < arraysize(kBuggyModules); ++i) {
+    if (lstrcmpiA(file_name, kBuggyModules[i].module_name_) == 0) {
+      uint32 version = 0;
+      GetModuleVersion(mod, &version, NULL);
+      const ModuleAndVersion& buggy = kBuggyModules[i];
+      if (HIWORD(version) < buggy.major_version_ ||
+          (HIWORD(version) == buggy.major_version_ &&
+           LOWORD(version) <= buggy.minor_version_)) {
+        return true;
+      }
+    }
+  }
+
+  return false;
+}
+
+BuggyBhoTls::BuggyBhoTls() : previous_instance_(s_bad_object_tls_.Get()) {
+  s_bad_object_tls_.Set(this);
+}
+
+BuggyBhoTls::~BuggyBhoTls() {
+  DCHECK(FromCurrentThread() == this);
+  s_bad_object_tls_.Set(previous_instance_);
+}
+
+void BuggyBhoTls::AddBuggyObject(IDispatch* obj) {
+  bad_objects_.push_back(obj);
+}
+
+bool BuggyBhoTls::IsBuggyObject(IDispatch* obj) const {
+  return std::find(bad_objects_.begin(), bad_objects_.end(), obj) !=
+         bad_objects_.end();
+}
+
+// static
+BuggyBhoTls* BuggyBhoTls::FromCurrentThread() {
+  return s_bad_object_tls_.Get();
+}
+
+// static
+STDMETHODIMP BuggyBhoTls::BuggyBhoInvoke(InvokeFunc original, IDispatch* me,
+                                         DISPID dispid, REFIID riid, LCID lcid,
+                                         WORD flags, DISPPARAMS* params,
+                                         VARIANT* result, EXCEPINFO* ei,
+                                         UINT* err) {
+  DLOG(INFO) << __FUNCTION__;
+
+  const BuggyBhoTls* tls = BuggyBhoTls::FromCurrentThread();
+  if (tls && tls->IsBuggyObject(me)) {
+    // Ignore this call and avoid the bug.
+    // TODO(tommi): Maybe we should check a specific list of DISPIDs too?
+    return S_OK;
+  }
+
+  return original(me, dispid, riid, lcid, flags, params, result, ei, err);
+}
+
+// static
+HRESULT BuggyBhoTls::PatchInvokeMethod(PROC* invoke) {
+  CCritSecLock lock(_pAtlModule->m_csStaticDataInitAndTypeInfo.m_sec, true);
+
+  FunctionStub* stub = FunctionStub::FromCode(*invoke);
+  if (stub)
+    return S_FALSE;
+
+  DWORD flags = 0;
+  if (!::VirtualProtect(invoke, sizeof(PROC), PAGE_EXECUTE_READWRITE, &flags))
+    return AtlHresultFromLastError();
+
+  HRESULT hr = S_OK;
+
+  stub = FunctionStub::Create(reinterpret_cast<uintptr_t>(*invoke),
+                              BuggyBhoInvoke);
+  if (!stub) {
+    hr = E_OUTOFMEMORY;
+  } else {
+    if (!vtable_patch::internal::ReplaceFunctionPointer(
+            reinterpret_cast<void**>(invoke), stub->code(),
+            reinterpret_cast<void*>(stub->argument()))) {
+      hr = E_UNEXPECTED;
+      FunctionStub::Destroy(stub);
+    } else {
+      ::FlushInstructionCache(::GetCurrentProcess(), invoke, sizeof(PROC));
+    }
+  }
+
+  ::VirtualProtect(invoke, sizeof(PROC), flags, &flags);
+
+  return hr;
+}
+
+// static
+bool BuggyBhoTls::PatchIfBuggy(CONNECTDATA* cd, const IID& diid) {
+  DCHECK(cd);
+  PROC* methods = *reinterpret_cast<PROC**>(cd->pUnk);
+  HMODULE mod = GetModuleFromAddress(methods[0]);
+  if (!IsBuggyBho(mod))
+    return false;
+
+  ScopedComPtr<IDispatch> disp;
+  HRESULT hr = cd->pUnk->QueryInterface(diid,
+      reinterpret_cast<void**>(disp.Receive()));
+  if (FAILED(hr))  // Sometimes only IDispatch QI is supported
+    hr = disp.QueryFrom(cd->pUnk);
+  DCHECK(SUCCEEDED(hr));
+
+  if (SUCCEEDED(hr)) {
+    const int kInvokeIndex = 6;
+    DCHECK(static_cast<IUnknown*>(disp) == cd->pUnk);
+    if (SUCCEEDED(PatchInvokeMethod(&methods[kInvokeIndex]))) {
+      BuggyBhoTls* tls = BuggyBhoTls::FromCurrentThread();
+      DCHECK(tls);
+      if (tls) {
+        tls->AddBuggyObject(disp);
+      }
+    }
+  }
+
+  return false;
+}
+
+// static
+HRESULT BuggyBhoTls::PatchBuggyBHOs(IWebBrowser2* browser) {
+  DCHECK(browser);
+  DCHECK(BuggyBhoTls::FromCurrentThread())
+      << "You must first have an instance of BuggyBhoTls on this thread";
+
+  ScopedComPtr<IConnectionPointContainer> cpc;
+  HRESULT hr = cpc.QueryFrom(browser);
+  if (SUCCEEDED(hr)) {
+    const GUID sinks[] = { DIID_DWebBrowserEvents2, DIID_DWebBrowserEvents };
+    for (size_t i = 0; i < arraysize(sinks); ++i) {
+      ScopedComPtr<IConnectionPoint> cp;
+      cpc->FindConnectionPoint(sinks[i], cp.Receive());
+      if (cp) {
+        ScopedComPtr<IEnumConnections> connections;
+        cp->EnumConnections(connections.Receive());
+        if (connections) {
+          CONNECTDATA cd = {0};
+          DWORD fetched = 0;
+          while (connections->Next(1, &cd, &fetched) == S_OK && fetched) {
+            PatchIfBuggy(&cd, sinks[i]);
+            cd.pUnk->Release();
+            fetched = 0;
+          }
+        }
+      }
+    }
+  }
+
+  return hr;
+}
+
+}  // end namespace buggy_bho
+
diff --git a/chrome_frame/buggy_bho_handling.h b/chrome_frame/buggy_bho_handling.h
new file mode 100644
index 0000000..8ed2d2c
--- /dev/null
+++ b/chrome_frame/buggy_bho_handling.h
@@ -0,0 +1,87 @@
+// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_FRAME_BUGGY_BHO_HANDLING_H_
+#define CHROME_FRAME_BUGGY_BHO_HANDLING_H_
+
+#include <atlbase.h>
+#include <atlcom.h>
+#include <exdisp.h>
+
+#include <vector>
+
+#include "base/thread_local.h"
+
+namespace buggy_bho {
+
+// Method prototype for IDispatch::Invoke.
+typedef HRESULT (__stdcall* InvokeFunc)(IDispatch* me, DISPID dispid,
+                                        REFIID riid, LCID lcid, WORD flags,
+                                        DISPPARAMS* params, VARIANT* result,
+                                        EXCEPINFO* ei, UINT* err);
+
+// Construct an instance of this class on the stack when firing web browser
+// events that can be sent to buggy BHOs.  This class will intercept those
+// BHOs (see list in cc file) and ignore notifications to those components
+// for as long as the BuggyBhoTls instance on the stack lives.
+class BuggyBhoTls {
+ public:
+  BuggyBhoTls();
+  ~BuggyBhoTls();
+
+  // Call after instantiating an instance of BuggyBhoTls.  This method traverses
+  // the list of DWebBrowserEvents and DWebBrowserEvents2 subscribers and checks
+  // if any of the sinks belong to a list of known-to-be-buggy BHOs.
+  // For each of those, a patch will be applied that temporarily ignores certain
+  // invokes.
+  static HRESULT PatchBuggyBHOs(IWebBrowser2* browser);
+
+ protected:
+  // internal implementation:
+
+  // Called when a buggy instance is found to be subscribing to browser events.
+  void AddBuggyObject(IDispatch* obj);
+
+  // Called from our patch to check if calls for this object should be ignored.
+  // The reason we do this check is because there might be more than one browser
+  // object running on the same thread (e.g. IE6) with one running CF and the
+  // other MSHTML.  We don't want to drop events being fired by MSHTML, only
+  // events fired by CF since these BHOs should handle MSHTML correctly.
+  bool IsBuggyObject(IDispatch* obj) const;
+
+  // Static, protected member methods
+
+  // Returns the currently registered (TLS) BuggyBhoTls instance or NULL.
+  static BuggyBhoTls* FromCurrentThread();
+
+  // Patches a subscriber if it belongs to a buggy dll.
+  static bool PatchIfBuggy(CONNECTDATA* cd, const IID& diid);
+
+  // Patches the IDispatch::Invoke method.
+  static HRESULT PatchInvokeMethod(PROC* invoke);
+
+  // This is our substitute function that is called instead of the buggy DLLs.
+  // Here we call IsBuggyObject to check if we should ignore invokes or allow
+  // them to go through.
+  static STDMETHODIMP BuggyBhoInvoke(InvokeFunc original, IDispatch* me,
+      DISPID dispid, REFIID riid, LCID lcid, WORD flags, DISPPARAMS* params,
+      VARIANT* result, EXCEPINFO* ei, UINT* err);
+
+ protected:
+  // List of buggy subscribers.
+  std::vector<IDispatch*> bad_objects_;
+
+  // Pointer to a previous instance of BuggyBhoTls on this thread if any.
+  // Under regular circumstances, this will be NULL.  However, there's a chance
+  // that we could get reentrant calls, hence we maintain a stack.
+  BuggyBhoTls* previous_instance_;
+
+  // Where we store the current thread's instance.
+  static base::ThreadLocalPointer<BuggyBhoTls> s_bad_object_tls_;
+};
+
+}  // end namespace buggy_bho
+
+#endif  // CHROME_FRAME_BUGGY_BHO_HANDLING_H_
+
diff --git a/chrome_frame/chrome_active_document.cc b/chrome_frame/chrome_active_document.cc
index 66ee3c0..79dc112 100644
--- a/chrome_frame/chrome_active_document.cc
+++ b/chrome_frame/chrome_active_document.cc
@@ -18,7 +18,6 @@
 
 #include "base/command_line.h"
 #include "base/file_util.h"
-#include "base/file_version_info.h"
 #include "base/logging.h"
 #include "base/path_service.h"
 #include "base/process_util.h"
@@ -39,6 +38,7 @@
 #include "chrome/test/automation/tab_proxy.h"
 #include "chrome_frame/bho.h"
 #include "chrome_frame/bind_context_info.h"
+#include "chrome_frame/buggy_bho_handling.h"
 #include "chrome_frame/crash_reporting/crash_metrics.h"
 #include "chrome_frame/utils.h"
 
@@ -50,7 +50,6 @@ static const wchar_t kHandleTopLevelRequests[] = L"HandleTopLevelRequests";
 DEFINE_GUID(CGID_DocHostCmdPriv, 0x000214D4L, 0, 0, 0xC0, 0, 0, 0, 0, 0, 0,
             0x46);
 
-
 base::ThreadLocalPointer<ChromeActiveDocument> g_active_doc_cache;
 
 bool g_first_launch_by_process_ = true;
@@ -724,6 +723,15 @@ void ChromeActiveDocument::UpdateNavigationState(
     ScopedComPtr<IDocObjectService> doc_object_svc;
     ScopedComPtr<IWebBrowserEventsService> web_browser_events_svc;
 
+    buggy_bho::BuggyBhoTls bad_bho_tls;
+    if (GetConfigBool(true, kEnableBuggyBhoIntercept)) {
+      ScopedComPtr<IWebBrowser2> wb2;
+      DoQueryService(SID_SWebBrowserApp, m_spClientSite, wb2.Receive());
+      if (wb2) {
+        buggy_bho::BuggyBhoTls::PatchBuggyBHOs(wb2);
+      }
+    }
+
     DoQueryService(__uuidof(web_browser_events_svc), m_spClientSite,
                    web_browser_events_svc.Receive());
 
@@ -764,12 +772,10 @@ void ChromeActiveDocument::UpdateNavigationState(
       // Now call the FireNavigateCompleteEvent which makes IE update the text
       // in the address-bar.
       doc_object_svc->FireNavigateComplete2(this, 0);
-      if (ShouldFireDocumentComplete())
-        doc_object_svc->FireDocumentComplete(this, 0);
+      doc_object_svc->FireDocumentComplete(this, 0);
     } else if (web_browser_events_svc) {
       web_browser_events_svc->FireNavigateComplete2Event();
-      if (ShouldFireDocumentComplete())
-        web_browser_events_svc->FireDocumentCompleteEvent();
+      web_browser_events_svc->FireDocumentCompleteEvent();
     }
   }
 
@@ -1278,63 +1284,3 @@ LRESULT ChromeActiveDocument::OnSetFocus(UINT message, WPARAM wparam,
   GiveFocusToChrome(false);
   return 0;
 }
-
-namespace {
-struct ModuleAndVersion {
-  const char* module_name_;
-  const uint32 major_version_;
-  const uint32 minor_version_;
-};
-}  // end namespace
-
-// static
-bool ChromeActiveDocument::ShouldFireDocumentComplete() {
-  typedef enum ModuleCheckResult {
-    CHECK_NOT_DONE,
-    DOCUMENT_COMPLETE_OK,
-    DOCUMENT_COMPLETE_NOT_OK
-  };
-
-  static ModuleCheckResult results = CHECK_NOT_DONE;
-
-  if (results == CHECK_NOT_DONE) {
-    // These modules are missing some checks in their DocumentComplete
-    // implementation that causes a crash.
-    static const ModuleAndVersion buggy_modules[] = {
-      { "askbar.dll", 4, 1 },  // biggest troublemaker: 4.1.0.5.
-      { "gbieh.dll", 3, 8 },  // biggest troublemaker: 3.8.14.12
-      { "gbiehcef.dll", 3, 8 },  // biggest troublemaker: 3.8.11.23
-      { "gbiehUni.dll", 3, 8 },  // Another Banco DLL.
-    };
-
-    for (size_t i = 0; results == CHECK_NOT_DONE &&
-                       i < arraysize(buggy_modules); ++i) {
-      const ModuleAndVersion& module = buggy_modules[i];
-      HMODULE mod = ::GetModuleHandleA(module.module_name_);
-      if (mod) {
-        wchar_t path[MAX_PATH * 2] = {0};
-        ::GetModuleFileNameW(mod, path, arraysize(path));
-        scoped_ptr<FileVersionInfo> version_info(
-            FileVersionInfo::CreateFileVersionInfo(FilePath(path)));
-        DCHECK(version_info.get());
-        if (version_info.get()) {
-          uint32 major = 0, minor = 0;
-          if (!ParseVersion(version_info->file_version(), &major, &minor))
-            ParseVersion(version_info->product_version(), &major, &minor);
-          if (major < module.major_version_ ||
-              (major == module.major_version_ &&
-               minor <= module.minor_version_)) {
-            DLOG(WARNING) << "Buggy module found: " << module.module_name_;
-            results = DOCUMENT_COMPLETE_NOT_OK;
-          }
-        }
-      }
-    }
-
-    if (results == CHECK_NOT_DONE)
-      results = DOCUMENT_COMPLETE_OK;
-  }
-
-  return results == DOCUMENT_COMPLETE_OK;
-}
-
diff --git a/chrome_frame/chrome_frame.gyp b/chrome_frame/chrome_frame.gyp
index a0362ca..341616a 100644
--- a/chrome_frame/chrome_frame.gyp
+++ b/chrome_frame/chrome_frame.gyp
@@ -606,6 +606,8 @@
         'bind_context_info.h',
         'bind_status_callback_impl.cc',
         'bind_status_callback_impl.h',
+        'buggy_bho_handling.cc',
+        'buggy_bho_handling.h',
         'chrome_active_document.cc',
         'chrome_active_document.h',
         'chrome_active_document.rgs',
diff --git a/chrome_frame/utils.cc b/chrome_frame/utils.cc
index 6a9d0a2..b65570d 100644
--- a/chrome_frame/utils.cc
+++ b/chrome_frame/utils.cc
@@ -48,6 +48,7 @@ static const wchar_t kChromeFrameConfigKey[] =
     L"Software\\Google\\ChromeFrame";
 static const wchar_t kChromeFrameOptinUrlsKey[] = L"OptinUrls";
 static const wchar_t kEnableGCFProtocol[] = L"EnableGCFProtocol";
+static const wchar_t kEnableBuggyBhoIntercept[] = L"EnableBuggyBhoIntercept";
 
 static const wchar_t kChromeFrameNPAPIKey[] =
     L"Software\\MozillaPlugins\\@google.com/ChromeFrame,version=1.0";
diff --git a/chrome_frame/utils.h b/chrome_frame/utils.h
index 5561331..b40a741 100644
--- a/chrome_frame/utils.h
+++ b/chrome_frame/utils.h
@@ -28,6 +28,7 @@ extern const wchar_t kChromeProtocolPrefix[];
 extern const wchar_t kChromeFrameHeadlessMode[];
 extern const wchar_t kChromeFrameUnpinnedMode[];
 extern const wchar_t kEnableGCFProtocol[];
+extern const wchar_t kEnableBuggyBhoIntercept[];
 extern const wchar_t kChromeMimeType[];
 
 typedef enum ProtocolPatchMethod {
diff --git a/chrome_frame/vtable_patch_manager.cc b/chrome_frame/vtable_patch_manager.cc
index dc88079..10852ee 100644
--- a/chrome_frame/vtable_patch_manager.cc
+++ b/chrome_frame/vtable_patch_manager.cc
@@ -4,9 +4,10 @@
 
 #include "chrome_frame/vtable_patch_manager.h"
 
-#include <algorithm>
 #include <atlcomcli.h>
 
+#include <algorithm>
+
 #include "base/atomicops.h"
 #include "base/lock.h"
 #include "base/logging.h"
@@ -90,7 +91,7 @@ HRESULT PatchInterfaceMethods(void* unknown, MethodPatchInfo* patches) {
     FunctionStub* stub = NULL;
 
 #ifndef NDEBUG
-    FunctionStub::FromCode(original_fn);
+    stub = FunctionStub::FromCode(original_fn);
     if (stub != NULL) {
       DLOG(ERROR) << "attempt to patch a function that's already patched";
       DCHECK(stub->destination_function() ==