Add new exception code for OOM generated from Chromium.

Add new exception code for OOM generated from Chromium.

See also https://codereview.chromium.org/2130293003/ for Chromium-side change and go/internal_cl_for_2130293003 for internal change.

BUG=chromium:614440
R=mark@chromium.org

Committed: https://chromium.googlesource.com/breakpad/breakpad/+/c99d374dde62654a024840accfb357b2851daea0

[Will Harris, 2016-07-19 20:21:26]

Description was changed from

==========
Add new exception code for OOM generated from Chromium.

BUG=chromium:614440
==========

to

==========
Add new exception code for OOM generated from Chromium.

BUG=chromium:614440
==========

[Will Harris, 2016-07-19 20:21:26]

wfh@chromium.org changed reviewers:
+ mark@chromium.org

[Will Harris, 2016-07-19 20:21:56]

Description was changed from

==========
Add new exception code for OOM generated from Chromium.

BUG=chromium:614440
==========

to

==========
Add new exception code for OOM generated from Chromium.

See also https://codereview.chromium.org/2130293003/ for Chromium-side change
and go/internal_cl_for_2130293003 for internal change.

BUG=chromium:614440
==========

[Will Harris, 2016-07-19 20:22:26]

PTAL?

[Mark Mentovai, 2016-07-19 20:30:50]

LGTM otherwise

https://codereview.chromium.org/2160373002/diff/1/src/google_breakpad/common/...
File src/google_breakpad/common/minidump_exception_win32.h (right):

https://codereview.chromium.org/2160373002/diff/1/src/google_breakpad/common/...
src/google_breakpad/common/minidump_exception_win32.h:104: /* Exception thrown
by Chromium allocators to indicate OOM. */
Wondering how you picked this. Is it a known user-defined range? The comment may
want to say if there’s anything significant to the choice.

https://codereview.chromium.org/2160373002/diff/1/src/processor/minidump_proc...
File src/processor/minidump_processor.cc (right):

https://codereview.chromium.org/2160373002/diff/1/src/processor/minidump_proc...
src/processor/minidump_processor.cc:1190: reason = "EXCEPTION_OUT_OF_MEMORY";
This makes it sound like it’s a constant from the Windows SDK, but it’s not. The
string here should make it sound more Chromey. It doesn’t need to SHOUT, and if
there’s a better constant name used for it in Chrome-land, we can use that here.

[Will Harris, 2016-07-19 21:30:35]

Thanks for reviews.

https://codereview.chromium.org/2160373002/diff/1/src/google_breakpad/common/...
File src/google_breakpad/common/minidump_exception_win32.h (right):

https://codereview.chromium.org/2160373002/diff/1/src/google_breakpad/common/...
src/google_breakpad/common/minidump_exception_win32.h:104: /* Exception thrown
by Chromium allocators to indicate OOM. */
On 2016/07/19 20:30:50, Mark Mentovai wrote:
> Wondering how you picked this. Is it a known user-defined range? The comment
may
> want to say if there’s anything significant to the choice.

see new comment in base/process/memory.h

https://codereview.chromium.org/2160373002/diff/1/src/processor/minidump_proc...
File src/processor/minidump_processor.cc (right):

https://codereview.chromium.org/2160373002/diff/1/src/processor/minidump_proc...
src/processor/minidump_processor.cc:1190: reason = "EXCEPTION_OUT_OF_MEMORY";
On 2016/07/19 20:30:50, Mark Mentovai wrote:
> This makes it sound like it’s a constant from the Windows SDK, but it’s not.
The
> string here should make it sound more Chromey. It doesn’t need to SHOUT, and
if
> there’s a better constant name used for it in Chrome-land, we can use that
here.

yes I agree, I've changed this to "Out of Memory". This is the string that
appears in crash/ for the crash, so makes sense for it to just say that.

[Mark Mentovai, 2016-07-19 21:32:23]

LGTM

[Will Harris, 2016-07-19 21:41:59]

Description was changed from

==========
Add new exception code for OOM generated from Chromium.

See also https://codereview.chromium.org/2130293003/ for Chromium-side change
and go/internal_cl_for_2130293003 for internal change.

BUG=chromium:614440
==========

to

==========
Add new exception code for OOM generated from Chromium.

See also https://codereview.chromium.org/2130293003/ for Chromium-side change
and go/internal_cl_for_2130293003 for internal change.

BUG=chromium:614440
R=mark@chromium.org

Committed:
https://chromium.googlesource.com/breakpad/breakpad/+/c99d374dde62654a024840a...
==========

[Will Harris, 2016-07-19 21:42:01]

Committed patchset #2 (id:20001) manually as
c99d374dde62654a024840accfb357b2851daea0 (presubmit successful).

[Ted Mielczarek, 2016-07-20 16:14:43]

Not really relevant to this patch, but to the Chromium changes it's
handling--had you considered stuffing the allocation size into the exception
arguments? I think it'd just be a matter of passing 1 and a pointer to the
allocation size as the last two parameters to `RaiseException`, and presumably
we'd then be able to pull it out of the `MDException` in the processor.

[Will Harris, 2016-07-20 16:17:25]

On 2016/07/20 16:14:43, Ted Mielczarek wrote:
> Not really relevant to this patch, but to the Chromium changes it's
> handling--had you considered stuffing the allocation size into the exception
> arguments? I think it'd just be a matter of passing 1 and a pointer to the
> allocation size as the last two parameters to `RaiseException`, and presumably
> we'd then be able to pull it out of the `MDException` in the processor.

that's a really cool idea, especially as our crash processors can parse out the
exception arguments... I will take a look.