flags: honor configured values of unexpired flags

Currently, feature initialization looks like this:

1. The pref store is initialized (which contains saved flag states)
2. A FlagsState instance is initialized from that prefs store
  a. FlagsState computes which switches and features should be in what
     states based on flags
  b. FlagsState excludes switches and features from flags that are
     currently expired
3. A FeatureList is initialized from that FlagsState

Unfortunately, deciding whether a flag is expired or not requires
knowing the state of the various TemporaryUnexpireFlagsM$M features,
which aren't initialized until step 3, so step 2b doesn't actually work
properly. During step 2b, every unexpiry feature is treated as if it was
in its default state (i.e. disabled), which means that every flag whose
expiration has passed is treated as expired, regardless of the state of
the unexpiry flag. As a result of *that*, when the FeatureList is
initialized at step 3, any such flag's configured value gets ignored.

The fix for this is deeply unfortunate: at step 2b, add a special case
in checking whether a flag should be considered expired or not.
Specifically, consider not just the state of the unexpiry feature (which
may not have been initialized yet) but also the state of the backing
unexpiry *flag*. To enable this, pass a FlagsStorage into the
FlagsState::Delegate.

Change-Id: Ia2b6423357d24318baaeb3ca34208d1a2f1c36aa
Bug: 1101828
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2302925Reviewed-by: Nico Weber <thakis@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Commit-Queue: Elly Fong-Jones <ellyjones@chromium.org>
Cr-Commit-Position: refs/heads/master@{#794557}

chrome/browser/about_flags.cc

@@ -5995,7 +5995,7 @@ const FeatureEntry kFeatureEntries[] = {
      FEATURE_WITH_PARAMS_VALUE_TYPE(
          blink::features::kAlignFontDisplayAutoTimeoutWithLCPGoal,
          kAlignFontDisplayAutoTimeoutWithLCPGoalVariations,
-         "AlignFontDisplayAutoTimeoutWithLCPGoalVariations")},
+         "AlignFontDisplayAutoTimeoutWithLCPGoal")},
 
 #if defined(OS_CHROMEOS)
     {"enable-palm-suppression", flag_descriptions::kEnablePalmSuppressionName,
@@ -6200,8 +6200,9 @@ class FlagsStateSingleton : public flags_ui::FlagsState::Delegate {
 
  private:
   // flags_ui::FlagsState::Delegate:
-  bool ShouldExcludeFlag(const FeatureEntry& entry) override {
-    return flags::IsFlagExpired(entry.internal_name);
+  bool ShouldExcludeFlag(const flags_ui::FlagsStorage* storage,
+                         const FeatureEntry& entry) override {
+    return flags::IsFlagExpired(storage, entry.internal_name);
   }
 
   std::unique_ptr<flags_ui::FlagsState> flags_state_;
@@ -6213,7 +6214,8 @@ bool ShouldSkipNonDeprecatedFeatureEntry(const FeatureEntry& entry) {
   return ~entry.supported_platforms & kDeprecated;
 }
 
-bool SkipConditionalFeatureEntry(const FeatureEntry& entry) {
+bool SkipConditionalFeatureEntry(const flags_ui::FlagsStorage* storage,
+                                 const FeatureEntry& entry) {
   version_info::Channel channel = chrome::GetChannel();
 #if defined(OS_CHROMEOS)
   // enable-ui-devtools is only available on for non Stable channels.
@@ -6281,7 +6283,7 @@ bool SkipConditionalFeatureEntry(const FeatureEntry& entry) {
   }
 #endif  // OS_ANDROID
 
-  if (flags::IsFlagExpired(entry.internal_name))
+  if (flags::IsFlagExpired(storage, entry.internal_name))
     return true;
 
   return false;
@@ -6331,7 +6333,10 @@ void GetFlagFeatureEntries(flags_ui::FlagsStorage* flags_storage,
                            base::ListValue* unsupported_entries) {
   FlagsStateSingleton::GetFlagsState()->GetFlagFeatureEntries(
       flags_storage, access, supported_entries, unsupported_entries,
-      base::BindRepeating(&SkipConditionalFeatureEntry));
+      base::BindRepeating(&SkipConditionalFeatureEntry,
+                          // Unretained: this callback doesn't outlive this
+                          // stack frame.
+                          base::Unretained(flags_storage)));
 }
 
 void GetFlagFeatureEntriesForDeprecatedPage(

chrome/browser/about_flags_browsertest.cc

@@ -140,13 +140,20 @@ class AboutFlagsBrowserTest : public InProcessBrowserTest,
                               public testing::WithParamInterface<bool> {
  public:
   AboutFlagsBrowserTest() {
-    about_flags::testing::SetFeatureEntries(
-        {{kFlagName, "name-1", "description-1", -1,
-          ORIGIN_LIST_VALUE_TYPE(kSwitchName, "")},
-         {kExpiredFlagName, "name-2", "description-2", -1,
-          SINGLE_VALUE_TYPE(kExpiredFlagSwitchName)},
-         {kFlagWithOptionSelectorName, "name-3", "description-3", -1,
-          SINGLE_VALUE_TYPE(kFlagWithOptionSelectorSwitchName)}});
+    std::vector<flags_ui::FeatureEntry> entries = {
+        {kFlagName, "name-1", "description-1", -1,
+         ORIGIN_LIST_VALUE_TYPE(kSwitchName, "")},
+        {kExpiredFlagName, "name-2", "description-2", -1,
+         SINGLE_VALUE_TYPE(kExpiredFlagSwitchName)},
+        {kFlagWithOptionSelectorName, "name-3", "description-3", -1,
+         SINGLE_VALUE_TYPE(kFlagWithOptionSelectorSwitchName)}};
+    unexpire_name_ = base::StringPrintf("temporary-unexpire-flags-m%d",
+                                        CHROME_VERSION_MAJOR - 1);
+    flags_ui::FeatureEntry expiry_entry = {
+        unexpire_name_.c_str(), "unexpire name", "unexpire desc", -1,
+        SINGLE_VALUE_TYPE("unexpire-dummy-switch")};
+    entries.push_back(expiry_entry);
+    about_flags::testing::SetFeatureEntries(entries);
 
     flags::testing::SetFlagExpiration(kExpiredFlagName,
                                       CHROME_VERSION_MAJOR - 1);
@@ -179,6 +186,7 @@ class AboutFlagsBrowserTest : public InProcessBrowserTest,
   }
 
   bool expiration_enabled_ = true;
+  std::string unexpire_name_;
 
   base::test::ScopedFeatureList feature_list_;
 };
@@ -362,6 +370,30 @@ IN_PROC_BROWSER_TEST_P(AboutFlagsBrowserTest, DISABLED_ExpiredFlagDoesntApply) {
 }
 #endif
 
+// Regression test for https://crbug.com/1101828:
+// Test that simply setting a flag (without the backing feature) is sufficient
+// to consider a flag unexpired. This test checks that by using a flag with the
+// expected unexpire name, but wired to a dummy switch rather than the usual
+// feature.
+//
+// This isn't a perfect regression test - that would require two separate
+// browser restarts:
+// 1) Enable temporary-unexpire-flags-m$M, restart
+// 2) Enable the test flag (which is only visible after the previous restart),
+//    restart
+// 3) Ensure that the test flag got applied at startup
+IN_PROC_BROWSER_TEST_P(AboutFlagsBrowserTest, RawFlagUnexpiryWorks) {
+  NavigateToFlagsPage();
+  content::WebContents* contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  EXPECT_FALSE(IsFlagPresent(contents, kExpiredFlagName));
+  ToggleEnableDropdown(contents, unexpire_name_.c_str(), true);
+
+  NavigateToFlagsPage();
+  contents = browser()->tab_strip_model()->GetActiveWebContents();
+  EXPECT_TRUE(IsFlagPresent(contents, kExpiredFlagName));
+}
+
 IN_PROC_BROWSER_TEST_P(AboutFlagsBrowserTest, FormRestore) {
   NavigateToFlagsPage();
   content::WebContents* contents =

chrome/browser/unexpire_flags.cc

@@ -9,6 +9,7 @@
 #include "chrome/browser/expired_flags_list.h"
 #include "chrome/browser/unexpire_flags_gen.h"
 #include "chrome/common/chrome_version.h"
+#include "components/flags_ui/flags_storage.h"
 
 namespace flags {
 
@@ -43,9 +44,27 @@ int ExpirationMilestoneForFlag(const char* flag) {
   return -1;
 }
 
+// This function is a nasty hack - normally, the logic to turn flags into
+// feature names happens inside flags_ui::FlagsState, but this function is used
+// from the setup code of FlagsState, so it can't rely on FlagsState having been
+// set up. As such, we look into the backing FlagsStorage and hardcode how
+// enabled flags look inside that storage.
+std::set<int> UnexpiredMilestonesFromStorage(
+    const flags_ui::FlagsStorage* storage) {
+  std::set<int> unexpired;
+  for (const auto& f : storage->GetFlags()) {
+    int mstone;
+    if (sscanf(f.c_str(), "temporary-unexpire-flags-m%d@1", &mstone) == 1)
+      unexpired.insert(mstone);
+  }
+  return unexpired;
+}
+
 }  // namespace
 
-bool IsFlagExpired(const char* internal_name) {
+bool IsFlagExpired(const flags_ui::FlagsStorage* storage,
+                   const char* internal_name) {
+  DCHECK(storage);
   constexpr int kChromeVersion[] = {CHROME_VERSION};
   constexpr int kChromeVersionMajor = kChromeVersion[0];
 
@@ -54,6 +73,37 @@ bool IsFlagExpired(const char* internal_name) {
   if (mstone == -1)
     return false;
 
+  // This is extremely horrible:
+  //
+  // In order to know if a flag is expired or not, normally this function
+  // queries the state of base::FeatureList to check whether the unexpire
+  // feature for that milestone is enabled. However, when *creating* the initial
+  // base::FeatureList instance, these features won't be initialized yet, which
+  // leads to this issue:
+  //
+  // * Assume a flag "foo-bar" for feature FooBar that expires in M83.
+  // * Also, assume that temporary-unexpire-flags-m83 is enabled.
+  //
+  // If both of those are true, then if IsFlagExpired("foo-bar") is called
+  // *during* initial feature list setup, it will return true rather than false,
+  // which will cause FooBar to be set to its default rather than the
+  // non-default value that the flag may be to. This happens because the
+  // TemporaryUnexpireFlagsM83 feature hasn't been initialized yet, so it gets
+  // treated as its default state (disabled).
+  //
+  // To deal with that and make this function behave more correctly during
+  // FeatureList initialization, also consult the backing FlagsStorage from the
+  // FlagsState and look at the temporary-unexpire-flags-m$M flags directly, as
+  // well as looking at their features.
+  //
+  // This still has a problem: during browser startup, if the unexpire feature
+  // will be configured by some other mechanism (group policy, etc), that
+  // feature's value won't apply in time here and the bug described will happen.
+  // TODO(ellyjones): Figure out how to fix that.
+  std::set<int> unexpired_milestones = UnexpiredMilestonesFromStorage(storage);
+  if (base::Contains(unexpired_milestones, mstone))
+    return false;
+
   const base::Feature* expiry_feature = GetUnexpireFeatureForMilestone(mstone);
 
   // If there's an unexpiry feature, and the unexpiry feature is *disabled*,

chrome/browser/unexpire_flags.h

@@ -8,9 +8,14 @@
 #include "base/callback.h"
 #include "base/feature_list.h"
 
+namespace flags_ui {
+class FlagsStorage;
+}  // namespace flags_ui
+
 namespace flags {
 
-bool IsFlagExpired(const char* internal_name);
+bool IsFlagExpired(const flags_ui::FlagsStorage* storage,
+                   const char* internal_name);
 
 namespace testing {
 

components/flags_ui/flags_state.cc

@@ -339,7 +339,8 @@ struct FlagsState::SwitchEntry {
   SwitchEntry() : feature_state(false) {}
 };
 
-bool FlagsState::Delegate::ShouldExcludeFlag(const FeatureEntry& entry) {
+bool FlagsState::Delegate::ShouldExcludeFlag(const FlagsStorage* state,
+                                             const FeatureEntry& entry) {
   return false;
 }
 
@@ -613,6 +614,7 @@ void FlagsState::GetFlagFeatureEntries(
     base::ListValue* supported_entries,
     base::ListValue* unsupported_entries,
     base::RepeatingCallback<bool(const FeatureEntry&)> skip_feature_entry) {
+  DCHECK(flags_storage);
   std::set<std::string> enabled_entries;
   GetSanitizedEnabledFlags(flags_storage, &enabled_entries);
 
@@ -824,6 +826,7 @@ void FlagsState::MergeFeatureCommandLineSwitch(
 }
 
 std::set<std::string> FlagsState::SanitizeList(
+    const FlagsStorage* storage,
     const std::set<std::string>& enabled_entries,
     int platform_mask) const {
   std::set<std::string> new_enabled_entries;
@@ -834,7 +837,7 @@ std::set<std::string> FlagsState::SanitizeList(
   // |feature_entries_| first because |feature_entries_| is large and
   // |enabled_entries| should generally be small/empty.
   for (const std::string& entry_name : enabled_entries) {
-    if (IsSupportedFeature(entry_name, platform_mask))
+    if (IsSupportedFeature(storage, entry_name, platform_mask))
       new_enabled_entries.insert(entry_name);
   }
 
@@ -844,7 +847,8 @@ std::set<std::string> FlagsState::SanitizeList(
 void FlagsState::GetSanitizedEnabledFlags(FlagsStorage* flags_storage,
                                           std::set<std::string>* result) const {
   std::set<std::string> enabled_entries = flags_storage->GetFlags();
-  std::set<std::string> new_enabled_entries = SanitizeList(enabled_entries, -1);
+  std::set<std::string> new_enabled_entries =
+      SanitizeList(flags_storage, enabled_entries, -1);
   if (new_enabled_entries.size() != enabled_entries.size())
     flags_storage->SetFlags(new_enabled_entries);
   result->swap(new_enabled_entries);
@@ -861,7 +865,8 @@ void FlagsState::GetSanitizedEnabledFlagsForCurrentPlatform(
 #if defined(OS_CHROMEOS)
   platform_mask |= kOsCrOSOwnerOnly;
 #endif
-  std::set<std::string> platform_entries = SanitizeList(*result, platform_mask);
+  std::set<std::string> platform_entries =
+      SanitizeList(flags_storage, *result, platform_mask);
   result->swap(platform_entries);
 }
 
@@ -942,7 +947,8 @@ const FeatureEntry* FlagsState::FindFeatureEntryByName(
   return nullptr;
 }
 
-bool FlagsState::IsSupportedFeature(const std::string& name,
+bool FlagsState::IsSupportedFeature(const FlagsStorage* storage,
+                                    const std::string& name,
                                     int platform_mask) const {
   for (const auto& entry : feature_entries_) {
     DCHECK(IsValidFeatureEntry(entry));
@@ -950,7 +956,7 @@ bool FlagsState::IsSupportedFeature(const std::string& name,
       continue;
     if (!entry.InternalNameMatches(name))
       continue;
-    if (delegate_ && delegate_->ShouldExcludeFlag(entry))
+    if (delegate_ && delegate_->ShouldExcludeFlag(storage, entry))
       continue;
     return true;
   }

components/flags_ui/flags_state.h

@@ -71,7 +71,8 @@ class FlagsState {
    public:
     // Returns whether |entry| should be excluded from the sets of
     // switches/features generated by ConvertFlagsToSwitches().
-    virtual bool ShouldExcludeFlag(const FeatureEntry& entry);
+    virtual bool ShouldExcludeFlag(const FlagsStorage* state,
+                                   const FeatureEntry& entry);
 
    protected:
     Delegate();
@@ -209,6 +210,7 @@ class FlagsState {
   // |feature_entries_| and whose |supported_platforms| matches |platform_mask|.
   // Pass -1 to |platform_mask| to not do platform filtering.
   std::set<std::string> SanitizeList(
+      const FlagsStorage* storage,
       const std::set<std::string>& enabled_entries,
       int platform_mask) const;
 
@@ -242,7 +244,9 @@ class FlagsState {
   // a) Is supported on this |platform_mask|, and
   // b) Is not excluded by |exclude_predicate_|, if it is set (i.e. for which
   //    |exclude_predicate_| returns false).
-  bool IsSupportedFeature(const std::string& name, int platform_mask) const;
+  bool IsSupportedFeature(const FlagsStorage* storage,
+                          const std::string& name,
+                          int platform_mask) const;
 
   const base::span<const FeatureEntry> feature_entries_;
 

components/flags_ui/flags_state_unittest.cc

@@ -172,7 +172,8 @@ class FlagsStateTest : public ::testing::Test,
   }
 
   // FlagsState::Delegate:
-  bool ShouldExcludeFlag(const FeatureEntry& entry) override {
+  bool ShouldExcludeFlag(const FlagsStorage* storage,
+                         const FeatureEntry& entry) override {
     return exclude_flags_.count(entry.internal_name) != 0;
   }
 

tools/flags/generate_unexpire_flags.py

@@ -128,6 +128,9 @@ def gen_flags_fragment(prog_name, mstone):
   This creates a C++ source fragment defining flags, which are bound to the
   features described in gen_features_impl().
   """
+  # Note: The exact format of the flag name (temporary-unexpire-flags-m{m}) is
+  # depended on by a hack in UnexpiredMilestonesFromStorage(). See
+  # https://crbug.com/1101828 for more details.
   fragment = """
     {{"temporary-unexpire-flags-m{m}",
      "Temporarily unexpire M{m} flags.",