Add an ExceptionBarrier around outbound calls to patched methods in IE. In so...

Add an ExceptionBarrier around outbound calls to patched methods in IE. In so doing, we have an SEH present in the SEH chain and so the VEH won't erroneously report crashes that occur in other modules when we happen to be on the stack.

BUG=42660
TEST=Less false positives in the crash reports.



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

[robertshield, 2010-04-27 20:25:24]

Here's a stab at adding an exception handler to the fray when calling out to a
couple of original implementations from our patches. I've muddled through this a
bit (i.e. it might be crap) and there are a couple of things I'm somewhat
uncertain about:

1) Is reusing the existing breakpad::ExceptionHandler instance sane and is the
locking necessary / safe / correct here? If the call to WriteDump throws an
exception that the VEH picks up, the VEH shouldn't call WriteDump() since our
handler would be in the SEH chain.

2) The license at the top of the exception_barrier* files is the Chrome license.
Is this kosher?

[amit, 2010-04-27 20:39:42]

[+cc stoyan for another round of sanity check]

LGTM

http://codereview.chromium.org/1733021/diff/1/4
File chrome_frame/crash_reporting/crash_report.h (right):

http://codereview.chromium.org/1733021/diff/1/4#newcode34
chrome_frame/crash_reporting/crash_report.h:34:
google_breakpad::ExceptionHandler* GetBreakpadHandler(Lock* breakpad_lock);
Wouldn't it be better to provide a global/static helper to WriteMinitump than
expose lock and breakpad instance outside?

http://codereview.chromium.org/1733021/diff/1/5
File chrome_frame/urlmon_moniker.cc (right):

http://codereview.chromium.org/1733021/diff/1/5#newcode188
chrome_frame/urlmon_moniker.cc:188: ChromeFrameExceptionBarrier barrier;
nit: please move this to the top as we have seen RPCRT exceptions in
BindContextInfo::FromBindContext

[robertshield, 2010-04-27 21:02:42]

Thanks, please take another look.

http://codereview.chromium.org/1733021/diff/1/4
File chrome_frame/crash_reporting/crash_report.h (right):

http://codereview.chromium.org/1733021/diff/1/4#newcode34
chrome_frame/crash_reporting/crash_report.h:34:
google_breakpad::ExceptionHandler* GetBreakpadHandler(Lock* breakpad_lock);
On 2010/04/27 20:39:42, amit wrote:
> Wouldn't it be better to provide a global/static helper to WriteMinitump than
> expose lock and breakpad instance outside?

Yes, that's much better. Thanks.

http://codereview.chromium.org/1733021/diff/1/5
File chrome_frame/urlmon_moniker.cc (right):

http://codereview.chromium.org/1733021/diff/1/5#newcode188
chrome_frame/urlmon_moniker.cc:188: ChromeFrameExceptionBarrier barrier;
On 2010/04/27 20:39:42, amit wrote:
> nit: please move this to the top as we have seen RPCRT exceptions in
> BindContextInfo::FromBindContext

Done.

[amit, 2010-04-27 21:16:03]

lgtm++

[stoyan, 2010-04-27 21:35:50]

http://codereview.chromium.org/1733021/diff/26001/27005
File chrome_frame/chrome_frame_exception_barrier.cc (right):

http://codereview.chromium.org/1733021/diff/26001/27005#newcode14
chrome_frame/chrome_frame_exception_barrier.cc:14: extern
google_breakpad::ExceptionHandler* g_breakpad;
these two lines seems unused.

http://codereview.chromium.org/1733021/diff/26001/27005#newcode18
chrome_frame/chrome_frame_exception_barrier.cc:18:
ExceptionBarrier::set_handler(BreakpadHandler);
Hm. I don't like this.
We don't need to set the static ExceptionBarrier::s_handler_ variable everytime
we create an instance of exception barrier.

So in short ChromeFrameExceptionBarrier class may not be needed.

[stoyan, 2010-04-27 21:38:59]

http://codereview.chromium.org/1733021/diff/26001/27007
File chrome_frame/exception_barrier.h (right):

http://codereview.chromium.org/1733021/diff/26001/27007#newcode57
chrome_frame/exception_barrier.h:57: virtual ~ExceptionBarrier();
Is the virtual really needed?

[robertshield, 2010-04-27 23:42:01]

Thanks, please take another look.

http://codereview.chromium.org/1733021/diff/26001/27005
File chrome_frame/chrome_frame_exception_barrier.cc (right):

http://codereview.chromium.org/1733021/diff/26001/27005#newcode14
chrome_frame/chrome_frame_exception_barrier.cc:14: extern
google_breakpad::ExceptionHandler* g_breakpad;
On 2010/04/27 21:35:50, stoyan wrote:
> these two lines seems unused.

old cruft, removed, thanks.

http://codereview.chromium.org/1733021/diff/26001/27005#newcode18
chrome_frame/chrome_frame_exception_barrier.cc:18:
ExceptionBarrier::set_handler(BreakpadHandler);
On 2010/04/27 21:35:50, stoyan wrote:
> Hm. I don't like this.
> We don't need to set the static ExceptionBarrier::s_handler_ variable
everytime
> we create an instance of exception barrier.
> 
> So in short ChromeFrameExceptionBarrier class may not be needed.

Hrmm.. you're right.. setting the static variable all the time is silly. I've
removed this class and now I set it once in InitializeCrashReporting().

http://codereview.chromium.org/1733021/diff/26001/27007
File chrome_frame/exception_barrier.h (right):

http://codereview.chromium.org/1733021/diff/26001/27007#newcode57
chrome_frame/exception_barrier.h:57: virtual ~ExceptionBarrier();
On 2010/04/27 21:38:59, stoyan wrote:
> Is the virtual really needed?

nope, removed.

[stoyan, 2010-04-28 00:18:02]

(blind) lgtm

[Sigurður Ásgeirsson, 2010-04-28 01:54:10]

There used to be a test for the EB as well, is it not in Omaha?

http://codereview.chromium.org/1733021/diff/47001/48002
File chrome_frame/crash_reporting/crash_report.cc (right):

http://codereview.chromium.org/1733021/diff/47001/48002#newcode16
chrome_frame/crash_reporting/crash_report.cc:16: static Lock g_breakpad_lock;
comment on what this lock protects, please. I'm not sure this helps any problem
we have. Because e.g. VEH is process global state, I don't know that we can
really ever totally safely tear down our handlers, etc.

http://codereview.chromium.org/1733021/diff/47001/48003
File chrome_frame/crash_reporting/crash_report.h (right):

http://codereview.chromium.org/1733021/diff/47001/48003#newcode12
chrome_frame/crash_reporting/crash_report.h:12: #include "base/lock.h"
you need this?

[robertshield, 2010-04-28 10:45:57]

Thanks Siggi, I'll address the comments in another patch (hopefully today). 

I couldn't find a unit test for the ExceptionBarrier in the Omaha code.

http://codereview.chromium.org/1733021/diff/47001/48002
File chrome_frame/crash_reporting/crash_report.cc (right):

http://codereview.chromium.org/1733021/diff/47001/48002#newcode16
chrome_frame/crash_reporting/crash_report.cc:16: static Lock g_breakpad_lock;
On 2010/04/28 01:54:10, Ruðrugis wrote:
> comment on what this lock protects, please. I'm not sure this helps any
problem
> we have. Because e.g. VEH is process global state, I don't know that we can
> really ever totally safely tear down our handlers, etc.

Will add a comment in another patch. Generally: this protects access to the
g_breakpad instance since it can now be accessed from any thread that has an
ExceptionBarrier on its stack as well as during setup and teardown of the veh.
It might be demonstrable that this isn't necessary, but I couldn't prove it to
my satisfaction.

http://codereview.chromium.org/1733021/diff/47001/48003
File chrome_frame/crash_reporting/crash_report.h (right):

http://codereview.chromium.org/1733021/diff/47001/48003#newcode12
chrome_frame/crash_reporting/crash_report.h:12: #include "base/lock.h"
On 2010/04/28 01:54:10, Ruðrugis wrote:
> you need this?

ugh, no. Will remove in forthcoming patch.

[Sigurður Ásgeirsson, 2010-04-28 14:27:21]

On Wed, Apr 28, 2010 at 6:45 AM, <robertshield@chromium.org> wrote:

> Thanks Siggi, I'll address the comments in another patch (hopefully today).
>
> I couldn't find a unit test for the ExceptionBarrier in the Omaha code.
>
>
>
> http://codereview.chromium.org/1733021/diff/47001/48002
> File chrome_frame/crash_reporting/crash_report.cc (right):
>
> http://codereview.chromium.org/1733021/diff/47001/48002#newcode16
> chrome_frame/crash_reporting/crash_report.cc:16: static Lock
> g_breakpad_lock;
> On 2010/04/28 01:54:10, Ruðrugis wrote:
>
>> comment on what this lock protects, please. I'm not sure this helps
>>
> any problem
>
>> we have. Because e.g. VEH is process global state, I don't know that
>>
> we can
>
>> really ever totally safely tear down our handlers, etc.
>>
>
> Will add a comment in another patch. Generally: this protects access to
> the g_breakpad instance since it can now be accessed from any thread
> that has an ExceptionBarrier on its stack as well as during setup and
> teardown of the veh. It might be demonstrable that this isn't necessary,
> but I couldn't prove it to my satisfaction.
>
> My point is more that this doesn't actually improve things, since the VEH
stuff could be servicing an exception in any thread in the process, and
there's no guarantee that a VEH exception isn't being processed during
teardown - e.g. it's impossible to make this safe, so we might as well not
confuse the issue with partial, but incomplete fixes.

>
> http://codereview.chromium.org/1733021/diff/47001/48003
> File chrome_frame/crash_reporting/crash_report.h (right):
>
> http://codereview.chromium.org/1733021/diff/47001/48003#newcode12
> chrome_frame/crash_reporting/crash_report.h:12: #include "base/lock.h"
> On 2010/04/28 01:54:10, Ruðrugis wrote:
>
>> you need this?
>>
>
> ugh, no. Will remove in forthcoming patch.
>
>
> http://codereview.chromium.org/1733021/show
>