Only allow SCOPED_CRASH_KEY_BOOL to be used with bool arguments.

Non-bool arguments need to be explicitly converted.

Bug: 1146573
Change-Id: I8cbe519470d7a4c5880c174c33e43abe913ee486
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2585985
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: danakj <danakj@chromium.org>
Reviewed-by: Aaron Colwell <acolwell@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#835941}

base/BUILD.gn

@@ -3518,6 +3518,7 @@ if (build_base_unittests) {
         "containers/buffer_iterator_unittest.nc",
         "containers/checked_iterators_unittest.nc",
         "containers/span_unittest.nc",
+        "debug/crash_logging_unittest.nc",
         "memory/checked_ptr_unittest.nc",
         "memory/ref_counted_unittest.nc",
         "memory/weak_ptr_unittest.nc",

base/debug/crash_logging.h

@@ -8,6 +8,7 @@
 #include <stddef.h>
 
 #include <memory>
+#include <type_traits>
 
 #include "base/base_export.h"
 #include "base/macros.h"
@@ -84,38 +85,40 @@ class BASE_EXPORT ScopedCrashKeyString {
 // The static_assert that checks the length of |key_name| is a compile-time
 // equivalent of the DCHECK in crash_reporter::internal::CrashKeyStringImpl::Set
 // that restricts the name of a crash key to 40 characters.
-#define SCOPED_CRASH_KEY_STRING_INTERNAL2(key_name, key_value, key_value_size, \
-                                          key_var, scoper_var)                 \
-  static_assert(::base::size(key_name) < 40,                                   \
-                "Crash key names need to be shorter than 40 characters.");     \
-  static ::base::debug::CrashKeyString* const key_var =                        \
-      ::base::debug::AllocateCrashKeyString(key_name, key_value_size);         \
-  ::base::debug::ScopedCrashKeyString scoper_var(key_var, (key_value))
-
-#define SCOPED_CRASH_KEY_STRING_INTERNAL(category, name, value, size) \
-  SCOPED_CRASH_KEY_STRING_INTERNAL2(#category "-" #name, value, size, \
-                                    crash_key_string_##name,          \
+#define SCOPED_CRASH_KEY_STRING_INTERNAL2(key_name, key_data, key_data_size, \
+                                          key_var, scoper_var)               \
+  static_assert(::base::size(key_name) < 40,                                 \
+                "Crash key names need to be shorter than 40 characters.");   \
+  static ::base::debug::CrashKeyString* const key_var =                      \
+      ::base::debug::AllocateCrashKeyString(key_name, key_data_size);        \
+  ::base::debug::ScopedCrashKeyString scoper_var(key_var, (key_data))
+
+#define SCOPED_CRASH_KEY_STRING_INTERNAL(category, name, data, size)   \
+  SCOPED_CRASH_KEY_STRING_INTERNAL2(#category "-" #name, (data), size, \
+                                    crash_key_string_##name,           \
                                     crash_scoped_##name)
 
 // Helper macros for putting a local variable crash key on the stack before
 // causing a crash or calling CrashWithoutDumping().
-#define SCOPED_CRASH_KEY_STRING32(category, name, value)  \
-  SCOPED_CRASH_KEY_STRING_INTERNAL(category, name, value, \
+#define SCOPED_CRASH_KEY_STRING32(category, name, data)    \
+  SCOPED_CRASH_KEY_STRING_INTERNAL(category, name, (data), \
                                    base::debug::CrashKeySize::Size32)
 
-#define SCOPED_CRASH_KEY_STRING64(category, name, value)  \
-  SCOPED_CRASH_KEY_STRING_INTERNAL(category, name, value, \
+#define SCOPED_CRASH_KEY_STRING64(category, name, data)    \
+  SCOPED_CRASH_KEY_STRING_INTERNAL(category, name, (data), \
                                    base::debug::CrashKeySize::Size64)
 
-#define SCOPED_CRASH_KEY_STRING256(category, name, value) \
-  SCOPED_CRASH_KEY_STRING_INTERNAL(category, name, value, \
+#define SCOPED_CRASH_KEY_STRING256(category, name, data)   \
+  SCOPED_CRASH_KEY_STRING_INTERNAL(category, name, (data), \
                                    base::debug::CrashKeySize::Size256)
 
-#define SCOPED_CRASH_KEY_BOOL(category, name, value) \
-  SCOPED_CRASH_KEY_STRING32(category, name, (value) ? "true" : "false")
+#define SCOPED_CRASH_KEY_BOOL(category, name, data)                       \
+  static_assert(std::is_same<std::decay_t<decltype(data)>, bool>::value,  \
+                "SCOPED_CRASH_KEY_BOOL must be passed a boolean value."); \
+  SCOPED_CRASH_KEY_STRING32(category, name, (data) ? "true" : "false")
 
-#define SCOPED_CRASH_KEY_NUMBER(category, name, value) \
-  SCOPED_CRASH_KEY_STRING32(category, name, base::NumberToString(value))
+#define SCOPED_CRASH_KEY_NUMBER(category, name, data) \
+  SCOPED_CRASH_KEY_STRING32(category, name, base::NumberToString(data))
 
 ////////////////////////////////////////////////////////////////////////////////
 // The following declarations are used to initialize the crash key system

base/debug/crash_logging_unittest.nc

+// Copyright (c) 2011 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.
+
+// This is a "No Compile Test" suite.
+// http://dev.chromium.org/developers/testing/no-compile-tests
+
+#include "base/debug/crash_logging.h"
+
+#if defined(NCTEST_SCOPED_CRASH_KEY_BOOL_ON_NON_BOOL_ARG)  // [r"static_assert failed due to requirement 'std::is_same<int, bool>::value' \"SCOPED_CRASH_KEY_BOOL must be passed a boolean value\.\""]
+
+void WontCompile() {
+  SCOPED_CRASH_KEY_BOOL(category, name, 1);
+}
+
+#endif

content/browser/renderer_host/render_frame_host_manager.cc

@@ -767,14 +767,14 @@ void RenderFrameHostManager::ValidateSpeculativeRenderFrameHostForBug1146573(
                                pending->GetSiteInstance()->GetSiteURL().spec());
     SCOPED_CRASH_KEY_BOOL(ValidateSpeculative, MustBeReplaced,
                           current->must_be_replaced());
-    SCOPED_CRASH_KEY_BOOL(ValidateSpeculative, OldProcessID,
-                          current->GetProcess()->GetID());
-    SCOPED_CRASH_KEY_BOOL(ValidateSpeculative, OldRoutingID,
-                          current->GetRoutingID());
-    SCOPED_CRASH_KEY_BOOL(ValidateSpeculative, NewProcessID,
-                          pending->GetProcess()->GetID());
-    SCOPED_CRASH_KEY_BOOL(ValidateSpeculative, NewRoutingID,
-                          pending->GetRoutingID());
+    SCOPED_CRASH_KEY_NUMBER(ValidateSpeculative, OldProcessID,
+                            current->GetProcess()->GetID());
+    SCOPED_CRASH_KEY_NUMBER(ValidateSpeculative, OldRoutingID,
+                            current->GetRoutingID());
+    SCOPED_CRASH_KEY_NUMBER(ValidateSpeculative, NewProcessID,
+                            pending->GetProcess()->GetID());
+    SCOPED_CRASH_KEY_NUMBER(ValidateSpeculative, NewRoutingID,
+                            pending->GetRoutingID());
     NOTREACHED();
     base::debug::DumpWithoutCrashing();
   }