Reland "[base] Prevent access to feature state before FeatureList registration."

This is a reland of commit 033319ab36

This should be safe to land now that this other CL
https://crrev.com/c/4002288 fixed an incorrect early feature access.

As an extra precaution, the CHECK was changed to a DCHECK:
https://crrev.com/c/4006674/1..2/base/feature_list.cc
A DumpWithoutCrashing was added to get error reports from the field
without making Chrome unusable.

Original change's description:
> [base] Prevent access to feature state before FeatureList registration.
>
> With this CL, we CHECK that no feature state was accessed previously
> when registering a FeatureList. This is to prevent accessing feature
> state too early in processes where we expect to find a FeatureList,
> while tolerating accesses in processes that never create a
> FeatureList.
>
> To keep this CL small, the CHECK is only enabled on Windows, Mac and
> Linux for now. Enabling this on Android, iOS and ChromeOS requires
> fixing many more accesses throughout the codebase.
>
> Bug: 1358639
> Change-Id: I2cbf3e2e1ac46ca0a14679fd438ecf0a774f9148
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3867967
> Reviewed-by: Kentaro Hara <haraken@chromium.org>
> Reviewed-by: Jian Li <jianli@chromium.org>
> Reviewed-by: Olga Sharonova <olka@chromium.org>
> Reviewed-by: Charlie Reis <creis@chromium.org>
> Commit-Queue: Francois Pierre Doray <fdoray@chromium.org>
> Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1066783}

Bug: 1358639
Change-Id: I9932f25272e95b2ae132ba19e76b6a6365e28a17
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4006674
Reviewed-by: Charlie Reis <creis@chromium.org>
Reviewed-by: Olga Sharonova <olka@chromium.org>
Reviewed-by: Jian Li <jianli@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Francois Pierre Doray <fdoray@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Auto-Submit: Francois Pierre Doray <fdoray@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1070450}

base/feature_list.cc

@@ -9,10 +9,9 @@
 
 #include <stddef.h>
 
-#include "base/base_paths.h"
 #include "base/base_switches.h"
-#include "base/containers/contains.h"
-#include "base/debug/alias.h"
+#include "base/debug/crash_logging.h"
+#include "base/debug/dump_without_crashing.h"
 #include "base/debug/stack_trace.h"
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
@@ -20,8 +19,8 @@
 #include "base/metrics/field_trial_param_associator.h"
 #include "base/metrics/field_trial_params.h"
 #include "base/metrics/persistent_memory_allocator.h"
+#include "base/no_destructor.h"
 #include "base/notreached.h"
-#include "base/path_service.h"
 #include "base/pickle.h"
 #include "base/rand_util.h"
 #include "base/strings/string_piece.h"
@@ -40,9 +39,66 @@ namespace {
 // have more control over initialization timing. Leaky.
 FeatureList* g_feature_list_instance = nullptr;
 
-// Tracks whether the FeatureList instance was initialized via an accessor, and
-// which Feature that accessor was for, if so.
-const Feature* g_initialized_from_accessor = nullptr;
+// Tracks access to Feature state before FeatureList registration.
+class EarlyFeatureAccessTracker {
+ public:
+  static EarlyFeatureAccessTracker* GetInstance() {
+    static NoDestructor<EarlyFeatureAccessTracker> instance;
+    return instance.get();
+  }
+
+  // Invoked when `feature` is accessed before FeatureList registration.
+  void AccessedFeature(const Feature& feature) {
+    AutoLock lock(lock_);
+    if (feature_)
+      return;
+
+    feature_ = &feature;
+    stack_trace_ = std::make_unique<debug::StackTrace>();
+  }
+
+  // Asserts that no feature was accessed before FeatureList registration.
+  void AssertNoAccess() {
+    AutoLock lock(lock_);
+    if (!feature_)
+      return;
+
+#if !BUILDFLAG(IS_NACL)
+    // Create a crash key with the name of the feature accessed too early, to
+    // facilitate crash triage.
+    SCOPED_CRASH_KEY_STRING256("FeatureList", "feature-accessed-too-early",
+                               feature_->name);
+#endif  // !BUILDFLAG(IS_NACL)
+    // Fail if DCHECKs are enabled.
+    DCHECK(!feature_) << "Accessed feature " << feature_->name
+                      << " before FeatureList registration, in stack: "
+                      << stack_trace_->ToString();
+    // Report the problem but don't crash if DCHECKs are disabled, to avoid
+    // making Chrome unusable if a feature is accessed too early.
+    base::debug::DumpWithoutCrashing();
+  }
+
+  // Resets the state of this tracker.
+  void Reset() {
+    AutoLock lock(lock_);
+    feature_ = nullptr;
+    stack_trace_.reset();
+  }
+
+ private:
+  friend class NoDestructor<EarlyFeatureAccessTracker>;
+
+  EarlyFeatureAccessTracker() = default;
+  ~EarlyFeatureAccessTracker() = default;
+
+  Lock lock_;
+
+  // First feature to be accessed before FeatureList registration.
+  const Feature* feature_ GUARDED_BY(lock_) = nullptr;
+
+  // Stack trace of the first feature access before FeatureList registration.
+  std::unique_ptr<debug::StackTrace> stack_trace_ GUARDED_BY(lock_);
+};
 
 // Controls whether a feature's override state will be cached in
 // `base::Feature::cached_value`. This field and the associated `base::Feature`
@@ -385,7 +441,7 @@ bool FeatureList::IsEnabled(const Feature& feature) {
   CHECK(g_use_allowed) << "base::Feature not permitted for this module.";
 #endif
   if (!g_feature_list_instance) {
-    g_initialized_from_accessor = &feature;
+    EarlyFeatureAccessTracker::GetInstance()->AccessedFeature(feature);
     return feature.default_state == FEATURE_ENABLED_BY_DEFAULT;
   }
   return g_feature_list_instance->IsFeatureEnabled(feature);
@@ -402,7 +458,7 @@ absl::optional<bool> FeatureList::GetStateIfOverridden(const Feature& feature) {
   CHECK(g_use_allowed) << "base::Feature not permitted for this module.";
 #endif
   if (!g_feature_list_instance) {
-    g_initialized_from_accessor = &feature;
+    EarlyFeatureAccessTracker::GetInstance()->AccessedFeature(feature);
     // If there is no feature list, there can be no overrides.
     return absl::nullopt;
   }
@@ -416,7 +472,7 @@ FieldTrial* FeatureList::GetFieldTrial(const Feature& feature) {
   CHECK(g_use_allowed) << "base::Feature not permitted for this module.";
 #endif
   if (!g_feature_list_instance) {
-    g_initialized_from_accessor = &feature;
+    EarlyFeatureAccessTracker::GetInstance()->AccessedFeature(feature);
     return nullptr;
   }
   return g_feature_list_instance->GetAssociatedFieldTrial(feature);
@@ -490,10 +546,7 @@ bool FeatureList::InitializeInstance(
   // If the singleton was previously initialized from within an accessor, we
   // want to prevent callers from reinitializing the singleton and masking the
   // accessor call(s) which likely returned incorrect information.
-  if (g_initialized_from_accessor) {
-    DEBUG_ALIAS_FOR_CSTR(accessor_name, g_initialized_from_accessor->name, 128);
-    CHECK(!g_initialized_from_accessor);
-  }
+  EarlyFeatureAccessTracker::GetInstance()->AssertNoAccess();
   bool instance_existed_before = false;
   if (g_feature_list_instance) {
     if (g_feature_list_instance->initialized_from_command_line_)
@@ -524,10 +577,16 @@ void FeatureList::SetInstance(std::unique_ptr<FeatureList> instance) {
   // Note: Intentional leak of global singleton.
   g_feature_list_instance = instance.release();
 
+  // TODO(crbug.com/1358639): Enable this check on all platforms.
+#if !BUILDFLAG(IS_IOS) && !BUILDFLAG(IS_ANDROID) && !BUILDFLAG(IS_CHROMEOS)
+  EarlyFeatureAccessTracker::GetInstance()->AssertNoAccess();
+#endif
+
 #if !BUILDFLAG(IS_NACL)
   // Configured first because it takes precedence over the getrandom() trial.
   internal::ConfigureBoringSSLBackedRandBytesFieldTrial();
 #endif
+
 #if BUILDFLAG(IS_ANDROID)
   internal::ConfigureRandBytesFieldTrial();
 #endif
@@ -559,7 +618,7 @@ void FeatureList::SetInstance(std::unique_ptr<FeatureList> instance) {
 std::unique_ptr<FeatureList> FeatureList::ClearInstanceForTesting() {
   FeatureList* old_instance = g_feature_list_instance;
   g_feature_list_instance = nullptr;
-  g_initialized_from_accessor = nullptr;
+  EarlyFeatureAccessTracker::GetInstance()->Reset();
   return WrapUnique(old_instance);
 }
 
@@ -575,7 +634,7 @@ void FeatureList::RestoreInstanceForTesting(
 void FeatureList::ForbidUseForCurrentModule() {
 #if DCHECK_IS_ON()
   // Verify there hasn't been any use prior to being called.
-  DCHECK(!g_initialized_from_accessor);
+  EarlyFeatureAccessTracker::GetInstance()->AssertNoAccess();
   g_use_allowed = false;
 #endif  // DCHECK_IS_ON()
 }

base/feature_list.h

@@ -378,10 +378,17 @@ class BASE_EXPORT FeatureList {
   // OWNERS.
   std::unique_ptr<Accessor> ConstructAccessor();
 
-  // Returns whether the given |feature| is enabled. Must only be called after
-  // the singleton instance has been registered via SetInstance(). Additionally,
-  // a feature with a given name must only have a single corresponding Feature
-  // struct, which is checked in builds with DCHECKs enabled.
+  // Returns whether the given `feature` is enabled.
+  //
+  // If no `FeatureList` instance is registered, this returns the feature's
+  // default state. Registering a `FeatureList` later will fail.
+  //
+  // TODO(crbug.com/1358639): Make registering a `FeatureList` later fail on
+  // iOS, Android and ChromeOS. This currently only works on Windows, Mac and
+  // Linux.
+  //
+  // A feature with a given name must only have a single corresponding Feature
+  // instance, which is checked in builds with DCHECKs enabled.
   static bool IsEnabled(const Feature& feature);
 
   // Some characters are not allowed to appear in feature names or the

base/system/sys_info_mac.mm

@@ -57,8 +57,8 @@ absl::optional<int> NumberOfPhysicalProcessors() {
 }
 
 absl::optional<int> NumberOfProcessorsWhenCpuSecurityMitigationEnabled() {
-  if (!FeatureList::IsEnabled(kNumberOfCoresWithCpuSecurityMitigation) ||
-      !g_is_cpu_security_mitigation_enabled) {
+  if (!g_is_cpu_security_mitigation_enabled ||
+      !FeatureList::IsEnabled(kNumberOfCoresWithCpuSecurityMitigation)) {
     return absl::nullopt;
   }
   return NumberOfPhysicalProcessors();

components/offline_pages/core/prefetch/tasks/download_archives_task_unittest.cc

@@ -27,7 +27,7 @@ namespace {
 
 const int64_t kSmallArchiveSize = 1LL * 1024 * 1024;
 const int64_t kLargeArchiveSize =
-    2 * PrefetchDownloaderQuota::GetMaxDailyQuotaBytes() / 3;
+    2 * PrefetchDownloaderQuota::kDefaultMaxDailyQuotaBytes / 3;
 
 const PrefetchItem* FindPrefetchItemByOfflineId(
     const std::set<PrefetchItem>& items,

content/public/test/audio_service_test_helper.cc

@@ -43,21 +43,20 @@ class AudioServiceTestHelper::TestingApi : public audio::mojom::TestingApi {
 };
 
 AudioServiceTestHelper::AudioServiceTestHelper()
-    : testing_api_(new TestingApi) {
-  if (base::FeatureList::IsEnabled(features::kAudioServiceOutOfProcess)) {
-    audio::Service::SetTestingApiBinderForTesting(
-        base::BindRepeating(&AudioServiceTestHelper::BindTestingApiReceiver,
-                            base::Unretained(this)));
-  }
+    : testing_api_(std::make_unique<TestingApi>()) {
+  audio::Service::SetTestingApiBinderForTesting(base::BindRepeating(
+      &AudioServiceTestHelper::BindTestingApiReceiver, base::Unretained(this)));
 }
 
 AudioServiceTestHelper::~AudioServiceTestHelper() {
-  if (base::FeatureList::IsEnabled(features::kAudioServiceOutOfProcess))
-    audio::Service::SetTestingApiBinderForTesting(base::NullCallback());
+  audio::Service::SetTestingApiBinderForTesting(base::NullCallback());
 }
 
 void AudioServiceTestHelper::BindTestingApiReceiver(
     mojo::PendingReceiver<audio::mojom::TestingApi> receiver) {
+  CHECK(base::FeatureList::IsEnabled(features::kAudioServiceOutOfProcess))
+      << "Audio Service API binder shouldn't be invoked from this process when "
+         "the AudioServiceOutOfProcess feature is disabled.";
   testing_api_->BindReceiver(std::move(receiver));
 }
 

gin/shell/gin_main.cc

@@ -72,12 +72,12 @@ int main(int argc, char** argv) {
   gin::V8Initializer::LoadV8Snapshot();
 #endif
 
-  base::SingleThreadTaskExecutor main_thread_task_executor;
-  base::ThreadPoolInstance::CreateAndStartWithDefaultParams("gin");
-
   // Initialize the base::FeatureList since IsolateHolder can depend on it.
   base::FeatureList::SetInstance(base::WrapUnique(new base::FeatureList));
 
+  base::SingleThreadTaskExecutor main_thread_task_executor;
+  base::ThreadPoolInstance::CreateAndStartWithDefaultParams("gin");
+
   {
     gin::IsolateHolder::Initialize(gin::IsolateHolder::kStrictMode,
                                    gin::ArrayBufferAllocator::SharedInstance());

third_party/blink/renderer/core/scroll/scroll_test.cc

@@ -32,9 +32,10 @@
 namespace blink {
 
 namespace {
-const double kScrollAnimationDuration =
-    (::features::IsImpulseScrollAnimationEnabled() ? 1.5 : 0.5);
+double ScrollAnimationDuration() {
+  return ::features::IsImpulseScrollAnimationEnabled() ? 1.5 : 0.5;
 }
+}  // namespace
 
 class FractionalScrollSimTest : public SimTest, public PaintTestConfigurations {
  public:
@@ -242,7 +243,7 @@ TEST_P(ScrollAnimatorSimTest, TestRootFrameLayoutViewportUserScrollCallBack) {
   // The callback is executed when the animation finishes at
   // ScrollAnimator::TickAnimation.
   Compositor().BeginFrame();
-  Compositor().BeginFrame(kScrollAnimationDuration);
+  Compositor().BeginFrame(ScrollAnimationDuration());
   ASSERT_TRUE(finished);
 }
 
@@ -286,7 +287,7 @@ TEST_P(ScrollAnimatorSimTest, TestRootFrameVisualViewporUserScrollCallBack) {
   // The callback is executed when the animation finishes at
   // ScrollAnimator::TickAnimation.
   Compositor().BeginFrame();
-  Compositor().BeginFrame(kScrollAnimationDuration);
+  Compositor().BeginFrame(ScrollAnimationDuration());
   ASSERT_TRUE(finished);
 }
 
@@ -330,7 +331,7 @@ TEST_P(ScrollAnimatorSimTest, TestRootFrameBothViewportsUserScrollCallBack) {
   // The callback is executed when the animation finishes at
   // ScrollAnimator::TickAnimation.
   Compositor().BeginFrame();
-  Compositor().BeginFrame(kScrollAnimationDuration);
+  Compositor().BeginFrame(ScrollAnimationDuration());
   ASSERT_TRUE(finished);
 }
 
@@ -380,7 +381,7 @@ TEST_P(ScrollAnimatorSimTest, TestDivUserScrollCallBack) {
 
   // The callback is executed when the animation finishes at
   // ScrollAnimator::TickAnimation.
-  Compositor().BeginFrame(kScrollAnimationDuration);
+  Compositor().BeginFrame(ScrollAnimationDuration());
   ASSERT_TRUE(finished);
 }