Add UMA histograms and logging for bad IPC message handling

Add UMA histograms and logging for bad IPC message handling

Many different places in the Chrome codebase handle unexpected IPC data
by killing the renderer. This can result in hard-to-debug sad tab pages.
Some, but not all, of these locations log an error; others record UMA
user actions. This CL fixes most sites to both log and record metrics.
It also switches the metrics to be histograms instead of user actions
because we don't need the extra timestamp data from user actions and
most of these sorts of metrics live in histograms.

BUG=462687
TEST=unit_tests

Committed: https://crrev.com/da2505817c8cac8331a0fcab1902e71f3c2b7daf
Cr-Commit-Position: refs/heads/master@{#321598}

[James Cook, 2015-03-13 23:38:25]

add file

[Charlie Reis, 2015-03-13 23:41:48]

creis@chromium.org changed reviewers:
+ creis@chromium.org, jam@chromium.org

[Charlie Reis, 2015-03-13 23:41:49]

Interesting.  John, can you think of a way to handle the enum for the histogram?

https://codereview.chromium.org/1009583004/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/1009583004/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_process_host_impl.cc:1570:
BrowserChildProcessHostImpl::HistogramBadMessageTerminated(
Huh, I wonder if we need this anymore.  I guess it's slightly different info
(process type rather than the place that killed it), so maybe it should stay.

https://codereview.chromium.org/1009583004/diff/20001/content/public/browser/...
File content/public/browser/bad_message.h (right):

https://codereview.chromium.org/1009583004/diff/20001/content/public/browser/...
content/public/browser/bad_message.h:14: // but the benefit of having all values
in one histogram outweighs the cost.
We have two problems here: the content-internal reasons shouldn't get expose in
the public API, and content/ shouldn't need to care about the content-external
reasons.

I do really like how this lets us keep track of all the kill causes, though. 
John, can you think of a way to do this cleanly?

[James Cook, 2015-03-18 20:28:04]

Patchset #2 (id:20001) has been deleted

[James Cook, 2015-03-18 20:31:08]

Charlie, please take a look. This splits the histogram into two, which allows
the content module internals to stay hidden. The two histograms should be easy
to compare in the UMA tool -- they will both pop up if you search for BadMessage
-- so I think this is a reasonable compromise.

https://codereview.chromium.org/1009583004/diff/80001/content/public/browser/...
File content/public/browser/render_process_host.h (left):

https://codereview.chromium.org/1009583004/diff/80001/content/public/browser/...
content/public/browser/render_process_host.h:92: virtual void
ReceivedBadMessage() = 0;
Ideally this method could be removed entirely from the public API, but then the
implementation would have to be duplicated in the extensions module.

[James Cook, 2015-03-18 21:15:21]

Patchset #2 (id:40001) has been deleted

[Charlie Reis, 2015-03-18 21:37:51]

I like it.  A few comments on things to clarify, but I think this should work.

https://codereview.chromium.org/1009583004/diff/100001/content/browser/bad_me...
File content/browser/bad_message.h (right):

https://codereview.chromium.org/1009583004/diff/100001/content/browser/bad_me...
content/browser/bad_message.h:16: // Content embedders should implement their
own bad message statistics, but
nit: no comma (or say ", but they")

https://codereview.chromium.org/1009583004/diff/100001/content/browser/bad_me...
content/browser/bad_message.h:19: // NOTE: Do not remove or reorder elements in
this end. Add new entries at the
nit: end -> list?

https://codereview.chromium.org/1009583004/diff/100001/content/browser/bad_me...
content/browser/bad_message.h:22: NC_IN_PAGE_NAVIGATION = 0,
We should have a note on the naming convention for when people add new ones. 
Looks like it's an abbreviation of the class name followed by a unique
description of the kill site, with the assumption that reason codes each used in
a single location.

https://codereview.chromium.org/1009583004/diff/100001/content/browser/bad_me...
content/browser/bad_message.h:28: RVH_CAN_ACCESS_FILES_OF_PAGE_STATE,
For future reference, what happens if we want to change the name of one of
these?  RVH is going away, and most of these will move to RFH.  Are we stuck
with the old name?  (Not a big deal, but worth noting along with the other
warnings above.)

https://codereview.chromium.org/1009583004/diff/100001/content/browser/bad_me...
content/browser/bad_message.h:45: // Log the event, records a histogram metric
for the |reason|, and terminates
nit: Logs

https://codereview.chromium.org/1009583004/diff/100001/content/browser/browse...
File content/browser/browser_child_process_host_impl.cc (right):

https://codereview.chromium.org/1009583004/diff/100001/content/browser/browse...
content/browser/browser_child_process_host_impl.cc:261:
child_process_->GetProcess().Terminate(RESULT_CODE_KILLED_BAD_MESSAGE, false);
Would be nice if this one fit the model as well, but I'm guessing that's an
intentional difference.

https://codereview.chromium.org/1009583004/diff/100001/content/public/browser...
File content/public/browser/render_process_host.h (right):

https://codereview.chromium.org/1009583004/diff/100001/content/public/browser...
content/public/browser/render_process_host.h:91: // Called when a received
message cannot be decoded. Terminates the renderer.
Emphasize up front that most callers should not call this directly, but rather
bad_message::ReceivedBadMessage (or equivalent methods outside content).

https://codereview.chromium.org/1009583004/diff/100001/extensions/browser/bad...
File extensions/browser/bad_message.h (right):

https://codereview.chromium.org/1009583004/diff/100001/extensions/browser/bad...
extensions/browser/bad_message.h:21: // the end. We rely on these values in
histograms.
Same comments as before.

[James Cook, 2015-03-18 22:19:44]

Charlie, PTAL.

https://codereview.chromium.org/1009583004/diff/100001/content/browser/bad_me...
File content/browser/bad_message.h (right):

https://codereview.chromium.org/1009583004/diff/100001/content/browser/bad_me...
content/browser/bad_message.h:16: // Content embedders should implement their
own bad message statistics, but
On 2015/03/18 21:37:51, Charlie Reis wrote:
> nit: no comma (or say ", but they")

Done.

https://codereview.chromium.org/1009583004/diff/100001/content/browser/bad_me...
content/browser/bad_message.h:19: // NOTE: Do not remove or reorder elements in
this end. Add new entries at the
On 2015/03/18 21:37:51, Charlie Reis wrote:
> nit: end -> list?

Done.

https://codereview.chromium.org/1009583004/diff/100001/content/browser/bad_me...
content/browser/bad_message.h:22: NC_IN_PAGE_NAVIGATION = 0,
On 2015/03/18 21:37:51, Charlie Reis wrote:
> We should have a note on the naming convention for when people add new ones. 
> Looks like it's an abbreviation of the class name followed by a unique
> description of the kill site, with the assumption that reason codes each used
in
> a single location.

Done.

https://codereview.chromium.org/1009583004/diff/100001/content/browser/bad_me...
content/browser/bad_message.h:28: RVH_CAN_ACCESS_FILES_OF_PAGE_STATE,
On 2015/03/18 21:37:51, Charlie Reis wrote:
> For future reference, what happens if we want to change the name of one of
> these?  RVH is going away, and most of these will move to RFH.  Are we stuck
> with the old name?  (Not a big deal, but worth noting along with the other
> warnings above.)

No, they can be renamed as long as the value doesn't change. I added a note in
the comments.

https://codereview.chromium.org/1009583004/diff/100001/content/browser/bad_me...
content/browser/bad_message.h:45: // Log the event, records a histogram metric
for the |reason|, and terminates
On 2015/03/18 21:37:51, Charlie Reis wrote:
> nit: Logs

Done.

https://codereview.chromium.org/1009583004/diff/100001/content/public/browser...
File content/public/browser/render_process_host.h (right):

https://codereview.chromium.org/1009583004/diff/100001/content/public/browser...
content/public/browser/render_process_host.h:91: // Called when a received
message cannot be decoded. Terminates the renderer.
On 2015/03/18 21:37:51, Charlie Reis wrote:
> Emphasize up front that most callers should not call this directly, but rather
> bad_message::ReceivedBadMessage (or equivalent methods outside content).

Done.

https://codereview.chromium.org/1009583004/diff/100001/extensions/browser/bad...
File extensions/browser/bad_message.h (right):

https://codereview.chromium.org/1009583004/diff/100001/extensions/browser/bad...
extensions/browser/bad_message.h:21: // the end. We rely on these values in
histograms.
On 2015/03/18 21:37:51, Charlie Reis wrote:
> Same comments as before.

Done.

[Charlie Reis, 2015-03-19 04:01:50]

Thanks!  LGTM with nits.

You might ping John for his opinion before adding him as the owner in the
histograms.xml file.

https://codereview.chromium.org/1009583004/diff/140001/content/browser/bad_me...
File content/browser/bad_message.h (right):

https://codereview.chromium.org/1009583004/diff/140001/content/browser/bad_me...
content/browser/bad_message.h:43: 
nit: s/here/above BAD_MESSAGE_MAX/
nit: Remove blank line.

https://codereview.chromium.org/1009583004/diff/140001/extensions/browser/bad...
File extensions/browser/bad_message.h (right):

https://codereview.chromium.org/1009583004/diff/140001/extensions/browser/bad...
extensions/browser/bad_message.h:31: 
Same nit.

https://codereview.chromium.org/1009583004/diff/140001/tools/metrics/actions/...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/1009583004/diff/140001/tools/metrics/actions/...
tools/metrics/actions/actions.xml:1129: Deprecated 3/15. Replaced by the
histogram
nit: 3/2015, perhaps?  (It looks like March 15.  Beware!)  :)

[James Cook, 2015-03-19 16:19:11]

jamescook@chromium.org changed reviewers:
+ isherman@chromium.org

[James Cook, 2015-03-19 16:19:12]

Ilya, can I get OWNERS for tools/metrics?

John, are you OK being an owner of the BadMessageTerminate UMA histogram or
would you prefer me to the be exclusive owner?  I put you there with me because
I figured someone with more content expertise should own it, and you already own
ChildProcess.BadMessgeTerminated [sic].

https://codereview.chromium.org/1009583004/diff/140001/content/browser/bad_me...
File content/browser/bad_message.h (right):

https://codereview.chromium.org/1009583004/diff/140001/content/browser/bad_me...
content/browser/bad_message.h:43: 
On 2015/03/19 04:01:49, Charlie Reis wrote:
> nit: s/here/above BAD_MESSAGE_MAX/
> nit: Remove blank line.

Done.

https://codereview.chromium.org/1009583004/diff/140001/extensions/browser/bad...
File extensions/browser/bad_message.h (right):

https://codereview.chromium.org/1009583004/diff/140001/extensions/browser/bad...
extensions/browser/bad_message.h:31: 
On 2015/03/19 04:01:49, Charlie Reis wrote:
> Same nit.

Done.

https://codereview.chromium.org/1009583004/diff/140001/tools/metrics/actions/...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/1009583004/diff/140001/tools/metrics/actions/...
tools/metrics/actions/actions.xml:1129: Deprecated 3/15. Replaced by the
histogram
On 2015/03/19 04:01:49, Charlie Reis wrote:
> nit: 3/2015, perhaps?  (It looks like March 15.  Beware!)  :)

Done.

[Ilya Sherman, 2015-03-19 21:34:46]

LGTM.  Thanks, James.

[Ilya Sherman, 2015-03-19 21:35:45]

LGTM.  Thanks, James.

[James Cook, 2015-03-19 22:17:41]

John, I see you're OOO. I'm going to land this -- let me know if you would
prefer not to own the histogram and I'll find another owner.

[James Cook, 2015-03-19 22:18:44]

The CQ bit was checked by jamescook@chromium.org

[James Cook, 2015-03-19 22:18:45]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org
Link to the patchset: https://codereview.chromium.org/1009583004/#ps160001
(title: "review 2")

[commit-bot: I haz the power, 2015-03-19 22:19:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1009583004/160001

[commit-bot: I haz the power, 2015-03-19 22:29:50]

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

[commit-bot: I haz the power, 2015-03-19 22:29:52]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[James Cook, 2015-03-19 22:44:47]

jamescook@chromium.org changed reviewers:
+ rockot@chromium.org

[James Cook, 2015-03-19 22:44:48]

Ken, can I get OWNERS for //extensions?

CQ it if you're OK with it.

[Ken Rockot(use gerrit already), 2015-03-19 23:23:42]

Nice. LGTM!

[Ken Rockot(use gerrit already), 2015-03-19 23:23:48]

The CQ bit was checked by rockot@chromium.org

[commit-bot: I haz the power, 2015-03-19 23:24:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1009583004/160001

[commit-bot: I haz the power, 2015-03-19 23:34:28]

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

[commit-bot: I haz the power, 2015-03-19 23:34:30]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[James Cook, 2015-03-20 16:31:35]

The CQ bit was checked by jamescook@chromium.org

[James Cook, 2015-03-20 16:31:36]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org,
isherman@chromium.org, rockot@chromium.org
Link to the patchset: https://codereview.chromium.org/1009583004/#ps180001
(title: "actions.xml claims to be pretty printed")

[commit-bot: I haz the power, 2015-03-20 16:32:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1009583004/180001

[commit-bot: I haz the power, 2015-03-20 18:01:31]

Committed patchset #8 (id:180001)

[commit-bot: I haz the power, 2015-03-20 18:02:18]

Patchset 8 (id:??) landed as
https://crrev.com/da2505817c8cac8331a0fcab1902e71f3c2b7daf
Cr-Commit-Position: refs/heads/master@{#321598}