Better bad message reporting from IO thread

This makes crash dumping optional for RPH::ShutdownForBadMessage,
and changes the thread-safe bad_message::ReceivedBadMessage to
generate its own crash dump immediately, before potentially posting
a UI thread task.

All other call sites have been updated to opt into crash dumping,
preserving previous behavior.

This ensures that bad message reports from the IO thread have a
useful crash stack.

BUG=652755
R=jam@chromium.org

Review-Url: https://codereview.chromium.org/2387113004
Cr-Commit-Position: refs/heads/master@{#423040}

chrome/browser/bad_message.cc

@@ -23,7 +23,8 @@ void LogBadMessage(BadMessageReason reason) {
 void ReceivedBadMessage(content::RenderProcessHost* host,
                         BadMessageReason reason) {
   LogBadMessage(reason);
-  host->ShutdownForBadMessage();
+  host->ShutdownForBadMessage(
+      content::RenderProcessHost::CrashReportMode::GENERATE_CRASH_DUMP);
 }
 
 void ReceivedBadMessage(content::BrowserMessageFilter* filter,

components/password_manager/content/browser/bad_message.cc

@@ -18,7 +18,8 @@ void ReceivedBadMessage(content::RenderProcessHost* host,
       << static_cast<int>(reason);
   UMA_HISTOGRAM_SPARSE_SLOWLY("Stability.BadMessageTerminated.PasswordManager",
                               static_cast<int>(reason));
-  host->ShutdownForBadMessage();
+  host->ShutdownForBadMessage(
+      content::RenderProcessHost::CrashReportMode::GENERATE_CRASH_DUMP);
 }
 
 }  // namespace bad_message

content/browser/bad_message.cc

@@ -6,6 +6,7 @@
 
 #include "base/bind.h"
 #include "base/debug/crash_logging.h"
+#include "base/debug/dump_without_crashing.h"
 #include "base/logging.h"
 #include "base/metrics/sparse_histogram.h"
 #include "base/strings/string_number_conversions.h"
@@ -29,18 +30,29 @@ void ReceivedBadMessageOnUIThread(int render_process_id,
                                   BadMessageReason reason) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   RenderProcessHost* host = RenderProcessHost::FromID(render_process_id);
-  if (host)
-    ReceivedBadMessage(host, reason);
+  if (!host)
+    return;
+
+  LogBadMessage(reason);
+
+  // A dump has already been generated by the caller. Don't generate another.
+  host->ShutdownForBadMessage(
+      RenderProcessHost::CrashReportMode::NO_CRASH_DUMP);
 }
 
 }  // namespace
 
 void ReceivedBadMessage(RenderProcessHost* host, BadMessageReason reason) {
   LogBadMessage(reason);
-  host->ShutdownForBadMessage();
+  host->ShutdownForBadMessage(
+      RenderProcessHost::CrashReportMode::GENERATE_CRASH_DUMP);
 }
 
 void ReceivedBadMessage(int render_process_id, BadMessageReason reason) {
+  // We generate a crash dump here since generating one after posting the UI
+  // thread is mostly useless.
+  base::debug::DumpWithoutCrashing();
+
   if (!BrowserThread::CurrentlyOn(BrowserThread::UI)) {
     BrowserThread::PostTask(
         BrowserThread::UI,

content/browser/media/android/browser_media_session_manager.cc

@@ -36,7 +36,8 @@ void BrowserMediaSessionManager::OnSetMetadata(
   // coming from a known and secure source. It must be processed accordingly.
   if (insecure_metadata.has_value() &&
       !MediaMetadataSanitizer::CheckSanity(insecure_metadata.value())) {
-    render_frame_host_->GetProcess()->ShutdownForBadMessage();
+    render_frame_host_->GetProcess()->ShutdownForBadMessage(
+        RenderProcessHost::CrashReportMode::GENERATE_CRASH_DUMP);
     return;
   }
 

content/browser/renderer_host/render_process_host_impl.cc

@@ -972,9 +972,7 @@ bool RenderProcessHostImpl::Init() {
     child_process_launcher_.reset(new ChildProcessLauncher(
         new RendererSandboxedProcessLauncherDelegate(channel_.get()), cmd_line,
         GetID(), this, child_token_,
-        base::Bind(&RenderProcessHostImpl::OnMojoError,
-                   weak_factory_.GetWeakPtr(),
-                   base::ThreadTaskRunnerHandle::Get())));
+        base::Bind(&RenderProcessHostImpl::OnMojoError, id_)));
     channel_->Pause();
 
     fast_shutdown_started_ = false;
@@ -1463,7 +1461,8 @@ void RenderProcessHostImpl::RemoveObserver(
   observers_.RemoveObserver(observer);
 }
 
-void RenderProcessHostImpl::ShutdownForBadMessage() {
+void RenderProcessHostImpl::ShutdownForBadMessage(
+    CrashReportMode crash_report_mode) {
   base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
   if (command_line->HasSwitch(switches::kDisableKillAfterBadIPC))
     return;
@@ -1478,8 +1477,10 @@ void RenderProcessHostImpl::ShutdownForBadMessage() {
   // browser to try to survive when it gets illegal messages from the renderer.
   Shutdown(RESULT_CODE_KILLED_BAD_MESSAGE, false);
 
-  // Report a crash, since none will be generated by the killed renderer.
-  base::debug::DumpWithoutCrashing();
+  if (crash_report_mode == CrashReportMode::GENERATE_CRASH_DUMP) {
+    // Report a crash, since none will be generated by the killed renderer.
+    base::debug::DumpWithoutCrashing();
+  }
 
   // Log the renderer kill to the histogram tracking all kills.
   BrowserChildProcessHostImpl::HistogramBadMessageTerminated(
@@ -3031,24 +3032,14 @@ void RenderProcessHostImpl::RecomputeAndUpdateWebKitPreferences() {
 }
 
 // static
-void RenderProcessHostImpl::OnMojoError(
-    base::WeakPtr<RenderProcessHostImpl> process,
-    scoped_refptr<base::SingleThreadTaskRunner> task_runner,
-    const std::string& error) {
-  if (!task_runner->BelongsToCurrentThread()) {
-    task_runner->PostTask(FROM_HERE,
-                          base::Bind(&RenderProcessHostImpl::OnMojoError,
-                                     process, task_runner, error));
-    return;
-  }
-  if (!process)
-    return;
+void RenderProcessHostImpl::OnMojoError(int render_process_id,
+                                        const std::string& error) {
   LOG(ERROR) << "Terminating render process for bad Mojo message: " << error;
 
   // The ReceivedBadMessage call below will trigger a DumpWithoutCrashing. Alias
   // enough information here so that we can determine what the bad message was.
   base::debug::Alias(&error);
-  bad_message::ReceivedBadMessage(process.get(),
+  bad_message::ReceivedBadMessage(render_process_id,
                                   bad_message::RPH_MOJO_PROCESS_ERROR);
 }
 

content/browser/renderer_host/render_process_host_impl.h

@@ -121,7 +121,7 @@ class CONTENT_EXPORT RenderProcessHostImpl
   void RemoveRoute(int32_t routing_id) override;
   void AddObserver(RenderProcessHostObserver* observer) override;
   void RemoveObserver(RenderProcessHostObserver* observer) override;
-  void ShutdownForBadMessage() override;
+  void ShutdownForBadMessage(CrashReportMode crash_report_mode) override;
   void WidgetRestored() override;
   void WidgetHidden() override;
   int VisibleWidgetCount() const override;
@@ -378,10 +378,7 @@ class CONTENT_EXPORT RenderProcessHostImpl
   base::FilePath GetAecDumpFilePathWithExtensions(const base::FilePath& file);
 #endif
 
-  static void OnMojoError(
-      base::WeakPtr<RenderProcessHostImpl> process,
-      scoped_refptr<base::SingleThreadTaskRunner> task_runner,
-      const std::string& error);
+  static void OnMojoError(int render_process_id, const std::string& error);
 
   template <typename InterfaceType>
   using AddInterfaceCallback =

content/public/browser/render_process_host.h

@@ -65,6 +65,12 @@ class CONTENT_EXPORT RenderProcessHost : public IPC::Sender,
     int exit_code;
   };
 
+  // Crash reporting mode for ShutdownForBadMessage.
+  enum class CrashReportMode {
+    NO_CRASH_DUMP,
+    GENERATE_CRASH_DUMP,
+  };
+
   // General functions ---------------------------------------------------------
 
   ~RenderProcessHost() override {}
@@ -95,7 +101,10 @@ class CONTENT_EXPORT RenderProcessHost : public IPC::Sender,
   // Most callers should not call this directly, but instead should call
   // bad_message::BadMessageReceived() or an equivalent method outside of the
   // content module.
-  virtual void ShutdownForBadMessage() = 0;
+  //
+  // If |crash_report_mode| is GENERATE_CRASH_DUMP, then a browser crash dump
+  // will be reported as well.
+  virtual void ShutdownForBadMessage(CrashReportMode crash_report_mode) = 0;
 
   // Track the count of visible widgets. Called by listeners to register and
   // unregister visibility.

content/public/test/mock_render_process_host.cc

@@ -128,7 +128,8 @@ void MockRenderProcessHost::RemoveObserver(
   observers_.RemoveObserver(observer);
 }
 
-void MockRenderProcessHost::ShutdownForBadMessage() {
+void MockRenderProcessHost::ShutdownForBadMessage(
+    CrashReportMode crash_report_mode) {
   ++bad_msg_count_;
 }
 

content/public/test/mock_render_process_host.h

@@ -50,7 +50,7 @@ class MockRenderProcessHost : public RenderProcessHost {
   void RemoveRoute(int32_t routing_id) override;
   void AddObserver(RenderProcessHostObserver* observer) override;
   void RemoveObserver(RenderProcessHostObserver* observer) override;
-  void ShutdownForBadMessage() override;
+  void ShutdownForBadMessage(CrashReportMode crash_report_mode) override;
   void WidgetRestored() override;
   void WidgetHidden() override;
   int VisibleWidgetCount() const override;

extensions/browser/bad_message.cc

@@ -17,7 +17,8 @@ void ReceivedBadMessage(content::RenderProcessHost* host,
              << reason;
   UMA_HISTOGRAM_SPARSE_SLOWLY("Stability.BadMessageTerminated.Extensions",
                               reason);
-  host->ShutdownForBadMessage();
+  host->ShutdownForBadMessage(
+      content::RenderProcessHost::CrashReportMode::GENERATE_CRASH_DUMP);
 }
 
 }  // namespace bad_message