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

Issue 799627 link

Starred by 2 users
Status: Verified
Owner:
█ lhchavez@chromium.org
Last visit 15 days ago
Closed: Jan 2018
Cc:
cmtm@chromium.org
yusukes@chromium.org
hidehiko@chromium.org
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Enterprise-related Android data removal is broken for mini-instance

Project Member Reported by lhchavez@chromium.org, Jan 5 2018 
Sample run:

https://stainless.corp.google.com/search?view=list&first_date=20180103&last_date=20180105&suite=%5Ebvt%5C-perbuild%24&test=%5Echeets_EnterpriseLogin&build=%5ER65%5C-10280%5C.0%5C.0%24&status=FAIL&status=ERROR&status=ABORT&exclude_cts=true&exclude_not_run=false&exclude_non_release=true&exclude_au=true&exclude_acts=true&exclude_retried=true&exclude_non_production=true

What happens:

* Shortly after Chrome starts, the login-prompt-visible Upstart signal is broadcast and ArcSessionRunner decides by itself that it should start the mini-instance[1]. The mini-instance happily waits until the upgrade is requested.
* The enterprise login flow happens and since it is a managed profile, it requests the data to be removed[2].
* ArcSessionManager sees the request and complies, because it is blissfully unaware that ArcSessionRunner decided by itself to start the container[3]. It then sends a signal to session_manager to clean up the Android state.
* session_manager balks at the request, since (from its perspective) Android is already running[4].

We need to allow session_manager to remove the data if the container is stateless.

1: https://chromium.googlesource.com/chromium/src/+/e6bd6cb0e492a3fe69145cdb0bc476432ab0a85c/components/arc/arc_session_runner.cc#331
2: https://chromium.googlesource.com/chromium/src/+/80c88c5f2e38809780dad854b33447d8041eec61/chrome/browser/chromeos/arc/arc_play_store_enabled_preference_handler.cc#69
3: https://chromium.googlesource.com/chromium/src/+/80c88c5f2e38809780dad854b33447d8041eec61/chrome/browser/chromeos/arc/arc_session_manager.cc#692
4: https://chromium.git.corp.google.com/chromiumos/platform2/+/2facb026e69de03835f66825d1e4d863cc7f1af1/login_manager/session_manager_impl.cc#1327

Comment 1 by lhchavez@chromium.org, Jan 5 2018 

At some point I also saw that there was a bit of code that caused the upgrade to fail when we were in this state, but now I'm not sure anymore. In any case, centralizing the knowledge of whether a container (be it the mini or the full one) is running to ArcSessionManager seems like a good first step.
Project Member

Comment 2 by bugdroid1@chromium.org, Jan 8 2018 

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

commit 6089a827ccdcaa29443fef2b8655c08f6bfb78fb
Author: Luis Hector Chavez <lhchavez@google.com>
Date: Mon Jan 08 18:41:05 2018

arc: Give ArcSessionManager full ownership of ArcSessionRunner::RequestStart()

This change moves 100% of the invocations of
ArcSessionRunner::RequestStart() to the ArcSessionManager class. This
makes the code a bit easier to reason about since there'll be no spooky
action-at-a-distance.

Bug:  799627 
Test: components_unittests, unit_tests
Change-Id: I564faf2ee855cffb727ca268f50a22c293c81d8b
Reviewed-on: https://chromium-review.googlesource.com/853203
Commit-Queue: Luis Hector Chavez <lhchavez@chromium.org>
Reviewed-by: Yusuke Sato <yusukes@chromium.org>
Reviewed-by: Hidehiko Abe <hidehiko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#527684}
[modify] https://crrev.com/6089a827ccdcaa29443fef2b8655c08f6bfb78fb/chrome/browser/chromeos/arc/arc_session_manager.cc
[modify] https://crrev.com/6089a827ccdcaa29443fef2b8655c08f6bfb78fb/chrome/browser/chromeos/arc/arc_session_manager.h
[modify] https://crrev.com/6089a827ccdcaa29443fef2b8655c08f6bfb78fb/chrome/browser/chromeos/arc/arc_session_manager_unittest.cc
[modify] https://crrev.com/6089a827ccdcaa29443fef2b8655c08f6bfb78fb/components/arc/arc_session_runner.cc
[modify] https://crrev.com/6089a827ccdcaa29443fef2b8655c08f6bfb78fb/components/arc/arc_session_runner.h
[modify] https://crrev.com/6089a827ccdcaa29443fef2b8655c08f6bfb78fb/components/arc/arc_session_runner_unittest.cc
Project Member

Comment 3 by bugdroid1@chromium.org, Jan 9 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/21520a56f5c6696b157649def33eb41ab2cd0651

commit 21520a56f5c6696b157649def33eb41ab2cd0651
Author: Luis Hector Chavez <lhchavez@google.com>
Date: Tue Jan 09 01:22:58 2018

login: Allow ARC to remove its state if it is running statelessly

This change allows RemoveArcData to be called if it is running in a
stateless mode. Enterprise-managed devices will try to remove the ARC
state after sign-in, when the stateless container is running, but before
the container is upgraded.

BUG= chromium:799627 
TEST=FEATURES="test" emerge-${BOARD} chromeos-login

Change-Id: Ie68a9ac9491e2285aae19fdedb232c6d84485a50
Reviewed-on: https://chromium-review.googlesource.com/853165
Commit-Ready: Luis Hector Chavez <lhchavez@chromium.org>
Tested-by: Luis Hector Chavez <lhchavez@chromium.org>
Reviewed-by: Yusuke Sato <yusukes@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>

[modify] https://crrev.com/21520a56f5c6696b157649def33eb41ab2cd0651/login_manager/container_manager_interface.h
[modify] https://crrev.com/21520a56f5c6696b157649def33eb41ab2cd0651/login_manager/fake_container_manager.h
[modify] https://crrev.com/21520a56f5c6696b157649def33eb41ab2cd0651/login_manager/session_manager_impl.cc
[modify] https://crrev.com/21520a56f5c6696b157649def33eb41ab2cd0651/login_manager/session_manager_impl_unittest.cc
[modify] https://crrev.com/21520a56f5c6696b157649def33eb41ab2cd0651/login_manager/container_manager_impl.h
[modify] https://crrev.com/21520a56f5c6696b157649def33eb41ab2cd0651/login_manager/container_manager_impl.cc
[modify] https://crrev.com/21520a56f5c6696b157649def33eb41ab2cd0651/login_manager/fake_container_manager.cc

Comment 4 by bartfab@chromium.org, Jan 9 2018 

The most recent commit broke -arcnext. AndroidOciWrapper does not implement the new GetStatefulMode() method, causing this line to barf:

https://chromium.git.corp.google.com/chromiumos/platform2/+/master/login_manager/session_manager_service.cc#133
Project Member

Comment 6 by bugdroid1@chromium.org, Jan 10 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/ced4e754a5fb738f6aa284928656f1f2faa01309

commit ced4e754a5fb738f6aa284928656f1f2faa01309
Author: Bartosz Fabianowski <bartfab@chromium.org>
Date: Wed Jan 10 00:48:50 2018

login: Allow ARC to remove its state if it is running statelessly

This is a follow-up to Ie68a9ac9491e2285aae19fdedb232c6d84485a50
that fixes the build on -arcnext boards.

BUG= chromium:799627 
TEST=FEATURES="test" emerge-${BOARD} chromeos-login

Change-Id: Ib289fa0c2ba64ae3d91a6ff38274f4f9cd03914a
Reviewed-on: https://chromium-review.googlesource.com/857458
Commit-Ready: Bartosz Fabianowski <bartfab@chromium.org>
Tested-by: Luis Hector Chavez <lhchavez@chromium.org>
Reviewed-by: Luis Hector Chavez <lhchavez@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>

[modify] https://crrev.com/ced4e754a5fb738f6aa284928656f1f2faa01309/login_manager/android_oci_wrapper.h
[modify] https://crrev.com/ced4e754a5fb738f6aa284928656f1f2faa01309/login_manager/android_oci_wrapper.cc

Comment 7 by lhchavez@chromium.org, Jan 10 2018

Status: Verified (was: Started)
We have lots of green builds today :D
►

Sign in to add a comment