A different approach to avoid crashes in buggy 3rd party BHOs.

A different approach to avoid crashes in buggy 3rd party BHOs.This time we're more preceise and only target the buggy components.Behaviour for components that handle browser events correctly, is unchanged even in the presence of buggy DLLs.The core class here is the BuggyBhoTls class and here's the comment for that class:

// Construct an instance of this class on the stack when 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.

TEST=A better fix for bug 44463 but also fixes bug 49373 and a number of crashes that haven't been associated with bug reports yet.
BUG=44463, 49373
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=53194

[amit, 2010-07-20 20:44:07]

[+ stoyan]

lgtm

[stoyan, 2010-07-20 21:44:55]

http://codereview.chromium.org/3031009/diff/34001/20003
File chrome_frame/buggy_bho_handling.cc (right):

http://codereview.chromium.org/3031009/diff/34001/20003#newcode110
chrome_frame/buggy_bho_handling.cc:110: if (!::VirtualProtect(invoke,
sizeof(PROC), PAGE_EXECUTE_READWRITE, &flags))
You need critical section or something else. There is a race on
VirtualProtect(READ_WRITE/READ_ONLY) calls.
T1: VP(READ_WRITE)
T2: VP(READ_WRITE)
T1: write_data, VP(READ_ONLY)
T2: write_data -> crash.

http://codereview.chromium.org/3031009/diff/34001/20003#newcode184
chrome_frame/buggy_bho_handling.cc:184: PatchIfBuggy(&cd, sinks[i]);
Since there is no unpatch - the virtual table entry will be patched every time,
resulting in a very long chain of calls. Unless I'm missing something.

[tommi (sloooow) - chröme, 2010-07-21 14:59:55]

http://codereview.chromium.org/3031009/diff/34001/20003
File chrome_frame/buggy_bho_handling.cc (right):

http://codereview.chromium.org/3031009/diff/34001/20003#newcode110
chrome_frame/buggy_bho_handling.cc:110: if (!::VirtualProtect(invoke,
sizeof(PROC), PAGE_EXECUTE_READWRITE, &flags))
On 2010/07/20 21:44:55, stoyan wrote:
> You need critical section or something else. There is a race on
> VirtualProtect(READ_WRITE/READ_ONLY) calls.
> T1: VP(READ_WRITE)
> T2: VP(READ_WRITE)
> T1: write_data, VP(READ_ONLY)
> T2: write_data -> crash.
> 
I don't think there would be a crash because I'm using an atomic write that
checks the existing value before writing.  However, you're right that the
protection flags could get mixed up and we could leak a function stub. 
Actually... on second thought, there could be more than one stubs chained if one
stub is applied before another thread dereferences invoke to create its stub.

So... yes, lock added.

http://codereview.chromium.org/3031009/diff/34001/20003#newcode184
chrome_frame/buggy_bho_handling.cc:184: PatchIfBuggy(&cd, sinks[i]);
On 2010/07/20 21:44:55, stoyan wrote:
> Since there is no unpatch - the virtual table entry will be patched every
time,
> resulting in a very long chain of calls. Unless I'm missing something.
> 

There's a check at the top of PatchIfBuggy to see if there's already a patch
there.  See FunctionStub::FromCode.

[stoyan, 2010-07-21 16:03:14]

lgtm