Add possibility to ignore unregistered crash keys. Use this from WebView

Add possibility to ignore unregistered crash keys. Use this from WebView

WebView should only use a subset of the crash keys used throughout the
different chrome-layers.
Before this change the crash-logging mechanism would simply try to add
unregistered keys regardless of whether they fit in the crash-key
storage.

BUG=691560

[gsennton, 2017-02-22 16:44:42]

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

[gsennton, 2017-02-22 16:44:43]

tobiasjs@chromium.org: android_webview/

mark@chromium.org: base/debug/

rsesek@: FYI

[Robert Sesek, 2017-02-22 16:45:20]

See my comment here: https://codereview.chromium.org/2694083004/#msg24

[Robert Sesek, 2017-02-23 20:35:11]

(continuing discussion from the other CL)

On 2017/02/22 16:48:59, Tobias Sargeant wrote:
> On 2017/02/22 16:41:36, Robert Sesek wrote:
> > I'm not convinced the "add a flag to base" is the right approach. For
> starters,
> > it may cover up issues that the pre-registration system is there to prevent.
> But
> > it also is not going to be compatible with the future system in which
> > pre-registration is no longer required. It seems like doing adding this flag
> > will implicitly take advantage of ignoring unregistered keys to filter out
> ones
> > WebView is not interested in collecting (due to privacy or other reasons).
But
> > as soon as the pre-registration is gone, that is not going to be possible
> > anymore. So I don't know how WebView is going to select which crash keys to
> use
> > in that situation. How large is/which keys are the subset of keys that
WebView
> > is interested in collecting?
> 
> What about adding a CrashReporterClient method that takes a key and returns
> whether to include it in the dump? Then WebView registers all the keys, but
> filters them at dump time?

Yes, I think this is what I'm leaning towards.

But I'm also wondering if WebView needs the dynamic aspect of crash keys, or if
you're only interested in some fixed data (device info, etc). If it's the
latter, then you could skip the crash key system entirely and just add the
annotations directly to the storage map.

On 2017/02/23 12:12:12, gsennton wrote:
> Is the idea that any client (e.g. WebView/Clank) that doesn't want a specific
> key should make the relevant calls to SetCrashKeyValue conditional on whether
> that specific client is used?
> Otherwise, it seems wrong not to be able to just use a sub-set of the keys for
> which we call SetCrashKeyValue - at least given WebView special PII
constraints.

The needs of WebView are unlike any other existing client of the crash key
system. So this is basically inventing a new use case the system wasn’t designed
to support. And since the system for crash keys is going to be changing in the
intermediate-term future, I want to make sure that the solution that WebView
comes up with is going to work going forward. What this CL proposes does not.

> What are the different constraints/requirements on this crash-key-value
system?
> Changing any of the calls to SetCrashKeyValue doesn't seem like the correct
way
> to solve this since those calls live in lower-level layers (so e.g. an
> android_webview/ OWNER wouldn't have any control over them).

We require pre-registration so that there's adequate storage for all the keys
that could be set in a session. If we didn't and just silently dropped keys to
either a) make space or b) ignore when out of space, then we could lose crash
key data. For some keys, that's fine, but for others (like experiment seeds)
that'd be pretty dangerous and would give us bad data.

I agree that #if'ing out the callsites is not the elegant way, but that is the
correct way in the current system. (Unless we add a filter mechanism as Toby
suggests).

> Toby's suggestions above should work but it feels like they are workarounds
(and
> adding a second layer of key-registration - like adding a key-filtering method
> in CrashReporterClient - seems error-prone).

WebView seems like it needs a different registration mechanism, because as Torne
notes:

On 2017/02/23 13:41:34, Torne wrote:
> Yeah, I think the basic problem is that WebView has different PII handling
> restrictions and so we don't just want all "normal" crash keys used in chrome
to
> automatically be reported for WebView as well when people add new ones without
> considering webview. Some kind of whitelisting logic is the conservative
> approach, but there's lots of different ways we could whitelist this.

Crash keys currently don't go through any type of review for PII that WebView
needs to consider, since the clients today are all Chrome with the consent bit
checked. I agree that some form of whitelist is going to be the way forward, so
that we can keep within WebView's privacy constraints.

[gsennton, 2017-02-24 14:59:34]

On 2017/02/23 20:35:11, Robert Sesek wrote:
> (continuing discussion from the other CL)
> 
> On 2017/02/22 16:48:59, Tobias Sargeant wrote:
> > On 2017/02/22 16:41:36, Robert Sesek wrote:
> > > I'm not convinced the "add a flag to base" is the right approach. For
> > starters,
> > > it may cover up issues that the pre-registration system is there to
prevent.
> > But
> > > it also is not going to be compatible with the future system in which
> > > pre-registration is no longer required. It seems like doing adding this
flag
> > > will implicitly take advantage of ignoring unregistered keys to filter out
> > ones
> > > WebView is not interested in collecting (due to privacy or other reasons).
> But
> > > as soon as the pre-registration is gone, that is not going to be possible
> > > anymore. So I don't know how WebView is going to select which crash keys
to
> > use
> > > in that situation. How large is/which keys are the subset of keys that
> WebView
> > > is interested in collecting?
> > 
> > What about adding a CrashReporterClient method that takes a key and returns
> > whether to include it in the dump? Then WebView registers all the keys, but
> > filters them at dump time?
> 
> Yes, I think this is what I'm leaning towards.
> 
> But I'm also wondering if WebView needs the dynamic aspect of crash keys, or
if
> you're only interested in some fixed data (device info, etc). If it's the
> latter, then you could skip the crash key system entirely and just add the
> annotations directly to the storage map.
> 
> On 2017/02/23 12:12:12, gsennton wrote:
> > Is the idea that any client (e.g. WebView/Clank) that doesn't want a
specific
> > key should make the relevant calls to SetCrashKeyValue conditional on
whether
> > that specific client is used?
> > Otherwise, it seems wrong not to be able to just use a sub-set of the keys
for
> > which we call SetCrashKeyValue - at least given WebView special PII
> constraints.
> 
> The needs of WebView are unlike any other existing client of the crash key
> system. So this is basically inventing a new use case the system wasn’t
designed
> to support. And since the system for crash keys is going to be changing in the
> intermediate-term future, I want to make sure that the solution that WebView
> comes up with is going to work going forward. What this CL proposes does not.
> 
> > What are the different constraints/requirements on this crash-key-value
> system?
> > Changing any of the calls to SetCrashKeyValue doesn't seem like the correct
> way
> > to solve this since those calls live in lower-level layers (so e.g. an
> > android_webview/ OWNER wouldn't have any control over them).
> 
> We require pre-registration so that there's adequate storage for all the keys
> that could be set in a session. If we didn't and just silently dropped keys to
> either a) make space or b) ignore when out of space, then we could lose crash
> key data. For some keys, that's fine, but for others (like experiment seeds)
> that'd be pretty dangerous and would give us bad data.
> 
> I agree that #if'ing out the callsites is not the elegant way, but that is the
> correct way in the current system. (Unless we add a filter mechanism as Toby
> suggests).
> 
> > Toby's suggestions above should work but it feels like they are workarounds
> (and
> > adding a second layer of key-registration - like adding a key-filtering
method
> > in CrashReporterClient - seems error-prone).
> 
> WebView seems like it needs a different registration mechanism, because as
Torne
> notes:
> 
> On 2017/02/23 13:41:34, Torne wrote:
> > Yeah, I think the basic problem is that WebView has different PII handling
> > restrictions and so we don't just want all "normal" crash keys used in
chrome
> to
> > automatically be reported for WebView as well when people add new ones
without
> > considering webview. Some kind of whitelisting logic is the conservative
> > approach, but there's lots of different ways we could whitelist this.
> 
> Crash keys currently don't go through any type of review for PII that WebView
> needs to consider, since the clients today are all Chrome with the consent bit
> checked. I agree that some form of whitelist is going to be the way forward,
so
> that we can keep within WebView's privacy constraints.

Alright whitelisting sounds like the way to go here.
We (me and Toby) scoped this out:

1. Whitelist:
We'll add a whitelist to breakpad_linux.cc, which will be applied in
breakpad_linux::SetCrashKeyValue.
The whitelist will only be enabled for WebView, and we will whitelist the keys
that are currently in android_webview//crash_keys.cc.
This whitelist mechanism will have to support adding crash values that span
several chunks - i.e. it will have to match '[crash key]-%d' for crash values
that are bigger than kSmallSize.

2. Registered keys for WebView
WebView will have to register almost all keys registered by Chrome (probably
including command-line related keys but excluding extension-related keys).

3. Test
We should be able to test that
a) A key that is whitelisted does get printed to a WebView minidump.
b) A key that isn't whitelisted doesn't get printed to a WebView minidumps.
We will do this using AwDebugTest
https://cs.chromium.org/chromium/src/android_webview/javatests/src/org/chromi...
by registering one or several debug-keys for WebView, adding (or not adding)
these keys to the whitelist, and then reading the dump from AwDebugTest to
ensure that the whitelisted keys are in there.


Does this sound alright with you Robert?

[Robert Sesek, 2017-02-24 16:00:18]

On 2017/02/24 14:59:34, gsennton wrote:
> Alright whitelisting sounds like the way to go here.
> We (me and Toby) scoped this out:
> 
> 1. Whitelist:
> We'll add a whitelist to breakpad_linux.cc, which will be applied in
> breakpad_linux::SetCrashKeyValue.

That seems fine, since there's no way to get a key-value. The other option would
be to filter at report time.

> The whitelist will only be enabled for WebView, and we will whitelist the keys
> that are currently in android_webview//crash_keys.cc.
> This whitelist mechanism will have to support adding crash values that span
> several chunks - i.e. it will have to match '[crash key]-%d' for crash values
> that are bigger than kSmallSize.

It's probably easiest to keep it simple: whitelist up to the N chunks that are
declared.

> 2. Registered keys for WebView
> WebView will have to register almost all keys registered by Chrome (probably
> including command-line related keys but excluding extension-related keys).

I'd keep the lists identical because that's easier to reason about, and there's
no benefit to skipping certain keys.

> 3. Test
> We should be able to test that
> a) A key that is whitelisted does get printed to a WebView minidump.
> b) A key that isn't whitelisted doesn't get printed to a WebView minidumps.
> We will do this using AwDebugTest
>
https://cs.chromium.org/chromium/src/android_webview/javatests/src/org/chromi...
> by registering one or several debug-keys for WebView, adding (or not adding)
> these keys to the whitelist, and then reading the dump from AwDebugTest to
> ensure that the whitelisted keys are in there.

This may be easier to test in native, since it's all C++, but yes, a test for
this would be important.

[gsennton, 2017-02-28 13:56:28]

Description was changed from

==========
Add possibility to ignore unregistered crash keys. Use this from WebView

WebView should only use a subset of the crash keys used throughout the
different chrome-layers.
Before this change the crash-logging mechanism would simply try to add
unregistered keys regardless of whether they fit in the crash-key
storage.

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

to

==========
Add possibility to ignore unregistered crash keys. Use this from WebView

WebView should only use a subset of the crash keys used throughout the
different chrome-layers.
Before this change the crash-logging mechanism would simply try to add
unregistered keys regardless of whether they fit in the crash-key
storage.

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