Reland "Make DUMP_WILL_BE_* fatal for non-official builds"

This is a reland of commit 029ef5893a

Original change's description:
> Make DUMP_WILL_BE_* fatal for non-official builds
>
> This is a proxy for non-user-facing builds (testing, fuzzing). Any early
> detection for will-be-fatal invariant violations seems good at face
> value, let's try it out.
>
> Bug: None
> Change-Id: I46322b144d8e871bb4a83e9fe45f0958ebe97eee
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5783255
> Reviewed-by: Daniel Cheng <dcheng@chromium.org>
> Commit-Queue: Daniel Cheng <dcheng@chromium.org>
> Code-Coverage: findit-for-me@appspot.gserviceaccount.com <findit-for-me@appspot.gserviceaccount.com>
> Auto-Submit: Peter Boström <pbos@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1341369}

Bug: None
Change-Id: Ifc76813daae7298d5d7913e864dbe2ba8b8658ec
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5792849
Owners-Override: Daniel Cheng <dcheng@chromium.org>
Code-Coverage: findit-for-me@appspot.gserviceaccount.com <findit-for-me@appspot.gserviceaccount.com>
Commit-Queue: Peter Boström <pbos@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1352536}

base/check.cc

@@ -30,12 +30,13 @@ namespace logging {
 namespace {
 
 LogSeverity GetDumpSeverity() {
-#if BUILDFLAG(USE_FUZZING_ENGINE)
-  // Crash in fuzzing builds because non-fatal CHECKs will eventually be
-  // migrated to fatal CHECKs.
-  return LOGGING_FATAL;
-#else
+#if defined(OFFICIAL_BUILD)
   return DCHECK_IS_ON() ? LOGGING_DCHECK : LOGGING_ERROR;
+#else
+  // Crash outside official builds (outside user-facing builds) to detect
+  // invariant violations early in release-build testing like fuzzing, etc.
+  // These should eventually be migrated to fatal CHECKs.
+  return LOGGING_FATAL;
 #endif
 }
 

base/check_unittest.cc

@@ -151,6 +151,7 @@ MATCHER_P2(LogErrorMatches, line, expected_msg, "") {
     logging::SetLogMessageHandler(nullptr);                                    \
   } while (0)
 
+#if defined(OFFICIAL_BUILD)
 #if DCHECK_IS_ON()
 #define EXPECT_DUMP_WILL_BE_CHECK EXPECT_DCHECK
 #else
@@ -162,6 +163,9 @@ MATCHER_P2(LogErrorMatches, line, expected_msg, "") {
                                    statement, expected_string "\n");        \
   } while (0)
 #endif  // DCHECK_IS_ON()
+#else
+#define EXPECT_DUMP_WILL_BE_CHECK EXPECT_CHECK
+#endif  // defined(OFFICIAL_BUILD)
 
 TEST(CheckDeathTest, Basics) {
   EXPECT_CHECK("Check failed: false. ", CHECK(false));

chrome/browser/ash/input_method/native_input_method_engine_without_ime_service_browsertest.cc

@@ -4,6 +4,7 @@
 
 #include "ash/constants/ash_features.h"
 #include "ash/constants/ash_pref_names.h"
+#include "base/dcheck_is_on.h"
 #include "base/memory/raw_ptr.h"
 #include "base/scoped_observation.h"
 #include "base/strings/utf_string_conversions.h"
@@ -348,8 +349,19 @@ IN_PROC_BROWSER_TEST_F(NativeInputMethodEngineWithoutImeServiceTest,
   }
 }
 
+// TODO(pbos): Re-enable on all build configurations. This hits a
+// DUMP_WILL_BE_NOTREACHED() in ~Profile as it's being destroyed with observers
+// still present in its ObserverList. Usually this is a sign of UAFs waiting to
+// happen (those observers will likely try to unregister themselves later). It's
+// unclear if this is a quirk of the test or a bug in production code.
+#if defined(OFFICIAL_BUILD) && !DCHECK_IS_ON()
+#define MAYBE_DestroyProfile DestroyProfile
+#else
+#define MAYBE_DestroyProfile DISABLED_DestroyProfile
+#endif  // defined(OFFICIAL_BUILD) && !DCHECK_IS_ON()
+
 IN_PROC_BROWSER_TEST_F(NativeInputMethodEngineWithoutImeServiceTest,
-                       DestroyProfile) {
+                       MAYBE_DestroyProfile) {
   EXPECT_NE(engine_->GetPrefChangeRegistrarForTesting(), nullptr);
   profile_->MaybeSendDestroyedNotification();
   EXPECT_EQ(engine_->GetPrefChangeRegistrarForTesting(), nullptr);

chrome/browser/ui/web_applications/test/system_web_app_interactive_uitest.cc

@@ -13,6 +13,7 @@
 #include "ash/public/cpp/shelf_model.h"
 #include "ash/shell.h"
 #include "ash/wm/window_util.h"
+#include "base/dcheck_is_on.h"
 #include "base/functional/callback_helpers.h"
 #include "base/run_loop.h"
 #include "base/scoped_observation.h"
@@ -741,11 +742,12 @@ IN_PROC_BROWSER_TEST_P(SystemWebAppLaunchProfileBrowserTest,
   EXPECT_TRUE(FindSystemWebAppBrowser(startup_profile, GetAppType()));
 }
 
-#if !DCHECK_IS_ON()
-// The following tests are disabled in DCHECK builds. LaunchSystemWebAppAsync
-// DCHECKs if it can't find a suitable profile. EXPECT_DCHECK_DEATH (or its
-// variants) aren't reliable in browsertests, so we don't test this. Here we
-// to verify LaunchSystemWebAppAsync doesn't crash in release builds
+#if defined(OFFICIAL_BUILD) && !DCHECK_IS_ON()
+// The following tests are disabled outside official non-DCHECK builds.
+// LaunchSystemWebAppAsync hits a DUMP_WILL_BE_NOTREACHED() if it can't find a
+// suitable profile. EXPECT_NOTREACHED_DEATH isn't reliable in browser_tests, so
+// we don't test this. Here we verify LaunchSystemWebAppAsync doesn't crash
+// in official builds.
 IN_PROC_BROWSER_TEST_P(SystemWebAppLaunchProfileBrowserTest,
                        LaunchFromSignInProfile) {
   WaitForTestSystemAppInstall();

chromeos/ash/components/network/metrics/network_metrics_helper_unittest.cc

@@ -257,16 +257,16 @@ void LogVpnResult(const std::string& provider,
                   bool* failed_to_log_result) {
   ASSERT_NE(failed_to_log_result, nullptr);
 
-// Emitting a metric for an unknown VPN provider will always cause a NOTREACHED
-// to be hit. This can cause a CHECK to fail, depending on the build flags. We
-// catch any failing CHECK below by asserting that we will crash when emitting.
-#if DCHECK_IS_ON() && !BUILDFLAG(DCHECK_IS_CONFIGURABLE)
+// Emitting a metric for an unknown VPN provider will always cause a
+// DUMP_WILL_BE_NOTREACHED() to be hit. This is fatal outside official builds,
+// so make sure that we die in those configurations.
+#if !defined(OFFICIAL_BUILD)
   if (provider == kTestUnknownVpn) {
     ASSERT_DEATH({ func.Run(); }, "");
     *failed_to_log_result = true;
     return;
   }
-#endif  // DCHECK_IS_ON() && !BUILDFLAG(DCHECK_IS_CONFIGURABLE)
+#endif  // !defined(OFFICIAL_BUILD)
   func.Run();
 }
 

chromeos/ash/components/network/metrics/vpn_network_metrics_helper_unittest.cc

@@ -144,10 +144,10 @@ TEST_F(VpnNetworkMetricsHelperTest, LogVpnVPNConfigurationSource) {
     ExpectConfigurationSourceCounts(it.first.c_str(), /*manual_count=*/0,
                                     /*policy_count=*/0);
 
-// Emitting a metric for an unknown VPN provider will always cause a NOTREACHED
-// to be hit. This can cause a CHECK to fail, depending on the build flags. We
-// catch any failing CHECK below by asserting that we will crash when emitting.
-#if DCHECK_IS_ON() && !BUILDFLAG(DCHECK_IS_CONFIGURABLE)
+// Emitting a metric for an unknown VPN provider will always cause a
+// DUMP_WILL_BE_NOTREACHED() to be hit. This is fatal outside official builds,
+// so make sure that we die in those configurations.
+#if !defined(OFFICIAL_BUILD)
     if (it.first == kTestUnknownVpn) {
       ASSERT_DEATH(
           {
@@ -165,7 +165,7 @@ TEST_F(VpnNetworkMetricsHelperTest, LogVpnVPNConfigurationSource) {
                                       /*policy_count=*/0);
       continue;
     }
-#endif  // DCHECK_IS_ON() && !BUILDFLAG(DCHECK_IS_CONFIGURABLE)
+#endif  // !defined(OFFICIAL_BUILD)
 
     CreateTestShillConfiguration(it.first, /*is_managed=*/false);
     ExpectConfigurationSourceCounts(it.second, /*manual_count=*/1,

components/prefs/pref_notifier_impl_unittest.cc

@@ -2,12 +2,14 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "components/prefs/pref_notifier_impl.h"
+
 #include <stddef.h>
 
+#include "base/dcheck_is_on.h"
 #include "base/functional/bind.h"
 #include "base/functional/callback.h"
 #include "components/prefs/mock_pref_change_callback.h"
-#include "components/prefs/pref_notifier_impl.h"
 #include "components/prefs/pref_observer.h"
 #include "components/prefs/pref_registry_simple.h"
 #include "components/prefs/pref_service.h"
@@ -122,9 +124,9 @@ TEST_F(PrefNotifierTest, AddAndRemovePrefObservers) {
   ASSERT_EQ(0u, notifier.CountObserver(pref_name2, &obs2_));
 
   // Re-adding the same observer for the same pref doesn't change anything.
-  // Skip this in debug mode, since it hits a DCHECK and death tests aren't
-  // thread-safe.
-#if defined(NDEBUG) && !defined(DCHECK_ALWAYS_ON)
+  // This hits a DUMP_WILL_BE_NOTREACHED() which is fatal in non-official
+  // builds.
+#if defined(OFFICIAL_BUILD) && !DCHECK_IS_ON()
   notifier.AddPrefObserver(pref_name, &obs1_);
   ASSERT_EQ(1u, notifier.CountObserver(pref_name, &obs1_));
   ASSERT_EQ(0u, notifier.CountObserver(pref_name2, &obs1_));

mojo/public/cpp/bindings/tests/validation_context_unittest.cc

@@ -2,13 +2,15 @@
 // 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/lib/validation_context.h"
+
 #include <stddef.h>
 #include <stdint.h>
 
 #include <limits>
 
+#include "base/dcheck_is_on.h"
 #include "mojo/public/cpp/bindings/lib/serialization_util.h"
-#include "mojo/public/cpp/bindings/lib/validation_context.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace mojo {
@@ -23,7 +25,7 @@ const void* ToPtr(uintptr_t ptr) {
   return reinterpret_cast<const void*>(ptr);
 }
 
-#if defined(NDEBUG) && !defined(DCHECK_ALWAYS_ON)
+#if defined(OFFICIAL_BUILD) && !DCHECK_IS_ON()
 TEST(ValidationContextTest, ConstructorRangeOverflow) {
   {
     // Test memory range overflow.

third_party/blink/renderer/platform/graphics/paint/paint_controller_test.cc

@@ -4,6 +4,7 @@
 
 #include "third_party/blink/renderer/platform/graphics/paint/paint_controller_test.h"
 
+#include "base/dcheck_is_on.h"
 #include "build/build_config.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "third_party/blink/renderer/platform/graphics/graphics_context.h"
@@ -2232,8 +2233,11 @@ TEST_P(PaintControllerTest, DuplicatedSubsequences) {
       EXPECT_TRUE(paint_controller.UseCachedSubsequenceIfPossible(client));
     }
     {
-      // Should not use the cached duplicated subsequence.
+      // Should not use the cached duplicated subsequence. This currently hits a
+      // DUMP_WILL_BE_NOTREACHED_NORETURN(), crashing in non-official builds.
+#if defined(OFFICIAL_BUILD)
       EXPECT_FALSE(paint_controller.UseCachedSubsequenceIfPossible(client));
+#endif  // defined(OFFICIAL_BUILD)
       SubsequenceRecorder r(context, client);
       DrawRect(context, client, kForegroundType, gfx::Rect(100, 100, 100, 100));
     }

ui/compositor/layer_animation_sequence_unittest.cc

@@ -7,6 +7,7 @@
 #include <memory>
 
 #include "base/compiler_specific.h"
+#include "base/dcheck_is_on.h"
 #include "base/strings/stringprintf.h"
 #include "base/time/time.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -316,7 +317,7 @@ TEST(LayerAnimationSequenceTest, ToString) {
 // TODO(b/352744702): Remove this test or convert to DEATH test after
 // https://crrev.com/c/5713998 has rolled out and any cases like this have been
 // removed.
-#if !DCHECK_IS_ON()
+#if defined(OFFICIAL_BUILD) && !DCHECK_IS_ON()
 // Check that the sequence doesn't get stuck in an infinite loop when it's
 // repeating and has a total duration of zero.
 TEST(LayerAnimationSequenceTest, RepeatingWithZeroDuration) {