Mustash: Ignore --mash flag when comparing flags in UserSessionManager.

chrome/browser/chromeos/login/session/user_session_manager.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/chromeos/login/session/user_session_manager.h"
 
 #include <stddef.h>
 
 #include <set>
 #include <string>
@@ skipping 59 matching lines @@
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/signin/account_tracker_service_factory.h"
 #include "chrome/browser/signin/easy_unlock_service.h"
 #include "chrome/browser/signin/signin_manager_factory.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/ui/app_list/app_list_service.h"
 #include "chrome/browser/ui/ash/ash_util.h"
 #include "chrome/browser/ui/startup/startup_browser_creator.h"
+#include "chrome/common/channel_info.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/logging_chrome.h"
 #include "chrome/common/pref_names.h"
 #include "chromeos/cert_loader.h"
 #include "chromeos/chromeos_switches.h"
 #include "chromeos/cryptohome/cryptohome_parameters.h"
 #include "chromeos/cryptohome/cryptohome_util.h"
 #include "chromeos/dbus/cryptohome_client.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/dbus/session_manager_client.h"
@@ skipping 10 matching lines @@
 #include "components/quirks/quirks_manager.h"
 #include "components/session_manager/core/session_manager.h"
 #include "components/signin/core/account_id/account_id.h"
 #include "components/signin/core/browser/account_tracker_service.h"
 #include "components/signin/core/browser/signin_manager_base.h"
 #include "components/user_manager/known_user.h"
 #include "components/user_manager/user.h"
 #include "components/user_manager/user_manager.h"
 #include "components/user_manager/user_names.h"
 #include "components/user_manager/user_type.h"
+#include "components/version_info/version_info.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/storage_partition.h"
 #include "content/public/common/content_switches.h"
 #include "extensions/common/features/feature_session_type.h"
 #include "net/cert/sth_distributor.h"
 #include "rlz/features/features.h"
 #include "ui/base/ime/chromeos/input_method_descriptor.h"
 #include "ui/base/ime/chromeos/input_method_manager.h"
 #include "url/gurl.h"
@@ skipping 157 matching lines @@
     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;
 
   // Only restart if needed and if not going into managed mode.
   if (user_manager::UserManager::Get()->IsLoggedInAsSupervisedUser())
     return false;
 
+  // When running Mus+ash, the flags stored in Profile Prefs will contain
+  // --mash, while the browser command line will not (and should not) have
+  // it, so ignore it for the purpose of comparison.
+  // TODO(mfomitchev): Find a better way. crbug.com/690083
+  bool in_mash = user_flags.HasSwitch(::switches::kMash);
+  base::CommandLine user_flags_no_mash = user_flags;
+  if (in_mash) {
+    user_flags_no_mash = base::CommandLine(user_flags.GetProgram());
+    base::CommandLine::SwitchMap switches = user_flags.GetSwitches();
+    switches.erase(::switches::kMash);
+    user_flags_no_mash = base::CommandLine(user_flags.GetProgram());
+    for (const auto sw : switches)

[James Cook, 2017/02/08 19:19:51]

ditto

[mfomitchev, 2017/02/08 20:39:24]

Done.

+      user_flags_no_mash.AppendSwitchNative(sw.first, sw.second);
+  }
+
   if (about_flags::AreSwitchesIdenticalToCurrentCommandLine(
-          user_flags, *base::CommandLine::ForCurrentProcess(),
+          user_flags_no_mash, *base::CommandLine::ForCurrentProcess(),
           out_command_line_difference)) {
     return false;
   }
 
+  // TODO(jamescook, mfomitchev): Browser restart doesn't currently work in
+  // Mus+ash. So if we are running Mustash and we need to restart - just crash
+  // right here. crbug.com/678683

[Alexander Alekseev, 2017/02/08 19:23:39]

crbug.com/678683 says about rebooting.
This is different: browser should restart inside existing user session.
I am not sure that all the services should be brought down.
There is a special DBUS API for this in session_manager.

Could you create a separate issue for this?

[mfomitchev, 2017/02/08 20:39:24]

Ok, done.  Just to clarify, if we crash chrome and bring down all services will
chrome boot up without problem the second time, or will it trip over this check
again, essentially getting stuck in the restart loop?

[James Cook, 2017/02/08 20:46:06]

Alexander can comment, but at least in mash there is a difference between
crashing content_browser and crashing the root chrome --mash process. Today,
crashing content_browser just kills the browser. Medium term I want to make
crashes of content_browser restart the root process. That will cause
session_manager to restart the root process, which will restart content_browser.
I am not sure what session_manager does with flags in that case. If it keeps the
same flags we could end up in a crash loop.

That said, if we need this CL to deal with a startup crash that's blocking
on-device mustash work, I would just land it.

[Alexander Alekseev, 2017/02/08 21:41:57]

If the crash is deterministic, yes it will happen again. For example, corrupted
profile sometimes leads to deterministic crash.

When Chrome is restarted due to flags difference, this is a different story.
There are two mechanisms in session_manager to restart chrome. One is used for
Guest sessions, another is used for restarting chrome inside user session.

I cannot comment on what should be restarted in case of mash (I am not familiar
with the design of this mode). Probably "everything that is controlled by chrome
command-line options".

+  CHECK(!in_mash);
+
   return true;
 }
 
 bool CanPerformEarlyRestart() {
   if (!ChromeUserManager::Get()
            ->GetCurrentUserFlow()
            ->SupportsEarlyRestartToApplyFlags()) {
     return false;
   }
 
@@ skipping 1583 matching lines @@
           ->browser_policy_connector_chromeos()
           ->IsEnterpriseManaged()) {
     return false;
   }
 
   // Do not show end of life notification if this is a guest session
   return !profile->IsGuestSession();
 }
 
 }  // namespace chromeos