Avoid reporting crashes for exceptions that hit our SEH from calls to the ori...

Avoid reporting crashes for exceptions that hit our SEH from calls to the original implementation of BindToStorage() when we do not wrap the bind status callback.

BUG=42660
TEST=Induce exception in code called under original IMoniker::BindToStorage implementation when we don't wrap the callback and notice that no crash is reported.



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

[tommi (sloooow) - chröme, 2010-04-29 18:48:51]

lgtm

http://codereview.chromium.org/1748016/diff/1/3
File chrome_frame/exception_barrier.cc (right):

http://codereview.chromium.org/1748016/diff/1/3#newcode48
chrome_frame/exception_barrier.cc:48: // unreferenced formal parameters
do we need to do this?

http://codereview.chromium.org/1748016/diff/1/2
File chrome_frame/exception_barrier.h (right):

http://codereview.chromium.org/1748016/diff/1/2#newcode30
chrome_frame/exception_barrier.h:30: void*  establisher_frame,
nit: two spaces

http://codereview.chromium.org/1748016/diff/1/4
File chrome_frame/exception_barrier_lowlevel.asm (right):

http://codereview.chromium.org/1748016/diff/1/4#newcode12
chrome_frame/exception_barrier_lowlevel.asm:12: _ExceptionBarrierMarkerHandler
PROTO
keep this proto with the .SAFESEH one below.

http://codereview.chromium.org/1748016/diff/1/4#newcode17
chrome_frame/exception_barrier_lowlevel.asm:17: ExceptionBarrierMarkerHandler
PROTO
same here

[amit, 2010-04-29 18:58:18]

We have to take crash dumps for crashes with address directly in our code. Also,
ditto for BindToStorage.

[robertshield, 2010-04-29 19:00:53]

Thanks!

http://codereview.chromium.org/1748016/diff/1/3
File chrome_frame/exception_barrier.cc (right):

http://codereview.chromium.org/1748016/diff/1/3#newcode48
chrome_frame/exception_barrier.cc:48: // unreferenced formal parameters
On 2010/04/29 18:48:51, tommi wrote:
> do we need to do this?

I don't really know. We want this function to be as unmolested as possible, and
this might help achieve that, but in truth I just monkeyed what the original
ExceptionBarrierHandler does above. This certainly doesn't hurt.

http://codereview.chromium.org/1748016/diff/1/2
File chrome_frame/exception_barrier.h (right):

http://codereview.chromium.org/1748016/diff/1/2#newcode30
chrome_frame/exception_barrier.h:30: void*  establisher_frame,
On 2010/04/29 18:48:51, tommi wrote:
> nit: two spaces

Fixed.

http://codereview.chromium.org/1748016/diff/1/4
File chrome_frame/exception_barrier_lowlevel.asm (right):

http://codereview.chromium.org/1748016/diff/1/4#newcode12
chrome_frame/exception_barrier_lowlevel.asm:12: _ExceptionBarrierMarkerHandler
PROTO
On 2010/04/29 18:48:51, tommi wrote:
> keep this proto with the .SAFESEH one below.

Done.

http://codereview.chromium.org/1748016/diff/1/4#newcode17
chrome_frame/exception_barrier_lowlevel.asm:17: ExceptionBarrierMarkerHandler
PROTO
On 2010/04/29 18:48:51, tommi wrote:
> same here

Done.

[robertshield, 2010-04-29 20:59:34]

Altered the ExceptionBarrier that reported nothing to instead only report
exceptions in our own module.

Adding Stoyan for any comments on the veh refactoring.

[robertshield, 2010-04-29 21:04:41]

On 2010/04/29 20:59:34, robertshield wrote:
> Altered the ExceptionBarrier that reported nothing to instead only report
> exceptions in our own module.
> 
> Adding Stoyan for any comments on the veh refactoring.

Actually, this isn't right. Calling VectoredHandlerT<E>::Handler() from inside
our SEH is silly, because we will always have an SEH in the chain at that point
and so Handler() will do nothing. Please ignore this revision. Fixing now..

[robertshield, 2010-04-29 21:12:49]

On 2010/04/29 21:04:41, robertshield wrote:
> On 2010/04/29 20:59:34, robertshield wrote:
> > Altered the ExceptionBarrier that reported nothing to instead only report
> > exceptions in our own module.
> > 
> > Adding Stoyan for any comments on the veh refactoring.
> 
> Actually, this isn't right. Calling VectoredHandlerT<E>::Handler() from inside
> our SEH is silly, because we will always have an SEH in the chain at that
point
> and so Handler() will do nothing. Please ignore this revision. Fixing now..

Ok, fixed up. Please take another look.

[amit, 2010-04-29 21:26:22]

http://codereview.chromium.org/1748016/diff/20001/11007
File chrome_frame/crash_reporting/vectored_handler-impl.h (right):

http://codereview.chromium.org/1748016/diff/20001/11007#newcode275
chrome_frame/crash_reporting/vectored_handler-impl.h:275: CrashHandlerTraits()
{}
dump_handler_(NULL)

http://codereview.chromium.org/1748016/diff/20001/11007#newcode296
chrome_frame/crash_reporting/vectored_handler-impl.h:296: return
dump_handler_(p);
if (dump_handler_)

http://codereview.chromium.org/1748016/diff/20001/11002
File chrome_frame/exception_barrier.h (right):

http://codereview.chromium.org/1748016/diff/20001/11002#newcode113
chrome_frame/exception_barrier.h:113: class ExceptionBarrierReportOnlyModule {
nit: ExceptionBarrierReportOnlyModule  looks very similar to ExceptionBarrier.
How can we factor this out?

http://codereview.chromium.org/1748016/diff/20001/11005
File chrome_frame/urlmon_moniker.cc (right):

http://codereview.chromium.org/1748016/diff/20001/11005#newcode165
chrome_frame/urlmon_moniker.cc:165: ExceptionBarrier barrier;
Since we don't do any wrapping here, it's better to use
ExceptionBarrierReportOnlyModule.

[stoyan, 2010-04-29 21:46:51]

Things seems to become messy.
In general we want to have ExceptionBarrier with different handlers since we
have some special logic inside.
The correct way to implement this is either to specify handler when we construct
ExceptionBarrier object in stack, or to have different ExceptionBarrier-like
classes that keep the handler as a static member and are initialized once (to
set this static member).
In the second case you may try some template tricks to achieve this.

The correct place for place handlers seems to be inside CrashHandler class. We
have VectoredHandlerT<CrashHandlerTraits> handler_ member there and the entry
point - VectoredHandlerEntryPoint. So adding multiple SEH_Barrier handler(s) and
entry point(s) seems reasonable.


Side note: In InitializeCrashReporting()
ExceptionBarrier::set_handler(BreakpadHandler); neglects the settings. So we may
take a dump even if the user does not opt-in for that.
ExceptionBarrierHandler checks whether the handler is zero, so we are safe here.

http://codereview.chromium.org/1748016/diff/28001/29005
File chrome_frame/crash_reporting/crash_report.cc (right):

http://codereview.chromium.org/1748016/diff/28001/29005#newcode91
chrome_frame/crash_reporting/crash_report.cc:91:
crash_api_.Init(&veh_segment_start, &veh_segment_end,
This line should be before ::AddVectoredExceptionHandler. It's probably my fault

http://codereview.chromium.org/1748016/diff/28001/29001
File chrome_frame/exception_barrier.h (right):

http://codereview.chromium.org/1748016/diff/28001/29001#newcode109
chrome_frame/exception_barrier.h:109: /// ExceptionBarrierMarkerHandler function
which does nothing other than return
ExceptionBarrierMarkerHandler?

[robertshield, 2010-04-30 19:24:20]

I have refactored the ExceptionBarrier code quite a bit, following one of
Stoyan's suggestions. Different exception handlers are now registered based on
the type of ExceptionBarrier object created.

I left the "register custom handler" functionality in there for the unit tests.

@stoyan: Note that the earlier call to initialize crash reporting in
InitializeCrashReporting() did not ignore the settings, if that code was
reached, then either we are in headless mode (which is only for testing) or the
user wants to report dumps. (Correct me if I am wrong about the intent of
headless mode).

http://codereview.chromium.org/1748016/diff/20001/11007
File chrome_frame/crash_reporting/vectored_handler-impl.h (right):

http://codereview.chromium.org/1748016/diff/20001/11007#newcode275
chrome_frame/crash_reporting/vectored_handler-impl.h:275: CrashHandlerTraits()
{}
On 2010/04/29 21:26:22, amit wrote:
> dump_handler_(NULL)

Done.

http://codereview.chromium.org/1748016/diff/20001/11007#newcode296
chrome_frame/crash_reporting/vectored_handler-impl.h:296: return
dump_handler_(p);
On 2010/04/29 21:26:22, amit wrote:
> if (dump_handler_)

Done.

http://codereview.chromium.org/1748016/diff/20001/11005
File chrome_frame/urlmon_moniker.cc (right):

http://codereview.chromium.org/1748016/diff/20001/11005#newcode165
chrome_frame/urlmon_moniker.cc:165: ExceptionBarrier barrier;
On 2010/04/29 21:26:22, amit wrote:
> Since we don't do any wrapping here, it's better to use
> ExceptionBarrierReportOnlyModule.

Done.

http://codereview.chromium.org/1748016/diff/28001/29005
File chrome_frame/crash_reporting/crash_report.cc (right):

http://codereview.chromium.org/1748016/diff/28001/29005#newcode91
chrome_frame/crash_reporting/crash_report.cc:91:
crash_api_.Init(&veh_segment_start, &veh_segment_end,
On 2010/04/29 21:46:52, stoyan wrote:
> This line should be before ::AddVectoredExceptionHandler. It's probably my
fault

Done.

http://codereview.chromium.org/1748016/diff/28001/29001
File chrome_frame/exception_barrier.h (right):

http://codereview.chromium.org/1748016/diff/28001/29001#newcode109
chrome_frame/exception_barrier.h:109: /// ExceptionBarrierMarkerHandler function
which does nothing other than return
On 2010/04/29 21:46:52, stoyan wrote:
> ExceptionBarrierMarkerHandler?

fixed, thanks

[amit, 2010-04-30 20:27:27]

http://codereview.chromium.org/1748016/diff/35001/36002
File chrome_frame/exception_barrier.cc (right):

http://codereview.chromium.org/1748016/diff/35001/36002#newcode20
chrome_frame/exception_barrier.cc:20:
ExceptionBarrierCustomHandler::s_custom_handler_ = NULL;
need indent?

http://codereview.chromium.org/1748016/diff/35001/36001
File chrome_frame/exception_barrier.h (right):

http://codereview.chromium.org/1748016/diff/35001/36001#newcode52
chrome_frame/exception_barrier.h:52: // This is the type dictated for an
exception handler by the platform ABI
nit: *API?

http://codereview.chromium.org/1748016/diff/35001/36001#newcode124
chrome_frame/exception_barrier.h:124: static bool s_report_crashes_;
nit: rename to enabled_? barrier->enabled() is probably more readable than
barrier->crash_handling().

http://codereview.chromium.org/1748016/diff/35001/36001#newcode141
chrome_frame/exception_barrier.h:141: inline
ExceptionBarrierT<EB_REPORT_ALL>::ExceptionBarrierT() {
This feels awkward since we are using an artificial enum for specialization.
ExceptionBarrier could simply take the handler in the ctor. If you want to have
a nice typedef, then the traits could implement Register and Unregister so that
ExceptionBarrierT can take traits as template arg.

[robertshield, 2010-04-30 21:29:29]

Please take another look.

http://codereview.chromium.org/1748016/diff/35001/36002
File chrome_frame/exception_barrier.cc (right):

http://codereview.chromium.org/1748016/diff/35001/36002#newcode20
chrome_frame/exception_barrier.cc:20:
ExceptionBarrierCustomHandler::s_custom_handler_ = NULL;
On 2010/04/30 20:27:27, amit wrote:
> need indent?

I don't believe so, I think the variable name is supposed to be flush when the
type_name + variable_name is longer than one line.

http://codereview.chromium.org/1748016/diff/35001/36001
File chrome_frame/exception_barrier.h (right):

http://codereview.chromium.org/1748016/diff/35001/36001#newcode52
chrome_frame/exception_barrier.h:52: // This is the type dictated for an
exception handler by the platform ABI
On 2010/04/30 20:27:27, amit wrote:
> nit: *API?

I believe the structure of an exception handler is a binary interface, I think
ABI is accurate here.

http://codereview.chromium.org/1748016/diff/35001/36001#newcode124
chrome_frame/exception_barrier.h:124: static bool s_report_crashes_;
On 2010/04/30 20:27:27, amit wrote:
> nit: rename to enabled_? barrier->enabled() is probably more readable than
> barrier->crash_handling().

Done.

http://codereview.chromium.org/1748016/diff/35001/36001#newcode141
chrome_frame/exception_barrier.h:141: inline
ExceptionBarrierT<EB_REPORT_ALL>::ExceptionBarrierT() {
On 2010/04/30 20:27:27, amit wrote:
> This feels awkward since we are using an artificial enum for specialization.
> ExceptionBarrier could simply take the handler in the ctor. If you want to
have
> a nice typedef, then the traits could implement Register and Unregister so
that
> ExceptionBarrierT can take traits as template arg.

Changed so that traits do the registering.

[amit, 2010-04-30 21:43:25]

lgtm

[stoyan, 2010-04-30 22:44:00]

http://codereview.chromium.org/1748016/diff/29008/41002
File chrome_frame/exception_barrier.cc (right):

http://codereview.chromium.org/1748016/diff/29008/41002#newcode55
chrome_frame/exception_barrier.cc:55: CrashHandlerTraits traits;
you know I really don't like this. :)

[robertshield, 2010-05-01 02:10:30]

http://codereview.chromium.org/1748016/diff/35001/36002
File chrome_frame/exception_barrier.cc (right):

http://codereview.chromium.org/1748016/diff/35001/36002#newcode20
chrome_frame/exception_barrier.cc:20:
ExceptionBarrierCustomHandler::s_custom_handler_ = NULL;
On 2010/04/30 21:29:29, robertshield wrote:
> On 2010/04/30 20:27:27, amit wrote:
> > need indent?
> 
> I don't believe so, I think the variable name is supposed to be flush when the
> type_name + variable_name is longer than one line.

I have had two requests for indent, so I added one.

[robert, 2010-05-01 02:17:42]

So I checked this in (with reservations), since it is more useful than what
we had. Stoyan had a good idea about a better approach that I will
investigate as soon as I can.

On Fri, Apr 30, 2010 at 10:10 PM, <robertshield@chromium.org> wrote:

>
> http://codereview.chromium.org/1748016/diff/35001/36002
>
> File chrome_frame/exception_barrier.cc (right):
>
> http://codereview.chromium.org/1748016/diff/35001/36002#newcode20
> chrome_frame/exception_barrier.cc:20:
> ExceptionBarrierCustomHandler::s_custom_handler_ = NULL;
> On 2010/04/30 21:29:29, robertshield wrote:
>
>> On 2010/04/30 20:27:27, amit wrote:
>> > need indent?
>>
>
>  I don't believe so, I think the variable name is supposed to be flush
>>
> when the
>
>> type_name + variable_name is longer than one line.
>>
>
> I have had two requests for indent, so I added one.
>
>
> http://codereview.chromium.org/1748016/show
>

[amit, 2010-05-01 23:28:45]

Excellent, thanks!

On Fri, Apr 30, 2010 at 7:17 PM, Robert Shield <robertshield@google.com>wrote:

> So I checked this in (with reservations), since it is more useful than what
> we had. Stoyan had a good idea about a better approach that I will
> investigate as soon as I can.
>
>
> On Fri, Apr 30, 2010 at 10:10 PM, <robertshield@chromium.org> wrote:
>
>>
>> http://codereview.chromium.org/1748016/diff/35001/36002
>>
>> File chrome_frame/exception_barrier.cc (right):
>>
>> http://codereview.chromium.org/1748016/diff/35001/36002#newcode20
>> chrome_frame/exception_barrier.cc:20:
>> ExceptionBarrierCustomHandler::s_custom_handler_ = NULL;
>> On 2010/04/30 21:29:29, robertshield wrote:
>>
>>> On 2010/04/30 20:27:27, amit wrote:
>>> > need indent?
>>>
>>
>>  I don't believe so, I think the variable name is supposed to be flush
>>>
>> when the
>>
>>> type_name + variable_name is longer than one line.
>>>
>>
>> I have had two requests for indent, so I added one.
>>
>>
>> http://codereview.chromium.org/1748016/show
>>
>
>