Add Windows implementation for base/debug/crash_logging.h.

Add Windows implementation for base/debug/crash_logging.h.

BUG=77656, 137062
TEST=Official build still reports crash keys.
TBR=thakis@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=178572

[cpu_(ooo_6.6-7.5), 2013-01-16 21:25:27]

note
https://code.google.com/p/chromium/issues/detail?id=137062

[cpu_(ooo_6.6-7.5), 2013-01-16 21:26:09]

first off, any reason to keep the old way?

[Robert Sesek, 2013-01-16 21:28:05]

On 2013/01/16 21:25:27, cpu wrote:
> note
> https://code.google.com/p/chromium/issues/detail?id=137062

In the BUG= line already :)

On 2013/01/16 21:26:09, cpu wrote:
> first off, any reason to keep the old way?

Just transitioning in pieces. Avi needs this functionality now-ish, so I thought
I'd provide him with that before going at this file in earnest. I intend to
merge all of our platforms' child_process_logging.cc into one that just uses
this new system next.

[Scott Hess - ex-Googler, 2013-01-16 22:45:28]

Just FYI review.

https://codereview.chromium.org/11776040/diff/2001/chrome/app/breakpad_win.cc
File chrome/app/breakpad_win.cc (right):

https://codereview.chromium.org/11776040/diff/2001/chrome/app/breakpad_win.cc...
chrome/app/breakpad_win.cc:111: DynamicEntriesMap* g_dynamic_entries = NULL;
I went to look if the key could be a string piece which referenced the value in
the entry, but then I found that CustomInfoEntry was full of wide chars and
realized that we almost never post keys or values using wide chars and that I
didn't want to know any more.

https://codereview.chromium.org/11776040/diff/2001/chrome/app/breakpad_win.cc...
chrome/app/breakpad_win.cc:690: entry =
&(*g_custom_entries)[g_dynamic_keys_offset++];
Seems like this might want some sort of range check for the case where a key
wasn't in the registered keys and got passed through.

[Robert Sesek, 2013-01-17 19:39:10]

https://codereview.chromium.org/11776040/diff/2001/chrome/app/breakpad_win.cc
File chrome/app/breakpad_win.cc (right):

https://codereview.chromium.org/11776040/diff/2001/chrome/app/breakpad_win.cc...
chrome/app/breakpad_win.cc:690: entry =
&(*g_custom_entries)[g_dynamic_keys_offset++];
On 2013/01/16 22:45:28, shess wrote:
> Seems like this might want some sort of range check for the case where a key
> wasn't in the registered keys and got passed through.

Done.

[Robert Sesek, 2013-01-18 17:01:47]

cpu: ping

[cpu_(ooo_6.6-7.5), 2013-01-19 02:57:42]

I swear I made a comment yesterday, it got lost or user error.

Anyhow, this (if done correctly) should replace the existing fixed map, right?
why keep it?

Adding eroman and raymes.

I bow out, consider their review authoritative.

[cpu_(ooo_6.6-7.5), 2013-01-19 02:58:09]

.

[Robert Sesek, 2013-01-23 16:54:01]

*ping*

[Robert Sesek, 2013-01-23 22:57:01]

eroman & raymes:   PING

[eroman, 2013-01-24 00:47:13]

How do these changes relate to base::debug::SetCrashKeyValue()

https://codereview.chromium.org/11776040/diff/4003/chrome/app/breakpad_win.cc
File chrome/app/breakpad_win.cc (right):

https://codereview.chromium.org/11776040/diff/4003/chrome/app/breakpad_win.cc...
chrome/app/breakpad_win.cc:690: if (it == g_dynamic_entries->end()) {
There is nothing to stop callers from inserting a key which was not enumerated
in RegisterChromeCrashKeys(), and then run out of slots.

Is that intentional?

[Robert Sesek, 2013-01-24 01:03:45]

On 2013/01/24 00:47:13, eroman wrote:
> How do these changes relate to base::debug::SetCrashKeyValue()

This is the platform-specific impl functions that are used with that API via
base::debug::SetCrashKeyReportingFunctions. I.e., this implements Windows
support for that functionality.

https://codereview.chromium.org/11776040/diff/4003/chrome/app/breakpad_win.cc
File chrome/app/breakpad_win.cc (right):

https://codereview.chromium.org/11776040/diff/4003/chrome/app/breakpad_win.cc...
chrome/app/breakpad_win.cc:690: if (it == g_dynamic_entries->end()) {
On 2013/01/24 00:47:13, eroman wrote:
> There is nothing to stop callers from inserting a key which was not enumerated
> in RegisterChromeCrashKeys(), and then run out of slots.
> 
> Is that intentional?

For now, yes. The base::debug API will prevent this in the near future.

https://code.google.com/searchframe#OAMlx_jo-ck/src/base/debug/crash_logging....

[eroman, 2013-01-24 01:26:58]

lgtm then

[commit-bot: I haz the power, 2013-01-24 01:32:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsesek@chromium.org/11776040/4003

[commit-bot: I haz the power, 2013-01-24 01:32:41]

Presubmit check for 11776040-4003 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome
    chrome/app

Presubmit checks took 1.4s to calculate.

[Robert Sesek, 2013-01-24 01:33:00]

+thakis for OWNERS

[Nico, 2013-01-24 18:01:14]

I should honor the spirit of this review and not respond until I've been pinged
three times.

https://codereview.chromium.org/11776040/diff/4003/chrome/app/breakpad_win.cc
File chrome/app/breakpad_win.cc (right):

https://codereview.chromium.org/11776040/diff/4003/chrome/app/breakpad_win.cc...
chrome/app/breakpad_win.cc:111: DynamicEntriesMap* g_dynamic_entries = NULL;
nit: Why is this not static too?

(I realize it's in an unnamed namespace, but so is the rest.)

https://codereview.chromium.org/11776040/diff/4003/chrome/chrome_exe.gypi
File chrome/chrome_exe.gypi (right):

https://codereview.chromium.org/11776040/diff/4003/chrome/chrome_exe.gypi#new...
chrome/chrome_exe.gypi:478: 'common/crash_keys.h',
Should these be in the sources section at the top and then be removed on non-win
platforms? That way they would show up in IDEs on other platforms, and it's what
we usually do I think.