Use "mojo-message-error" instead of "bad_message_reason" crash key name.

NetworkService was incorrectly using "bad_message_reason" crash key name
when reporting a bad mojo message.  This CL switches to using the
correct "mojo-message-error" name (one that is recognized by the crash
analysis services).  This CL also moves the crash key definition to the
//mojo layer, so that it can be reused both from //content and from
//services/network.

Bug: 987986
Change-Id: I772ae563df2f62ea430235524397b8492455de23
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2118690Reviewed-by: Ken Rockot <rockot@google.com>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Maksim Orlovich <morlovich@chromium.org>
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#753484}

content/browser/bad_message.cc

@@ -70,12 +70,6 @@ void ReceivedBadMessage(BrowserMessageFilter* filter, BadMessageReason reason) {
   filter->ShutdownForBadMessage();
 }
 
-base::debug::CrashKeyString* GetMojoErrorCrashKey() {
-  static auto* crash_key = base::debug::AllocateCrashKeyString(
-      "mojo-message-error", base::debug::CrashKeySize::Size256);
-  return crash_key;
-}
-
 base::debug::CrashKeyString* GetRequestedSiteURLKey() {
   static auto* crash_key = base::debug::AllocateCrashKeyString(
       "requested_site_url", base::debug::CrashKeySize::Size64);

content/browser/bad_message.h

@@ -275,10 +275,6 @@ CONTENT_EXPORT void ReceivedBadMessage(int render_process_id,
 // for the |reason|, and terminates the process for |filter|.
 void ReceivedBadMessage(BrowserMessageFilter* filter, BadMessageReason reason);
 
-// Returns a crash key named "mojo-message-error" for storing Mojo error
-// messages.
-base::debug::CrashKeyString* GetMojoErrorCrashKey();
-
 // Site isolation. These keys help debug renderer kills such as
 // https://crbug.com/773140.
 // Retuns a key named "requested_site_url".

content/browser/browser_child_process_host_impl.cc

@@ -7,7 +7,6 @@
 #include "base/base_switches.h"
 #include "base/bind.h"
 #include "base/command_line.h"
-#include "base/debug/crash_logging.h"
 #include "base/debug/dump_without_crashing.h"
 #include "base/feature_list.h"
 #include "base/files/file_path.h"
@@ -30,7 +29,6 @@
 #include "build/build_config.h"
 #include "components/tracing/common/trace_startup_config.h"
 #include "components/tracing/common/tracing_switches.h"
-#include "content/browser/bad_message.h"
 #include "content/browser/browser_main_loop.h"
 #include "content/browser/histogram_controller.h"
 #include "content/browser/tracing/background_tracing_manager_impl.h"
@@ -50,6 +48,7 @@
 #include "content/public/common/process_type.h"
 #include "content/public/common/result_codes.h"
 #include "content/public/common/sandboxed_process_launcher_delegate.h"
+#include "mojo/public/cpp/bindings/scoped_message_error_crash_key.h"
 #include "mojo/public/cpp/system/platform_handle.h"
 #include "net/websockets/websocket_basic_stream.h"
 #include "net/websockets/websocket_channel.h"
@@ -694,8 +693,7 @@ void BrowserChildProcessHostImpl::OnMojoError(
   // It is important to call DumpWithoutCrashing synchronously - this will help
   // to preserve the callstack and the crash keys present when the bad mojo
   // message was received.
-  base::debug::ScopedCrashKeyString scoped_error_key(
-      bad_message::GetMojoErrorCrashKey(), error);
+  mojo::debug::ScopedMessageErrorCrashKey scoped_error_key(error);
   base::debug::DumpWithoutCrashing();
 
   if (task_runner->BelongsToCurrentThread()) {

content/browser/renderer_host/render_process_host_impl.cc

@@ -202,6 +202,7 @@
 #include "media/webrtc/webrtc_switches.h"
 #include "mojo/public/cpp/bindings/pending_receiver.h"
 #include "mojo/public/cpp/bindings/receiver.h"
+#include "mojo/public/cpp/bindings/scoped_message_error_crash_key.h"
 #include "mojo/public/cpp/system/platform_handle.h"
 #include "net/url_request/url_request_context_getter.h"
 #include "services/device/public/mojom/battery_monitor.mojom.h"
@@ -4980,8 +4981,7 @@ void RenderProcessHostImpl::OnMojoError(int render_process_id,
 
   // The ReceivedBadMessage call below will trigger a DumpWithoutCrashing.
   // Capture the error message in a crash key value.
-  base::debug::ScopedCrashKeyString error_key_value(
-      bad_message::GetMojoErrorCrashKey(), error);
+  mojo::debug::ScopedMessageErrorCrashKey error_key_value(error);
   bad_message::ReceivedBadMessage(render_process_id,
                                   bad_message::RPH_MOJO_PROCESS_ERROR);
 }

mojo/public/cpp/bindings/BUILD.gn

@@ -96,6 +96,8 @@ component("bindings_base") {
     "message.h",
     "message_header_validator.h",
     "scoped_interface_endpoint_handle.h",
+    "scoped_message_error_crash_key.cc",
+    "scoped_message_error_crash_key.h",
     "string_data_view.h",
     "string_traits.h",
     "string_traits_stl.h",

mojo/public/cpp/bindings/scoped_message_error_crash_key.cc

+// Copyright 2020 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 "mojo/public/cpp/bindings/scoped_message_error_crash_key.h"
+
+namespace mojo {
+namespace debug {
+
+namespace {
+
+base::debug::CrashKeyString* GetMojoMessageErrorCrashKey() {
+  // The "mojo-message-error" name used below is recognized by Chrome crash
+  // analysis services - please avoid changing the name if possible.
+  static auto* crash_key = base::debug::AllocateCrashKeyString(
+      "mojo-message-error", base::debug::CrashKeySize::Size256);
+  return crash_key;
+}
+
+}  // namespace
+
+ScopedMessageErrorCrashKey::ScopedMessageErrorCrashKey(
+    const std::string& mojo_message_error)
+    : base::debug::ScopedCrashKeyString(GetMojoMessageErrorCrashKey(),
+                                        mojo_message_error) {}
+
+ScopedMessageErrorCrashKey::~ScopedMessageErrorCrashKey() = default;
+
+}  // namespace debug
+}  // namespace mojo

mojo/public/cpp/bindings/scoped_message_error_crash_key.h

+// Copyright 2020 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.
+
+#ifndef MOJO_PUBLIC_CPP_BINDINGS_SCOPED_MESSAGE_ERROR_CRASH_KEY_H_
+#define MOJO_PUBLIC_CPP_BINDINGS_SCOPED_MESSAGE_ERROR_CRASH_KEY_H_
+
+#include <string>
+
+#include "base/component_export.h"
+#include "base/debug/crash_logging.h"
+
+namespace mojo {
+namespace debug {
+
+// Helper class for storing |mojo_message_error| in the right crash key (when
+// initiating a base::debug::DumpWithoutCrashing because of a bad message
+// report).
+class COMPONENT_EXPORT(MOJO_CPP_BINDINGS_BASE) ScopedMessageErrorCrashKey
+    : public base::debug::ScopedCrashKeyString {
+ public:
+  explicit ScopedMessageErrorCrashKey(const std::string& mojo_message_error);
+  ~ScopedMessageErrorCrashKey();
+
+  ScopedMessageErrorCrashKey(const ScopedMessageErrorCrashKey&) = delete;
+  ScopedMessageErrorCrashKey& operator=(const ScopedMessageErrorCrashKey&) =
+      delete;
+};
+
+}  // namespace debug
+}  // namespace mojo
+
+#endif  // MOJO_PUBLIC_CPP_BINDINGS_SCOPED_MESSAGE_ERROR_CRASH_KEY_H_

services/network/network_service.cc

@@ -27,6 +27,7 @@
 #include "components/network_session_configurator/common/network_features.h"
 #include "components/os_crypt/os_crypt.h"
 #include "mojo/core/embedder/embedder.h"
+#include "mojo/public/cpp/bindings/scoped_message_error_crash_key.h"
 #include "net/base/logging_network_change_observer.h"
 #include "net/base/network_change_notifier.h"
 #include "net/base/network_change_notifier_posix.h"
@@ -181,9 +182,7 @@ std::unique_ptr<net::HttpAuthMechanism> CreateAuthSystem(
 // message handling inside the Browser process is sufficient).
 void HandleBadMessage(const std::string& error) {
   LOG(WARNING) << "Mojo error in NetworkService:" << error;
-  static auto* bad_message_reason = base::debug::AllocateCrashKeyString(
-      "bad_message_reason", base::debug::CrashKeySize::Size256);
-  base::debug::SetCrashKeyString(bad_message_reason, error);
+  mojo::debug::ScopedMessageErrorCrashKey crash_key_value(error);
   base::debug::DumpWithoutCrashing();
 }