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

chrome_frame/buggy_bho_handling.cc

 // 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/exception_barrier.h"
@@ skipping 57 matching lines @@
           (HIWORD(version) == buggy.major_version_ &&
            LOWORD(version) <= buggy.minor_version_)) {
         return true;
       }
     }
   }
 
   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,
                                          VARIANT* result, EXCEPINFO* ei,
                                          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;
   }
 
   // No need to report crashes in those known-to-be-buggy DLLs.
   ExceptionBarrierReportOnlyModule barrier;
   return original(me, dispid, riid, lcid, flags, params, result, ei, err);
 }
 
@@ skipping 19 matching lines @@
     if (!vtable_patch::internal::ReplaceFunctionPointer(
             reinterpret_cast<void**>(invoke), stub->code(),
             reinterpret_cast<void*>(stub->argument()))) {
       hr = E_UNEXPECTED;
       FunctionStub::Destroy(stub);
     } else {
       PinModule();  // No backing out now.
       ::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