Stability instrumentation: add a Vectored Exception Handler

Stability instrumentation: add a Vectored Exception Handler

Adds a vectored exception handler that records basic exception
information (exception code and address, as well as pc).

Details:
- the handler is registered as first handler, but may get bumped due
to subsequent registrations
- in order for the exception info to be recorded, a thread tracker
must already exist

BUG=719026
Review-Url: https://codereview.chromium.org/2927653003
Cr-Commit-Position: refs/heads/master@{#479498}
Committed: https://chromium.googlesource.com/chromium/src/+/e17e32d3fba9614e1dc54ffd21aa3ae56409a9f6

[manzagop (departed), 2017-06-07 16:06:48]

Description was changed from

==========
First version of VEH

BUG=
==========

to

==========
Stability instrumentation: add a Vectored Exception Handler

Adds a vectored exception handler that records basic exception
information (exception code and address, as well as pc).

Details:
- the handler is registered as first handler, but may get bumped due
to subsequent registrations
- in order for the exception info to be recorded, a thread tracker
must already exist

BUG=719026
==========

[manzagop (departed), 2017-06-07 16:15:17]

manzagop@chromium.org changed reviewers:
+ siggi@chromium.org

[manzagop (departed), 2017-06-07 16:15:18]

Hi Siggi,

This is still missing a test, but could you have a first look?

Specific questions:

1) I need to invoke RemoveVectoredExceptionHandler, right? Any suggestion as to
where to do this?

2) Offhand, are you concerned with other VEH handlers cutting in front and
handling the exception?

3) Recording the exceptions requires the thread to have created a thread tracker
ahead of time. I've seen some reports where ~50% of threads are instrumented, as
instrumentation happens the first time something is recorded (running a task,
waiting, etc). Maybe we need to tickle the instrumentation early to ensure
better coverage.

Thanks!

P-A

[Sigurður Ásgeirsson, 2017-06-07 17:43:25]

nice - could do with testing as you note :).

https://codereview.chromium.org/2927653003/diff/40001/components/browser_watc...
File components/browser_watcher/stability_debugging.cc (right):

https://codereview.chromium.org/2927653003/diff/40001/components/browser_watc...
components/browser_watcher/stability_debugging.cc:16: // A registered vecotred
exception handler.
speling [sic]

https://codereview.chromium.org/2927653003/diff/40001/components/browser_watc...
components/browser_watcher/stability_debugging.cc:27: LONG CALLBACK
VectoredExceptionHandler(PEXCEPTION_POINTERS exception_pointers) {
I think we shy away from the Hungarian notation PFoo, and use Foo* instead?

https://codereview.chromium.org/2927653003/diff/40001/components/browser_watc...
components/browser_watcher/stability_debugging.cc:29:
base::debug::GlobalActivityTracker::Get();
QQ: is this a straight-up accessor, or can this cause allocations and such? If
the latter, maybe you want to implement a straight-up accessor, as allocations
and locking and such in vectored exception handling is bad...

https://codereview.chromium.org/2927653003/diff/40001/components/browser_watc...
components/browser_watcher/stability_debugging.cc:68: // DO NOT SUBMIT: figure
out when to ::RemoveVectoredExceptionHandler.
pro tip, there really isn't a good time to RemoveVEH, as is true for all kinds
of registrations that are process global. In Chrome this'd be a non-issue, as
chrome.dll doesn't really unload, but using e.g. a static uninitializer or any
other DllMain-time invocation would be your best bet.

[manzagop (departed), 2017-06-08 22:46:38]

Thanks! Another look?

Note that a big chunk of the diff between ps 4 and 5 is moving some tests from
stability_debugging_win_unittest.cc to stability_paths_unittest.cc.

https://codereview.chromium.org/2927653003/diff/40001/components/browser_watc...
File components/browser_watcher/stability_debugging.cc (right):

https://codereview.chromium.org/2927653003/diff/40001/components/browser_watc...
components/browser_watcher/stability_debugging.cc:16: // A registered vecotred
exception handler.
On 2017/06/07 17:43:25, Sigurður Ásgeirsson wrote:
> speling [sic]

Done.

https://codereview.chromium.org/2927653003/diff/40001/components/browser_watc...
components/browser_watcher/stability_debugging.cc:27: LONG CALLBACK
VectoredExceptionHandler(PEXCEPTION_POINTERS exception_pointers) {
On 2017/06/07 17:43:25, Sigurður Ásgeirsson wrote:
> I think we shy away from the Hungarian notation PFoo, and use Foo* instead?

Done.

https://codereview.chromium.org/2927653003/diff/40001/components/browser_watc...
components/browser_watcher/stability_debugging.cc:29:
base::debug::GlobalActivityTracker::Get();
On 2017/06/07 17:43:25, Sigurður Ásgeirsson wrote:
> QQ: is this a straight-up accessor, or can this cause allocations and such? If
> the latter, maybe you want to implement a straight-up accessor, as allocations
> and locking and such in vectored exception handling is bad...

This is a straight up accessor. ACK.

https://codereview.chromium.org/2927653003/diff/40001/components/browser_watc...
components/browser_watcher/stability_debugging.cc:68: // DO NOT SUBMIT: figure
out when to ::RemoveVectoredExceptionHandler.
On 2017/06/07 17:43:25, Sigurður Ásgeirsson wrote:
> pro tip, there really isn't a good time to RemoveVEH, as is true for all kinds
> of registrations that are process global. In Chrome this'd be a non-issue, as
> chrome.dll doesn't really unload, but using e.g. a static uninitializer or any
> other DllMain-time invocation would be your best bet.

Does this look ok?

[Sigurður Ásgeirsson, 2017-06-09 13:55:16]

Nice, I think the testing could be a little more robust, but otherwise looks
good!

https://codereview.chromium.org/2927653003/diff/40001/components/browser_watc...
File components/browser_watcher/stability_debugging.cc (right):

https://codereview.chromium.org/2927653003/diff/40001/components/browser_watc...
components/browser_watcher/stability_debugging.cc:68: // DO NOT SUBMIT: figure
out when to ::RemoveVectoredExceptionHandler.
On 2017/06/08 22:46:38, manzagop wrote:
> On 2017/06/07 17:43:25, Sigurður Ásgeirsson wrote:
> > pro tip, there really isn't a good time to RemoveVEH, as is true for all
kinds
> > of registrations that are process global. In Chrome this'd be a non-issue,
as
> > chrome.dll doesn't really unload, but using e.g. a static uninitializer or
any
> > other DllMain-time invocation would be your best bet.
> 
> Does this look ok?

looks good to me.

https://codereview.chromium.org/2927653003/diff/80001/components/browser_watc...
File components/browser_watcher/stability_debugging_win_unittest.cc (right):

https://codereview.chromium.org/2927653003/diff/80001/components/browser_watc...
components/browser_watcher/stability_debugging_win_unittest.cc:64: char* invalid
= nullptr;
I'm not sure the compiler is obliged to honor your intent here. Also it's
probably nice to validate that a caught exception can occur without terminating
the process (which could happen in case of an error in VEH).
So, maybe better to RaiseException with a distinct exception code from within a
__try, then return a different exit code?

https://codereview.chromium.org/2927653003/diff/80001/components/browser_watc...
components/browser_watcher/stability_debugging_win_unittest.cc:80: // Validate
expectations: 1 process with 1 thread that has an exception.
My bet is that this is going to flake a bit, as you'll have all and sundry
piling into the test process and suffering exceptions now and then. More robust
is to have "at least one" test here, for a distinct exception code.

https://codereview.chromium.org/2927653003/diff/80001/components/browser_watc...
File components/browser_watcher/stability_report_extractor_unittest.cc (right):

https://codereview.chromium.org/2927653003/diff/80001/components/browser_watc...
components/browser_watcher/stability_report_extractor_unittest.cc:259:
TEST_F(StabilityReportExtractorThreadTrackerTest, CollectNoException) {
nice testing, I like it!

[manzagop (departed), 2017-06-09 21:27:35]

Another look?

https://codereview.chromium.org/2927653003/diff/40001/components/browser_watc...
File components/browser_watcher/stability_debugging.cc (right):

https://codereview.chromium.org/2927653003/diff/40001/components/browser_watc...
components/browser_watcher/stability_debugging.cc:68: // DO NOT SUBMIT: figure
out when to ::RemoveVectoredExceptionHandler.
On 2017/06/09 13:55:15, Sigurður Ásgeirsson wrote:
> On 2017/06/08 22:46:38, manzagop wrote:
> > On 2017/06/07 17:43:25, Sigurður Ásgeirsson wrote:
> > > pro tip, there really isn't a good time to RemoveVEH, as is true for all
> kinds
> > > of registrations that are process global. In Chrome this'd be a non-issue,
> as
> > > chrome.dll doesn't really unload, but using e.g. a static uninitializer or
> any
> > > other DllMain-time invocation would be your best bet.
> > 
> > Does this look ok?
> 
> looks good to me.

Acknowledged.

https://codereview.chromium.org/2927653003/diff/80001/components/browser_watc...
File components/browser_watcher/stability_debugging_win_unittest.cc (right):

https://codereview.chromium.org/2927653003/diff/80001/components/browser_watc...
components/browser_watcher/stability_debugging_win_unittest.cc:64: char* invalid
= nullptr;
On 2017/06/09 13:55:15, Sigurður Ásgeirsson wrote:
> I'm not sure the compiler is obliged to honor your intent here. Also it's
> probably nice to validate that a caught exception can occur without
terminating
> the process (which could happen in case of an error in VEH).
> So, maybe better to RaiseException with a distinct exception code from within
a
> __try, then return a different exit code?

Switched to a single process test, as that's simpler if the exception is
recovered from (otherwise we'd need to synchronize the processes to avoid crumb
file cleanup on exit).

https://codereview.chromium.org/2927653003/diff/80001/components/browser_watc...
components/browser_watcher/stability_debugging_win_unittest.cc:80: // Validate
expectations: 1 process with 1 thread that has an exception.
On 2017/06/09 13:55:15, Sigurður Ásgeirsson wrote:
> My bet is that this is going to flake a bit, as you'll have all and sundry
> piling into the test process and suffering exceptions now and then. More
robust
> is to have "at least one" test here, for a distinct exception code.

Sgtm! Actually, I'm in the same thread it's easy to look for a specific thread. 

Done.

https://codereview.chromium.org/2927653003/diff/80001/components/browser_watc...
File components/browser_watcher/stability_report_extractor_unittest.cc (right):

https://codereview.chromium.org/2927653003/diff/80001/components/browser_watc...
components/browser_watcher/stability_report_extractor_unittest.cc:259:
TEST_F(StabilityReportExtractorThreadTrackerTest, CollectNoException) {
On 2017/06/09 13:55:15, Sigurður Ásgeirsson wrote:
> nice testing, I like it!

Thanks you sir! Ack.

[manzagop (departed), 2017-06-09 21:45:55]

The CQ bit was checked by manzagop@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-09 21:46:10]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-09 23:18:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-09 23:18:50]

Dry run: This issue passed the CQ dry run.

[Sigurður Ásgeirsson, 2017-06-12 13:02:45]

lgtm

[manzagop (departed), 2017-06-12 13:38:02]

Thanks!

[manzagop (departed), 2017-06-12 13:39:12]

The CQ bit was checked by manzagop@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-12 13:39:19]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[manzagop (departed), 2017-06-12 13:40:30]

Hi Brian, Rob,

Could you have an OWNERS' look?

Brian: base\debug\activity_tracker.*
Rob: chrome\browser\chrome_browser_field_trials_desktop.cc

Thanks!

Pierre-Antoine

[commit-bot: I haz the power, 2017-06-12 15:17:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-12 15:17:57]

Dry run: This issue passed the CQ dry run.

[manzagop (departed), 2017-06-13 13:27:17]

manzagop@chromium.org changed reviewers:
+ bcwhite@chromium.org, rkaplow@chromium.org

[manzagop (departed), 2017-06-13 13:27:17]

Adding reviewers, for realz.

[manzagop (departed), 2017-06-13 13:27:51]

On 2017/06/13 13:27:17, manzagop wrote:
> Adding reviewers, for realz.

The original message:

Hi Brian, Rob,

Could you have an OWNERS' look?

Brian: base\debug\activity_tracker.*
Rob: chrome\browser\chrome_browser_field_trials_desktop.cc

Thanks!

[bcwhite, 2017-06-13 18:14:54]

lgtm

[manzagop (departed), 2017-06-14 14:05:07]

On 2017/06/13 18:14:54, bcwhite wrote:
> lgtm

Thanks!

[rkaplow, 2017-06-14 18:50:46]

lgtm

[manzagop (departed), 2017-06-14 19:03:44]

The CQ bit was checked by manzagop@chromium.org

[manzagop (departed), 2017-06-14 19:03:45]

The patchset sent to the CQ was uploaded after l-g-t-m from siggi@chromium.org
Link to the patchset: https://codereview.chromium.org/2927653003/#ps140001
(title: "merge")

[commit-bot: I haz the power, 2017-06-14 19:04:22]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[manzagop (departed), 2017-06-14 19:04:23]

On 2017/06/14 18:50:46, rkaplow wrote:
> lgtm

Thanks for the reviews!

[commit-bot: I haz the power, 2017-06-14 20:51:30]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1497467024840390,
"parent_rev": "cbed9b93405ead414c3acd8d2b199af9029941cb", "commit_rev":
"e17e32d3fba9614e1dc54ffd21aa3ae56409a9f6"}

[commit-bot: I haz the power, 2017-06-14 20:51:44]

Description was changed from

==========
Stability instrumentation: add a Vectored Exception Handler

Adds a vectored exception handler that records basic exception
information (exception code and address, as well as pc).

Details:
- the handler is registered as first handler, but may get bumped due
to subsequent registrations
- in order for the exception info to be recorded, a thread tracker
must already exist

BUG=719026
==========

to

==========
Stability instrumentation: add a Vectored Exception Handler

Adds a vectored exception handler that records basic exception
information (exception code and address, as well as pc).

Details:
- the handler is registered as first handler, but may get bumped due
to subsequent registrations
- in order for the exception info to be recorded, a thread tracker
must already exist

BUG=719026

Review-Url: https://codereview.chromium.org/2927653003
Cr-Commit-Position: refs/heads/master@{#479498}
Committed:
https://chromium.googlesource.com/chromium/src/+/e17e32d3fba9614e1dc54ffd21aa...
==========

[commit-bot: I haz the power, 2017-06-14 20:51:48]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/e17e32d3fba9614e1dc54ffd21aa...