Introduce bad_message.h for chrome and NaCl.

Introduce bad_message.h for chrome and NaCl.

Where needed, add variants of bad_message::ReceivedBadMessage that take a
BrowserMessageFilter instead of a RenderProcessHost.

Rename BrowserMessageFilter::BadMessageReceived to be ::ShutdownForBadMessage. It's now only called from the bad_message helper functions after logging the error to UMA.

Use the new bad_message helpers in all places that previously called BrowserMessageFilter::BadMessageReceived().

BUG=None
TEST=None

Committed: https://crrev.com/167948263fa1c560de096b877be8a151cfed3ff7
Cr-Commit-Position: refs/heads/master@{#332488}

[MasterD, 2015-05-22 17:28:45]

r.scheffelt@gmail.com changed reviewers:
+ r.scheffelt@gmail.com

[ncarter (slow), 2015-05-22 17:29:21]

nick@chromium.org changed reviewers:
+ jamescook@chromium.org
- r.scheffelt@gmail.com

[ncarter (slow), 2015-05-22 17:30:54]

nick@chromium.org changed reviewers:
+ creis@chromium.org

[ncarter (slow), 2015-05-22 17:30:55]

Hi James -- could you have a look at this and let me know if you think it's the
right way forward for bad_message in general?

Charlie -- Could you review the content/ changes?

[not at google - send to devlin, 2015-05-22 17:36:10]

kalman@chromium.org changed reviewers:
+ kalman@chromium.org

[not at google - send to devlin, 2015-05-22 17:36:12]

https://codereview.chromium.org/1145013004/diff/20001/extensions/browser/bad_...
File extensions/browser/bad_message.h (right):

https://codereview.chromium.org/1145013004/diff/20001/extensions/browser/bad_...
extensions/browser/bad_message.h:40: }  // namespace bad_message
lgtm

[James Cook, 2015-05-22 18:13:10]

LGTM. I like it. Thank you for converting all the UserActions to histograms.

+CC oshima FYI - he's working on a "He's dead Jim" bug that might be related to
bad message termination

Also, after landing this you might want to check UMA in a couple days and see if
anything unusual pops up in the bad message stats for canary.

https://codereview.chromium.org/1145013004/diff/20001/components/nacl/browser...
File components/nacl/browser/nacl_file_host.cc (right):

https://codereview.chromium.org/1145013004/diff/20001/components/nacl/browser...
components/nacl/browser/nacl_file_host.cc:255: delete reply_msg;
We leaked this before? Good catch.

https://codereview.chromium.org/1145013004/diff/20001/content/browser/notific...
File content/browser/notifications/notification_message_filter.cc (right):

https://codereview.chromium.org/1145013004/diff/20001/content/browser/notific...
content/browser/notifications/notification_message_filter.cc:8: 
nit: no blank line

https://codereview.chromium.org/1145013004/diff/20001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1145013004/diff/20001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:2472: // Exercises
Exercises?

Also, is this meant to be part of a different CL?

[ncarter (slow), 2015-05-22 18:41:52]

https://codereview.chromium.org/1145013004/diff/20001/components/nacl/browser...
File components/nacl/browser/nacl_file_host.cc (right):

https://codereview.chromium.org/1145013004/diff/20001/components/nacl/browser...
components/nacl/browser/nacl_file_host.cc:255: delete reply_msg;
On 2015/05/22 18:13:10, James Cook wrote:
> We leaked this before? Good catch.

Acknowledged.

https://codereview.chromium.org/1145013004/diff/20001/content/browser/notific...
File content/browser/notifications/notification_message_filter.cc (right):

https://codereview.chromium.org/1145013004/diff/20001/content/browser/notific...
content/browser/notifications/notification_message_filter.cc:8: 
On 2015/05/22 18:13:10, James Cook wrote:
> nit: no blank line

Done.

https://codereview.chromium.org/1145013004/diff/20001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1145013004/diff/20001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:2472: // Exercises
On 2015/05/22 18:13:10, James Cook wrote:
> Exercises?
> 
> Also, is this meant to be part of a different CL?

Oh gosh yes, this doesn't belong here at all. Stale edits in my text editor
after switching branches...

[Charlie Reis, 2015-05-22 20:55:52]

Nice!  LGTM with nits.

https://codereview.chromium.org/1145013004/diff/80001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_ui_bindings.cc (right):

https://codereview.chromium.org/1145013004/diff/80001/chrome/browser/devtools...
chrome/browser/devtools/devtools_ui_bindings.cc:778: // TODO(nick): Replace with
chrome::bad_message::ReceivedBadMessage()
nit: End with period.

(I'm fine with leaving a TODO for this for now, given how many issues you
encountered when trying to do it in this CL.)

https://codereview.chromium.org/1145013004/diff/80001/components/nacl/browser...
File components/nacl/browser/nacl_host_message_filter.cc (right):

https://codereview.chromium.org/1145013004/diff/80001/components/nacl/browser...
components/nacl/browser/nacl_host_message_filter.cc:161: this,
bad_message::NACL_LAUNCH_CONTINUATION_BAD_ROUTING_ID);
Your name looks nicer, but the convention would be to start with a class name
abbreviation (NHMF_).  Should we be consistent with the others here?  (I'm
probably ok leaving it.)

https://codereview.chromium.org/1145013004/diff/80001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_dispatcher_host.cc (right):

https://codereview.chromium.org/1145013004/diff/80001/content/browser/indexed...
content/browser/indexed_db/indexed_db_dispatcher_host.cc:432:
bad_message::ReceivedBadMessage(this, bad_message::IDBDH_GET_OR_TERMINATE);
Should we distinguish from the kill above?

https://codereview.chromium.org/1145013004/diff/80001/content/browser/rendere...
File content/browser/renderer_host/database_message_filter.cc (right):

https://codereview.chromium.org/1145013004/diff/80001/content/browser/rendere...
content/browser/renderer_host/database_message_filter.cc:348: this,
bad_message::DBMF_INVALID_ORIGIN_ON_HANDLE_ERROR);
DBMF_INVALID_ORIGIN_ON_SQLITE_ERROR?

https://codereview.chromium.org/1145013004/diff/80001/content/public/browser/...
File content/public/browser/browser_message_filter.cc (right):

https://codereview.chromium.org/1145013004/diff/80001/content/public/browser/...
content/public/browser/browser_message_filter.cc:184: // TODO(nick): Should this
work differently on Android?
Maybe reference the fact that RenderProcessHostImpl::Shutdown special cases
Android?  

In the past, klobag@ pointed me to StopChildProcess in
child_process_launcher_android.cc, since the browser process can't kill
renderers directly there.  Not sure if that's available on the IO thread or not.

[ncarter (slow), 2015-05-26 22:59:04]

https://codereview.chromium.org/1145013004/diff/80001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_ui_bindings.cc (right):

https://codereview.chromium.org/1145013004/diff/80001/chrome/browser/devtools...
chrome/browser/devtools/devtools_ui_bindings.cc:778: // TODO(nick): Replace with
chrome::bad_message::ReceivedBadMessage()
On 2015/05/22 20:55:51, Charlie Reis wrote:
> nit: End with period.
> 
> (I'm fine with leaving a TODO for this for now, given how many issues you
> encountered when trying to do it in this CL.)

Done.

https://codereview.chromium.org/1145013004/diff/80001/components/nacl/browser...
File components/nacl/browser/nacl_host_message_filter.cc (right):

https://codereview.chromium.org/1145013004/diff/80001/components/nacl/browser...
components/nacl/browser/nacl_host_message_filter.cc:161: this,
bad_message::NACL_LAUNCH_CONTINUATION_BAD_ROUTING_ID);
On 2015/05/22 20:55:51, Charlie Reis wrote:
> Your name looks nicer, but the convention would be to start with a class name
> abbreviation (NHMF_).  Should we be consistent with the others here?  (I'm
> probably ok leaving it.)

Changed to NHMF_ and NFH_. I used NACL_ originally because the first usage I hit
(in nacl_file_host.cc) was outside of any class.

https://codereview.chromium.org/1145013004/diff/80001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_dispatcher_host.cc (right):

https://codereview.chromium.org/1145013004/diff/80001/content/browser/indexed...
content/browser/indexed_db/indexed_db_dispatcher_host.cc:432:
bad_message::ReceivedBadMessage(this, bad_message::IDBDH_GET_OR_TERMINATE);
On 2015/05/22 20:55:51, Charlie Reis wrote:
> Should we distinguish from the kill above?

These are really just two versions of the same templated function, so I kept
them the same. I also had a really tough time coming up with names to
differentiate the two cases here. If we really wanted to drill down here I think
we'd want a different bad_message reason for each call site, but I think it's
okay to hold off on adding that instrumentation for now.

https://codereview.chromium.org/1145013004/diff/80001/content/browser/rendere...
File content/browser/renderer_host/database_message_filter.cc (right):

https://codereview.chromium.org/1145013004/diff/80001/content/browser/rendere...
content/browser/renderer_host/database_message_filter.cc:348: this,
bad_message::DBMF_INVALID_ORIGIN_ON_HANDLE_ERROR);
On 2015/05/22 20:55:51, Charlie Reis wrote:
> DBMF_INVALID_ORIGIN_ON_SQLITE_ERROR?

Done.

https://codereview.chromium.org/1145013004/diff/80001/content/public/browser/...
File content/public/browser/browser_message_filter.cc (right):

https://codereview.chromium.org/1145013004/diff/80001/content/public/browser/...
content/public/browser/browser_message_filter.cc:184: // TODO(nick): Should this
work differently on Android?
On 2015/05/22 20:55:52, Charlie Reis wrote:
> Maybe reference the fact that RenderProcessHostImpl::Shutdown special cases
> Android?  
> 
> In the past, klobag@ pointed me to StopChildProcess in
> child_process_launcher_android.cc, since the browser process can't kill
> renderers directly there.  Not sure if that's available on the IO thread or
not.

That's exactly what I was referring to with the TODO. I held off because I think
making that change will require adding a significant number of
PwnMessageReceived tests to make sure it works and I don't have a working linux
box to test the android changes handy at the moment.

I updated the comment to mention StopChildProcess explicitly.

[ncarter (slow), 2015-05-26 23:00:05]

nick@chromium.org changed reviewers:
+ isherman@chromium.org, mseaborn@chromium.org, thestig@chromium.org

[ncarter (slow), 2015-05-26 23:00:06]

thestig@chromium.org: Please review changes in chrome/

mseaborn@chromium.org: Please review changes in components/nacl

isherman@chromium.org: Please review changes in tools/metrics

[Ilya Sherman, 2015-05-26 23:07:59]

histograms lgtm

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

https://codereview.chromium.org/1145013004/diff/120001/content/browser/bad_me...
content/browser/bad_message.h:101: IDBDH_GET_OR_TERMINATE = 77,
nit: The manual numbering worries me a bit -- it's too easy to make a typo.  Is
there any advantage to using manual numbering over simply letting the compiler
auto-number?

[Lei Zhang, 2015-05-26 23:14:38]

https://codereview.chromium.org/1145013004/diff/120001/chrome/browser/bad_mes...
File chrome/browser/bad_message.h (right):

https://codereview.chromium.org/1145013004/diff/120001/chrome/browser/bad_mes...
chrome/browser/bad_message.h:13: namespace chrome {
We've decided not to use namespace chrome, so let's not introduce it in more
code.

https://codereview.chromium.org/1145013004/diff/120001/components/nacl.gyp
File components/nacl.gyp (right):

https://codereview.chromium.org/1145013004/diff/120001/components/nacl.gyp#ne...
components/nacl.gyp:93: 'nacl/browser/bad_message.cc',
Update components/nacl/BUILD.gn

[Takashi Toyoshima, 2015-05-27 01:59:19]

content/browser/media/midi_* lgtm

[Mark Seaborn, 2015-05-27 11:50:33]

LGTM for components/nacl/

https://codereview.chromium.org/1145013004/diff/120001/components/nacl/browse...
File components/nacl/browser/bad_message.h (right):

https://codereview.chromium.org/1145013004/diff/120001/components/nacl/browse...
components/nacl/browser/bad_message.h:29: // name (e.g. NaclHostMessageFilter
becomes NHMF) plus a unique description of
What does "NFH" stand for?  Is it nacl_file_host.cc?  Nit: that's not a class
name.

Are these prefixes supposed to get renamed if the file reporting the error gets
renamed?

https://codereview.chromium.org/1145013004/diff/120001/components/nacl/browse...
components/nacl/browser/bad_message.h:35: // renderer or other child process.
Logs the event, records a histogram metric
This says "renderer or other child process".  The text above says "renderer". 
components/nacl/browser/bad_message.cc says "renderer".  Can you make this
consistent?

Currently this is only called to terminate the renderer, so you could drop "or
other child process" for now.

[Charlie Reis, 2015-05-29 23:35:19]

Thanks, still LGTM.

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

https://codereview.chromium.org/1145013004/diff/120001/content/browser/bad_me...
content/browser/bad_message.h:101: IDBDH_GET_OR_TERMINATE = 77,
On 2015/05/26 23:07:59, Ilya Sherman wrote:
> nit: The manual numbering worries me a bit -- it's too easy to make a typo. 
Is
> there any advantage to using manual numbering over simply letting the compiler
> auto-number?

isherman: See mpearson's comment here:
https://codereview.chromium.org/1128993005/diff/1/content/browser/bad_message...

[Ilya Sherman, 2015-05-31 00:30:27]

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

https://codereview.chromium.org/1145013004/diff/120001/content/browser/bad_me...
content/browser/bad_message.h:101: IDBDH_GET_OR_TERMINATE = 77,
On 2015/05/29 23:35:19, Charlie Reis wrote:
> On 2015/05/26 23:07:59, Ilya Sherman wrote:
> > nit: The manual numbering worries me a bit -- it's too easy to make a typo. 
> Is
> > there any advantage to using manual numbering over simply letting the
compiler
> > auto-number?
> 
> isherman: See mpearson's comment here:
>
https://codereview.chromium.org/1128993005/diff/1/content/browser/bad_message...

Hmm, ok.  I guess Mark and I have different opinions about which class of error
to optimize for :P  Thanks for the pointer.

[ncarter (slow), 2015-06-01 17:12:24]

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

https://codereview.chromium.org/1145013004/diff/120001/content/browser/bad_me...
content/browser/bad_message.h:101: IDBDH_GET_OR_TERMINATE = 77,
On 2015/05/31 00:30:27, Ilya Sherman wrote:
> On 2015/05/29 23:35:19, Charlie Reis wrote:
> > On 2015/05/26 23:07:59, Ilya Sherman wrote:
> > > nit: The manual numbering worries me a bit -- it's too easy to make a
typo. 
> > Is
> > > there any advantage to using manual numbering over simply letting the
> compiler
> > > auto-number?
> > 
> > isherman: See mpearson's comment here:
> >
>
https://codereview.chromium.org/1128993005/diff/1/content/browser/bad_message...
> 
> Hmm, ok.  I guess Mark and I have different opinions about which class of
error
> to optimize for :P  Thanks for the pointer.

FWIW, I used the TextPastry plugin to Sublime Text to auto-number these.

Having the numbers be explicit prevents a bug like what happened in
https://codereview.chromium.org/1153503003/ . Especially in these files, if we
auto-numbered, the temptation to group new entries with similarly-prefixed
existing entries would be too great.

Personally I'd rather have the "two entries sharing the same code for a couple
releases" bug than have the "everything was shifted off by one for a couple
releases" bug.

If we beef up the presubmits to detect renumbering or double-numbering problems,
I think we can defend against all kinds of bugs that people are worried about.

[ncarter (slow), 2015-06-01 17:36:20]

https://codereview.chromium.org/1145013004/diff/120001/chrome/browser/bad_mes...
File chrome/browser/bad_message.h (right):

https://codereview.chromium.org/1145013004/diff/120001/chrome/browser/bad_mes...
chrome/browser/bad_message.h:13: namespace chrome {
On 2015/05/26 23:14:38, Lei Zhang wrote:
> We've decided not to use namespace chrome, so let's not introduce it in more
> code.

Done.

https://codereview.chromium.org/1145013004/diff/120001/components/nacl.gyp
File components/nacl.gyp (right):

https://codereview.chromium.org/1145013004/diff/120001/components/nacl.gyp#ne...
components/nacl.gyp:93: 'nacl/browser/bad_message.cc',
On 2015/05/26 23:14:38, Lei Zhang wrote:
> Update components/nacl/BUILD.gn

Done.

[ncarter (slow), 2015-06-01 19:08:11]

https://codereview.chromium.org/1145013004/diff/120001/components/nacl/browse...
File components/nacl/browser/bad_message.h (right):

https://codereview.chromium.org/1145013004/diff/120001/components/nacl/browse...
components/nacl/browser/bad_message.h:29: // name (e.g. NaclHostMessageFilter
becomes NHMF) plus a unique description of
On 2015/05/27 11:50:33, Mark Seaborn wrote:
> What does "NFH" stand for?  Is it nacl_file_host.cc?  Nit: that's not a class
> name.
> 
> Are these prefixes supposed to get renamed if the file reporting the error
gets
> renamed?

I think the point of these is partially mnemonic, and partially to avoid
namespace collisions if two different files handle the same kind of error. NFH
isn't structured as a class, but it is a name that indicates which filename is
better.

I had these as NACL_ in a previous patchset and changed them to NHMF_ based on
Charlie pointing out that it was inconsistent with the pattern used elsewhere. I
had them as NACL_ originally because nacl_file_host wasn't a class name and I
didn't know what else to do. The collision issue seems less likely here, since
this component is so much smaller than the others.

Mark -- are you okay with NFH_ or should I switch back to NACL_ or something
else?

https://codereview.chromium.org/1145013004/diff/120001/components/nacl/browse...
components/nacl/browser/bad_message.h:35: // renderer or other child process.
Logs the event, records a histogram metric
On 2015/05/27 11:50:33, Mark Seaborn wrote:
> This says "renderer or other child process".  The text above says "renderer". 
> components/nacl/browser/bad_message.cc says "renderer".  Can you make this
> consistent?
> 
> Currently this is only called to terminate the renderer, so you could drop "or
> other child process" for now.

A RenderProcessHost implies it's a renderer process. A BrowserMessageFilter
could be created for renderer or non-renderer child processes, hence this
language.

[ncarter (slow), 2015-06-01 19:09:42]

https://codereview.chromium.org/1145013004/diff/120001/components/nacl/browse...
File components/nacl/browser/bad_message.h (right):

https://codereview.chromium.org/1145013004/diff/120001/components/nacl/browse...
components/nacl/browser/bad_message.h:29: // name (e.g. NaclHostMessageFilter
becomes NHMF) plus a unique description of
On 2015/06/01 19:08:11, ncarter wrote:
> On 2015/05/27 11:50:33, Mark Seaborn wrote:
> > What does "NFH" stand for?  Is it nacl_file_host.cc?  Nit: that's not a
class
> > name.
> > 
> > Are these prefixes supposed to get renamed if the file reporting the error
> gets
> > renamed?
> 
> I think the point of these is partially mnemonic, and partially to avoid
> namespace collisions if two different files handle the same kind of error. NFH
> isn't structured as a class, but it is a name that indicates which filename is
> better.
> 
> I had these as NACL_ in a previous patchset and changed them to NHMF_ based on
> Charlie pointing out that it was inconsistent with the pattern used elsewhere.
I
> had them as NACL_ originally because nacl_file_host wasn't a class name and I
> didn't know what else to do. The collision issue seems less likely here, since
> this component is so much smaller than the others.
> 
> Mark -- are you okay with NFH_ or should I switch back to NACL_ or something
> else?

"which filename is better" made no sense -- I meant, NFH gave you an indication
of where to look.

[ncarter (slow), 2015-06-01 21:38:34]

thestig: PTAL

[Lei Zhang, 2015-06-02 17:58:24]

lgtm

[ncarter (slow), 2015-06-02 17:59:14]

The CQ bit was checked by nick@chromium.org

[ncarter (slow), 2015-06-02 17:59:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamescook@chromium.org, kalman@chromium.org, toyoshim@chromium.org,
mseaborn@chromium.org, creis@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/1145013004/#ps140001
(title: "rebase")

[commit-bot: I haz the power, 2015-06-02 17:59:38]

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

[commit-bot: I haz the power, 2015-06-02 19:32:54]

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

[commit-bot: I haz the power, 2015-06-02 19:32:58]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[ncarter (slow), 2015-06-02 19:53:49]

The CQ bit was checked by nick@chromium.org

[commit-bot: I haz the power, 2015-06-02 19:57:22]

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

[commit-bot: I haz the power, 2015-06-02 22:11:00]

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

[commit-bot: I haz the power, 2015-06-02 22:11:03]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[ncarter (slow), 2015-06-02 22:16:51]

The CQ bit was checked by nick@chromium.org

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

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

[commit-bot: I haz the power, 2015-06-02 23:23:50]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-06-02 23:24:33]

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