Add DCHECKs to prevent accidental use of base::Feature in chrome.exe

There are two copies of base in a non-component Windows build, one
inside chrome.exe and one inside chrome.dll. This will cause two
copies of the base::FeatureList singleton to be created.

However, only one of those is correctly initialized from command-line
and server-side variations. Attempting to use a base::Feature from
chrome.exe code (e.g. sandbox or base) will silently result in default
behavior of the feature and never correctly respect command-line or
variations state.

This CL prevents the accidental use of the base::FeatureList in
chrome.exe by marking it as forbidden.

BUG=1154449

Change-Id: I7c32037b1930e3474e52eecd44b82dcbd9dbeef5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2568810Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Reviewed-by: Carlos Pizano <cpu@chromium.org>
Commit-Queue: Carlos Pizano <cpu@chromium.org>
Auto-Submit: Will Harris <wfh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#837300}

base/feature_list.cc

@@ -9,12 +9,14 @@
 #include <utility>
 #include <vector>
 
+#include "base/base_paths.h"
 #include "base/base_switches.h"
 #include "base/debug/alias.h"
 #include "base/debug/stack_trace.h"
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/field_trial.h"
+#include "base/path_service.h"
 #include "base/pickle.h"
 #include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
@@ -35,6 +37,10 @@ FeatureList* g_feature_list_instance = nullptr;
 const Feature* g_initialized_from_accessor = nullptr;
 
 #if DCHECK_IS_ON()
+// Tracks whether the use of base::Feature is allowed for this module.
+// See ForbidUseForCurrentModule().
+bool g_use_allowed = true;
+
 const char* g_reason_overrides_disallowed = nullptr;
 
 void DCheckOverridesAllowed() {
@@ -357,6 +363,9 @@ void FeatureList::GetCommandLineFeatureOverrides(
 
 // static
 bool FeatureList::IsEnabled(const Feature& feature) {
+#if DCHECK_IS_ON()
+  CHECK(g_use_allowed) << "base::Feature not permitted for this module.";
+#endif
   if (!g_feature_list_instance) {
     g_initialized_from_accessor = &feature;
     return feature.default_state == FEATURE_ENABLED_BY_DEFAULT;
@@ -366,6 +375,10 @@ bool FeatureList::IsEnabled(const Feature& feature) {
 
 // static
 FieldTrial* FeatureList::GetFieldTrial(const Feature& feature) {
+#if DCHECK_IS_ON()
+  // See documentation for ForbidUseForCurrentModule.
+  CHECK(g_use_allowed) << "base::Feature not permitted for this module.";
+#endif
   if (!g_feature_list_instance) {
     g_initialized_from_accessor = &feature;
     return nullptr;
@@ -470,6 +483,15 @@ void FeatureList::RestoreInstanceForTesting(
   g_feature_list_instance = instance.release();
 }
 
+// static
+void FeatureList::ForbidUseForCurrentModule() {
+#if DCHECK_IS_ON()
+  // Verify there hasn't been any use prior to being called.
+  DCHECK(!g_initialized_from_accessor);
+  g_use_allowed = false;
+#endif  // DCHECK_IS_ON()
+}
+
 void FeatureList::FinalizeInitialization() {
   DCHECK(!initialized_);
   // Store the field trial list pointer for DCHECKing.

base/feature_list.h

@@ -277,6 +277,12 @@ class BASE_EXPORT FeatureList {
   // to support base::test::ScopedFeatureList helper class.
   static void RestoreInstanceForTesting(std::unique_ptr<FeatureList> instance);
 
+  // On some platforms, the base::FeatureList singleton might be duplicated to
+  // more than one module. If this function is called, then using base::Feature
+  // API will result in DCHECK if accessed from the same module as the callee.
+  // Has no effect if DCHECKs are not enabled.
+  static void ForbidUseForCurrentModule();
+
  private:
   FRIEND_TEST_ALL_PREFIXES(FeatureListTest, CheckFeatureIdentity);
   FRIEND_TEST_ALL_PREFIXES(FeatureListTest,

chrome/app/chrome_exe_main_win.cc

@@ -11,8 +11,10 @@
 #include <string>
 
 #include "base/at_exit.h"
+#include "base/base_switches.h"
 #include "base/command_line.h"
 #include "base/debug/alias.h"
+#include "base/feature_list.h"
 #include "base/files/file_path.h"
 #include "base/logging.h"
 #include "base/path_service.h"
@@ -267,6 +269,13 @@ int main() {
   const std::string process_type =
       command_line->GetSwitchValueASCII(switches::kProcessType);
 
+  // In non-component mode, chrome.exe contains a separate instance of
+  // base::FeatureList. Prevent accidental use of this here by forbidding use of
+  // the one that's linked with chrome.exe.
+#if !defined(COMPONENT_BUILD) && DCHECK_IS_ON()
+  base::FeatureList::ForbidUseForCurrentModule();
+#endif
+
   // Confirm that an explicit prefetch profile is used for all process types
   // except for the browser process. Any new process type will have to assign
   // itself a prefetch id. See kPrefetchArgument* constants in