New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 699850 link

Starred by 1 user

Issue metadata

Status: Archived
Owner:
Closed: May 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug

Blocked on:
issue 710285
issue 710289

Blocking:
issue 688900



Sign in to add a comment

Add a way to properly disable ARC when the user postponed doing ext4-crypto migration.

Project Member Reported by kinaba@chromium.org, Mar 9 2017

Issue description

We'll give a choice for the user to do the migration later,
but in that case we temporarily want to suppress ARC from running.

We need a way to cleanly achieve that.
@hidehiko, can I discuss on this some time later?
 

Comment 1 by kinaba@chromium.org, Mar 13 2017

Labels: -R-59 M-59

Comment 2 by kinaba@chromium.org, Mar 29 2017

Cc: uekawa@chromium.org
Status: Accepted (was: Assigned)
Updates after discussion with hidehiko@ and investigation after that:

The thing is, if we can let arc::IsArcAllowedForProfile()
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/arc/arc_util.cc?q=IsArcAllowedForProfil+package:%5Echromium$&l=28
return false, all ARC related stuff can be gracefully disabled. And probably this is the "only if" condition.

The question is how to do this. There are two approaches:

1) Pass some flag from the login manager to here when the user postponed migration.

The problem of this approach is the case of Chrome browser crash and restart.
When it happens, the session is restarted without going through the login manager.
Since an on-memory flag does not survive the browser crash, to cover this case we
probably need to notify the situation from session-manager to Chrome as well.

2) Let the check function itself detect the bad combination (= "N" on "ecryptfs"),
independent from the migration wizard.

The problem of this approach is that it requires disk IO or Dbus call to cryptohome.
Since we need to detect the situation in a relatively early stage of Chrome startup,
before KeyedServices of Profiles are initialized, it's not clear where to insert this
async task ping-pong.


I'm continuing to investigate both approaches.
Cc: mitsuji@chromium.org
Update:
CL for completely disabling ARC https://codereview.chromium.org/2788383003/ is getting to be in a good shape.
I'll land this first.


Request from UX/PM/Leads is that we still want to keep ARC app launcher/taskbar icons
(and inform about migration when they are clicked.) I'll proceed to this direction next.

Project Member

Comment 4 by bugdroid1@chromium.org, Apr 6 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/715a7d91b74fce5547c7a643a092898a7abebe50

commit 715a7d91b74fce5547c7a643a092898a7abebe50
Author: kinaba <kinaba@chromium.org>
Date: Thu Apr 06 03:13:45 2017

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}

[modify] https://crrev.com/715a7d91b74fce5547c7a643a092898a7abebe50/chrome/browser/chromeos/arc/arc_session_manager.cc
[modify] https://crrev.com/715a7d91b74fce5547c7a643a092898a7abebe50/chrome/browser/chromeos/arc/arc_util.cc
[modify] https://crrev.com/715a7d91b74fce5547c7a643a092898a7abebe50/chrome/browser/chromeos/arc/arc_util.h
[modify] https://crrev.com/715a7d91b74fce5547c7a643a092898a7abebe50/chrome/browser/chromeos/login/session/user_session_manager.cc
[modify] https://crrev.com/715a7d91b74fce5547c7a643a092898a7abebe50/chrome/browser/chromeos/login/session/user_session_manager.h
[modify] https://crrev.com/715a7d91b74fce5547c7a643a092898a7abebe50/chrome/common/pref_names.cc
[modify] https://crrev.com/715a7d91b74fce5547c7a643a092898a7abebe50/chrome/common/pref_names.h

Project Member

Comment 5 by bugdroid1@chromium.org, Apr 10 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/21c492878576af55fb89bf56efd9cbed20ef4eaa

commit 21c492878576af55fb89bf56efd9cbed20ef4eaa
Author: kinaba <kinaba@chromium.org>
Date: Mon Apr 10 14:18:27 2017

arc: Add IsArcAllowedInAppListForProfile.

This will be used to keep app list even when ARC is disabled due to
filesystem incompatibility. This CL just splits IsArcAllowedForProfile
for use in subsequent changes.

BUG= 699850 
TEST=ChromeArcUtilTest

Review-Url: https://codereview.chromium.org/2806443002
Cr-Commit-Position: refs/heads/master@{#463255}

[modify] https://crrev.com/21c492878576af55fb89bf56efd9cbed20ef4eaa/chrome/browser/chromeos/arc/arc_util.cc
[modify] https://crrev.com/21c492878576af55fb89bf56efd9cbed20ef4eaa/chrome/browser/chromeos/arc/arc_util.h
[modify] https://crrev.com/21c492878576af55fb89bf56efd9cbed20ef4eaa/chrome/browser/chromeos/arc/arc_util_unittest.cc

Comment 6 by kinaba@chromium.org, Apr 11 2017

Blockedon: 710285

Comment 7 by kinaba@chromium.org, Apr 11 2017

Blockedon: 710289
Project Member

Comment 8 by bugdroid1@chromium.org, Apr 11 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6cb1c9a147eff32e3bacec76f0700c6a401c44d2

commit 6cb1c9a147eff32e3bacec76f0700c6a401c44d2
Author: kinaba <kinaba@chromium.org>
Date: Tue Apr 11 02:23:27 2017

arc: Notification for guiding the user to filesystem migration.

When ARC is temporarily disabled due to filesystem incompatibility,
we want to keep the launcher/tray icons for ARC apps displayed,
and when the icons are clicked we want to guide the user for proper
reconfiguration step (i.e., sign back in and complete migration.)

This CL implements the latter "show notificaiton" part.
No actual code should hit this notification yet--after this change
I'll change several conditions in the app list code to make them alive.

BUG= 699850 

Review-Url: https://codereview.chromium.org/2813633002
Cr-Commit-Position: refs/heads/master@{#463499}

[modify] https://crrev.com/6cb1c9a147eff32e3bacec76f0700c6a401c44d2/chrome/app/chromeos_strings.grdp
[modify] https://crrev.com/6cb1c9a147eff32e3bacec76f0700c6a401c44d2/chrome/browser/chromeos/BUILD.gn
[add] https://crrev.com/6cb1c9a147eff32e3bacec76f0700c6a401c44d2/chrome/browser/chromeos/arc/arc_migration_guide_notification.cc
[add] https://crrev.com/6cb1c9a147eff32e3bacec76f0700c6a401c44d2/chrome/browser/chromeos/arc/arc_migration_guide_notification.h
[modify] https://crrev.com/6cb1c9a147eff32e3bacec76f0700c6a401c44d2/chrome/browser/ui/app_list/arc/arc_app_utils.cc

Comment 9 by uekawa@google.com, Apr 24 2017

Labels: ArcExt4Migration
Labels: -M-59 M-60
I'll rebooting the work on this from tomorrow.

Update (or clarification from some peoples point of view) after the UI review meetup:
we *want* the ARC related section in the chrome://settings as well in the skipped-session, not just app icons.
Status: Started (was: Accepted)
Status: Fixed (was: Started)
Now you'll see the icons and the notifications.
Verification steps:

1. Add a user to M59 Kevin. AU to M60.
2. Log-in to the user and skip "critical update" migration wizard.
3. Play store icon should still stay in the launcher.

Expected: Clicking the Play Store icon pops up a notification to suggest user to update & sign-out & in.
Expected: Task manager (Shift-Esc) doesn't list any Android process like /system/bin/surfaceflinger

Comment 14 by dchan@chromium.org, Jan 22 2018

Status: Archived (was: Fixed)

Sign in to add a comment