Better crash stacktraces for ExtensionFunction bad messages.

extensions/browser/extension_function.cc

Index: extensions/browser/extension_function.cc
diff --git a/extensions/browser/extension_function.cc b/extensions/browser/extension_function.cc
index 548153710550e3e388f154ae1092651d1b7def8e..1fb83353e3f6f9c0c30ed7ecf0a262e1d0889fa6 100644
--- a/extensions/browser/extension_function.cc
+++ b/extensions/browser/extension_function.cc
@@ -16,8 +16,10 @@
 #include "content/public/browser/notification_source.h"
 #include "content/public/browser/notification_types.h"
 #include "content/public/browser/render_frame_host.h"
+#include "content/public/browser/user_metrics.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_contents_observer.h"
+#include "extensions/browser/bad_message.h"
 #include "extensions/browser/extension_function_dispatcher.h"
 #include "extensions/browser/extension_message_filter.h"
 #include "extensions/browser/extensions_browser_client.h"
@@ -76,6 +78,27 @@ void LogUma(bool success,
   }
 }
 
+void LogBadMessage(extensions::functions::HistogramValue histogram_value) {
+  content::RecordAction(base::UserMetricsAction("BadMessageTerminate_EFD"));
+  // Track the specific function's |histogram_value|, as this may indicate a
+  // bug in that API's implementation.
+  UMA_HISTOGRAM_ENUMERATION("Extensions.BadMessageFunctionName",
+                            histogram_value,
+                            extensions::functions::ENUM_BOUNDARY);
+}
+
+template <class T>
+void ReceivedBadMessage(T* bad_message_sender,
+                        extensions::bad_message::BadMessageReason reason,
+                        extensions::functions::HistogramValue histogram_value) {
+  LogBadMessage(histogram_value);
+  // The renderer has done validation before sending extension api requests.
+  // Therefore, we should never receive a request that is invalid in a way
+  // that JSON validation in the renderer should have caught. It could be an
+  // attacker trying to exploit the browser, so we crash the renderer instead.
+  extensions::bad_message::ReceivedBadMessage(bad_message_sender, reason);
+}
+
 class ArgumentListResponseValue
     : public ExtensionFunction::ResponseValueObject {
  public:
@@ -122,7 +145,7 @@ class ErrorResponseValue : public ExtensionFunction::ResponseValueObject {
 class BadMessageResponseValue : public ExtensionFunction::ResponseValueObject {
  public:
   explicit BadMessageResponseValue(ExtensionFunction* function) {
-    function->set_bad_message(true);
+    function->SetBadMessage();
     NOTREACHED() << function->name() << ": bad message";
   }
 
@@ -319,6 +342,10 @@ const std::string& ExtensionFunction::GetError() const {
   return error_;
 }
 
+void ExtensionFunction::SetBadMessage() {

[Devlin, 2017/01/27 17:38:58]

Does this compile?  ExtensionFunction::SetBadMessage() is pure virtual in the
header.

[lazyboy, 2017/01/27 18:17:24]

Ah, I've put a mental note to fix it after noticing that this surprisingly
compiled locally: both chrome & browser_tests target were fine. But forgot.

Needs a separate look into this issue. Will do.

Done.

+  bad_message_ = true;
+}
+
 bool ExtensionFunction::user_gesture() const {
   return user_gesture_ || UserGestureForTests::GetInstance()->HaveGesture();
 }
@@ -508,6 +535,18 @@ bool UIThreadExtensionFunction::PreRunValidation(std::string* error) {
   return true;
 }
 
+void UIThreadExtensionFunction::SetBadMessage() {
+  ExtensionFunction::SetBadMessage();
+
+  if (render_frame_host()) {
+    ReceivedBadMessage(render_frame_host()->GetProcess(),
+                       is_from_service_worker()
+                           ? extensions::bad_message::EFD_BAD_MESSAGE_WORKER
+                           : extensions::bad_message::EFD_BAD_MESSAGE,
+                       histogram_value());
+  }
+}
+
 bool UIThreadExtensionFunction::OnMessageReceived(const IPC::Message& message) {
   return false;
 }
@@ -577,6 +616,15 @@ IOThreadExtensionFunction::AsIOThreadExtensionFunction() {
   return this;
 }
 
+void IOThreadExtensionFunction::SetBadMessage() {
+  ExtensionFunction::SetBadMessage();
+  if (ipc_sender_) {
+    ReceivedBadMessage((content::BrowserMessageFilter*)ipc_sender_.get(),

[Devlin, 2017/01/27 17:38:58]

nit: prefer static_cast<> over c-style casts

[lazyboy, 2017/01/27 18:17:24]

Done.

+                       extensions::bad_message::EFD_BAD_MESSAGE,
+                       histogram_value());
+  }
+}
+
 void IOThreadExtensionFunction::Destruct() const {
   BrowserThread::DeleteOnIOThread::Destruct(this);
 }