Upload crash reports when ExtensionFunctions generate bad messages.

extensions/browser/bad_message.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "extensions/browser/bad_message.h"
 
 #include "base/logging.h"
 #include "base/metrics/histogram_macros.h"
 #include "content/public/browser/browser_message_filter.h"
 #include "content/public/browser/render_process_host.h"
 
 namespace extensions {
 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",
                               reason);
 }
 
 }  // namespace
 
 void ReceivedBadMessage(content::RenderProcessHost* host,
                         BadMessageReason reason) {
   LogBadMessage(reason);
   host->ShutdownForBadMessage(
       content::RenderProcessHost::CrashReportMode::GENERATE_CRASH_DUMP);
 }
 
 void ReceivedBadMessage(content::BrowserMessageFilter* filter,
                         BadMessageReason reason) {
   LogBadMessage(reason);
   filter->ShutdownForBadMessage();
 }
 
 }  // namespace bad_message
 }  // namespace extensions