Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2681663004:
Mustash: Ignore --mash flag when comparing flags in UserSessionManager. (Closed)
|
Created:
3 years, 10 months ago by mfomitchev Modified:
3 years, 10 months ago CC:
chromium-reviews, alemate+watch_chromium.org, achuith+watch_chromium.org, oshima+watch_chromium.org, davemoore+watch_chromium.org, tonikitoo Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMustash: Ignore --mash flag when comparing flags in UserSessionManager.
When running Mustash, the flags stored in Profile Prefs will contain --mash,
while the browser command line will not (and should not) have it. Also, the
browser restart functionality doesn't currently work in Mustash.
So ignore --mash flag when comparing flags in UserSessionManager, and if
there are other mismatches when running Mustash - just crash right away.
Also adding the --mash flag to chrome_switches, since we are now using it in
a few different places.
BUG=NONE
Review-Url: https://codereview.chromium.org/2681663004
Cr-Commit-Position: refs/heads/master@{#449442}
Committed: https://chromium.googlesource.com/chromium/src/+/ca35d83a3e75630536fc1065068098ee1699877f
Patch Set 1 #
Total comments: 4
Patch Set 2 : Adding braces around the return. #Patch Set 3 : Adding the --mash flag to chrome_switches. #
Total comments: 2
Patch Set 4 : Crashing Mustash if a flag mismatch other than --mash is detected. #Patch Set 5 : Updating BUILD.gn #
Total comments: 9
Patch Set 6 : Addressing feedback. #
Total comments: 5
Patch Set 7 : Addressing feedback #
Total comments: 2
Patch Set 8 : Adding braces. #
Total comments: 2
Messages
Total messages: 50 (23 generated)
mfomitchev@chromium.org changed reviewers: + jamescook@chromium.org
https://codereview.chromium.org/2681663004/diff/1/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/session/user_session_manager.cc (right): https://codereview.chromium.org/2681663004/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/login/session/user_session_manager.cc:301: version_info::Channel channel = chrome::GetChannel(); Is there any particular reason to do the channel check? https://codereview.chromium.org/2681663004/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/login/session/user_session_manager.cc:305: out_command_line_difference->end()) nit: not strictly needed for style guide, but I would put {} here
Description was changed from ========== Mustash: Don't restart chrome when detecting a flags mismatch in UserSessionManager. When running Mustash, the flags stored in Profile Prefs will contain --mash, while the browser command line will not (and should not) have it. Also, the browser restart functionality doesn't currently work in Mustash. So ignore all the flag mismatches in UserSessionManager when running Mustash. BUG=NONE ========== to ========== Mustash: Don't restart chrome when detecting a flags mismatch in UserSessionManager. When running Mustash, the flags stored in Profile Prefs will contain --mash, while the browser command line will not (and should not) have it. Also, the browser restart functionality doesn't currently work in Mustash. So ignore all the flag mismatches in UserSessionManager when running Mustash. Also adding the --mash flag to chrome_switches, since we are now using it in a few different places. BUG=NONE ==========
I added the kMash flag, can you PTAL? https://codereview.chromium.org/2681663004/diff/1/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/session/user_session_manager.cc (right): https://codereview.chromium.org/2681663004/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/login/session/user_session_manager.cc:301: version_info::Channel channel = chrome::GetChannel(); On 2017/02/07 21:07:32, James Cook wrote: > Is there any particular reason to do the channel check? Just being extra careful to not affect anything in prod. This is what we do in chrome_main as well, so I thought it made sense to be consistent: https://cs.chromium.org/chromium/src/chrome/app/chrome_main.cc?l=104 https://codereview.chromium.org/2681663004/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/login/session/user_session_manager.cc:305: out_command_line_difference->end()) On 2017/02/07 21:07:31, James Cook wrote: > nit: not strictly needed for style guide, but I would put {} here I changed the code - let me know if you still want the braces.
Description was changed from ========== Mustash: Don't restart chrome when detecting a flags mismatch in UserSessionManager. When running Mustash, the flags stored in Profile Prefs will contain --mash, while the browser command line will not (and should not) have it. Also, the browser restart functionality doesn't currently work in Mustash. So ignore all the flag mismatches in UserSessionManager when running Mustash. Also adding the --mash flag to chrome_switches, since we are now using it in a few different places. BUG=NONE ========== to ========== Mustash: Don't restart chrome when detecting a flags mismatch in UserSessionManager. When running Mustash, the flags stored in Profile Prefs will contain --mash, while the browser command line will not (and should not) have it. Also, the browser restart functionality doesn't currently work in Mustash. So ignore all the flag mismatches in UserSessionManager when running Mustash. Also adding the --mash flag to chrome_switches, since we are now using it in a few different places. BUG=NONE ==========
As discussed offline. Thanks for cleaning up the flag. One other nit. https://codereview.chromium.org/2681663004/diff/40001/chrome/app/chrome_main.cc File chrome/app/chrome_main.cc (right): https://codereview.chromium.org/2681663004/diff/40001/chrome/app/chrome_main.... chrome/app/chrome_main.cc:105: if (command_line->HasSwitch(switches::kMash)) Can you fix mash_runner.cc also?
steel@chromium.org changed reviewers: + steel@chromium.org
not lgtm for now - please address the following concerns before landing. Thanks! We can ignore the mash flag during the comparison - but bypassing restart because the mash flash is present seems like the wrong thing to do. Can we get some context on why this is being done? There is no bug associated with the CL and the bug in the TODO doesn't seem to have any details on when we'll re-enable this functionality.
On 2017/02/07 22:07:54, Rahul Chaturvedi wrote: > not lgtm for now - please address the following concerns before landing. Thanks! > > We can ignore the mash flag during the comparison - but bypassing restart > because the mash flash is present seems like the wrong thing to do. > > Can we get some context on why this is being done? There is no bug associated > with the CL and the bug in the TODO doesn't seem to have any details on when > we'll re-enable this functionality. I've just been chatting with James offline about this: we don't support browser process restart in Mustash right now, so the new plan is just to CHECK-fail if we detect a mismatch other than the --mash flag. Does this sound okay? I can also log a bug to support browser process restart in Mustash.
Having a fatal check sounds good to me. Please do have a bug filed and added in the TODO to track this. This is security sensitive code, I would really prefer to have stronger checks to make sure that we don't inadvertently ever end up *not* restarting when we really should be. Thanks again!
The CQ bit was checked by mfomitchev@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by mfomitchev@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Feedback addressed, can you PTAL? https://codereview.chromium.org/2681663004/diff/40001/chrome/app/chrome_main.cc File chrome/app/chrome_main.cc (right): https://codereview.chromium.org/2681663004/diff/40001/chrome/app/chrome_main.... chrome/app/chrome_main.cc:105: if (command_line->HasSwitch(switches::kMash)) On 2017/02/07 22:07:44, James Cook wrote: > Can you fix mash_runner.cc also? Done.
Latest patch looks great. lgtm. You'll still need alemate@'s lgtm for owners check.
Description was changed from ========== Mustash: Don't restart chrome when detecting a flags mismatch in UserSessionManager. When running Mustash, the flags stored in Profile Prefs will contain --mash, while the browser command line will not (and should not) have it. Also, the browser restart functionality doesn't currently work in Mustash. So ignore all the flag mismatches in UserSessionManager when running Mustash. Also adding the --mash flag to chrome_switches, since we are now using it in a few different places. BUG=NONE ========== to ========== Mustash: Ignore --mash flag when comparing flags in UserSessionManager. When running Mustash, the flags stored in Profile Prefs will contain --mash, while the browser command line will not (and should not) have it. Also, the browser restart functionality doesn't currently work in Mustash. So ignore --mash flag when comparing flags in UserSessionManager, and if there are other mismatches when running Mustash - just crash right away. Also adding the --mash flag to chrome_switches, since we are now using it in a few different places. BUG=NONE ==========
mfomitchev@chromium.org changed reviewers: + alemate@google.com, sky@chromium.org
+alemate for chrome/browser/chromeos/login/session/user_session_manager.cc +sky for chrome/app/
LGTM with a couple questions https://codereview.chromium.org/2681663004/diff/80001/chrome/app/mash/DEPS File chrome/app/mash/DEPS (right): https://codereview.chromium.org/2681663004/diff/80001/chrome/app/mash/DEPS#ne... chrome/app/mash/DEPS:8: "+chrome/common/chrome_switches.h", sky, are you OK with this? Or do you think we need a new mash_switches.h? https://codereview.chromium.org/2681663004/diff/80001/chrome/app/mash/mash_ru... File chrome/app/mash/mash_runner.cc (right): https://codereview.chromium.org/2681663004/diff/80001/chrome/app/mash/mash_ru... chrome/app/mash/mash_runner.cc:123: for (const auto sw : new_switches) can this be const auto& ? I'm not sure about our style guidance for ranged-for iteration over maps. https://codereview.chromium.org/2681663004/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/login/session/user_session_manager.cc (right): https://codereview.chromium.org/2681663004/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/login/session/user_session_manager.cc:300: for (const auto sw : switches) ditto
alemate@chromium.org changed reviewers: + alemate@chromium.org
https://codereview.chromium.org/2681663004/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/login/session/user_session_manager.cc (right): https://codereview.chromium.org/2681663004/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/login/session/user_session_manager.cc:312: // right here. crbug.com/678683 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?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2681663004/diff/80001/chrome/app/mash/mash_ru... File chrome/app/mash/mash_runner.cc (right): https://codereview.chromium.org/2681663004/diff/80001/chrome/app/mash/mash_ru... chrome/app/mash/mash_runner.cc:123: for (const auto sw : new_switches) On 2017/02/08 19:19:51, James Cook wrote: > can this be const auto& ? > > I'm not sure about our style guidance for ranged-for iteration over maps. Done. https://codereview.chromium.org/2681663004/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/login/session/user_session_manager.cc (right): https://codereview.chromium.org/2681663004/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/login/session/user_session_manager.cc:300: for (const auto sw : switches) On 2017/02/08 19:19:51, James Cook wrote: > ditto Done. https://codereview.chromium.org/2681663004/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/login/session/user_session_manager.cc:312: // right here. crbug.com/678683 On 2017/02/08 19:23:39, Alexander Alekseev wrote: > 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? 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?
https://codereview.chromium.org/2681663004/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/login/session/user_session_manager.cc (right): https://codereview.chromium.org/2681663004/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/login/session/user_session_manager.cc:312: // right here. crbug.com/678683 On 2017/02/08 20:39:24, mfomitchev wrote: > On 2017/02/08 19:23:39, Alexander Alekseev wrote: > > 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? > > 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? 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.
+1 on landing this CL the way it is. James, is there a doc or an issue discussing session manager's role in mus+ash? If so, please add me to it, I'd be very interested to see how we plan on handling session start/end/lock/restart/crashes etc when running on a device.
user_session_manager.cc lgtm https://codereview.chromium.org/2681663004/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/login/session/user_session_manager.cc (right): https://codereview.chromium.org/2681663004/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/login/session/user_session_manager.cc:312: // right here. crbug.com/678683 On 2017/02/08 20:39:24, mfomitchev wrote: > On 2017/02/08 19:23:39, Alexander Alekseev wrote: > > 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? > > 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? 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".
On 2017/02/08 21:41:57, Alexander Alekseev wrote: > user_session_manager.cc lgtm > > https://codereview.chromium.org/2681663004/diff/80001/chrome/browser/chromeos... > File chrome/browser/chromeos/login/session/user_session_manager.cc (right): > > https://codereview.chromium.org/2681663004/diff/80001/chrome/browser/chromeos... > chrome/browser/chromeos/login/session/user_session_manager.cc:312: // right > here. crbug.com/678683 > On 2017/02/08 20:39:24, mfomitchev wrote: > > On 2017/02/08 19:23:39, Alexander Alekseev wrote: > > > 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? > > > > 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? > > 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". I don't have a design doc yet. I should probably write one. TL;DR: chrome --mash starts a "root" process that launches ash, window server, browser. Crashes in any of those 3 processes (eventually) will result in the chrome --mash root doing exit 1 and session_manager will restart the world.
https://codereview.chromium.org/2681663004/diff/100001/chrome/app/mash/mash_r... File chrome/app/mash/mash_runner.cc (right): https://codereview.chromium.org/2681663004/diff/100001/chrome/app/mash/mash_r... chrome/app/mash/mash_runner.cc:123: for (const auto& sw : new_switches) optional: don't use auto, it hurts readability here. https://codereview.chromium.org/2681663004/diff/100001/chrome/common/chrome_s... File chrome/common/chrome_switches.h (right): https://codereview.chromium.org/2681663004/diff/100001/chrome/common/chrome_s... chrome/common/chrome_switches.h:169: extern const char kMash[]; Should we wrap this in BUILDFLAG(ENABLE_PACKAGE_MASH_SERVICES)?
https://codereview.chromium.org/2681663004/diff/100001/chrome/app/mash/mash_r... File chrome/app/mash/mash_runner.cc (right): https://codereview.chromium.org/2681663004/diff/100001/chrome/app/mash/mash_r... chrome/app/mash/mash_runner.cc:123: for (const auto& sw : new_switches) On 2017/02/08 23:30:36, sky wrote: > optional: don't use auto, it hurts readability here. Done. https://codereview.chromium.org/2681663004/diff/100001/chrome/common/chrome_s... File chrome/common/chrome_switches.h (right): https://codereview.chromium.org/2681663004/diff/100001/chrome/common/chrome_s... chrome/common/chrome_switches.h:169: extern const char kMash[]; On 2017/02/08 23:30:36, sky wrote: > Should we wrap this in BUILDFLAG(ENABLE_PACKAGE_MASH_SERVICES)? Okay. I've updated about_flags.cc and user_session_manager.cc as well - I assume we want to be able to turn it off on CrOS if needed?
LGTM https://codereview.chromium.org/2681663004/diff/100001/chrome/common/chrome_s... File chrome/common/chrome_switches.h (right): https://codereview.chromium.org/2681663004/diff/100001/chrome/common/chrome_s... chrome/common/chrome_switches.h:169: extern const char kMash[]; On 2017/02/09 01:10:33, mfomitchev wrote: > On 2017/02/08 23:30:36, sky wrote: > > Should we wrap this in BUILDFLAG(ENABLE_PACKAGE_MASH_SERVICES)? > > Okay. I've updated about_flags.cc and user_session_manager.cc as well - I assume > we want to be able to turn it off on CrOS if needed? I'm concerned with non-chromeos code. For chromeos code it's fine to assume ENABLE_PACKAGE_MASH_SERVICES is true. https://codereview.chromium.org/2681663004/diff/120001/chrome/app/mash/mash_r... File chrome/app/mash/mash_runner.cc (right): https://codereview.chromium.org/2681663004/diff/120001/chrome/app/mash/mash_r... chrome/app/mash/mash_runner.cc:124: new_switches) {}
https://codereview.chromium.org/2681663004/diff/120001/chrome/app/mash/mash_r... File chrome/app/mash/mash_runner.cc (right): https://codereview.chromium.org/2681663004/diff/120001/chrome/app/mash/mash_r... chrome/app/mash/mash_runner.cc:124: new_switches) On 2017/02/09 17:20:48, sky wrote: > {} Done.
The CQ bit was checked by mfomitchev@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from steel@chromium.org, jamescook@chromium.org, alemate@chromium.org Link to the patchset: https://codereview.chromium.org/2681663004/#ps120001 (title: "Addressing feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by mfomitchev@chromium.org
The CQ bit was checked by mfomitchev@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from steel@chromium.org, jamescook@chromium.org, sky@chromium.org, alemate@chromium.org Link to the patchset: https://codereview.chromium.org/2681663004/#ps140001 (title: "Adding braces.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1486674798804600,
"parent_rev": "67f3f47d2661a6354199df01384607474bdd9fb1", "commit_rev":
"ca35d83a3e75630536fc1065068098ee1699877f"}
Message was sent while issue was closed.
Description was changed from ========== Mustash: Ignore --mash flag when comparing flags in UserSessionManager. When running Mustash, the flags stored in Profile Prefs will contain --mash, while the browser command line will not (and should not) have it. Also, the browser restart functionality doesn't currently work in Mustash. So ignore --mash flag when comparing flags in UserSessionManager, and if there are other mismatches when running Mustash - just crash right away. Also adding the --mash flag to chrome_switches, since we are now using it in a few different places. BUG=NONE ========== to ========== Mustash: Ignore --mash flag when comparing flags in UserSessionManager. When running Mustash, the flags stored in Profile Prefs will contain --mash, while the browser command line will not (and should not) have it. Also, the browser restart functionality doesn't currently work in Mustash. So ignore --mash flag when comparing flags in UserSessionManager, and if there are other mismatches when running Mustash - just crash right away. Also adding the --mash flag to chrome_switches, since we are now using it in a few different places. BUG=NONE Review-Url: https://codereview.chromium.org/2681663004 Cr-Commit-Position: refs/heads/master@{#449442} Committed: https://chromium.googlesource.com/chromium/src/+/ca35d83a3e75630536fc10650680... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/ca35d83a3e75630536fc10650680...
Message was sent while issue was closed.
fwang@igalia.com changed reviewers: + fwang@igalia.com
Message was sent while issue was closed.
https://codereview.chromium.org/2681663004/diff/140001/chrome/browser/about_f... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2681663004/diff/140001/chrome/browser/about_f... chrome/browser/about_flags.cc:932: #endif // BUILDFLAG(ENABLE_PACKAGE_MASH_SERVICES) This change broke the LinuxOS/Ozone build bot (https://build.chromium.org/p/chromium.fyi/builders/Ozone%20Linux) which currently enable package mash services. For consistency, I guess we should either leave them inside the #ifdef or (probably better) also move them outside the chromeos conditional in generated_resources.grd.
Message was sent while issue was closed.
On 2017/02/10 07:22:33, fwang wrote: > For consistency, I guess we should > either leave them inside the #ifdef or (probably better) also move them outside > the chromeos conditional in generated_resources.grd. Done in https://codereview.chromium.org/2687293002/
Message was sent while issue was closed.
https://codereview.chromium.org/2681663004/diff/140001/chrome/browser/about_f... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2681663004/diff/140001/chrome/browser/about_f... chrome/browser/about_flags.cc:932: #endif // BUILDFLAG(ENABLE_PACKAGE_MASH_SERVICES) Thanks for looking into this! |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
