Upload crash reports when ExtensionFunctions generate bad messages.

extensions/browser/bad_message.cc

Index: extensions/browser/bad_message.cc
diff --git a/extensions/browser/bad_message.cc b/extensions/browser/bad_message.cc
index da8edbc440315c4067e4e140e033dcac99aa6afc..ee8927896681da10585613aa387c1ae64ef29c82 100644
--- a/extensions/browser/bad_message.cc
+++ b/extensions/browser/bad_message.cc
@@ -15,6 +15,16 @@ namespace bad_message {
 namespace {
 
 void LogBadMessage(BadMessageReason reason) {
+  // EFD_BAD_MESSAGE uses different logging and UMA.
+  // See callers of ReceivedBadMessage() in ExtensionFunctionDispatcher.
+  if (reason == EFD_BAD_MESSAGE)

[Charlie Reis, 2017/01/13 21:22:25]

It feels unfortunate to bake knowledge about particular BadMessageReasons into
this class.  Maybe we can live with it in this extensions/ version of
BadMessage, but would it be a problem to just let both log/uma types happen on
EFD_BAD_MESSAGE?  That seems simpler to me.

[lazyboy, 2017/01/14 01:04:43]

SG, Done.

+    return;
+
+  // TODO(creis): We should call SetCrashKeyValue("bad_message_reason") similar
+  // to the content/ implementation, so the generated crash reports are
+  // easier to search. Not straightforward because
+  // extensions::bad_message::BadMessageReason and
+  // content::bad_message::BadMessageReason enum values overlap.

[Charlie Reis, 2017/01/13 21:22:25]

The crash key only helps if there are multiple ReceivedBadMessage calls from the
same method, so that we can offer the same magic signature for them.  I don't
think you're adding a case of that in this CL, but I suppose there is a
pre-existing case in AppViewGuest::CompletePendingRequest...

Ok, let's rephrase the TODO:
TODO(creis): We should add a crash key similar to the "bad_message_reason" key
logged in content::bad_message::ReceivedBadMessage, for disambiguating multiple
kills in the same method.  It's important not to overlap with the
content::bad_message::BadMessageReason enum values. 

[lazyboy, 2017/01/14 01:04:43]

Interesting.
Although I feel you meant different signatures?
Got it.
but I suppose there is a
Done.

[Charlie Reis, 2017/01/15 18:08:51]

Right!  Sorry about the typo.  :)

   LOG(ERROR) << "Terminating extension renderer for bad IPC message, reason "
              << reason;
   UMA_HISTOGRAM_SPARSE_SLOWLY("Stability.BadMessageTerminated.Extensions",