Populate allocation size into OOM exception on Windows.

Populate allocation size into OOM exception on Windows.

This will allow exposing the attempted allocation size on the crash backend.
Also set up for OOM handling in the crashpad-handler process.

BUG=683158
Review-Url: https://codereview.chromium.org/2674253003
Cr-Commit-Position: refs/heads/master@{#448383}
Committed: https://chromium.googlesource.com/chromium/src/+/7a7fbf7c5016a85e4610b9e2fe653a77807e932b

[Sigurður Ásgeirsson, 2017-02-06 18:00:51]

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

[commit-bot: I haz the power, 2017-02-06 18:01:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

siggi@chromium.org changed reviewers:
+ mark@chromium.org, scottmg@chromium.org, wfh@chromium.org

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

Hey guys,

I think this is an easy way to get the allocation size into Crash. I'd like to
be able to aggregate on attempted allocation size for OOM crashes.

Siggi

[Will Harris, 2017-02-06 18:03:53]

On 2017/02/06 18:02:45, Sigurður Ásgeirsson wrote:
> Hey guys,
> 
> I think this is an easy way to get the allocation size into Crash. I'd like to
> be able to aggregate on attempted allocation size for OOM crashes.
> 
> Siggi

Can you clarify who is reviewing which part, or specifically what you want from
the three reviewers? I am not an owner of any of this code.

[Sigurður Ásgeirsson, 2017-02-06 18:04:41]

On 2017/02/06 18:02:45, Sigurður Ásgeirsson wrote:
> Hey guys,
> 
> I think this is an easy way to get the allocation size into Crash. I'd like to
> be able to aggregate on attempted allocation size for OOM crashes.
> 
> Siggi

Also, Will for memory_win.cc, scott for run_as_crashpad_handler.cc, and mark for
OWNERS please.

[scottmg, 2017-02-06 18:17:42]

lgtm, but Will would know better if adding exception args is going to confuse
something.

https://codereview.chromium.org/2674253003/diff/1/base/process/memory_win.cc
File base/process/memory_win.cc (right):

https://codereview.chromium.org/2674253003/diff/1/base/process/memory_win.cc#...
base/process/memory_win.cc:38: // Pass the size of the failed request
'.' at end of sentence.

[Sigurður Ásgeirsson, 2017-02-06 18:22:06]

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

[commit-bot: I haz the power, 2017-02-06 18:22:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Mark Mentovai, 2017-02-06 18:22:39]

LGTM otherwise

https://codereview.chromium.org/2674253003/diff/1/components/crash/content/ap...
File components/crash/content/app/run_as_crashpad_handler_win.cc (right):

https://codereview.chromium.org/2674253003/diff/1/components/crash/content/ap...
components/crash/content/app/run_as_crashpad_handler_win.cc:24:
base::EnableTerminationOnOutOfMemory();
We should centralize all of the Chrome-specific crashpad_handler setup stuff in
one location. The other thing that we now do is call
SetupFallbackCrashHandling(). That happens in
chrome/app/chrome_exe_main_win.cc’s wWinMain(), should it also happen here?

[Will Harris, 2017-02-06 18:24:04]

adding params shouldn't mess with anything, but minidump_stackwalk might have to
be updated to print the exception values, if you want crash to be able to see
them.

https://codereview.chromium.org/2674253003/diff/1/base/process/memory_unittes...
File base/process/memory_unittest.cc (right):

https://codereview.chromium.org/2674253003/diff/1/base/process/memory_unittes...
base/process/memory_unittest.cc:431: const size_t kAttemptedAllocationSize =
0xBADA55;
Can you test with a value that does not fit in 32bit value, on 64-bit platforms?

[commit-bot: I haz the power, 2017-02-06 19:26:04]

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

[commit-bot: I haz the power, 2017-02-06 19:26:04]

Dry run: This issue passed the CQ dry run.

[Sigurður Ásgeirsson, 2017-02-06 19:43:58]

Thanks.

Mark, if you feel strongly the setup code should be moved, lemme know.

https://codereview.chromium.org/2674253003/diff/1/base/process/memory_unittes...
File base/process/memory_unittest.cc (right):

https://codereview.chromium.org/2674253003/diff/1/base/process/memory_unittes...
base/process/memory_unittest.cc:431: const size_t kAttemptedAllocationSize =
0xBADA55;
On 2017/02/06 18:24:03, Will Harris wrote:
> Can you test with a value that does not fit in 32bit value, on 64-bit
platforms?

Done.

https://codereview.chromium.org/2674253003/diff/1/base/process/memory_win.cc
File base/process/memory_win.cc (right):

https://codereview.chromium.org/2674253003/diff/1/base/process/memory_win.cc#...
base/process/memory_win.cc:38: // Pass the size of the failed request
On 2017/02/06 18:17:41, scottmg wrote:
> '.' at end of sentence.

Ooops, abortive comment - thanks. Done.

https://codereview.chromium.org/2674253003/diff/1/components/crash/content/ap...
File components/crash/content/app/run_as_crashpad_handler_win.cc (right):

https://codereview.chromium.org/2674253003/diff/1/components/crash/content/ap...
components/crash/content/app/run_as_crashpad_handler_win.cc:24:
base::EnableTerminationOnOutOfMemory();
On 2017/02/06 18:22:39, Mark Mentovai wrote:
> We should centralize all of the Chrome-specific crashpad_handler setup stuff
in
> one location. The other thing that we now do is call
> SetupFallbackCrashHandling(). That happens in
> chrome/app/chrome_exe_main_win.cc’s wWinMain(), should it also happen here?

I felt it was clearer to have the
SetupFallbackCrashHandling/RunFallbackCrashHandler incantations in the same
scope, but I'm happy to move the setup if you feel strongly about it.

[Sigurður Ásgeirsson, 2017-02-06 19:44:15]

The CQ bit was checked by siggi@chromium.org

[Sigurður Ásgeirsson, 2017-02-06 19:44:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
scottmg@chromium.org, mark@chromium.org
Link to the patchset: https://codereview.chromium.org/2674253003/#ps40001
(title: "Address Will's comment.")

[commit-bot: I haz the power, 2017-02-06 19:45:05]

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-02-06 20:45:07]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1486410255164430,
"parent_rev": "4660c749a14cf8d15440dc99595e226b8d1a2dd6", "commit_rev":
"7a7fbf7c5016a85e4610b9e2fe653a77807e932b"}

[commit-bot: I haz the power, 2017-02-06 20:45:40]

Description was changed from

==========
Populate allocation size into OOM exception on Windows.

This will allow exposing the attempted allocation size on the crash backend.
Also set up for OOM handling in the crashpad-handler process.

BUG=683158
==========

to

==========
Populate allocation size into OOM exception on Windows.

This will allow exposing the attempted allocation size on the crash backend.
Also set up for OOM handling in the crashpad-handler process.

BUG=683158

Review-Url: https://codereview.chromium.org/2674253003
Cr-Commit-Position: refs/heads/master@{#448383}
Committed:
https://chromium.googlesource.com/chromium/src/+/7a7fbf7c5016a85e4610b9e2fe65...
==========

[commit-bot: I haz the power, 2017-02-06 20:45:41]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/7a7fbf7c5016a85e4610b9e2fe65...