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

This is because they expect ChromeFrame to implement interfaces like IHTMLDocument2 on the same
lines as regular IE documents. Currently in ChromeFrame we patch the invoke methods of these
BHO's prior to firing navigation events from ChromeFrame. However this is not enough as
these objects also crash for regular navigation events fired from IE when ChromeFrame is loaded.

We now don't fire navigation events for buggy BHO's if ChromeFrame is the current document.
The BuggyBho handler instance is now created once for the thread. We patch when we receive
navigation notifications from Chrome as before. When we receive a notification
on our patched event sink we check if CF is loaded and if yes skip the call. 

Added helpers to chrome frame utils to check if CF is loaded in the current web browser instance.

BUG=55932
TEST=none

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

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

6 files changed, 157 insertions, 105 deletions

diff --git a/chrome_frame/bho.cc b/chrome_frame/bho.cc
index 2de3bc8d..0cd3a4a 100644
--- a/chrome_frame/bho.cc
+++ b/chrome_frame/bho.cc
@@ -12,7 +12,7 @@
 #include "base/string_util.h"
 #include "base/stringprintf.h"
 #include "base/win/scoped_bstr.h"
-#include "chrome_tab.h" // NOLINT
+#include "chrome_frame/buggy_bho_handling.h"
 #include "chrome_frame/crash_reporting/crash_metrics.h"
 #include "chrome_frame/extra_system_apis.h"
 #include "chrome_frame/html_utils.h"
@@ -138,6 +138,10 @@ STDMETHODIMP Bho::SetSite(IUnknown* site) {
     MetricsService::Start();
   } else {
     UnregisterThreadInstance();
+    buggy_bho::BuggyBhoTls::DestroyInstance();
+    ScopedComPtr<IWebBrowser2> web_browser2;
+    web_browser2.QueryFrom(m_spUnkSite);
+    DispEventUnadvise(web_browser2, &DIID_DWebBrowserEvents2);
     Release();
   }
 
diff --git a/chrome_frame/buggy_bho_handling.cc b/chrome_frame/buggy_bho_handling.cc
index 0805033..bcd2427 100644
--- a/chrome_frame/buggy_bho_handling.cc
+++ b/chrome_frame/buggy_bho_handling.cc
@@ -75,27 +75,105 @@ bool IsBuggyBho(HMODULE mod) {
   return false;
 }
 
-BuggyBhoTls::BuggyBhoTls() : previous_instance_(s_bad_object_tls_.Get()) {
+BuggyBhoTls::BuggyBhoTls()
+    : patched_(false) {
+  DCHECK(s_bad_object_tls_.Get() == NULL);
   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;
+    DCHECK(s_bad_object_tls_.Get() == NULL);
+  }
+}
+
+HRESULT BuggyBhoTls::PatchBuggyBHOs(IWebBrowser2* browser) {
+  if (patched_)
+    return S_FALSE;
+
+  DCHECK(browser);
+  DCHECK(web_browser2_ == NULL);
+
+  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.pUnk, sinks[i]);
+            cd.pUnk->Release();
+            fetched = 0;
+          }
+        }
+      }
+    }
+  }
+  patched_ = true;
+  web_browser2_ = browser;
+  return hr;
+}
+
+bool BuggyBhoTls::PatchIfBuggy(IUnknown* unk, const IID& diid) {
+  DCHECK(unk);
+  PROC* methods = *reinterpret_cast<PROC**>(unk);
+  HMODULE mod = GetModuleFromAddress(methods[0]);
+  if (!IsBuggyBho(mod))
+    return false;
+
+  ScopedComPtr<IDispatch> disp;
+  HRESULT hr = unk->QueryInterface(diid,
+      reinterpret_cast<void**>(disp.Receive()));
+  if (FAILED(hr))  // Sometimes only IDispatch QI is supported
+    hr = disp.QueryFrom(unk);
+  DCHECK(SUCCEEDED(hr));
+
+  if (SUCCEEDED(hr)) {
+    const int kInvokeIndex = 6;
+    DCHECK(static_cast<IUnknown*>(disp) == unk);
+    if (SUCCEEDED(PatchInvokeMethod(&methods[kInvokeIndex]))) {
+      AddBuggyObject(disp);
+    }
+  }
+  return false;
 }
 
 // static
@@ -106,8 +184,10 @@ STDMETHODIMP BuggyBhoTls::BuggyBhoInvoke(InvokeFunc original, IDispatch* me,
                                          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() &&
+      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,71 +227,7 @@ HRESULT BuggyBhoTls::PatchInvokeMethod(PROC* invoke) {
       ::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;
 }
 
diff --git a/chrome_frame/buggy_bho_handling.h b/chrome_frame/buggy_bho_handling.h
index 4f1e7f5..83c635a 100644
--- a/chrome_frame/buggy_bho_handling.h
+++ b/chrome_frame/buggy_bho_handling.h
@@ -12,6 +12,7 @@
 #include <vector>
 
 #include "base/threading/thread_local.h"
+#include "base/win/scoped_comptr.h"
 
 namespace buggy_bho {
 
@@ -21,23 +22,32 @@ typedef HRESULT (__stdcall* InvokeFunc)(IDispatch* me, DISPID dispid,
                                         DISPPARAMS* params, VARIANT* result,
                                         EXCEPINFO* ei, UINT* err);
 
-// Construct an instance of this class on the stack when firing web browser
+// Construct a per thread instance of this class before 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.
+// as long as ChromeFrame is the active view.
 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.
+  // 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);
+  HRESULT PatchBuggyBHOs(IWebBrowser2* browser);
+
+  // Returns the instance of the BuggyBhoTls object for the current thread.
+  static BuggyBhoTls* GetInstance();
+
+  // Destroys the BuggyBhoTls instance foe the current thread.
+  static void DestroyInstance();
+
+  void set_web_browser(IWebBrowser2* web_browser2) {
+    web_browser2_ = web_browser2;
+  }
 
  protected:
+  BuggyBhoTls();
+  ~BuggyBhoTls();
   // internal implementation:
 
   // Called when a buggy instance is found to be subscribing to browser events.
@@ -48,15 +58,12 @@ class BuggyBhoTls {
   // 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;
+  bool ShouldSkipInvoke(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);
+  bool PatchIfBuggy(IUnknown* unk, const IID& diid);
 
   // Patches the IDispatch::Invoke method.
   static HRESULT PatchInvokeMethod(PROC* invoke);
@@ -71,14 +78,12 @@ class BuggyBhoTls {
  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_;
+  // The IWebBrowser2 instance for this thread.
+  base::win::ScopedComPtr<IWebBrowser2> web_browser2_;
+  // Set to true when we are done patching the event sinks of buggy bho's.
+  bool patched_;
 };
 
 }  // end namespace buggy_bho
diff --git a/chrome_frame/chrome_active_document.cc b/chrome_frame/chrome_active_document.cc
index 7e78931..2fc4a00 100644
--- a/chrome_frame/chrome_active_document.cc
+++ b/chrome_frame/chrome_active_document.cc
@@ -738,6 +738,19 @@ void ChromeActiveDocument::UpdateNavigationState(
            OLECMDEXECOPT_DODEFAULT, secure_lock_status.AsInput(), NULL);
   }
 
+  // A number of poorly written bho's crash in their event sink callbacks if
+  // chrome frame is the currently loaded document. This is because they expect
+  // chrome frame to implement interfaces like IHTMLDocument, etc. We patch the
+  // event sink's of these bho's and don't invoke the event sink if chrome
+  // frame is the currently loaded document.
+  if (GetConfigBool(true, kEnableBuggyBhoIntercept)) {
+    ScopedComPtr<IWebBrowser2> wb2;
+    DoQueryService(SID_SWebBrowserApp, m_spClientSite, wb2.Receive());
+    if (wb2 && buggy_bho::BuggyBhoTls::GetInstance()) {
+      buggy_bho::BuggyBhoTls::GetInstance()->PatchBuggyBHOs(wb2);
+    }
+  }
+
   // Ideally all navigations should come to Chrome Frame so that we can call
   // BeforeNavigate2 on installed BHOs and give them a chance to cancel the
   // navigation. However, in practice what happens is as below:
@@ -768,15 +781,6 @@ 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());
 
diff --git a/chrome_frame/utils.cc b/chrome_frame/utils.cc
index 210d6488..049294c 100644
--- a/chrome_frame/utils.cc
+++ b/chrome_frame/utils.cc
@@ -39,6 +39,8 @@
 #include "net/base/escape.h"
 #include "net/http/http_util.h"
 
+#include "chrome_tab.h"  // NOLINT
+
 using base::win::RegKey;
 using base::win::ScopedComPtr;
 
@@ -1583,3 +1585,21 @@ std::wstring GetCurrentModuleVersion() {
   DCHECK(module_version_info.get() != NULL);
   return module_version_info->file_version();
 }
+
+bool IsChromeFrameDocument(IWebBrowser2* web_browser) {
+  if (!web_browser)
+    return false;
+
+  ScopedComPtr<IDispatch> doc;
+  web_browser->get_Document(doc.Receive());
+  if (doc) {
+    // Detect if CF is rendering based on whether the document is a
+    // ChromeActiveDocument. Detecting based on hwnd is problematic as
+    // the CF Active Document window may not have been created yet.
+    ScopedComPtr<IChromeFrame> chrome_frame;
+    chrome_frame.QueryFrom(doc);
+    return chrome_frame.get() != NULL;
+  }
+  return false;
+}
+
diff --git a/chrome_frame/utils.h b/chrome_frame/utils.h
index 6b8b0ee..5847917 100644
--- a/chrome_frame/utils.h
+++ b/chrome_frame/utils.h
@@ -8,7 +8,6 @@
 #include <OAidl.h>
 #include <windows.h>
 #include <wininet.h>
-
 #include <string>
 #include <vector>
 
@@ -23,6 +22,7 @@
 
 class FilePath;
 interface IBrowserService;
+interface IWebBrowser2;
 
 // utils.h : Various utility functions and classes
 
@@ -615,4 +615,7 @@ bool CheckXUaCompatibleDirective(const std::string& directive,
 // Returns the version of the current module as a string.
 std::wstring GetCurrentModuleVersion();
 
+// Returns true if ChromeFrame is the currently loaded document.
+bool IsChromeFrameDocument(IWebBrowser2* web_browser);
+
 #endif  // CHROME_FRAME_UTILS_H_