flags: treat expired flags as default state

Currently, when a flag expires, it is hidden from chrome://flags but still
stored inside the backend flags store, so that if the user un-expires flags
their settings are not lost. However, the code for applying flag values by
turning them into features/switches was not aware of flag expiration, so flags
that were persisted this way would continue to apply, despite the user having no
way to undo them.

This change:
1) Adds an exclusion predicate to FlagsState, to allow client classes of
   FlagsState to prevent flags from having their values applied as features or
   switches;
2) Adds an implementation of that predicate in the //chrome FlagsStateSingleton
   that checks the flag expiration state;
3) Adds unit tests to cover the new behavior added in (1);
4) Adds a new AboutFlagsBrowserTest test to validate the end-to-end behavior
   described in this commit message by:
    a) Setting an expired flag to a non-default value
    b) Restarting (simulated via separate PRE_ vs regular tests here)
    c) Checking that the flag's switch isn't in the browser command line

TBR=rohitrao@chromium.org

Bug: 1009729
Change-Id: I7d0ee2e7660fa378b9ecec3b663cc794f7e776fd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1879431Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Commit-Queue: Elly Fong-Jones <ellyjones@chromium.org>
Cr-Commit-Position: refs/heads/master@{#709169}

chrome/browser/about_flags.cc

@@ -4725,9 +4725,14 @@ class FlagsStateSingleton {
   FlagsStateSingleton()
       : flags_state_(std::make_unique<flags_ui::FlagsState>(
             kFeatureEntries,
-            base::size(kFeatureEntries))) {}
+            base::size(kFeatureEntries),
+            base::Bind(&FlagsStateSingleton::IsFlagExpired))) {}
   ~FlagsStateSingleton() {}
 
+  static bool IsFlagExpired(const flags_ui::FeatureEntry& entry) {
+    return flags::IsFlagExpired(entry.internal_name);
+  }
+
   static FlagsStateSingleton* GetInstance() {
     return base::Singleton<FlagsStateSingleton>::get();
   }
@@ -4737,8 +4742,9 @@ class FlagsStateSingleton {
   }
 
   void RebuildState(const std::vector<flags_ui::FeatureEntry>& entries) {
-    flags_state_ =
-        std::make_unique<flags_ui::FlagsState>(entries.data(), entries.size());
+    flags_state_ = std::make_unique<flags_ui::FlagsState>(
+        entries.data(), entries.size(),
+        base::Bind(&FlagsStateSingleton::IsFlagExpired));
   }
 
  private:

chrome/browser/about_flags_browsertest.cc

@@ -315,4 +315,28 @@ IN_PROC_BROWSER_TEST_P(AboutFlagsBrowserTest, ExpiryHidesFlag) {
   EXPECT_TRUE(IsFlagPresent(contents, kExpiredFlagName));
 }
 
+#if !defined(OS_CHROMEOS)
+IN_PROC_BROWSER_TEST_P(AboutFlagsBrowserTest, PRE_ExpiredFlagDoesntApply) {
+  set_expiration_enabled(false);
+  NavigateToFlagsPage();
+  content::WebContents* contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  EXPECT_TRUE(IsFlagPresent(contents, kExpiredFlagName));
+  EXPECT_FALSE(IsDropdownEnabled(contents, kExpiredFlagName));
+
+  ToggleEnableDropdown(contents, kExpiredFlagName, true);
+}
+
+IN_PROC_BROWSER_TEST_P(AboutFlagsBrowserTest, ExpiredFlagDoesntApply) {
+  set_expiration_enabled(true);
+  NavigateToFlagsPage();
+  content::WebContents* contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  EXPECT_FALSE(IsFlagPresent(contents, kExpiredFlagName));
+
+  EXPECT_FALSE(base::CommandLine::ForCurrentProcess()->HasSwitch(
+      kExpiredFlagSwitchName));
+}
+#endif
+
 }  // namespace

components/flags_ui/flags_state.cc

@@ -119,7 +119,7 @@ void AddOsStrings(unsigned bitmask, base::ListValue* list) {
 
 // Confirms that an entry is valid, used in a DCHECK in
 // SanitizeList below.
-bool ValidateFeatureEntry(const FeatureEntry& e) {
+bool IsValidFeatureEntry(const FeatureEntry& e) {
   switch (e.type) {
     case FeatureEntry::SINGLE_VALUE:
     case FeatureEntry::SINGLE_DISABLE_VALUE:
@@ -348,11 +348,14 @@ struct FlagsState::SwitchEntry {
   SwitchEntry() : feature_state(false) {}
 };
 
-FlagsState::FlagsState(const FeatureEntry* feature_entries,
-                       size_t num_feature_entries)
+FlagsState::FlagsState(
+    const FeatureEntry* feature_entries,
+    size_t num_feature_entries,
+    base::RepeatingCallback<bool(const FeatureEntry&)> exclude_predicate)
     : feature_entries_(feature_entries),
       num_feature_entries_(num_feature_entries),
-      needs_restart_(false) {}
+      needs_restart_(false),
+      exclude_predicate_(exclude_predicate) {}
 
 FlagsState::~FlagsState() {}
 
@@ -836,17 +839,9 @@ std::set<std::string> FlagsState::SanitizeList(
   // an O(n^2) search, this is more efficient than creating a set from
   // |feature_entries_| first because |feature_entries_| is large and
   // |enabled_entries| should generally be small/empty.
-  const FeatureEntry* features_end = feature_entries_ + num_feature_entries_;
   for (const std::string& entry_name : enabled_entries) {
-    if (features_end !=
-        std::find_if(feature_entries_, features_end,
-                     [entry_name, platform_mask](const FeatureEntry& e) {
-                       DCHECK(ValidateFeatureEntry(e));
-                       return (e.supported_platforms & platform_mask) &&
-                              e.InternalNameMatches(entry_name);
-                     })) {
+    if (IsSupportedFeature(entry_name, platform_mask))
       new_enabled_entries.insert(entry_name);
-    }
   }
 
   return new_enabled_entries;
@@ -948,4 +943,21 @@ const FeatureEntry* FlagsState::FindFeatureEntryByName(
   return nullptr;
 }
 
+bool FlagsState::IsSupportedFeature(const std::string& name,
+                                    int platform_mask) const {
+  base::span<const FeatureEntry> features(feature_entries_,
+                                          num_feature_entries_);
+  for (const auto& e : features) {
+    DCHECK(IsValidFeatureEntry(e));
+    if (!(e.supported_platforms & platform_mask))
+      continue;
+    if (!e.InternalNameMatches(name))
+      continue;
+    if (exclude_predicate_ && exclude_predicate_.Run(e))
+      continue;
+    return true;
+  }
+  return false;
+}
+
 }  // namespace flags_ui

components/flags_ui/flags_state.h

@@ -12,7 +12,7 @@
 #include <string>
 #include <vector>
 
-#include "base/callback_forward.h"
+#include "base/callback.h"
 #include "base/command_line.h"
 #include "base/feature_list.h"
 #include "base/macros.h"
@@ -60,7 +60,13 @@ enum FlagAccess { kGeneralAccessFlagsOnly, kOwnerAccessToFlags };
 // Stores and encapsulates the little state that about:flags has.
 class FlagsState {
  public:
-  FlagsState(const FeatureEntry* feature_entries, size_t num_feature_entries);
+  // The |exclude_predicate| parameter is a predicate used to prevent flags from
+  // actually applying, while retaining them in the list of feature entries.
+  // This is used to implement flag expiration.
+  FlagsState(
+      const FeatureEntry* feature_entries,
+      size_t num_feature_entries,
+      base::RepeatingCallback<bool(const FeatureEntry&)> exclude_predicate);
   ~FlagsState();
 
   // Reads the state from |flags_storage| and adds the command line flags
@@ -210,6 +216,13 @@ class FlagsState {
   const FeatureEntry* FindFeatureEntryByName(
       const std::string& internal_name) const;
 
+  // Returns whether there is a FeatureEntry named by |name| in
+  // |feature_entries_| that:
+  // 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;
+
   const FeatureEntry* feature_entries_;
   size_t num_feature_entries_;
 
@@ -220,6 +233,11 @@ class FlagsState {
   // were appended to existing (list value) switches.
   std::map<std::string, std::set<std::string>> appended_switches_;
 
+  // Used as a predicate to exclude FeatureEntries from applying to
+  // switches or base::Features; those for which this predicate returns true
+  // will not have any effect.
+  base::RepeatingCallback<bool(const FeatureEntry&)> exclude_predicate_;
+
   DISALLOW_COPY_AND_ASSIGN(FlagsState);
 };
 

components/flags_ui/flags_state_unittest.cc

@@ -164,17 +164,24 @@ class FlagsStateTest : public ::testing::Test {
     while (os_other_than_current == FlagsState::GetCurrentPlatform())
       os_other_than_current <<= 1;
     kEntries[2].supported_platforms = os_other_than_current;
-    flags_state_.reset(new FlagsState(kEntries, base::size(kEntries)));
+    flags_state_.reset(new FlagsState(
+        kEntries, base::size(kEntries),
+        base::Bind(&FlagsStateTest::IsFlagExcluded, base::Unretained(this))));
   }
 
   ~FlagsStateTest() override {
     variations::testing::ClearAllVariationParams();
   }
 
+  bool IsFlagExcluded(const FeatureEntry& entry) {
+    return exclude_flags_.count(entry.internal_name) != 0;
+  }
+
   TestingPrefServiceSimple prefs_;
   PrefServiceFlagsStorage flags_storage_;
   std::unique_ptr<FlagsState> flags_state_;
   base::FieldTrialList trial_list_;
+  std::set<std::string> exclude_flags_;
 };
 
 TEST_F(FlagsStateTest, NoChangeNoRestart) {
@@ -826,6 +833,20 @@ TEST_F(FlagsStateTest, EnableDisableValues) {
     EXPECT_FALSE(command_line.HasSwitch(kMultiSwitch1));
     EXPECT_FALSE(command_line.HasSwitch(kMultiSwitch2));
   }
+
+  // "Disable" option selected, but flag filtered out by exclude predicate.
+  flags_state_->SetFeatureEntryEnabled(&flags_storage_, entry.NameForOption(2),
+                                       true);
+  exclude_flags_.insert(entry.internal_name);
+  {
+    base::CommandLine command_line(base::CommandLine::NO_PROGRAM);
+    flags_state_->ConvertFlagsToSwitches(&flags_storage_, &command_line,
+                                         kAddSentinels, kEnableFeatures,
+                                         kDisableFeatures);
+    EXPECT_FALSE(command_line.HasSwitch(kSwitch1));
+    EXPECT_FALSE(command_line.HasSwitch(kSwitch2));
+  }
+  exclude_flags_.clear();
 }
 
 TEST_F(FlagsStateTest, FeatureValues) {

ios/chrome/browser/flags/about_flags.mm

@@ -13,6 +13,7 @@
 
 #include "base/base_switches.h"
 #include "base/bind.h"
+#include "base/bind_helpers.h"
 #include "base/command_line.h"
 #include "base/logging.h"
 #include "base/no_destructor.h"
@@ -636,7 +637,7 @@ bool SkipConditionalFeatureEntry(const flags_ui::FeatureEntry& entry) {
 
 flags_ui::FlagsState& GetGlobalFlagsState() {
   static base::NoDestructor<flags_ui::FlagsState> flags_state(
-      kFeatureEntries, base::size(kFeatureEntries));
+      kFeatureEntries, base::size(kFeatureEntries), base::NullCallback());
   return *flags_state;
 }
 }  // namespace

ios/web_view/internal/cwv_flags.mm

@@ -8,6 +8,7 @@
 
 #include "base/base_switches.h"
 #include "base/bind.h"
+#include "base/bind_helpers.h"
 #include "base/command_line.h"
 #include "base/logging.h"
 #include "base/macros.h"
@@ -87,7 +88,7 @@ const flags_ui::FeatureEntry kFeatureEntries[] = {
         std::make_unique<flags_ui::PrefServiceFlagsStorage>(_prefService);
     _flagsState = std::make_unique<flags_ui::FlagsState>(
         ios_web_view::kFeatureEntries,
-        base::size(ios_web_view::kFeatureEntries));
+        base::size(ios_web_view::kFeatureEntries), base::NullCallback());
   }
   return self;
 }