Revert "Use SetFeatureFlagsForUser when applying flags."

This reverts commit 9a265b6. Reason for revert: Caused tree clousure Original change's description: > Use SetFeatureFlagsForUser when applying flags. > > On Chrome OS, Chrome asks session_manager to apply feature flags on > restart. Previously the flags would be passed as raw command line > switches, but session_manager now maintains a separate set of feature > flags that it passes in encoded form in a command line switch. This > change adjusts Chrome to make use of that facility and deletes a bunch > of support code for the legacy Chrome OS approach. > > BUG=chromium:1073940 > TEST=Existing tests (e.g. SiteIsolationFlagHandlingTest), additional manual testing > > Change-Id: I04773a766837854da7218280539e2481a89e067a > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2560272 > Commit-Queue: Mattias Nissler <mnissler@chromium.org> > Reviewed-by: Xiyuan Xia <xiyuan@chromium.org> > Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org> > Reviewed-by: Charlie Reis <creis@chromium.org> > Reviewed-by: Alexei Svitkine <asvitkine@chromium.org> > Reviewed-by: Marc Treib <treib@chromium.org> > Reviewed-by: Pavol Marko <pmarko@chromium.org> > Cr-Commit-Position: refs/heads/master@{#857260} Bug: chromium:1073940 Change-Id: Ibbaa99b07aabc300f48e2848602e7e4df795de44 No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2716333 Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com> Reviewed-by: Darwin Huang <huangdarwin@chromium.org> Commit-Queue: Yi Gu <yigu@chromium.org> Commit-Queue: Darwin Huang <huangdarwin@chromium.org> Owners-Override: Yi Gu <yigu@chromium.org> Cr-Commit-Position: refs/heads/master@{#857286}
marcosholgado · Feb 24, 2021 · 8771589 · 8771589
1 parent 94cf4ae
commit 8771589
Show file tree

Hide file tree

Showing 15 changed files with 377 additions and 120 deletions.
diff --git a/chrome/browser/BUILD.gn b/chrome/browser/BUILD.gn
@@ -1707,7 +1707,6 @@ static_library("browser") {
     "signin/signin_promo_util.h",
     "signin/signin_util.cc",
     "signin/signin_util.h",
-    "site_isolation/about_flags.h",
     "site_isolation/prefs_observer.cc",
     "site_isolation/prefs_observer.h",
     "site_isolation/site_details.cc",

diff --git a/chrome/browser/about_flags.cc b/chrome/browser/about_flags.cc
@@ -54,7 +54,6 @@
 #include "chrome/browser/sharing/shared_clipboard/feature_flags.h"
 #include "chrome/browser/sharing/sms/sms_flags.h"
 #include "chrome/browser/signin/signin_features.h"
-#include "chrome/browser/site_isolation/about_flags.h"
 #include "chrome/browser/ui/ui_features.h"
 #include "chrome/browser/unexpire_flags.h"
 #include "chrome/browser/unexpire_flags_gen.h"
@@ -3325,7 +3324,7 @@ const FeatureEntry kFeatureEntries[] = {
     {"isolate-origins", flag_descriptions::kIsolateOriginsName,
      flag_descriptions::kIsolateOriginsDescription, kOsAll,
      ORIGIN_LIST_VALUE_TYPE(switches::kIsolateOrigins, "")},
-    {about_flags::kSiteIsolationTrialOptOutInternalName,
+    {"site-isolation-trial-opt-out",
      flag_descriptions::kSiteIsolationOptOutName,
      flag_descriptions::kSiteIsolationOptOutDescription, kOsAll,
      MULTI_VALUE_TYPE(kSiteIsolationOptOutChoices)},
@@ -7276,6 +7275,24 @@ std::vector<std::string> RegisterAllFeatureVariationParameters(
       ->RegisterAllFeatureVariationParameters(flags_storage, feature_list);
 }
 
+bool AreSwitchesIdenticalToCurrentCommandLine(
+    const base::CommandLine& new_cmdline,
+    const base::CommandLine& active_cmdline,
+    std::set<base::CommandLine::StringType>* out_difference) {
+  const char* extra_flag_sentinel_begin_flag_name = nullptr;
+  const char* extra_flag_sentinel_end_flag_name = nullptr;
+#if BUILDFLAG(IS_CHROMEOS_ASH)
+  // Put the flags between --policy-switches--begin and --policy-switches-end on
+  // ChromeOS.
+  extra_flag_sentinel_begin_flag_name =
+      chromeos::switches::kPolicySwitchesBegin;
+  extra_flag_sentinel_end_flag_name = chromeos::switches::kPolicySwitchesEnd;
+#endif  // BUILDFLAG(IS_CHROMEOS_ASH)
+  return flags_ui::FlagsState::AreSwitchesIdenticalToCurrentCommandLine(
+      new_cmdline, active_cmdline, out_difference,
+      extra_flag_sentinel_begin_flag_name, extra_flag_sentinel_end_flag_name);
+}
+
 void GetFlagFeatureEntries(flags_ui::FlagsStorage* flags_storage,
                            flags_ui::FlagAccess access,
                            base::ListValue* supported_entries,
@@ -7328,13 +7345,13 @@ void ResetAllFlags(flags_ui::FlagsStorage* flags_storage) {
   FlagsStateSingleton::GetFlagsState()->ResetAllFlags(flags_storage);
 }
 
-void RecordUMAStatistics(flags_ui::FlagsStorage* flags_storage,
-                         const std::string& histogram_name) {
+void RecordUMAStatistics(flags_ui::FlagsStorage* flags_storage) {
   std::set<std::string> switches;
   std::set<std::string> features;
   FlagsStateSingleton::GetFlagsState()->GetSwitchesAndFeaturesFromFlags(
       flags_storage, &switches, &features);
-  flags_ui::ReportAboutFlagsHistogram(histogram_name, switches, features);
+  flags_ui::ReportAboutFlagsHistogram("Launch.FlagsAtStartup", switches,
+                                      features);
 }
 
 namespace testing {

diff --git a/chrome/browser/about_flags.h b/chrome/browser/about_flags.h
@@ -46,6 +46,15 @@ std::vector<std::string> RegisterAllFeatureVariationParameters(
     flags_ui::FlagsStorage* flags_storage,
     base::FeatureList* feature_list);
 
+// Compares a set of switches of the two provided command line objects and
+// returns true if they are the same and false otherwise.
+// If |out_difference| is not NULL, it's filled with set_symmetric_difference
+// between sets.
+bool AreSwitchesIdenticalToCurrentCommandLine(
+    const base::CommandLine& new_cmdline,
+    const base::CommandLine& active_cmdline,
+    std::set<base::CommandLine::StringType>* out_difference);
+
 // Gets the list of feature entries. Entries that are available for the current
 // platform are appended to |supported_entries|; all other entries are appended
 // to |unsupported_entries|.
@@ -95,8 +104,7 @@ void ResetAllFlags(flags_ui::FlagsStorage* flags_storage);
 
 // Sends UMA stats about experimental flag usage. This should be called once per
 // startup.
-void RecordUMAStatistics(flags_ui::FlagsStorage* flags_storage,
-                         const std::string& histogram_name);
+void RecordUMAStatistics(flags_ui::FlagsStorage* flags_storage);
 
 namespace testing {
 

diff --git a/chrome/browser/ash/app_mode/kiosk_app_manager.cc b/chrome/browser/ash/app_mode/kiosk_app_manager.cc
@@ -225,7 +225,7 @@ void KioskAppManager::InitSession(Profile* profile,
     chromeos::UserSessionManager::GetInstance()->SetSwitchesForUser(
         user_manager::UserManager::Get()->GetActiveUser()->GetAccountId(),
         chromeos::UserSessionManager::CommandLineSwitchesType::
-            kPolicyAndKioskControl,
+            kPolicyAndFlagsAndKioskControl,
         flags);
   }
 

diff --git a/chrome/browser/chromeos/login/guest_login_browsertest.cc b/chrome/browser/chromeos/login/guest_login_browsertest.cc
@@ -4,17 +4,14 @@
 
 #include "ash/public/cpp/keyboard/keyboard_controller.h"
 #include "ash/public/cpp/login_screen_test_api.h"
-#include "chrome/browser/about_flags.h"
 #include "chrome/browser/chromeos/login/test/login_manager_mixin.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_window.h"
 #include "chrome/test/base/mixin_based_in_process_browser_test.h"
 #include "chromeos/dbus/power/fake_power_manager_client.h"
 #include "chromeos/dbus/session_manager/fake_session_manager_client.h"
-#include "components/flags_ui/feature_entry_macros.h"
 #include "components/user_manager/user_manager.h"
 #include "content/public/test/browser_test.h"
-#include "third_party/cros_system_api/switches/chrome_switches.h"
 
 namespace chromeos {
 
@@ -45,21 +42,13 @@ class GuestLoginTest : public MixinBasedInProcessBrowserTest {
 
 class GuestLoginWithLoginSwitchesTest : public GuestLoginTest {
  public:
-  GuestLoginWithLoginSwitchesTest() {
-    std::vector<flags_ui::FeatureEntry> entries = {
-        {"feature-name", "name-1", "description-1", -1,
-         SINGLE_VALUE_TYPE("feature-switch")},
-    };
-    about_flags::testing::SetFeatureEntries(entries);
-  }
+  GuestLoginWithLoginSwitchesTest() = default;
   ~GuestLoginWithLoginSwitchesTest() override = default;
 
   // GuestLoginTest:
   void SetDefaultLoginSwitches() override {
     login_manager_.SetDefaultLoginSwitches(
-        {std::make_pair(chromeos::switches::kFeatureFlags,
-                        "[\"feature-name\"]"),
-         std::make_pair("test_switch_1", ""),
+        {std::make_pair("test_switch_1", ""),
          std::make_pair("test_switch_2", "test_switch_2_value")});
   }
 };
@@ -140,8 +129,6 @@ IN_PROC_BROWSER_TEST_F(GuestLoginWithLoginSwitchesTest, PRE_Login) {
   FakeSessionManagerClient::Get()->set_restart_job_callback(
       restart_job_waiter.QuitClosure());
 
-  EXPECT_TRUE(
-      base::CommandLine::ForCurrentProcess()->HasSwitch("feature-switch"));
   ASSERT_TRUE(ash::LoginScreenTestApi::ClickGuestButton());
 
   restart_job_waiter.Run();
@@ -155,8 +142,6 @@ IN_PROC_BROWSER_TEST_F(GuestLoginWithLoginSwitchesTest, Login) {
   user_manager::UserManager* user_manager = user_manager::UserManager::Get();
   EXPECT_TRUE(user_manager->IsLoggedInAsGuest());
 
-  EXPECT_FALSE(
-      base::CommandLine::ForCurrentProcess()->HasSwitch("feature-switch"));
   EXPECT_FALSE(
       base::CommandLine::ForCurrentProcess()->HasSwitch("test_switch_1"));
   EXPECT_FALSE(

diff --git a/chrome/browser/chromeos/login/session/user_session_manager.cc b/chrome/browser/chromeos/login/session/user_session_manager.cc
@@ -81,7 +81,6 @@
 #include "chrome/browser/chromeos/policy/minimum_version_policy_handler.h"
 #include "chrome/browser/chromeos/policy/tpm_auto_update_mode_policy_handler.h"
 #include "chrome/browser/chromeos/settings/cros_settings.h"
-#include "chrome/browser/chromeos/settings/owner_flags_storage.h"
 #include "chrome/browser/chromeos/sync/os_sync_util.h"
 #include "chrome/browser/chromeos/sync/turn_sync_on_helper.h"
 #include "chrome/browser/chromeos/tether/tether_service.h"
@@ -95,7 +94,6 @@
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/signin/identity_manager_factory.h"
-#include "chrome/browser/site_isolation/about_flags.h"
 #include "chrome/browser/supervised_user/child_accounts/child_account_service.h"
 #include "chrome/browser/supervised_user/child_accounts/child_account_service_factory.h"
 #include "chrome/browser/sync/profile_sync_service_factory.h"
@@ -280,6 +278,36 @@ void InitLocaleAndInputMethodsForNewUser(
   prefs->SetBoolean(::prefs::kLanguageShouldMergeInputMethods, true);
 }
 
+// Returns new CommandLine with per-user flags.
+base::CommandLine CreatePerSessionCommandLine(Profile* profile) {
+  base::CommandLine user_flags(base::CommandLine::NO_PROGRAM);
+  flags_ui::PrefServiceFlagsStorage flags_storage(profile->GetPrefs());
+  about_flags::ConvertFlagsToSwitches(&flags_storage, &user_flags,
+                                      flags_ui::kAddSentinels);
+
+  UserSessionManager::ApplyUserPolicyToSwitches(profile->GetPrefs(),
+                                                &user_flags);
+
+  return user_flags;
+}
+
+// Returns true if restart is needed to apply per-session flags.
+bool NeedRestartToApplyPerSessionFlags(
+    const base::CommandLine& user_flags,
+    std::set<base::CommandLine::StringType>* out_command_line_difference) {
+  // Don't restart browser if it is not first profile in session.
+  if (user_manager::UserManager::Get()->GetLoggedInUsers().size() != 1)
+    return false;
+
+  auto* current_command_line = base::CommandLine::ForCurrentProcess();
+  if (about_flags::AreSwitchesIdenticalToCurrentCommandLine(
+          user_flags, *current_command_line, out_command_line_difference)) {
+    return false;
+  }
+
+  return true;
+}
+
 bool CanPerformEarlyRestart() {
   const ExistingUserController* controller =
       ExistingUserController::current_controller();
@@ -301,15 +329,13 @@ bool CanPerformEarlyRestart() {
   return true;
 }
 
-void LogCustomFeatureFlags(const std::set<std::string>& feature_flags) {
-  if (VLOG_IS_ON(1)) {
-    for (const auto& feature_flag : feature_flags) {
-      VLOG(1) << "Feature flag leading to restart: '" << feature_flag << "'";
-    }
+void LogCustomSwitches(const std::set<std::string>& switches) {
+  if (!VLOG_IS_ON(1))
+    return;
+  for (std::set<std::string>::const_iterator it = switches.begin();
+       it != switches.end(); ++it) {
+    VLOG(1) << "Switch leading to restart: '" << *it << "'";
   }
-
-  about_flags::ReadOnlyFlagsStorage flags_storage(feature_flags);
-  ::about_flags::RecordUMAStatistics(&flags_storage, "Login.CustomFlags");
 }
 
 // Calls the real AttemptRestart method. This is used to avoid taking a function
@@ -420,17 +446,27 @@ void UserSessionManager::RegisterPrefs(PrefRegistrySimple* registry) {
 }
 
 // static
-void UserSessionManager::ApplyUserPolicyToFlags(PrefService* user_profile_prefs,
-                                                std::set<std::string>* flags) {
+void UserSessionManager::ApplyUserPolicyToSwitches(
+    PrefService* user_profile_prefs,
+    base::CommandLine* user_flags) {
   // Get target value for --site-per-process for the user session according to
   // policy. If it is supposed to be enabled, make sure it can not be disabled
   // using flags-induced command-line switches.
   const PrefService::Preference* site_per_process_pref =
       user_profile_prefs->FindPreference(::prefs::kSitePerProcess);
   if (site_per_process_pref->IsManaged() &&
       site_per_process_pref->GetValue()->GetBool()) {
-    flags->erase(::about_flags::SiteIsolationTrialOptOutChoiceEnabled());
+    user_flags->RemoveSwitch(::switches::kDisableSiteIsolation);
   }
+
+  // Note: If a user policy is introduced again which translates to command-line
+  // switches, make sure to wrap the policy-added command-line switches in
+  // `"--policy-switches-begin"` / `"--policy-switches-end"` sentinels.
+  // This is important, because only command-line switches between the
+  // `"--policy-switches-begin"` / `"--policy-switches-end"` and the
+  // `"--flag-switches-begin"` / `"--flag-switches-end"` sentinels will be
+  // compared when comparing the current command line and the user session
+  // command line in order to decide if chrome should be restarted.
 }
 
 UserSessionManager::UserSessionManager()
@@ -547,10 +583,19 @@ void UserSessionManager::CompleteGuestSessionLogin(const GURL& start_url) {
   // the guest profile session flags will not match the current command line and
   // another restart will be attempted in order to reset the user flags for the
   // guest user.
-  SessionManagerClient::Get()->SetFeatureFlagsForUser(
-      cryptohome::CreateAccountIdentifierFromAccountId(
-          user_manager::GuestAccountId()),
-      {});
+  const base::CommandLine user_flags(base::CommandLine::NO_PROGRAM);
+  if (!about_flags::AreSwitchesIdenticalToCurrentCommandLine(
+          user_flags, *base::CommandLine::ForCurrentProcess(), NULL)) {
+    SessionManagerClient::Get()->SetFeatureFlagsForUser(
+        cryptohome::CreateAccountIdentifierFromAccountId(
+            user_manager::GuestAccountId()),
+        {});
+
+    SessionManagerClient::Get()->SetFlagsForUser(
+        cryptohome::CreateAccountIdentifierFromAccountId(
+            user_manager::GuestAccountId()),
+        base::CommandLine::StringVector());
+  }
 
   RestartChrome(command_line);
 }
@@ -874,32 +919,23 @@ bool UserSessionManager::RestartToApplyPerSessionFlagsIfNeed(
   if (user_manager::UserManager::Get()->GetLoggedInUsers().size() > 1)
     return false;
 
-  // Don't restart browser if it is not the first profile in the session.
-  if (user_manager::UserManager::Get()->GetLoggedInUsers().size() != 1)
+  const base::CommandLine user_flags(CreatePerSessionCommandLine(profile));
+  std::set<base::CommandLine::StringType> command_line_difference;
+  if (!NeedRestartToApplyPerSessionFlags(user_flags, &command_line_difference))
     return false;
 
-  // Compare feature flags configured for the device vs. user. Restart is only
-  // required when there's a difference.
-  std::set<std::string> designated_flags =
-      flags_ui::PrefServiceFlagsStorage(profile->GetPrefs()).GetFlags();
-  ApplyUserPolicyToFlags(profile->GetPrefs(), &designated_flags);
-  std::set<std::string> actual_flags = about_flags::ParseFlagsFromCommandLine();
-  std::set<std::string> flags_difference;
-  std::set_symmetric_difference(
-      designated_flags.begin(), designated_flags.end(), actual_flags.begin(),
-      actual_flags.end(),
-      std::inserter(flags_difference, flags_difference.begin()));
-  if (flags_difference.empty())
-    return false;
+  LogCustomSwitches(command_line_difference);
 
-  // Restart is required. Emit metrics and logs and trigger the restart.
-  LogCustomFeatureFlags(flags_difference);
-  LOG(WARNING) << "Restarting to apply per-session flags...";
+  flags_ui::ReportAboutFlagsHistogram(
+      "Login.CustomFlags", command_line_difference, std::set<std::string>());
 
-  auto account_id = cryptohome::CreateAccountIdentifierFromAccountId(
-      user_manager::UserManager::Get()->GetActiveUser()->GetAccountId());
-  SessionManagerClient::Get()->SetFeatureFlagsForUser(
-      account_id, {designated_flags.begin(), designated_flags.end()});
+  base::CommandLine::StringVector flags;
+  // argv[0] is the program name `base::CommandLine::NO_PROGRAM`.
+  flags.assign(user_flags.argv().begin() + 1, user_flags.argv().end());
+  LOG(WARNING) << "Restarting to apply per-session flags...";
+  SetSwitchesForUser(
+      user_manager::UserManager::Get()->GetActiveUser()->GetAccountId(),
+      CommandLineSwitchesType::kPolicyAndFlagsAndKioskControl, flags);
   attempt_restart_closure_.Run();
   return true;
 }
@@ -2272,6 +2308,13 @@ void UserSessionManager::SetSwitchesForUser(
                         pair.second.end());
   }
 
+  // Clear session_manager's feature flag state so it doesn't pass flags on
+  // restart. This is necessary until in-session feature flags have been
+  // converted to use the new way.
+  // TODO(crbug.com/1073940): Remove after conversion is complete.
+  SessionManagerClient::Get()->SetFeatureFlagsForUser(
+      cryptohome::CreateAccountIdentifierFromAccountId(account_id), {});
+
   SessionManagerClient::Get()->SetFlagsForUser(
       cryptohome::CreateAccountIdentifierFromAccountId(account_id),
       all_switches);

diff --git a/chrome/browser/chromeos/login/session/user_session_manager.h b/chrome/browser/chromeos/login/session/user_session_manager.h
@@ -48,6 +48,10 @@ class Profile;
 class TokenHandleFetcher;
 class TurnSyncOnHelper;
 
+namespace base {
+class CommandLine;
+}
+
 namespace user_manager {
 class User;
 }  // namespace user_manager
@@ -119,12 +123,13 @@ class UserSessionManager
     // Switches for controlling session initialization, such as if the profile
     // requires enterprise policy.
     kSessionControl,
-    // Switches derived from user policy and kiosk app control switches.
+    // Switches derived from user policy, from user-set flags and kiosk app
+    // control switches.
     // TODO(pmarko): Split this into multiple categories, such as kPolicy,
-    // kKioskControl. Consider also adding sentinels automatically and
+    // kFlags, kKioskControl. Consider also adding sentinels automatically and
     // pre-filling these switches from the command-line if the chrome has been
     // started with the --login-user flag (https://crbug.com/832857).
-    kPolicyAndKioskControl
+    kPolicyAndFlagsAndKioskControl
   };
 
   // To keep track of which systems need the login password to be stored in the
@@ -151,11 +156,11 @@ class UserSessionManager
   // Registers session related preferences.
   static void RegisterPrefs(PrefRegistrySimple* registry);
 
-  // Applies user policies to `flags`. This could mean removing flags that have
-  // been added by the flag handling logic or appending additional flags due to
-  // enterprise policy.
-  static void ApplyUserPolicyToFlags(PrefService* user_profile_prefs,
-                                     std::set<std::string>* flags);
+  // Applies user policies to `user_flags` .
+  // This could mean removing command-line switchis that have been added by the
+  // flag handling logic or appending additional switches due to policy.
+  static void ApplyUserPolicyToSwitches(PrefService* user_profile_prefs,
+                                        base::CommandLine* user_flags);
 
   // Invoked after the tmpfs is successfully mounted.
   // Asks session_manager to restart Chrome in Guest session mode.