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

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

Index: chrome/browser/chromeos/login/session/user_session_manager.cc
diff --git a/chrome/browser/chromeos/login/session/user_session_manager.cc b/chrome/browser/chromeos/login/session/user_session_manager.cc
index 61c3de9feb182e269cc116c0439633da34662e5a..3dc9d2649afe78553588dd43ddcbd7af6bc56c9f 100644
--- a/chrome/browser/chromeos/login/session/user_session_manager.cc
+++ b/chrome/browser/chromeos/login/session/user_session_manager.cc
@@ -77,6 +77,7 @@
 #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"
@@ -107,6 +108,7 @@
 #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"
@@ -284,12 +286,32 @@ bool NeedRestartToApplyPerSessionFlags(
   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;
 }