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

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


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=74691

[amit, 2011-02-11 02:12:58]

What happens when there are two windows (two IWebBrowser2?) in the same thread?

[ananta, 2011-02-11 04:28:32]

On 2011/02/11 02:12:58, amit wrote:
> What happens when there are two windows (two IWebBrowser2?) in the same
thread?

Tested this on IE6/7/8. On IE6 every IE window is associated with its own
thread. Same behavior with IE7/8 where every tab and window is created on a
separate thread.

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

This method moves away from the tls approach we had before and replaces it with
a global singleton+lock.  I prefer the tls approach and think we can achieve the
same thing without locks or thread id checks.  Discuss offline?

http://codereview.chromium.org/6493002/diff/14007/chrome_frame/bho.cc
File chrome_frame/bho.cc (right):

http://codereview.chromium.org/6493002/diff/14007/chrome_frame/bho.cc#newcode139
chrome_frame/bho.cc:139: RegisterThreadInstance(web_browser2);
Maybe instead of registering a pointer to IWebBrowser2, we should store a vector
of Bho's?  The Bho already has a pointer to the web browser via the site.

http://codereview.chromium.org/6493002/diff/14007/chrome_frame/bho.cc#newcode142
chrome_frame/bho.cc:142: UnregisterThreadInstance();
Hmm... there's no matching DispEventUnadvise to the DispEventAdvise above...  Is
our Bho ever fully released?

http://codereview.chromium.org/6493002/diff/14007/chrome_frame/buggy_bho_hand...
File chrome_frame/buggy_bho_handling.cc (right):

http://codereview.chromium.org/6493002/diff/14007/chrome_frame/buggy_bho_hand...
chrome_frame/buggy_bho_handling.cc:94: base::AutoLock
lock(GetInstance()->lock_);
If you store the object list in the tls, you wouldn't need this lock and no
thread_id is needed.

http://codereview.chromium.org/6493002/diff/14007/chrome_frame/buggy_bho_hand...
File chrome_frame/buggy_bho_handling.h (right):

http://codereview.chromium.org/6493002/diff/14007/chrome_frame/buggy_bho_hand...
chrome_frame/buggy_bho_handling.h:85: // accessed from multiple IE UI threads.
Would it be better to store this list with the TLS and have no locks?  That
would also separate the list of objects so that we only ever access an object
pointer from the correct thread.

[ananta, 2011-02-11 18:55:07]

http://codereview.chromium.org/6493002/diff/14007/chrome_frame/bho.cc
File chrome_frame/bho.cc (right):

http://codereview.chromium.org/6493002/diff/14007/chrome_frame/bho.cc#newcode142
chrome_frame/bho.cc:142: UnregisterThreadInstance();
On 2011/02/11 14:49:02, tommi wrote:
> Hmm... there's no matching DispEventUnadvise to the DispEventAdvise above... 
Is
> our Bho ever fully released?

Nice catch. Fixed.

[ananta, 2011-02-11 18:57:39]

On 2011/02/11 14:49:02, tommi wrote:
> This method moves away from the tls approach we had before and replaces it
with
> a global singleton+lock.  I prefer the tls approach and think we can achieve
the
> same thing without locks or thread id checks.  Discuss offline?
> 
> http://codereview.chromium.org/6493002/diff/14007/chrome_frame/bho.cc
> File chrome_frame/bho.cc (right):
> 
>
http://codereview.chromium.org/6493002/diff/14007/chrome_frame/bho.cc#newcode139
> chrome_frame/bho.cc:139: RegisterThreadInstance(web_browser2);
> Maybe instead of registering a pointer to IWebBrowser2, we should store a
vector
> of Bho's?  The Bho already has a pointer to the web browser via the site.
> 
>
http://codereview.chromium.org/6493002/diff/14007/chrome_frame/bho.cc#newcode142
> chrome_frame/bho.cc:142: UnregisterThreadInstance();
> Hmm... there's no matching DispEventUnadvise to the DispEventAdvise above... 
Is
> our Bho ever fully released?
> 
>
http://codereview.chromium.org/6493002/diff/14007/chrome_frame/buggy_bho_hand...
> File chrome_frame/buggy_bho_handling.cc (right):
> 
>
http://codereview.chromium.org/6493002/diff/14007/chrome_frame/buggy_bho_hand...
> chrome_frame/buggy_bho_handling.cc:94: base::AutoLock
> lock(GetInstance()->lock_);
> If you store the object list in the tls, you wouldn't need this lock and no
> thread_id is needed.
> 
>
http://codereview.chromium.org/6493002/diff/14007/chrome_frame/buggy_bho_hand...
> File chrome_frame/buggy_bho_handling.h (right):
> 
>
http://codereview.chromium.org/6493002/diff/14007/chrome_frame/buggy_bho_hand...
> chrome_frame/buggy_bho_handling.h:85: // accessed from multiple IE UI threads.
> Would it be better to store this list with the TLS and have no locks?  That
> would also separate the list of objects so that we only ever access an object
> pointer from the correct thread.

As per our discussion the BuggyBho instance now lives on the TLS andis created
once. We clear it when we receive a SetSite of NULL in our Bho. Some of the
static methods used for patching no longer need to be static. Changed that. We
save away the IWebBrowser2 interface in the object. This is safe as there can be
only one IWebBrowser2 instance per thread.

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

lgtm with a few more nits

http://codereview.chromium.org/6493002/diff/20001/chrome_frame/bho.cc
File chrome_frame/bho.cc (right):

http://codereview.chromium.org/6493002/diff/20001/chrome_frame/bho.cc#newcode144
chrome_frame/bho.cc:144: DispEventUnadvise(web_browser2,
&DIID_DWebBrowserEvents2);
thanks :)

http://codereview.chromium.org/6493002/diff/20001/chrome_frame/buggy_bho_hand...
File chrome_frame/buggy_bho_handling.cc (right):

http://codereview.chromium.org/6493002/diff/20001/chrome_frame/buggy_bho_hand...
chrome_frame/buggy_bho_handling.cc:79: DCHECK(BuggyBhoTls::GetInstance() ==
NULL);
lint will probably ask you to use DCHECK_EQ here and below

http://codereview.chromium.org/6493002/diff/20001/chrome_frame/buggy_bho_hand...
chrome_frame/buggy_bho_handling.cc:115: delete tls_instance;
also set s_bad_object_tls to NULL

http://codereview.chromium.org/6493002/diff/20001/chrome_frame/buggy_bho_hand...
chrome_frame/buggy_bho_handling.cc:120: DCHECK(browser);
should we also dcheck that web_browser2_ is NULL?

http://codereview.chromium.org/6493002/diff/20001/chrome_frame/utils.cc
File chrome_frame/utils.cc (right):

http://codereview.chromium.org/6493002/diff/20001/chrome_frame/utils.cc#newco...
chrome_frame/utils.cc:36: #include "chrome_tab.h"  // NOLINT
move to the bottom? (plus an empty line before)

http://codereview.chromium.org/6493002/diff/20001/chrome_frame/utils.cc#newco...
chrome_frame/utils.cc:1600: return !!chrome_frame.get();
instead of !! use
return chrome_frame.get() != NULL;
double negatives do sometimes not make any sense to nobody :)

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

Looks much better. One more thing that occurred to me is that this scheme does
not work if UpdateNavigationState never occurs.

http://codereview.chromium.org/6493002/diff/20001/chrome_frame/buggy_bho_hand...
File chrome_frame/buggy_bho_handling.cc (right):

http://codereview.chromium.org/6493002/diff/20001/chrome_frame/buggy_bho_hand...
chrome_frame/buggy_bho_handling.cc:148: bool
BuggyBhoTls::PatchIfBuggy(CONNECTDATA* cd, const IID& diid) {
nit: why not pass cd->punk directly?

http://codereview.chromium.org/6493002/diff/20001/chrome_frame/buggy_bho_hand...
chrome_frame/buggy_bho_handling.cc:182: if (BuggyBhoTls::GetInstance() &&
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

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

http://codereview.chromium.org/6493002/diff/20001/chrome_frame/buggy_bho_hand...
File chrome_frame/buggy_bho_handling.cc (right):

http://codereview.chromium.org/6493002/diff/20001/chrome_frame/buggy_bho_hand...
chrome_frame/buggy_bho_handling.cc:79: DCHECK(BuggyBhoTls::GetInstance() ==
NULL);
On 2011/02/11 19:34:15, tommi wrote:
> lint will probably ask you to use DCHECK_EQ here and below

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

http://codereview.chromium.org/6493002/diff/20001/chrome_frame/buggy_bho_hand...
chrome_frame/buggy_bho_handling.cc:115: delete tls_instance;
On 2011/02/11 19:34:15, tommi wrote:
> also set s_bad_object_tls to NULL

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

http://codereview.chromium.org/6493002/diff/20001/chrome_frame/buggy_bho_hand...
chrome_frame/buggy_bho_handling.cc:120: DCHECK(browser);
On 2011/02/11 19:34:15, tommi wrote:
> should we also dcheck that web_browser2_ is NULL?

Done.

http://codereview.chromium.org/6493002/diff/20001/chrome_frame/buggy_bho_hand...
chrome_frame/buggy_bho_handling.cc:148: bool
BuggyBhoTls::PatchIfBuggy(CONNECTDATA* cd, const IID& diid) {
On 2011/02/11 19:49:40, amit wrote:
> nit: why not pass cd->punk directly?

Done.

http://codereview.chromium.org/6493002/diff/20001/chrome_frame/utils.cc
File chrome_frame/utils.cc (right):

http://codereview.chromium.org/6493002/diff/20001/chrome_frame/utils.cc#newco...
chrome_frame/utils.cc:36: #include "chrome_tab.h"  // NOLINT
On 2011/02/11 19:34:15, tommi wrote:
> move to the bottom? (plus an empty line before)

Done.

http://codereview.chromium.org/6493002/diff/20001/chrome_frame/utils.cc#newco...
chrome_frame/utils.cc:1600: return !!chrome_frame.get();
On 2011/02/11 19:34:15, tommi wrote:
> instead of !! use
> return chrome_frame.get() != NULL;
> double negatives do sometimes not make any sense to nobody :)

Done.

[amit, 2011-02-11 21:51:56]

lgtm