Ensure that any IPC sent from a child process that couldn't be deserialized causes that p…

Ensure that any IPC sent from a child process that couldn't be deserialized causes that process to be killed.

Today we do this only for a subset of IPCs and not all process types.

R=jar@chromium.org, tsepez@chromium.org

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

[jam, 2014-05-15 19:47:36]

+jar for tools/metrics/actions

[jar (doing other things), 2014-05-15 19:54:21]

Did you consider using an enumerated histograms?
UMA_HISTOGRAM_ENUMERATION(
  "histogram.name",
  sample,
   MAX_ENUMERATION_VALUE + 1)
Usually you put in in a function, and just call it in all the places you would
otherwise have called for recording an action.

I think you want to compare these tallies, and you *probably* don't care as much
about sequencing and such.  You'll also have an easier time debugging, and
looking at your machine (or others) to see what has been going on.

[Tom Sepez, 2014-05-15 19:57:41]

https://codereview.chromium.org/283313002/diff/90001/content/browser/browser_...
File content/browser/browser_child_process_host_impl.cc (right):

https://codereview.chromium.org/283313002/diff/90001/content/browser/browser_...
content/browser/browser_child_process_host_impl.cc:260: // The English name of a
process type sometimes has spaces. Remove them since
Maybe we want to add GetProcessTypeNameForUMA() rather than mucking about with
the syntax here.  Later on, someone may add other funny characters other than
spaces when they create a new process type.

https://codereview.chromium.org/283313002/diff/90001/content/common/process_t...
File content/common/process_type.cc (right):

https://codereview.chromium.org/283313002/diff/90001/content/common/process_t...
content/common/process_type.cc:13: // If any of these names are changed or new
ones added, update
If we added GetProcessTypeNameForUMA here, it could either do its own switch or
call into ...InEnglish and then muck with characters.

Replacing all but alphanumerics might be a better strategy.

https://codereview.chromium.org/283313002/diff/90001/ipc/ipc_message_macros.h
File ipc/ipc_message_macros.h (right):

https://codereview.chromium.org/283313002/diff/90001/ipc/ipc_message_macros.h...
ipc/ipc_message_macros.h:937:
const_cast<IPC::Message&>(ipc_message__).set_dispatch_error();       \
What if we wanted to avoid the const cast by making the all the other places
non-const (since it's more accurate with this change). Kinda like pulling the
thread on a sweater ... ?

[jam, 2014-05-15 21:14:28]

On 2014/05/15 19:54:21, jar wrote:
> Did you consider using an enumerated histograms?
> UMA_HISTOGRAM_ENUMERATION(
>   "histogram.name",
>   sample,
>    MAX_ENUMERATION_VALUE + 1)
> Usually you put in in a function, and just call it in all the places you would
> otherwise have called for recording an action.
> 
> I think you want to compare these tallies, and you *probably* don't care as
much
> about sequencing and such.  You'll also have an easier time debugging, and
> looking at your machine (or others) to see what has been going on.

I'm not sure what the benefit of this is? Right now TPMs need an easy way to
figure out how many crashes are caused by an invalid IPC, and specifically which
kind of processes. This approach of using counts has been fine for the last few
years, so I prefer to keep the same unless there's a big benefit to changing it.

[jam, 2014-05-15 21:19:33]

https://codereview.chromium.org/283313002/diff/90001/content/browser/browser_...
File content/browser/browser_child_process_host_impl.cc (right):

https://codereview.chromium.org/283313002/diff/90001/content/browser/browser_...
content/browser/browser_child_process_host_impl.cc:260: // The English name of a
process type sometimes has spaces. Remove them since
On 2014/05/15 19:57:42, Tom Sepez wrote:
> Maybe we want to add GetProcessTypeNameForUMA() rather than mucking about with
> the syntax here.  Later on, someone may add other funny characters other than
> spaces when they create a new process type.

i prefer not adding yet another way of getting a name unless i know for sure we
need it. i'm not sure what you mean by funny chars, so let's consider that later
if it happens

https://codereview.chromium.org/283313002/diff/90001/ipc/ipc_message_macros.h
File ipc/ipc_message_macros.h (right):

https://codereview.chromium.org/283313002/diff/90001/ipc/ipc_message_macros.h...
ipc/ipc_message_macros.h:937:
const_cast<IPC::Message&>(ipc_message__).set_dispatch_error();       \
On 2014/05/15 19:57:42, Tom Sepez wrote:
> What if we wanted to avoid the const cast by making the all the other places
> non-const (since it's more accurate with this change). Kinda like pulling the
> thread on a sweater ... ?

actually, i can simplify this more now by making dispatch_error_ mutable. before
i was using the header field but that failed for other reasons. const_cast
removed now.

[Tom Sepez, 2014-05-15 21:21:51]

LGTM

[jar (doing other things), 2014-05-15 23:03:54]

histograms.xml LGTM