Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2788383003:
ChromeOS: Disable ARC when incompatible filesystem is detected. (Closed)
|
Created:
3 years, 8 months ago by kinaba Modified:
3 years, 8 months ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, alemate+watch_chromium.org, oshima+watch_chromium.org, yusukes+watch_chromium.org, hidehiko+watch_chromium.org, achuith+watch_chromium.org, lhchavez+watch_chromium.org, victorhsieh+watch_chromium.org, davemoore+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionChromeOS: Disable ARC when incompatible filesystem is detected.
Checks both the user data dir's filesystem and Android version to
determine the status.
The tricky case is when the browser is crashed and restarted.
In that case Profile creation does not go through UserSessionManager.
There are three cases to consider:
* If the browser has crashed after flushing the pref, restart session
correctly reads the value.
* If the browser has crashed before flushing the pref in the first
sign-in after getting this change, the default value "false" is read.
* If the browser has crashed before flushing the pref in subsequent
runs, it reads the previously set value--which is safe. Since we
never do backward (true->false) migration. We don't see false-positive
even though we may encounter false-negative (incorrectly disabling ARC.)
Note that the assumption of the 3rd case is valid because we do the
ARC version check separately. We may do ARC uprev that can turn the
disabling condition true->false as a whole, but that does not revert
the filesystem check result that is stored in pref.
BUG=699850
TEST=Manually tested all fs+version combinations
Review-Url: https://codereview.chromium.org/2788383003
Cr-Commit-Position: refs/heads/master@{#462339}
Committed: https://chromium.googlesource.com/chromium/src/+/715a7d91b74fce5547c7a643a092898a7abebe50
Patch Set 1 #
Total comments: 7
Patch Set 2 : use SysInfo for /etc/lsb-release check #Patch Set 3 : AssertIOAllowed #
Total comments: 20
Patch Set 4 : Addressed review comments #
Messages
Total messages: 29 (19 generated)
The CQ bit was checked by kinaba@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...
kinaba@chromium.org changed reviewers: + hidehiko@chromium.org, uekawa@chromium.org, xiyuan@chromium.org
PTAL: +xiyuan +hidehiko +uekawa
On 2017/04/03 11:27:42, kinaba wrote: > PTAL: +xiyuan +hidehiko +uekawa (oops, I'll take care of failing tests tomorrow.)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2788383003/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_util.cc (right): https://codereview.chromium.org/2788383003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_util.cc:166: if (!base::ReadFileToString( Think lsb release info is loaded and parsed early on browser start up. Can you replace it with base::SysInfo::GetLsbReleaseValue() ? https://codereview.chromium.org/2788383003/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_util.h (right): https://codereview.chromium.org/2788383003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_util.h:68: // Returns whether ARC can run on the filesystem mounted at |path|. nit: Document this function needs to access file system and should not run on UI thread. And maybe add a DCHECK ? https://codereview.chromium.org/2788383003/diff/1/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/session/user_session_manager.cc (right): https://codereview.chromium.org/2788383003/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/login/session/user_session_manager.cc:992: base::PostTaskWithTraitsAndReplyWithResult(FROM_HERE, nit: Skip the check if prefs::kArcCompatibleFilesystemChosen is set ?
Description was changed from
==========
ChromeOS: Disable ARC when incompatible filesystem is detected.
BUG=699850
==========
to
==========
ChromeOS: Disable ARC when incompatible filesystem is detected.
Checks both the user data dir's filesystem and Android version to
determine the status.
The tricky case is when the browser is crashed and restarted.
In that case Profile creation does not go through UserSessionManager.
There are three cases to consider:
* If the browser has crashed after flushing the pref, restart session
correctly reads the value.
* If the browser has crashed before flushing the pref in the first
sign-in after getting this change, the default value "false" is read.
* If the browser has crashed before flushing the pref in subsequent
runs, it reads the previously set value--which is safe. Since we
never do backward (true->false) migration. We don't see false-positive
even though we may encounter false-negative (incorrectly disabling ARC.)
Note that the assumption of the 3rd case is valid because we do the
ARC version check separately. We may do M->N update that can turn the
disabling condition true->false as a whole, but that does not revert
the filesystem check result that is stored in pref.
BUG=699850
TEST=Manually tested ecryptfs+M, ecryptfs+N, ext4crypto+N combinations
==========
Thanks. Updated the CL https://codereview.chromium.org/2788383003/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_util.cc (right): https://codereview.chromium.org/2788383003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_util.cc:166: if (!base::ReadFileToString( On 2017/04/03 16:20:49, xiyuan wrote: > Think lsb release info is loaded and parsed early on browser start up. Can you > replace it with base::SysInfo::GetLsbReleaseValue() ? Thanks! That's very nice to know. Replaced the whole check to use GetLsbReleaseValue() and inlined to IsArcAllowedForProfile() (which resolved one corner case, see my CL description.) https://codereview.chromium.org/2788383003/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_util.h (right): https://codereview.chromium.org/2788383003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_util.h:68: // Returns whether ARC can run on the filesystem mounted at |path|. On 2017/04/03 16:20:49, xiyuan wrote: > nit: Document this function needs to access file system and should not run on UI > thread. And maybe add a DCHECK ? Added documentation, and added AssertIOAllowed() https://codereview.chromium.org/2788383003/diff/1/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/session/user_session_manager.cc (right): https://codereview.chromium.org/2788383003/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/login/session/user_session_manager.cc:992: base::PostTaskWithTraitsAndReplyWithResult(FROM_HERE, On 2017/04/03 16:20:49, xiyuan wrote: > nit: Skip the check if prefs::kArcCompatibleFilesystemChosen is set ? Profile is not created at this point, so it is difficult to skip.
The CQ bit was checked by kinaba@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: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2788383003/diff/1/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/session/user_session_manager.cc (right): https://codereview.chromium.org/2788383003/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/login/session/user_session_manager.cc:992: base::PostTaskWithTraitsAndReplyWithResult(FROM_HERE, On 2017/04/04 04:13:56, kinaba wrote: > On 2017/04/03 16:20:49, xiyuan wrote: > > nit: Skip the check if prefs::kArcCompatibleFilesystemChosen is set ? > > Profile is not created at this point, so it is difficult to skip. Right. Overlooked that. https://codereview.chromium.org/2788383003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_util.cc (right): https://codereview.chromium.org/2788383003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_util.cc:11: #include "base/files/file_util.h" nit: not used? https://codereview.chromium.org/2788383003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_util.cc:13: #include "base/strings/string_split.h" nit: not used?
LGTM with requesting more comments. https://codereview.chromium.org/2788383003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_util.cc (right): https://codereview.chromium.org/2788383003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_util.cc:104: if (base::SysInfo::IsRunningOnChromeOS()) { Could you add a bit more comment about IsRunningOnChromeOS? Because this is put under chromeos/ directory, so we expect this code compiles for ChromeOS. IIUC, the guard is needed for {unit,browser}tests, right? https://codereview.chromium.org/2788383003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_util.cc:106: if (!profile->GetPrefs()->GetBoolean( Could you comment the strategy here? We're making sure; - Underlying file system is compatible with ARC. Or - SDK version is 23 (meaning M). Optional: Also, it may be good to briefly mention about the function who and/or when kArcCompatibileFilesystemChosen value is set? https://codereview.chromium.org/2788383003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_util.cc:111: VLOG(1) << "Users postponed to migrate to dircrypto are not supported."; nit/optional: how about output arc_sdk_version here? https://codereview.chromium.org/2788383003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_util.h (right): https://codereview.chromium.org/2788383003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_util.h:69: // This function should not run on UI/IO threads since it accesses files. Optional: how about "This function should run on the thread where IO operation is allowed." to be consistent with your code a bit more?
some questions https://codereview.chromium.org/2788383003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_util.cc (right): https://codereview.chromium.org/2788383003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_util.cc:109: &arc_sdk_version) || If CHROMEOS_ARC_ANDROID_SDK_VERSION is not there, we assume Android isn't on this board? So this path is slightly different. https://codereview.chromium.org/2788383003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_util.cc:110: arc_sdk_version != "23")) { can you name this constant, like: constexpr std::string kAndroidMSdkVersion("23"); https://codereview.chromium.org/2788383003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/login/session/user_session_manager.cc (right): https://codereview.chromium.org/2788383003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/login/session/user_session_manager.cc:990: ProfileHelper::GetProfilePathByUserIdHash(user_context_.GetUserIDHash()); QQ: what directory is this? https://codereview.chromium.org/2788383003/diff/40001/chrome/common/pref_name... File chrome/common/pref_names.cc (right): https://codereview.chromium.org/2788383003/diff/40001/chrome/common/pref_name... chrome/common/pref_names.cc:50: // A preference that indicates am ARC comaptible filesystem was chosen for s/am/an/
Description was changed from
==========
ChromeOS: Disable ARC when incompatible filesystem is detected.
Checks both the user data dir's filesystem and Android version to
determine the status.
The tricky case is when the browser is crashed and restarted.
In that case Profile creation does not go through UserSessionManager.
There are three cases to consider:
* If the browser has crashed after flushing the pref, restart session
correctly reads the value.
* If the browser has crashed before flushing the pref in the first
sign-in after getting this change, the default value "false" is read.
* If the browser has crashed before flushing the pref in subsequent
runs, it reads the previously set value--which is safe. Since we
never do backward (true->false) migration. We don't see false-positive
even though we may encounter false-negative (incorrectly disabling ARC.)
Note that the assumption of the 3rd case is valid because we do the
ARC version check separately. We may do M->N update that can turn the
disabling condition true->false as a whole, but that does not revert
the filesystem check result that is stored in pref.
BUG=699850
TEST=Manually tested ecryptfs+M, ecryptfs+N, ext4crypto+N combinations
==========
to
==========
ChromeOS: Disable ARC when incompatible filesystem is detected.
Checks both the user data dir's filesystem and Android version to
determine the status.
The tricky case is when the browser is crashed and restarted.
In that case Profile creation does not go through UserSessionManager.
There are three cases to consider:
* If the browser has crashed after flushing the pref, restart session
correctly reads the value.
* If the browser has crashed before flushing the pref in the first
sign-in after getting this change, the default value "false" is read.
* If the browser has crashed before flushing the pref in subsequent
runs, it reads the previously set value--which is safe. Since we
never do backward (true->false) migration. We don't see false-positive
even though we may encounter false-negative (incorrectly disabling ARC.)
Note that the assumption of the 3rd case is valid because we do the
ARC version check separately. We may do ARC uprev that can turn the
disabling condition true->false as a whole, but that does not revert
the filesystem check result that is stored in pref.
BUG=699850
TEST=Manually tested all fs+version combinations
==========
Thanks for the comments https://codereview.chromium.org/2788383003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_util.cc (right): https://codereview.chromium.org/2788383003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_util.cc:11: #include "base/files/file_util.h" On 2017/04/04 15:32:21, xiyuan wrote: > nit: not used? Done. https://codereview.chromium.org/2788383003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_util.cc:13: #include "base/strings/string_split.h" On 2017/04/04 15:32:21, xiyuan wrote: > nit: not used? Done. https://codereview.chromium.org/2788383003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_util.cc:104: if (base::SysInfo::IsRunningOnChromeOS()) { On 2017/04/04 18:35:12, hidehiko wrote: > Could you add a bit more comment about IsRunningOnChromeOS? > Because this is put under chromeos/ directory, so we expect this code compiles > for ChromeOS. > IIUC, the guard is needed for {unit,browser}tests, right? Done. https://codereview.chromium.org/2788383003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_util.cc:106: if (!profile->GetPrefs()->GetBoolean( On 2017/04/04 18:35:12, hidehiko wrote: > Could you comment the strategy here? We're making sure; > - Underlying file system is compatible with ARC. Or > - SDK version is 23 (meaning M). > > Optional: Also, it may be good to briefly mention about the function who and/or > when kArcCompatibileFilesystemChosen value is set? Done. https://codereview.chromium.org/2788383003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_util.cc:109: &arc_sdk_version) || On 2017/04/04 23:48:12, Junichi Uekawa wrote: > If CHROMEOS_ARC_ANDROID_SDK_VERSION is not there, we assume Android isn't on > this board? > So this path is slightly different. If Android isn't on the board at all, a check above (IsArcAvailable()) should have exited this function already. We don't need to care what to do here in that case. If Android is there but we failed to read lsb-release, something bad is happening and thus I'd like to take safer (disable ARC) branch. https://codereview.chromium.org/2788383003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_util.cc:110: arc_sdk_version != "23")) { On 2017/04/04 23:48:13, Junichi Uekawa wrote: > can you name this constant, like: > constexpr std::string kAndroidMSdkVersion("23"); Done. https://codereview.chromium.org/2788383003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_util.cc:111: VLOG(1) << "Users postponed to migrate to dircrypto are not supported."; On 2017/04/04 18:35:12, hidehiko wrote: > nit/optional: how about output arc_sdk_version here? Done. https://codereview.chromium.org/2788383003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_util.h (right): https://codereview.chromium.org/2788383003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_util.h:69: // This function should not run on UI/IO threads since it accesses files. On 2017/04/04 18:35:12, hidehiko wrote: > Optional: how about > "This function should run on the thread where IO operation is allowed." > to be consistent with your code a bit more? Done. https://codereview.chromium.org/2788383003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/login/session/user_session_manager.cc (right): https://codereview.chromium.org/2788383003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/login/session/user_session_manager.cc:990: ProfileHelper::GetProfilePathByUserIdHash(user_context_.GetUserIDHash()); On 2017/04/04 23:48:13, Junichi Uekawa wrote: > QQ: what directory is this? /home/chronos/u-$HASH https://codereview.chromium.org/2788383003/diff/40001/chrome/common/pref_name... File chrome/common/pref_names.cc (right): https://codereview.chromium.org/2788383003/diff/40001/chrome/common/pref_name... chrome/common/pref_names.cc:50: // A preference that indicates am ARC comaptible filesystem was chosen for On 2017/04/04 23:48:13, Junichi Uekawa wrote: > s/am/an/ Done.
The CQ bit was checked by kinaba@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: This issue passed the CQ dry run.
The CQ bit was checked by kinaba@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hidehiko@chromium.org, xiyuan@chromium.org Link to the patchset: https://codereview.chromium.org/2788383003/#ps60001 (title: "Addressed review comments")
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": 60001, "attempt_start_ts": 1491448161067260,
"parent_rev": "c8cb5f2da2acc2d2da66c9e31957c0474e9978e7", "commit_rev":
"715a7d91b74fce5547c7a643a092898a7abebe50"}
Message was sent while issue was closed.
Description was changed from
==========
ChromeOS: Disable ARC when incompatible filesystem is detected.
Checks both the user data dir's filesystem and Android version to
determine the status.
The tricky case is when the browser is crashed and restarted.
In that case Profile creation does not go through UserSessionManager.
There are three cases to consider:
* If the browser has crashed after flushing the pref, restart session
correctly reads the value.
* If the browser has crashed before flushing the pref in the first
sign-in after getting this change, the default value "false" is read.
* If the browser has crashed before flushing the pref in subsequent
runs, it reads the previously set value--which is safe. Since we
never do backward (true->false) migration. We don't see false-positive
even though we may encounter false-negative (incorrectly disabling ARC.)
Note that the assumption of the 3rd case is valid because we do the
ARC version check separately. We may do ARC uprev that can turn the
disabling condition true->false as a whole, but that does not revert
the filesystem check result that is stored in pref.
BUG=699850
TEST=Manually tested all fs+version combinations
==========
to
==========
ChromeOS: Disable ARC when incompatible filesystem is detected.
Checks both the user data dir's filesystem and Android version to
determine the status.
The tricky case is when the browser is crashed and restarted.
In that case Profile creation does not go through UserSessionManager.
There are three cases to consider:
* If the browser has crashed after flushing the pref, restart session
correctly reads the value.
* If the browser has crashed before flushing the pref in the first
sign-in after getting this change, the default value "false" is read.
* If the browser has crashed before flushing the pref in subsequent
runs, it reads the previously set value--which is safe. Since we
never do backward (true->false) migration. We don't see false-positive
even though we may encounter false-negative (incorrectly disabling ARC.)
Note that the assumption of the 3rd case is valid because we do the
ARC version check separately. We may do ARC uprev that can turn the
disabling condition true->false as a whole, but that does not revert
the filesystem check result that is stored in pref.
BUG=699850
TEST=Manually tested all fs+version combinations
Review-Url: https://codereview.chromium.org/2788383003
Cr-Commit-Position: refs/heads/master@{#462339}
Committed:
https://chromium.googlesource.com/chromium/src/+/715a7d91b74fce5547c7a643a092...
==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/715a7d91b74fce5547c7a643a092... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
