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)
@@ -75,30 +75,101 @@
   return false;
 }
 
-BuggyBhoTls::BuggyBhoTls() : previous_instance_(s_bad_object_tls_.Get()) {
+BuggyBhoTls::BuggyBhoTls() {
+  DCHECK(BuggyBhoTls::GetInstance() == NULL);

[tommi (sloooow) - chröme, 2011/02/11 19:34:15]

lint will probably ask you to use DCHECK_EQ here and below

[ananta, 2011/02/11 21:19:35]

DCHECK_EQ would need a reintepret_cast. I left this as is.

   s_bad_object_tls_.Set(this);
 }
 
 BuggyBhoTls::~BuggyBhoTls() {
-  DCHECK(FromCurrentThread() == this);
-  s_bad_object_tls_.Set(previous_instance_);
+  DCHECK(BuggyBhoTls::GetInstance() == this);
+  s_bad_object_tls_.Set(NULL);
 }
 
 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();
+bool BuggyBhoTls::ShouldSkipInvoke(IDispatch* obj) const {
+  DCHECK(web_browser2_ != NULL);
+  if (IsChromeFrameDocument(web_browser2_)) {
+    return std::find(bad_objects_.begin(), bad_objects_.end(), obj) !=
+           bad_objects_.end();
+  }
+  return false;
 }
 
 // static
-BuggyBhoTls* BuggyBhoTls::FromCurrentThread() {
-  return s_bad_object_tls_.Get();
+BuggyBhoTls* BuggyBhoTls::GetInstance() {
+  BuggyBhoTls* tls_instance = s_bad_object_tls_.Get();
+  if (!tls_instance) {
+    tls_instance = new BuggyBhoTls();
+    DCHECK(s_bad_object_tls_.Get() != NULL);
+  }
+  return tls_instance;
 }
 
 // static
+void BuggyBhoTls::DestroyInstance() {
+  BuggyBhoTls* tls_instance = s_bad_object_tls_.Get();
+  if (tls_instance) {
+    delete tls_instance;

[tommi (sloooow) - chröme, 2011/02/11 19:34:15]

also set s_bad_object_tls to NULL

[ananta, 2011/02/11 21:19:35]

That happens in the destructor. Added a DCHECK after delete.

+  }
+}
+
+HRESULT BuggyBhoTls::PatchBuggyBHOs(IWebBrowser2* browser) {
+  DCHECK(browser);

[tommi (sloooow) - chröme, 2011/02/11 19:34:15]

should we also dcheck that web_browser2_ is NULL?

[ananta, 2011/02/11 21:19:35]

Done.

+
+  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;
+          }
+        }
+      }
+    }
+  }
+  web_browser2_ = browser;
+  return hr;
+}
+
+bool BuggyBhoTls::PatchIfBuggy(CONNECTDATA* cd, const IID& diid) {

[amit, 2011/02/11 19:49:40]

nit: why not pass cd->punk directly?

[ananta, 2011/02/11 21:19:35]

Done.

+  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]))) {
+      AddBuggyObject(disp);
+    }
+  }
+  return false;
+}
+
+// static
 STDMETHODIMP BuggyBhoTls::BuggyBhoInvoke(InvokeFunc original, IDispatch* me,
                                          DISPID dispid, REFIID riid, LCID lcid,
                                          WORD flags, DISPPARAMS* params,
@@ -106,8 +177,10 @@
                                          UINT* err) {
   DVLOG(1) << __FUNCTION__;
 
-  const BuggyBhoTls* tls = BuggyBhoTls::FromCurrentThread();
-  if (tls && tls->IsBuggyObject(me)) {
+  DCHECK(BuggyBhoTls::GetInstance())
+      << "You must first have an instance of BuggyBhoTls on this thread";
+  if (BuggyBhoTls::GetInstance() &&

[amit, 2011/02/11 19:49:40]

Maybe we can avoid the TLS altogether with following steps:
- store BuggyBhoTls instance as a property in a IWebBrowser2
- EnumWindows for tab window
- GetWindowLong for GWL_LPARAM and QI for IWebBrowser2
- Get the saved instance

+      BuggyBhoTls::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;
@@ -147,72 +220,8 @@
       ::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