A number of poorly written IE BHO's crash IE if ChromeFrame is the currently ...

chrome_frame/buggy_bho_handling.cc

Index: chrome_frame/buggy_bho_handling.cc
===================================================================
--- chrome_frame/buggy_bho_handling.cc	(revision 74374)
+++ chrome_frame/buggy_bho_handling.cc	(working copy)
@@ -4,17 +4,18 @@
 
 #include "chrome_frame/buggy_bho_handling.h"
 
-#include "base/logging.h"
+#include "base/lazy_instance.h"
 #include "base/scoped_comptr_win.h"
-
 #include "chrome_frame/exception_barrier.h"
 #include "chrome_frame/function_stub.h"
+#include "chrome_frame/urlmon_moniker.h"
 #include "chrome_frame/utils.h"
 #include "chrome_frame/vtable_patch_manager.h"
 
 namespace buggy_bho {
 
-base::ThreadLocalPointer<BuggyBhoTls> BuggyBhoTls::s_bad_object_tls_;
+base::LazyInstance<BuggyBhoHandler>
+    g_buggy_bho_handler(base::LINKER_INITIALIZED);
 
 struct ModuleAndVersion {
   const char* module_name_;
@@ -75,39 +76,68 @@
   return false;
 }
 
-BuggyBhoTls::BuggyBhoTls() : previous_instance_(s_bad_object_tls_.Get()) {
-  s_bad_object_tls_.Set(this);
+BuggyBhoHandler::BuggyBhoHandler() {
 }
 
-BuggyBhoTls::~BuggyBhoTls() {
-  DCHECK(FromCurrentThread() == this);
-  s_bad_object_tls_.Set(previous_instance_);
+BuggyBhoHandler::~BuggyBhoHandler() {
 }
 
-void BuggyBhoTls::AddBuggyObject(IDispatch* obj) {
-  bad_objects_.push_back(obj);
+BuggyBhoHandler* BuggyBhoHandler::GetInstance() {
+  return g_buggy_bho_handler.Pointer();
 }
 
-bool BuggyBhoTls::IsBuggyObject(IDispatch* obj) const {
-  return std::find(bad_objects_.begin(), bad_objects_.end(), obj) !=
-         bad_objects_.end();
+void BuggyBhoHandler::ClearBuggyObjectsForThread(
+    base::PlatformThreadId thread_id) {
+  DCHECK(GetInstance())
+      << "You must first have an instance of BuggyBhoHandler.";
+
+  base::AutoLock lock(GetInstance()->lock_);

[tommi (sloooow) - chröme, 2011/02/11 14:49:02]

If you store the object list in the tls, you wouldn't need this lock and no
thread_id is needed.

+
+  std::vector<ObjectInfo>::iterator index =
+      GetInstance()->bad_objects_.begin();
+  while (index != GetInstance()->bad_objects_.end()) {
+    if (index->object_thread_id == thread_id) {
+      index = GetInstance()->bad_objects_.erase(index);
+    } else {
+      ++index;
+    }
+  }
 }
 
-// static
-BuggyBhoTls* BuggyBhoTls::FromCurrentThread() {
-  return s_bad_object_tls_.Get();
+void BuggyBhoHandler::AddBuggyObject(IDispatch* obj) {
+  base::AutoLock lock(lock_);
+  ObjectInfo object_info;
+  object_info.disp_interface = obj;
+  object_info.object_thread_id = base::PlatformThread::CurrentId();
+  bad_objects_.push_back(object_info);
 }
 
+bool BuggyBhoHandler::ShouldSkipInvoke(IDispatch* obj) {
+  NavigationManager* navigation_mgr = NavigationManager::GetThreadInstance();
+  DCHECK(navigation_mgr);
+  if (navigation_mgr && IsChromeFrameDocument(navigation_mgr->web_browser())) {
+    base::AutoLock lock(lock_);
+    std::vector<ObjectInfo>::iterator index;
+    for (index = bad_objects_.begin(); index != bad_objects_.end(); ++index) {
+      if (index->disp_interface == obj)
+        return true;
+    }
+  }
+  return false;
+}
+
 // static
-STDMETHODIMP BuggyBhoTls::BuggyBhoInvoke(InvokeFunc original, IDispatch* me,
+STDMETHODIMP BuggyBhoHandler::BuggyBhoInvoke(InvokeFunc original, IDispatch* me,
                                          DISPID dispid, REFIID riid, LCID lcid,
                                          WORD flags, DISPPARAMS* params,
                                          VARIANT* result, EXCEPINFO* ei,
                                          UINT* err) {
   DVLOG(1) << __FUNCTION__;
 
-  const BuggyBhoTls* tls = BuggyBhoTls::FromCurrentThread();
-  if (tls && tls->IsBuggyObject(me)) {
+  DCHECK(GetInstance())
+      << "You must first have an instance of BuggyBhoHandler.";
+
+  if (GetInstance()->ShouldSkipInvoke(me)) {
     // Ignore this call and avoid the bug.
     // TODO(tommi): Maybe we should check a specific list of DISPIDs too?
     return S_OK;
@@ -119,7 +149,7 @@
 }
 
 // static
-HRESULT BuggyBhoTls::PatchInvokeMethod(PROC* invoke) {
+HRESULT BuggyBhoHandler::PatchInvokeMethod(PROC* invoke) {
   CCritSecLock lock(_pAtlModule->m_csStaticDataInitAndTypeInfo.m_sec, true);
 
   FunctionStub* stub = FunctionStub::FromCode(*invoke);
@@ -154,7 +184,7 @@
 }
 
 // static
-bool BuggyBhoTls::PatchIfBuggy(CONNECTDATA* cd, const IID& diid) {
+bool BuggyBhoHandler::PatchIfBuggy(CONNECTDATA* cd, const IID& diid) {
   DCHECK(cd);
   PROC* methods = *reinterpret_cast<PROC**>(cd->pUnk);
   HMODULE mod = GetModuleFromAddress(methods[0]);
@@ -172,11 +202,9 @@
     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);
-      }
+      BuggyBhoHandler* instance = GetInstance();
+      DCHECK(instance);
+      instance->AddBuggyObject(disp);
     }
   }
 
@@ -184,10 +212,13 @@
 }
 
 // static
-HRESULT BuggyBhoTls::PatchBuggyBHOs(IWebBrowser2* browser) {
+HRESULT BuggyBhoHandler::PatchBuggyBHOs(IWebBrowser2* browser) {
   DCHECK(browser);
-  DCHECK(BuggyBhoTls::FromCurrentThread())
-      << "You must first have an instance of BuggyBhoTls on this thread";
+  DCHECK(GetInstance())
+      << "You must first have an instance of BuggyBhoHandler.";
+  // Serialized as we could be called from multiple threads and the same BHO
+  // could be loaded in multiple IE windows.
+  base::AutoLock lock(GetInstance()->lock_);
 
   ScopedComPtr<IConnectionPointContainer> cpc;
   HRESULT hr = cpc.QueryFrom(browser);
@@ -211,7 +242,6 @@
       }
     }
   }
-
   return hr;
 }