Add WebView-specific whitelist for crash keys.

Add WebView-specific whitelist for crash keys.

WebView shouldn't upload data representing all crash keys declared by
Chrome since some of that data could be private.
To be able to re-use the existing crash-keys mechanism we here add a
WebView-only whitelist that will be applied at the time data is set for
a key, rather than at registration time.
In this way we ensure that all keys that are being set are also being
registered, while allowing WebView to avoid adding all registered keys.

BUG=691560
Review-Url: https://codereview.chromium.org/2717223003
Cr-Commit-Position: refs/heads/master@{#454611}
Committed: https://chromium.googlesource.com/chromium/src/+/59b1a9c899d5f75c24d848df7f0975e217f8bfc3

[gsennton, 2017-02-27 18:31:36]

gsennton@chromium.org changed reviewers:
+ rsesek@chromium.org, tobiasjs@chromium.org

[gsennton, 2017-02-27 18:31:38]

Hey, this is an initial CL for the crash-keys whitelist.

It's not ready for review, but I have some questions inline, Robert would you
ptal at those questions? :)

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

https://codereview.chromium.org/2717223003/diff/1/components/crash/content/ap...
components/crash/content/app/breakpad_linux.cc:1104:
std::unordered_set<std::string> g_crash_keys_whitelist;
I'm not sure what constraints we have on what data-structures to use here, any
thoughts?

https://codereview.chromium.org/2717223003/diff/1/components/crash/content/ap...
components/crash/content/app/breakpad_linux.cc:1107: // that implementation
instead?
^^^ is BASE_EXPORTING NumChunksForLength OK?

https://codereview.chromium.org/2717223003/diff/1/components/crash/content/ap...
components/crash/content/app/breakpad_linux.cc:1114: const char
kChunkFormatString[] = "%s-%" PRIuS;
^^ same question here, should we BASE_EXPORT this?

https://codereview.chromium.org/2717223003/diff/1/components/crash/content/ap...
components/crash/content/app/breakpad_linux.cc:1119: // TODO do we care that we
are storing lots of keys in our map?
In our whitelist set we are storing an entry for each possible chunk, this can
be up to 16 entries per kLargeSize-key... is this OK or should I parse the
integer part of the key-value in IsInWhiteList instead? (do we have any utility
methods to parse integers from strings, without failing badly if there is no
number there?)

https://codereview.chromium.org/2717223003/diff/1/components/crash/content/ap...
components/crash/content/app/breakpad_linux.cc:1141: // TODO does this need to
be thread-safe?
I'm guessing we allow SetCrashKeyValue to be called from different threads?

[Robert Sesek, 2017-02-27 23:15:29]

https://codereview.chromium.org/2717223003/diff/1/android_webview/common/cras...
File android_webview/common/crash_reporter/crash_keys.cc (right):

https://codereview.chromium.org/2717223003/diff/1/android_webview/common/cras...
android_webview/common/crash_reporter/crash_keys.cc:181: base::debug::CrashKey
fixed_keys[] = {
Since we're mostly storing small keys, can you avoid chunking totally? For the
medium key, you could just whitelist the -1, -2, -3, -4 variants.

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

https://codereview.chromium.org/2717223003/diff/1/components/crash/content/ap...
components/crash/content/app/breakpad_linux.cc:1104:
std::unordered_set<std::string> g_crash_keys_whitelist;
On 2017/02/27 18:31:37, gsennton wrote:
> I'm not sure what constraints we have on what data-structures to use here, any
> thoughts?

Basically nothing that allocates, if possible. Creating an iterator here may, so
I wouldn't use this. A NULL-terminated C array is probably safest.

https://codereview.chromium.org/2717223003/diff/1/components/crash/content/ap...
components/crash/content/app/breakpad_linux.cc:1107: // that implementation
instead?
On 2017/02/27 18:31:38, gsennton wrote:
> ^^^ is BASE_EXPORTING NumChunksForLength OK?

Hm, it should be OK. What keys are going to be whitelisted? Are they going to
get chunked, or can this be avoided totally?

https://codereview.chromium.org/2717223003/diff/1/components/crash/content/ap...
components/crash/content/app/breakpad_linux.cc:1119: // TODO do we care that we
are storing lots of keys in our map?
On 2017/02/27 18:31:38, gsennton wrote:
> In our whitelist set we are storing an entry for each possible chunk, this can
> be up to 16 entries per kLargeSize-key... is this OK or should I parse the
> integer part of the key-value in IsInWhiteList instead? (do we have any
utility
> methods to parse integers from strings, without failing badly if there is no
> number there?)

I think it's fine to just have up to 16 entries. How many large keys are you
going to store?

https://codereview.chromium.org/2717223003/diff/1/components/crash/content/ap...
components/crash/content/app/breakpad_linux.cc:1141: // TODO does this need to
be thread-safe?
On 2017/02/27 18:31:38, gsennton wrote:
> I'm guessing we allow SetCrashKeyValue to be called from different threads?

Yup. I'd ensure the whitelist is immutable after it's set.

[gsennton, 2017-02-28 21:02:42]

Alright, cool! This should be close to done now (want to add some unit tests to
ensure all keys whitelisted for webview are also registered - so we don't try to
whitelist keys that are not there).

Ptal! :)

https://codereview.chromium.org/2717223003/diff/1/android_webview/common/cras...
File android_webview/common/crash_reporter/crash_keys.cc (right):

https://codereview.chromium.org/2717223003/diff/1/android_webview/common/cras...
android_webview/common/crash_reporter/crash_keys.cc:181: base::debug::CrashKey
fixed_keys[] = {
On 2017/02/27 23:15:28, Robert Sesek wrote:
> Since we're mostly storing small keys, can you avoid chunking totally? For the
> medium key, you could just whitelist the -1, -2, -3, -4 variants.

Ah right, I was thinking of the general case where we could white-list any kind
of keys.

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

https://codereview.chromium.org/2717223003/diff/1/components/crash/content/ap...
components/crash/content/app/breakpad_linux.cc:1104:
std::unordered_set<std::string> g_crash_keys_whitelist;
On 2017/02/27 23:15:28, Robert Sesek wrote:
> On 2017/02/27 18:31:37, gsennton wrote:
> > I'm not sure what constraints we have on what data-structures to use here,
any
> > thoughts?
> 
> Basically nothing that allocates, if possible. Creating an iterator here may,
so
> I wouldn't use this. A NULL-terminated C array is probably safest.

This should be fine now :).

https://codereview.chromium.org/2717223003/diff/1/components/crash/content/ap...
components/crash/content/app/breakpad_linux.cc:1107: // that implementation
instead?
On 2017/02/27 23:15:28, Robert Sesek wrote:
> On 2017/02/27 18:31:38, gsennton wrote:
> > ^^^ is BASE_EXPORTING NumChunksForLength OK?
> 
> Hm, it should be OK. What keys are going to be whitelisted? Are they going to
> get chunked, or can this be avoided totally?

Removed these - with a more hard-coded (ugh!) version these aren't needed.

https://codereview.chromium.org/2717223003/diff/1/components/crash/content/ap...
components/crash/content/app/breakpad_linux.cc:1119: // TODO do we care that we
are storing lots of keys in our map?
On 2017/02/27 23:15:28, Robert Sesek wrote:
> On 2017/02/27 18:31:38, gsennton wrote:
> > In our whitelist set we are storing an entry for each possible chunk, this
can
> > be up to 16 entries per kLargeSize-key... is this OK or should I parse the
> > integer part of the key-value in IsInWhiteList instead? (do we have any
> utility
> > methods to parse integers from strings, without failing badly if there is no
> > number there?)
> 
> I think it's fine to just have up to 16 entries. How many large keys are you
> going to store?

Yeah, we only whitelist small/medium-sized keys, so this won't be an issue (for
now).

https://codereview.chromium.org/2717223003/diff/1/components/crash/content/ap...
components/crash/content/app/breakpad_linux.cc:1119: // TODO do we care that we
are storing lots of keys in our map?
On 2017/02/27 23:15:28, Robert Sesek wrote:
> On 2017/02/27 18:31:38, gsennton wrote:
> > In our whitelist set we are storing an entry for each possible chunk, this
can
> > be up to 16 entries per kLargeSize-key... is this OK or should I parse the
> > integer part of the key-value in IsInWhiteList instead? (do we have any
> utility
> > methods to parse integers from strings, without failing badly if there is no
> > number there?)
> 
> I think it's fine to just have up to 16 entries. How many large keys are you
> going to store?

+1, no large keys.

https://codereview.chromium.org/2717223003/diff/1/components/crash/content/ap...
components/crash/content/app/breakpad_linux.cc:1141: // TODO does this need to
be thread-safe?
On 2017/02/27 23:15:28, Robert Sesek wrote:
> On 2017/02/27 18:31:38, gsennton wrote:
> > I'm guessing we allow SetCrashKeyValue to be called from different threads?
> 
> Yup. I'd ensure the whitelist is immutable after it's set.

Done (well, I don't touch it again after the initial set-up).

[gsennton, 2017-02-28 22:07:39]

Description was changed from

==========
Add WebView-specific whitelist for crash keys.

BUG=691560
==========

to

==========
Add WebView-specific whitelist for crash keys.

WebView shouldn't upload data representing all crash keys declared by
Chrome since some of that data could be private.
To be able to re-use the existing crash-keys mechanism we here add a
WebView-only whitelist that will be applied at the time data is set for
a key, rather than at registration time.
In this way we ensure that all keys that are being set are also being
registered, while allowing WebView to avoid adding all registered keys.

BUG=691560
==========

[Tobias Sargeant, 2017-02-28 22:18:55]

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

https://codereview.chromium.org/2717223003/diff/1/components/crash/content/ap...
components/crash/content/app/breakpad_linux.cc:1104:
std::unordered_set<std::string> g_crash_keys_whitelist;
On 2017/02/28 21:02:42, gsennton wrote:
> On 2017/02/27 23:15:28, Robert Sesek wrote:
> > On 2017/02/27 18:31:37, gsennton wrote:
> > > I'm not sure what constraints we have on what data-structures to use here,
> any
> > > thoughts?
> > 
> > Basically nothing that allocates, if possible. Creating an iterator here
may,
> so
> > I wouldn't use this. A NULL-terminated C array is probably safest.
> 
> This should be fine now :).

I think Robert's suggestion of a NULL terminated C array is safest (I expect
that std::vector won't allocate given the usage below, but I don't know for
sure, and I'm not sure that that guarantee is given in the STL spec).

However, we don't access this from a compromised context (Robert: I'm assuming
that keys are only set prior to a crash. Is that correct) so I think that the
unordered_set should be ok?

https://codereview.chromium.org/2717223003/diff/20001/components/crash/conten...
File components/crash/content/app/crash_reporter_client.h (right):

https://codereview.chromium.org/2717223003/diff/20001/components/crash/conten...
components/crash/content/app/crash_reporter_client.h:150: virtual
std::vector<const char*> GetWhiteListedCrashKeys();
Because the whitelist is not accessed in a compromised context (only when
setting the crash key value), WDYT about having only:

virtual void GetWhiteListedCrashKeys(std::unordererd_set<std::string>*
whitelist);

I don't think separate Use* and Get* methods are really needed. The default
implementation can just not add any whitelisted keys.

[gsennton, 2017-03-01 15:53:02]

https://codereview.chromium.org/2717223003/diff/20001/components/crash/conten...
File components/crash/content/app/crash_reporter_client.h (right):

https://codereview.chromium.org/2717223003/diff/20001/components/crash/conten...
components/crash/content/app/crash_reporter_client.h:150: virtual
std::vector<const char*> GetWhiteListedCrashKeys();
On 2017/02/28 22:18:55, Tobias Sargeant wrote:
> Because the whitelist is not accessed in a compromised context (only when
> setting the crash key value), WDYT about having only:
> 
> virtual void GetWhiteListedCrashKeys(std::unordererd_set<std::string>*
> whitelist);
> 
> I don't think separate Use* and Get* methods are really needed. The default
> implementation can just not add any whitelisted keys.

If there aren't any whitelisted keys, shouldn't that mean that you don't want
any keys to be uploaded? :)

[Tobias Sargeant, 2017-03-01 16:06:46]

On 2017/03/01 15:53:02, gsennton wrote:

> If there aren't any whitelisted keys, shouldn't that mean that you don't want
> any keys to be uploaded? :)

I was imagining that an empty whitelist meant include everything (which removes
your ability to blacklist everything, but then you could just not initialize
crash keys if you wanted that behaviour).

Admittedly if we're allowed to assume that crash keys are not set in a
compromised context, then having bool
CrashReporterClient::ShouldSetCrashKey(const std::string&) would be the smallest
change...

[gsennton, 2017-03-02 09:41:39]

On 2017/03/01 16:06:46, Tobias Sargeant wrote:
> On 2017/03/01 15:53:02, gsennton wrote:
> 
> > If there aren't any whitelisted keys, shouldn't that mean that you don't
want
> > any keys to be uploaded? :)
> 
> I was imagining that an empty whitelist meant include everything (which
removes
> your ability to blacklist everything, but then you could just not initialize
> crash keys if you wanted that behaviour).
> 
> Admittedly if we're allowed to assume that crash keys are not set in a
> compromised context, then having bool
> CrashReporterClient::ShouldSetCrashKey(const std::string&) would be the
smallest
> change...

Agreed

[Robert Sesek, 2017-03-02 15:00:38]

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

https://codereview.chromium.org/2717223003/diff/1/components/crash/content/ap...
components/crash/content/app/breakpad_linux.cc:1104:
std::unordered_set<std::string> g_crash_keys_whitelist;
On 2017/02/28 22:18:55, Tobias Sargeant wrote:
> On 2017/02/28 21:02:42, gsennton wrote:
> > On 2017/02/27 23:15:28, Robert Sesek wrote:
> > > On 2017/02/27 18:31:37, gsennton wrote:
> > > > I'm not sure what constraints we have on what data-structures to use
here,
> > any
> > > > thoughts?
> > > 
> > > Basically nothing that allocates, if possible. Creating an iterator here
> may,
> > so
> > > I wouldn't use this. A NULL-terminated C array is probably safest.
> > 
> > This should be fine now :).
> 
> I think Robert's suggestion of a NULL terminated C array is safest (I expect
> that std::vector won't allocate given the usage below, but I don't know for
> sure, and I'm not sure that that guarantee is given in the STL spec).
> 
> However, we don't access this from a compromised context (Robert: I'm assuming
> that keys are only set prior to a crash. Is that correct) so I think that the
> unordered_set should be ok?

No, crash keys can be set in a compromised context.

https://codereview.chromium.org/2717223003/diff/20001/android_webview/common/...
File android_webview/common/crash_reporter/crash_keys.cc (right):

https://codereview.chromium.org/2717223003/diff/20001/android_webview/common/...
android_webview/common/crash_reporter/crash_keys.cc:167: // Register the
extension IDs.
Why extensions? Isn't that a //chrome-level concept that shouldn't bleed into
//content?

[gsennton, 2017-03-02 16:18:01]

Alright, this should be fine now.

PTAL :)

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

https://codereview.chromium.org/2717223003/diff/1/components/crash/content/ap...
components/crash/content/app/breakpad_linux.cc:1104:
std::unordered_set<std::string> g_crash_keys_whitelist;
On 2017/03/02 15:00:38, Robert Sesek wrote:
> On 2017/02/28 22:18:55, Tobias Sargeant wrote:
> > On 2017/02/28 21:02:42, gsennton wrote:
> > > On 2017/02/27 23:15:28, Robert Sesek wrote:
> > > > On 2017/02/27 18:31:37, gsennton wrote:
> > > > > I'm not sure what constraints we have on what data-structures to use
> here,
> > > any
> > > > > thoughts?
> > > > 
> > > > Basically nothing that allocates, if possible. Creating an iterator here
> > may,
> > > so
> > > > I wouldn't use this. A NULL-terminated C array is probably safest.
> > > 
> > > This should be fine now :).
> > 
> > I think Robert's suggestion of a NULL terminated C array is safest (I expect
> > that std::vector won't allocate given the usage below, but I don't know for
> > sure, and I'm not sure that that guarantee is given in the STL spec).
> > 
> > However, we don't access this from a compromised context (Robert: I'm
assuming
> > that keys are only set prior to a crash. Is that correct) so I think that
the
> > unordered_set should be ok?
> 
> No, crash keys can be set in a compromised context.

I changed the vector to a NULL terminated C array now :)

https://codereview.chromium.org/2717223003/diff/20001/android_webview/common/...
File android_webview/common/crash_reporter/crash_keys.cc (right):

https://codereview.chromium.org/2717223003/diff/20001/android_webview/common/...
android_webview/common/crash_reporter/crash_keys.cc:167: // Register the
extension IDs.
On 2017/03/02 15:00:38, Robert Sesek wrote:
> Why extensions? Isn't that a //chrome-level concept that shouldn't bleed into
> //content?

Removed now (also removed the printer-id stuff since that's only used in
Chrome).

[Tobias Sargeant, 2017-03-02 16:36:48]

https://codereview.chromium.org/2717223003/diff/60001/android_webview/native/...
File android_webview/native/aw_debug.cc (right):

https://codereview.chromium.org/2717223003/diff/60001/android_webview/native/...
android_webview/native/aw_debug.cc:43: breakpad::InitCrashKeysForTesting();
Could you move this to aw/common/crash_reporter/crash_keys.cc so as not to need
a DEPS change?

https://codereview.chromium.org/2717223003/diff/60001/components/crash/conten...
File components/crash/content/app/breakpad_linux.cc (right):

https://codereview.chromium.org/2717223003/diff/60001/components/crash/conten...
components/crash/content/app/breakpad_linux.cc:1101: bool
g_use_crash_keys_whitelist;
initialize these, and move them to the top of the file.

[gsennton, 2017-03-02 17:35:38]

Thanks for the quick review Toby!

https://codereview.chromium.org/2717223003/diff/60001/android_webview/native/...
File android_webview/native/aw_debug.cc (right):

https://codereview.chromium.org/2717223003/diff/60001/android_webview/native/...
android_webview/native/aw_debug.cc:43: breakpad::InitCrashKeysForTesting();
On 2017/03/02 16:36:48, Tobias Sargeant wrote:
> Could you move this to aw/common/crash_reporter/crash_keys.cc so as not to
need
> a DEPS change?

Done.

https://codereview.chromium.org/2717223003/diff/60001/components/crash/conten...
File components/crash/content/app/breakpad_linux.cc (right):

https://codereview.chromium.org/2717223003/diff/60001/components/crash/conten...
components/crash/content/app/breakpad_linux.cc:1101: bool
g_use_crash_keys_whitelist;
On 2017/03/02 16:36:48, Tobias Sargeant wrote:
> initialize these, and move them to the top of the file.

Done.

[Tobias Sargeant, 2017-03-02 18:49:07]

On 2017/03/02 17:35:38, gsennton wrote:
> Thanks for the quick review Toby!
> 
>
https://codereview.chromium.org/2717223003/diff/60001/android_webview/native/...
> File android_webview/native/aw_debug.cc (right):
> 
>
https://codereview.chromium.org/2717223003/diff/60001/android_webview/native/...
> android_webview/native/aw_debug.cc:43: breakpad::InitCrashKeysForTesting();
> On 2017/03/02 16:36:48, Tobias Sargeant wrote:
> > Could you move this to aw/common/crash_reporter/crash_keys.cc so as not to
> need
> > a DEPS change?
> 
> Done.
> 
>
https://codereview.chromium.org/2717223003/diff/60001/components/crash/conten...
> File components/crash/content/app/breakpad_linux.cc (right):
> 
>
https://codereview.chromium.org/2717223003/diff/60001/components/crash/conten...
> components/crash/content/app/breakpad_linux.cc:1101: bool
> g_use_crash_keys_whitelist;
> On 2017/03/02 16:36:48, Tobias Sargeant wrote:
> > initialize these, and move them to the top of the file.
> 
> Done.

//android_webview LGTM.

[Robert Sesek, 2017-03-02 22:29:40]

https://codereview.chromium.org/2717223003/diff/80001/android_webview/common/...
File android_webview/common/crash_reporter/crash_keys.cc (right):

https://codereview.chromium.org/2717223003/diff/80001/android_webview/common/...
android_webview/common/crash_reporter/crash_keys.cc:142: const char*
WHITE_LISTED_WEBVIEW_CRASH_KEYS[] = {
Can this be even more const (const char* const) ?

https://codereview.chromium.org/2717223003/diff/80001/android_webview/common/...
android_webview/common/crash_reporter/crash_keys.cc:142: const char*
WHITE_LISTED_WEBVIEW_CRASH_KEYS[] = {
Naming: Globals in C++ are kCamelCase.

https://codereview.chromium.org/2717223003/diff/80001/android_webview/common/...
android_webview/common/crash_reporter/crash_keys.cc:142: const char*
WHITE_LISTED_WEBVIEW_CRASH_KEYS[] = {
You can also move this into GetWhiteListedWebViewCrashKeys so it has internal
linkage.

https://codereview.chromium.org/2717223003/diff/80001/android_webview/common/...
android_webview/common/crash_reporter/crash_keys.cc:143:
"AW_WHITELISTED_DEBUG_KEY", kGPUDriverVersion, kGPUPixelShaderVersion,
I know this was probably clang-format's doing, but this may be more easily read
one-per-line. If you don't feel strongly, ignore, but if you do, you can coerce
clang-format into submission with "// clang-format off" and "// clang-format
on".

https://codereview.chromium.org/2717223003/diff/80001/android_webview/common/...
android_webview/common/crash_reporter/crash_keys.cc:150:
"total-discardable-memory-allocated", NULL};
nullptr

https://codereview.chromium.org/2717223003/diff/80001/android_webview/common/...
android_webview/common/crash_reporter/crash_keys.cc:160: void
SetCrashKeyValue(const std::string& key, const std::string& value) {
What's the point of this function?

https://codereview.chromium.org/2717223003/diff/80001/android_webview/common/...
android_webview/common/crash_reporter/crash_keys.cc:162: }
nit: blank line after

https://codereview.chromium.org/2717223003/diff/80001/android_webview/javates...
File
android_webview/javatests/src/org/chromium/android_webview/test/AwDebugTest.java
(right):

https://codereview.chromium.org/2717223003/diff/80001/android_webview/javates...
android_webview/javatests/src/org/chromium/android_webview/test/AwDebugTest.java:89:
int debug_key_index = dumpContents.indexOf(debugKeyLine);
naming: debugKeyIndex

https://codereview.chromium.org/2717223003/diff/80001/components/crash/conten...
File components/crash/content/app/breakpad_linux.cc (right):

https://codereview.chromium.org/2717223003/diff/80001/components/crash/conten...
components/crash/content/app/breakpad_linux.cc:104: bool
g_use_crash_keys_whitelist = false;
Update the names here, too. Also, if in the CamelCase you do WhiteList it should
be white_list.

https://codereview.chromium.org/2717223003/diff/80001/components/crash/conten...
components/crash/content/app/breakpad_linux.cc:1106: for (size_t i = 0;
g_crash_keys_whitelist[i] != NULL; ++i) {
NULL -> nullptr, or just omit and use implicit boolean conversion.

https://codereview.chromium.org/2717223003/diff/80001/components/crash/conten...
File components/crash/content/app/crash_reporter_client.cc (right):

https://codereview.chromium.org/2717223003/diff/80001/components/crash/conten...
components/crash/content/app/crash_reporter_client.cc:130: return NULL;
nullptr

https://codereview.chromium.org/2717223003/diff/80001/components/crash/conten...
File components/crash/content/app/crash_reporter_client.h (right):

https://codereview.chromium.org/2717223003/diff/80001/components/crash/conten...
components/crash/content/app/crash_reporter_client.h:147: virtual bool
UseCrashKeysWhiteList();
How about ShouldWhiteListCrashKeys() ?

https://codereview.chromium.org/2717223003/diff/80001/components/crash/conten...
components/crash/content/app/crash_reporter_client.h:150: virtual const char**
GetWhiteListedCrashKeys();
And GetCrashKeyWhiteList().

[Robert Sesek, 2017-03-02 23:10:49]

https://codereview.chromium.org/2717223003/diff/80001/components/crash/conten...
File components/crash/content/app/crash_reporter_client.h (right):

https://codereview.chromium.org/2717223003/diff/80001/components/crash/conten...
components/crash/content/app/crash_reporter_client.h:147: virtual bool
UseCrashKeysWhiteList();
On 2017/03/02 22:29:40, Robert Sesek wrote:
> How about ShouldWhiteListCrashKeys() ?

On second thought, your name is better. Ignore.

[gsennton, 2017-03-03 00:09:33]

Thanks Robert!
If you are taking another look tonight and no more changes are needed, please
tick the CQ checkbox :)

https://codereview.chromium.org/2717223003/diff/80001/android_webview/common/...
File android_webview/common/crash_reporter/crash_keys.cc (right):

https://codereview.chromium.org/2717223003/diff/80001/android_webview/common/...
android_webview/common/crash_reporter/crash_keys.cc:142: const char*
WHITE_LISTED_WEBVIEW_CRASH_KEYS[] = {
On 2017/03/02 22:29:39, Robert Sesek wrote:
> You can also move this into GetWhiteListedWebViewCrashKeys so it has internal
> linkage.

If I move this inside GetWhiteListedWebViewCrashKeys it will just be declared as
a local variable? (and then popped of the stack). Declaring this as static
instead. (or did you have some other usage in mind inside
GetWhiteListedWebViewCrashKeys? :))

https://codereview.chromium.org/2717223003/diff/80001/android_webview/common/...
android_webview/common/crash_reporter/crash_keys.cc:143:
"AW_WHITELISTED_DEBUG_KEY", kGPUDriverVersion, kGPUPixelShaderVersion,
On 2017/03/02 22:29:39, Robert Sesek wrote:
> I know this was probably clang-format's doing, but this may be more easily
read
> one-per-line. If you don't feel strongly, ignore, but if you do, you can
coerce
> clang-format into submission with "// clang-format off" and "// clang-format
> on".

Awesome! I didn't know we had this.

https://codereview.chromium.org/2717223003/diff/80001/android_webview/common/...
android_webview/common/crash_reporter/crash_keys.cc:150:
"total-discardable-memory-allocated", NULL};
On 2017/03/02 22:29:39, Robert Sesek wrote:
> nullptr

Done.

https://codereview.chromium.org/2717223003/diff/80001/android_webview/common/...
android_webview/common/crash_reporter/crash_keys.cc:160: void
SetCrashKeyValue(const std::string& key, const std::string& value) {
On 2017/03/02 22:29:40, Robert Sesek wrote:
> What's the point of this function?

Ooops! To ensure you're awake of course ;)

https://codereview.chromium.org/2717223003/diff/80001/android_webview/common/...
android_webview/common/crash_reporter/crash_keys.cc:162: }
On 2017/03/02 22:29:40, Robert Sesek wrote:
> nit: blank line after

Done.

https://codereview.chromium.org/2717223003/diff/80001/android_webview/javates...
File
android_webview/javatests/src/org/chromium/android_webview/test/AwDebugTest.java
(right):

https://codereview.chromium.org/2717223003/diff/80001/android_webview/javates...
android_webview/javatests/src/org/chromium/android_webview/test/AwDebugTest.java:89:
int debug_key_index = dumpContents.indexOf(debugKeyLine);
On 2017/03/02 22:29:40, Robert Sesek wrote:
> naming: debugKeyIndex

Done.

https://codereview.chromium.org/2717223003/diff/80001/components/crash/conten...
File components/crash/content/app/breakpad_linux.cc (right):

https://codereview.chromium.org/2717223003/diff/80001/components/crash/conten...
components/crash/content/app/breakpad_linux.cc:104: bool
g_use_crash_keys_whitelist = false;
On 2017/03/02 22:29:40, Robert Sesek wrote:
> Update the names here, too. Also, if in the CamelCase you do WhiteList it
should
> be white_list.

Done.

https://codereview.chromium.org/2717223003/diff/80001/components/crash/conten...
components/crash/content/app/breakpad_linux.cc:1106: for (size_t i = 0;
g_crash_keys_whitelist[i] != NULL; ++i) {
On 2017/03/02 22:29:40, Robert Sesek wrote:
> NULL -> nullptr, or just omit and use implicit boolean conversion.

Done.

https://codereview.chromium.org/2717223003/diff/80001/components/crash/conten...
File components/crash/content/app/crash_reporter_client.cc (right):

https://codereview.chromium.org/2717223003/diff/80001/components/crash/conten...
components/crash/content/app/crash_reporter_client.cc:130: return NULL;
On 2017/03/02 22:29:40, Robert Sesek wrote:
> nullptr

Done.

https://codereview.chromium.org/2717223003/diff/80001/components/crash/conten...
File components/crash/content/app/crash_reporter_client.h (right):

https://codereview.chromium.org/2717223003/diff/80001/components/crash/conten...
components/crash/content/app/crash_reporter_client.h:150: virtual const char**
GetWhiteListedCrashKeys();
On 2017/03/02 22:29:40, Robert Sesek wrote:
> And GetCrashKeyWhiteList().

Done.

[Robert Sesek, 2017-03-03 15:57:47]

LGTM

https://codereview.chromium.org/2717223003/diff/100001/android_webview/common...
File android_webview/common/crash_reporter/crash_keys.cc (right):

https://codereview.chromium.org/2717223003/diff/100001/android_webview/common...
android_webview/common/crash_reporter/crash_keys.cc:143: static const char*
const kWhiteListedWebViewCrashKeys[] = {
Change this name to be in the same style as the CrashReproterClient?
(kWebViewCrashKeyWhiteList)

https://codereview.chromium.org/2717223003/diff/100001/android_webview/common...
android_webview/common/crash_reporter/crash_keys.cc:163: const char* const*
GetWhiteListedWebViewCrashKeys() {
Same: GetWebViewCrashKeyWhiteList ?

Although since this is called exactly once, you could also just make the
kWebViewCrashKeyWhiteList declared extern in the header and reference the array
directly from the CrashReporterClient.

[gsennton, 2017-03-03 16:21:35]

https://codereview.chromium.org/2717223003/diff/100001/android_webview/common...
File android_webview/common/crash_reporter/crash_keys.cc (right):

https://codereview.chromium.org/2717223003/diff/100001/android_webview/common...
android_webview/common/crash_reporter/crash_keys.cc:143: static const char*
const kWhiteListedWebViewCrashKeys[] = {
On 2017/03/03 15:57:47, Robert Sesek wrote:
> Change this name to be in the same style as the CrashReproterClient?
> (kWebViewCrashKeyWhiteList)

Done.

https://codereview.chromium.org/2717223003/diff/100001/android_webview/common...
android_webview/common/crash_reporter/crash_keys.cc:163: const char* const*
GetWhiteListedWebViewCrashKeys() {
On 2017/03/03 15:57:47, Robert Sesek wrote:
> Same: GetWebViewCrashKeyWhiteList ?
> 
> Although since this is called exactly once, you could also just make the
> kWebViewCrashKeyWhiteList declared extern in the header and reference the
array
> directly from the CrashReporterClient.

Done.

[gsennton, 2017-03-03 16:22:07]

The CQ bit was checked by gsennton@chromium.org

[gsennton, 2017-03-03 16:22:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
tobiasjs@chromium.org, rsesek@chromium.org
Link to the patchset: https://codereview.chromium.org/2717223003/#ps120001
(title: "Naming webview whitelist array.")

[commit-bot: I haz the power, 2017-03-03 16:22:23]

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-03-03 17:42:48]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1488558127323900,
"parent_rev": "6eb31f11892eeca9829db653e068ce4e121efd62", "commit_rev":
"59b1a9c899d5f75c24d848df7f0975e217f8bfc3"}

[commit-bot: I haz the power, 2017-03-03 17:43:24]

Description was changed from

==========
Add WebView-specific whitelist for crash keys.

WebView shouldn't upload data representing all crash keys declared by
Chrome since some of that data could be private.
To be able to re-use the existing crash-keys mechanism we here add a
WebView-only whitelist that will be applied at the time data is set for
a key, rather than at registration time.
In this way we ensure that all keys that are being set are also being
registered, while allowing WebView to avoid adding all registered keys.

BUG=691560
==========

to

==========
Add WebView-specific whitelist for crash keys.

WebView shouldn't upload data representing all crash keys declared by
Chrome since some of that data could be private.
To be able to re-use the existing crash-keys mechanism we here add a
WebView-only whitelist that will be applied at the time data is set for
a key, rather than at registration time.
In this way we ensure that all keys that are being set are also being
registered, while allowing WebView to avoid adding all registered keys.

BUG=691560

Review-Url: https://codereview.chromium.org/2717223003
Cr-Commit-Position: refs/heads/master@{#454611}
Committed:
https://chromium.googlesource.com/chromium/src/+/59b1a9c899d5f75c24d848df7f09...
==========

[commit-bot: I haz the power, 2017-03-03 17:43:25]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/59b1a9c899d5f75c24d848df7f09...