From f7f53b43e98ad8464d6659838393cb1555700b85 Mon Sep 17 00:00:00 2001
From: Leszek Swirski <leszeks@google.com>
Date: Mon, 30 Sep 2024 10:12:10 +0000
Subject: [PATCH] [v8] Pass through prefixed feature flags to V8
V8 has its own command line flag mechanism, and V8-relevant
base::Feature flags are defined in gin and passed through to V8 during
gin initialization.
This has the consequence that V8 flags need to be defined twice to be
controllable via Chromium's variations mechanism -- once in V8 for their
functionality, and once in Blink/gin just for pass-through. This adds
overhead when wanting to A/B test V8 flags, and can cause issues if an
enabled V8 flag needs to be disabled via kill switch (namely, we have to
make sure that there exists a base::Feature equivalent of the flag that
we can disable). It also causes some confusion about where the canonical
default feature state exists (in Blink or V8).
This CL introduces a generic `V8Flags_` prefix for features in the
FeatureList. The gin initializer walks all feature overrides, and
any `V8Flags_foo` feature that is overridden to enabled is passed
through to V8 as a `--foo` flag (and disabled features are passed
through as `--no-foo`). This will allow us to, effectively, expose all
V8 flags as a pseudo base::Feature, without needing Chromium-side
changes to expose such flags to Finch.
Change-Id: Id88beafb8688d9f046706b0a67e23ad94d08a7a9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5873788
Commit-Queue: Leszek Swirski <leszeks@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Auto-Submit: Leszek Swirski <leszeks@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1361715}
---
base/BUILD.gn | 5 +-
base/feature_list.cc | 45 +++++++++---
base/feature_list.h | 16 ++---
base/feature_list_unittest.cc | 37 ++++++++--
base/feature_visitor.h | 13 ++--
gin/v8_initializer.cc | 72 ++++++++++++++++++-
.../v8_context_snapshot_generator.cc | 4 ++
7 files changed, 153 insertions(+), 39 deletions(-)
diff --git a/base/BUILD.gn b/base/BUILD.gn
index 9b52a9bd084ff..8cf35e71cb94c 100644
--- a/base/BUILD.gn
+++ b/base/BUILD.gn
@@ -294,6 +294,7 @@ component("base") {
"export_template.h",
"feature_list.cc",
"feature_list.h",
+ "feature_visitor.h",
"features.cc",
"features.h",
"file_version_info.h",
@@ -1002,10 +1003,6 @@ component("base") {
]
}
- if (is_chromeos_ash) {
- sources += [ "feature_visitor.h" ]
- }
-
if (is_linux || is_chromeos || is_android) {
sources += [
"files/file_path_watcher_inotify.cc",
diff --git a/base/feature_list.cc b/base/feature_list.cc
index e981c80430d58..75dc0b1f1fc23 100644
--- a/base/feature_list.cc
+++ b/base/feature_list.cc
@@ -2,24 +2,27 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
+#include "base/location.h"
#ifdef UNSAFE_BUFFERS_BUILD
// TODO(crbug.com/40284755): Remove this and spanify to fix the errors.
#pragma allow_unsafe_buffers
#endif
-#include "base/feature_list.h"
-
#include <stddef.h>
+#include <algorithm>
#include <string>
#include <string_view>
#include <tuple>
#include "base/base_switches.h"
+#include "base/check_is_test.h"
#include "base/containers/contains.h"
#include "base/containers/span.h"
#include "base/debug/crash_logging.h"
#include "base/debug/dump_without_crashing.h"
+#include "base/feature_list.h"
+#include "base/feature_visitor.h"
#include "base/logging.h"
#include "base/memory/ptr_util.h"
#include "base/memory/raw_ptr.h"
@@ -37,10 +40,6 @@
#include "build/build_config.h"
#include "build/chromeos_buildflags.h"
-#if BUILDFLAG(IS_CHROMEOS_ASH)
-#include "base/feature_visitor.h"
-#endif // BUILDFLAG(IS_CHROMEOS_ASH)
-
namespace base {
namespace {
@@ -687,14 +686,39 @@ void FeatureList::AddEarlyAllowedFeatureForTesting(std::string feature_name) {
allowed_feature_names_.insert(std::move(feature_name));
}
-#if BUILDFLAG(IS_CHROMEOS_ASH)
// static
-void FeatureList::VisitFeaturesAndParams(FeatureVisitor& visitor) {
- CHECK(g_feature_list_instance);
+void FeatureList::VisitFeaturesAndParams(FeatureVisitor& visitor,
+ std::string_view filter_prefix) {
+ // If there is no feature list, there are no overrides. This should only happen
+ // in tests.
+ // TODO(leszeks): Add a CHECK_IS_TEST() to verify the above.
+ if (!g_feature_list_instance) {
+ return;
+ }
+
FieldTrialParamAssociator* params_associator =
FieldTrialParamAssociator::GetInstance();
- for (auto& feature_override : g_feature_list_instance->overrides_) {
+ using FeatureOverride = std::pair<std::string, OverrideEntry>;
+ base::span<FeatureOverride> filtered_overrides(
+ g_feature_list_instance->overrides_);
+ if (!filter_prefix.empty()) {
+ // If there is a filter prefix, then change the begin/end range to be the
+ // range where the values are prefixed with the given prefix (overrides are
+ // lexically sorted, so this will be a continuous range). This is
+ // implemented as a binary search of the upper and lower bounds of the
+ // override iterator, projecting each iterator value to just the
+ // key, trimmed to the length of the prefix.
+ DCHECK(std::ranges::is_sorted(
+ filtered_overrides, std::less<>(),
+ [](const FeatureOverride& entry) { return entry.first; }));
+ filtered_overrides = std::ranges::equal_range(
+ filtered_overrides, filter_prefix, std::less<>(),
+ [filter_prefix](const FeatureOverride& entry) {
+ return std::string_view(entry.first).substr(0, filter_prefix.size());
+ });
+ }
+ for (const FeatureOverride& feature_override : filtered_overrides) {
FieldTrial* field_trial = feature_override.second.field_trial;
std::string trial_name;
@@ -712,7 +736,6 @@ void FeatureList::VisitFeaturesAndParams(FeatureVisitor& visitor) {
group_name);
}
}
-#endif // BULDFLAG(IS_CHROMEOS_ASH)
void FeatureList::FinalizeInitialization() {
DCHECK(!initialized_);
diff --git a/base/feature_list.h b/base/feature_list.h
index 602fbce18fb81..8e595511eb398 100644
--- a/base/feature_list.h
+++ b/base/feature_list.h
@@ -26,17 +26,13 @@
#include "base/memory/raw_ptr.h"
#include "base/synchronization/lock.h"
#include "build/build_config.h"
-#include "build/chromeos_buildflags.h"
namespace base {
class FieldTrial;
class FieldTrialList;
class PersistentMemoryAllocator;
-
-#if BUILDFLAG(IS_CHROMEOS_ASH)
class FeatureVisitor;
-#endif // BUILDFLAG(IS_CHROMEOS_ASH)
// Specifies whether a given feature is enabled or disabled by default.
// NOTE: The actual runtime state may be different, due to a field trial or a
@@ -579,15 +575,15 @@ class BASE_EXPORT FeatureList {
// only be called on a FeatureList that was set with SetEarlyAccessInstance().
void AddEarlyAllowedFeatureForTesting(std::string feature_name);
-#if BUILDFLAG(IS_CHROMEOS_ASH)
// Allows a visitor to record override state, parameters, and field trial
- // associated with each feature.
+ // associated with each feature. Optionally, provide a prefix which filters
+ // the visited features.
//
// NOTE: This is intended only for the special case of needing to get all
- // overrides. This use case is specific to CrOS-Ash. Most users should call
- // IsEnabled() to query a feature's state.
- static void VisitFeaturesAndParams(FeatureVisitor& visitor);
-#endif // BULDFLAG(IS_CHROMEOS_ASH)
+ // overrides. This use case is specific to CrOS-Ash and V8. Most users should
+ // call IsEnabled() to query a feature's state.
+ static void VisitFeaturesAndParams(FeatureVisitor& visitor,
+ std::string_view filter_prefix = "");
private:
FRIEND_TEST_ALL_PREFIXES(FeatureListTest, CheckFeatureIdentity);
diff --git a/base/feature_list_unittest.cc b/base/feature_list_unittest.cc
index 2bb02ed75f11a..618078b3c9677 100644
--- a/base/feature_list_unittest.cc
+++ b/base/feature_list_unittest.cc
@@ -19,6 +19,7 @@
#include <vector>
#include "base/feature_list_buildflags.h"
+#include "base/feature_visitor.h"
#include "base/format_macros.h"
#include "base/memory/read_only_shared_memory_region.h"
#include "base/metrics/field_trial.h"
@@ -32,10 +33,6 @@
#include "build/chromeos_buildflags.h"
#include "testing/gtest/include/gtest/gtest.h"
-#if BUILDFLAG(IS_CHROMEOS_ASH)
-#include "base/feature_visitor.h"
-#endif // BUILDFLAG(IS_CHROMEOS_ASH)
-
namespace base {
namespace {
@@ -851,7 +848,6 @@ TEST(FeatureListAccessorTest, InitFromCommandLineWithFeatureParams) {
}
}
-#if BUILDFLAG(IS_CHROMEOS_ASH)
// Test only class to verify correctness of
// FeatureList::VisitFeaturesAndParams().
class TestFeatureVisitor : public FeatureVisitor {
@@ -979,6 +975,35 @@ TEST(TestFeatureVisitor, FeatureHasParams) {
EXPECT_EQ(actual_feature_state, expected_feature_state);
}
-#endif // BUILDFLAG(IS_CHROMEOS_ASH)
+
+TEST(TestFeatureVisitor, FeatureWithPrefix) {
+ base::test::ScopedFeatureList outer_scope;
+ outer_scope.InitWithEmptyFeatureAndFieldTrialLists();
+
+ base::test::ScopedFeatureList initialized_feature_list;
+
+ initialized_feature_list.InitFromCommandLine(
+ /*enable_features=*/
+ "AFeature,AnotherFeature,TestFeature,TestFeature2,PrefixedFeature,"
+ "PrefixedFeature2",
+ /*disable_features=*/"");
+
+ TestFeatureVisitor visitor;
+ base::FeatureList::VisitFeaturesAndParams(visitor, "Prefixed");
+ std::multiset<TestFeatureVisitor::VisitedFeatureState> actual_feature_state =
+ visitor.feature_state();
+
+ std::multiset<TestFeatureVisitor::VisitedFeatureState>
+ expected_feature_state = {
+ {"PrefixedFeature",
+ FeatureList::OverrideState::OVERRIDE_ENABLE_FEATURE,
+ FieldTrialParams{}, "", ""},
+ {"PrefixedFeature2",
+ FeatureList::OverrideState::OVERRIDE_ENABLE_FEATURE,
+ FieldTrialParams{}, "", ""},
+ };
+
+ EXPECT_EQ(actual_feature_state, expected_feature_state);
+}
} // namespace base
diff --git a/base/feature_visitor.h b/base/feature_visitor.h
index b85440d122147..47732397bfb5c 100644
--- a/base/feature_visitor.h
+++ b/base/feature_visitor.h
@@ -9,21 +9,21 @@
#include <string>
#include "base/feature_list.h"
-#include "build/chromeos_buildflags.h"
-
-static_assert(BUILDFLAG(IS_CHROMEOS_ASH), "For ChromeOS ash-chrome only");
namespace variations::cros_early_boot::evaluate_seed {
class EarlyBootFeatureVisitor;
}
+namespace gin {
+class V8FeatureVisitor;
+}
+
namespace base {
class TestFeatureVisitor;
-// An interface for EarlyBootFeatureVisitor that provides a method to
-// iterate over a feature's name, override state, parameters, and associated
-// field trial.
+// An interface for FeatureList that provides a method to iterate over a
+// feature's name, override state, parameters, and associated field trial.
//
// NOTE: This is intended only for the special case of needing to get all
// feature overrides. Most users should call FeatureList::IsEnabled() to query
@@ -45,6 +45,7 @@ class FeatureVisitor {
private:
friend variations::cros_early_boot::evaluate_seed::EarlyBootFeatureVisitor;
+ friend gin::V8FeatureVisitor;
friend TestFeatureVisitor;
// The constructor is private so only friend classes can inherit from this
diff --git a/gin/v8_initializer.cc b/gin/v8_initializer.cc
index b1237e4ee88e1..0894e32248a2f 100644
--- a/gin/v8_initializer.cc
+++ b/gin/v8_initializer.cc
@@ -22,6 +22,7 @@
#include "base/debug/alias.h"
#include "base/debug/crash_logging.h"
#include "base/feature_list.h"
+#include "base/feature_visitor.h"
#include "base/files/file.h"
#include "base/files/file_path.h"
#include "base/files/memory_mapped_file.h"
@@ -218,10 +219,77 @@ void SetV8FlagsIfOverridden(const base::Feature& feature,
}
}
+constexpr std::string_view kV8FlagFeaturePrefix = "V8Flag_";
+
+} // namespace
+
+class V8FeatureVisitor : public base::FeatureVisitor {
+ public:
+ void Visit(const std::string& feature_name,
+ base::FeatureList::OverrideState override_state,
+ const std::map<std::string, std::string>& params,
+ const std::string& trial_name,
+ const std::string& group_name) override {
+ std::string_view feature_name_view(feature_name);
+
+ // VisitFeaturesAndParams is called with kV8FlagFeaturePrefix as a filter
+ // prefix, so we expect all feature names to start with "V8Flag_". Strip
+ // this prefix off to get the corresponding V8 flag name.
+ DCHECK(feature_name_view.starts_with(kV8FlagFeaturePrefix));
+ std::string_view flag_name =
+ feature_name_view.substr(kV8FlagFeaturePrefix.size());
+
+ switch (override_state) {
+ case base::FeatureList::OverrideState::OVERRIDE_USE_DEFAULT:
+ return;
+
+ case base::FeatureList::OverrideState::OVERRIDE_DISABLE_FEATURE:
+ SetV8FlagsFormatted("--no-%s", flag_name);
+ // Do not set parameters for disabled features.
+ break;
+
+ case base::FeatureList::OverrideState::OVERRIDE_ENABLE_FEATURE:
+ SetV8FlagsFormatted("--%s", flag_name);
+ for (const auto& [param_name, param_value] : params) {
+ SetV8FlagsFormatted("--%s=%s", param_name.c_str(),
+ param_value.c_str());
+ }
+ break;
+ }
+ }
+};
+
+namespace {
+
void SetFlags(IsolateHolder::ScriptMode mode,
const std::string js_command_line_flags) {
- // We assume that all feature flag defaults correspond to the default
- // values of the corresponding V8 flags.
+ // Chromium features prefixed with "V8Flag_" are forwarded to V8 as V8 flags,
+ // with the "V8Flag_" prefix stripped off. For example, an enabled feature
+ // "V8Flag_foo_bar" will be passed to V8 as the flag `--foo_bar`. Similarly,
+ // if that feature is explicitly disabled, it will be passed to V8 as
+ // `--no-foo_bar`. No Chromium-side declaration of a V8Flag_foo_bar feature
+ // is necessary, the matching is done on strings.
+ //
+ // Parameters attached to features will also be passed through, with the same
+ // name as the parameter and the value passed by string, to be decoded by V8's
+ // flag parsing.
+ //
+ // Thus, running Chromium with:
+ //
+ // --enable-features=V8Flag_foo,V8Flag_bar:bar_param/20
+ // --disable-features=V8Flag_baz
+ //
+ // will be converted, on V8 initialization, to V8 flags:
+ //
+ // --foo --bar --bar_param=20 --no-baz
+ V8FeatureVisitor feature_visitor;
+ base::FeatureList::VisitFeaturesAndParams(feature_visitor,
+ kV8FlagFeaturePrefix);
+
+ // Otherwise, feature flags explicitly defined in Chromium are translated
+ // to V8 flags as follows. We ignore feature flag default values, instead
+ // using the corresponding V8 flags default values if there is no explicit
+ // feature override.
SetV8FlagsIfOverridden(features::kV8CompactCodeSpaceWithStack,
"--compact-code-space-with-stack",
"--no-compact-code-space-with-stack");
diff --git a/tools/v8_context_snapshot/v8_context_snapshot_generator.cc b/tools/v8_context_snapshot/v8_context_snapshot_generator.cc
index 472c6492a8e12..37ee2e0391301 100644
--- a/tools/v8_context_snapshot/v8_context_snapshot_generator.cc
+++ b/tools/v8_context_snapshot/v8_context_snapshot_generator.cc
@@ -35,6 +35,10 @@ class SnapshotPlatform final : public blink::Platform {
int main(int argc, char** argv) {
base::AtExitManager at_exit;
+ // Initialize an empty feature list for gin startup.
+ auto early_access_feature_list = std::make_unique<base::FeatureList>();
+ base::FeatureList::SetInstance(std::move(early_access_feature_list));
+
const bool kRemoveRecognizedFlags = true;
v8::V8::SetFlagsFromCommandLine(&argc, argv, kRemoveRecognizedFlags);
base::CommandLine::Init(argc, argv);