cros: ensure update check to complete at OOBE before allowing login

cros: ensure update check to complete at OOBE before allowing login

This CL updates the CrOS login flow to ensure the update check is
complete at OOBE before allowing the first user to log in. Without a
successful check for update, we can't be sure if there is a critical
update. To play safe and avoid missing any critical update during OOBE,
we modify the UI simply goes back to the network selection screen
instead of proceeding to the login screen. For the non-OOBE flow, we
don't want to block the users while failing to check for update, and
thus preseve the existing UI behavior to proceed to the login screen.

BUG=chromium:683424
TEST=Tested the following:
1. Under the OOBE scenario, interrupt the network connectivity when
   the UI is checking for update. Verify that the UI goes back to the
   network selection screen.
2. Repeat (1) under the non-OOBE scenario. Verify that the UI proceeds
   to the login screen.

Review-Url: https://codereview.chromium.org/2676383002
Cr-Commit-Position: refs/heads/master@{#450428}
Committed: https://chromium.googlesource.com/chromium/src/+/ec6cb24b6ad420b32f0836b8e3d4767378b90b93

[Ben Chan, 2017-02-06 18:47:53]

The CQ bit was checked by benchan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-06 18:48:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-06 20:08:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-06 20:08:48]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ben Chan, 2017-02-06 21:44:22]

Patchset #1 (id:1) has been deleted

[Ben Chan, 2017-02-06 21:44:28]

The CQ bit was checked by benchan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-06 21:45:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-06 22:12:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-06 22:12:45]

Dry run: This issue passed the CQ dry run.

[Ben Chan, 2017-02-06 22:17:12]

Description was changed from

==========
cros: ensure update check to complete at OOBE before allowing login

This CL updates the CrOS login flow to ensure the update check is
complete at OOBE before allowing the first user to log in. Without a
successful check for update, we can't be sure if there is a critical
update. To play safe and avoid missing any critical update during OOBE,
we modify the UI simply goes back to the network selection screen
instead of proceeding to the login screen. For the non-OOBE flow, we
don't want to block the users while failing to check for update, and
thus preseve the existing UI behavior to proceed to the login screen.

BUG=chromium:683424
TEST=Tested the following:
1. Under the OOBE scenario, interrupt the network connectivity when
   the UI is checking for update. Verify that the UI goes back to the
   network selection screen.
2. Repeat (1) under the non-OOBE scenario. Verify that the UI proceeds
   to the login screen.
==========

to

==========
cros: ensure update check to complete at OOBE before allowing login

This CL updates the CrOS login flow to ensure the update check is
complete at OOBE before allowing the first user to log in. Without a
successful check for update, we can't be sure if there is a critical
update. To play safe and avoid missing any critical update during OOBE,
we modify the UI simply goes back to the network selection screen
instead of proceeding to the login screen. For the non-OOBE flow, we
don't want to block the users while failing to check for update, and
thus preseve the existing UI behavior to proceed to the login screen.

BUG=chromium:683424
TEST=Tested the following:
1. Under the OOBE scenario, interrupt the network connectivity when
   the UI is checking for update. Verify that the UI goes back to the
   network selection screen.
2. Repeat (1) under the non-OOBE scenario. Verify that the UI proceeds
   to the login screen.
==========

[Ben Chan, 2017-02-06 22:17:12]

benchan@chromium.org changed reviewers:
+ achuith@chromium.org

[achuithb, 2017-02-06 22:44:03]

Are you sure this doesn't break developer flow?

Have you tried creating an image locally, flashing it onto a DUT and logging in?
Doesn't the update check fail for this case?

[Ben Chan, 2017-02-10 16:49:18]

On 2017/02/06 22:44:03, achuithb wrote:
> Are you sure this doesn't break developer flow?
> 
> Have you tried creating an image locally, flashing it onto a DUT and logging
in?
> Doesn't the update check fail for this case?

I tried building a local image as you suggested. With a non-official build, the
update screen could be interrupted/bypassed by pressing ESC key. With an
official build (e.g. built with release trybot), the update check went through
as long as there is network connectivity and proceeded to the login screen.

[achuithb, 2017-02-13 14:02:12]

On 2017/02/10 16:49:18, Ben Chan wrote:
> On 2017/02/06 22:44:03, achuithb wrote:
> > Are you sure this doesn't break developer flow?
> > 
> > Have you tried creating an image locally, flashing it onto a DUT and logging
> in?
> > Doesn't the update check fail for this case?
> 
> I tried building a local image as you suggested. With a non-official build,
the
> update screen could be interrupted/bypassed by pressing ESC key. With an
> official build (e.g. built with release trybot), the update check went through
> as long as there is network connectivity and proceeded to the login screen.

Also could you please check that local linux builds with chromeos=1 continue to
work.

lgtm

[Ben Chan, 2017-02-13 18:44:38]

Patchset #2 (id:40001) has been deleted

[Ben Chan, 2017-02-13 19:19:24]

The CQ bit was checked by benchan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-13 19:20:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ben Chan, 2017-02-13 19:54:38]

On 2017/02/13 14:02:12, achuithb wrote:
> On 2017/02/10 16:49:18, Ben Chan wrote:
> > On 2017/02/06 22:44:03, achuithb wrote:
> > > Are you sure this doesn't break developer flow?
> > > 
> > > Have you tried creating an image locally, flashing it onto a DUT and
logging
> > in?
> > > Doesn't the update check fail for this case?
> > 
> > I tried building a local image as you suggested. With a non-official build,
> the
> > update screen could be interrupted/bypassed by pressing ESC key. With an
> > official build (e.g. built with release trybot), the update check went
through
> > as long as there is network connectivity and proceeded to the login screen.
> 
> Also could you please check that local linux builds with chromeos=1 continue
to
> work.
> 
> lgtm

Tested running a local linux build with target_os="chromeos".

[commit-bot: I haz the power, 2017-02-13 22:25:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-13 22:25:44]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ben Chan, 2017-02-14 02:10:13]

The CQ bit was checked by benchan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-14 02:10:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-14 03:36:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-14 03:36:41]

Dry run: This issue passed the CQ dry run.

[Ben Chan, 2017-02-14 03:59:52]

On 2017/02/13 19:54:38, Ben Chan wrote:
> On 2017/02/13 14:02:12, achuithb wrote:
> > On 2017/02/10 16:49:18, Ben Chan wrote:
> > > On 2017/02/06 22:44:03, achuithb wrote:
> > > > Are you sure this doesn't break developer flow?
> > > > 
> > > > Have you tried creating an image locally, flashing it onto a DUT and
> logging
> > > in?
> > > > Doesn't the update check fail for this case?
> > > 
> > > I tried building a local image as you suggested. With a non-official
build,
> > the
> > > update screen could be interrupted/bypassed by pressing ESC key. With an
> > > official build (e.g. built with release trybot), the update check went
> through
> > > as long as there is network connectivity and proceeded to the login
screen.
> > 
> > Also could you please check that local linux builds with chromeos=1 continue
> to
> > work.
> > 
> > lgtm
> 
> Tested running a local linux build with target_os="chromeos".

Fixed HandsOffNetworkScreenTest.ContinueClickedOnlyOnce test failure. PTAL.

[achuithb, 2017-02-14 09:05:25]

lgtm

https://codereview.chromium.org/2676383002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/wizard_controller.cc (right):

https://codereview.chromium.org/2676383002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/login/wizard_controller.cc:735: bool is_hands_off_mode =
nit const

https://codereview.chromium.org/2676383002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/login/wizard_controller.cc:746: if (is_out_of_box_ &&
!is_hands_off_mode)
Is hands off mode behavior covered by browser tests?

[Ben Chan, 2017-02-14 18:31:11]

https://codereview.chromium.org/2676383002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/wizard_controller.cc (right):

https://codereview.chromium.org/2676383002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/login/wizard_controller.cc:735: bool is_hands_off_mode =
On 2017/02/14 09:05:25, achuithb wrote:
> nit const

Done.

https://codereview.chromium.org/2676383002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/login/wizard_controller.cc:746: if (is_out_of_box_ &&
!is_hands_off_mode)
On 2017/02/14 09:05:25, achuithb wrote:
> Is hands off mode behavior covered by browser tests?

It's indirectly covered by those HandsOff*Test tests (which was initially broken
by this change). I'll evaluate if additional tests should be added in a
follow-up CL.

[Ben Chan, 2017-02-14 18:31:20]

The CQ bit was checked by benchan@chromium.org

[Ben Chan, 2017-02-14 18:31:21]

The patchset sent to the CQ was uploaded after l-g-t-m from achuith@chromium.org
Link to the patchset: https://codereview.chromium.org/2676383002/#ps80001
(title: "cros: ensure update check to complete at OOBE before allowing login")

[commit-bot: I haz the power, 2017-02-14 18:32:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-14 19:16:07]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1487097080550520,
"parent_rev": "509840116ed907fb1e466c7ca0a35eb76f0eb6d5", "commit_rev":
"ec6cb24b6ad420b32f0836b8e3d4767378b90b93"}

[commit-bot: I haz the power, 2017-02-14 19:16:44]

Description was changed from

==========
cros: ensure update check to complete at OOBE before allowing login

This CL updates the CrOS login flow to ensure the update check is
complete at OOBE before allowing the first user to log in. Without a
successful check for update, we can't be sure if there is a critical
update. To play safe and avoid missing any critical update during OOBE,
we modify the UI simply goes back to the network selection screen
instead of proceeding to the login screen. For the non-OOBE flow, we
don't want to block the users while failing to check for update, and
thus preseve the existing UI behavior to proceed to the login screen.

BUG=chromium:683424
TEST=Tested the following:
1. Under the OOBE scenario, interrupt the network connectivity when
   the UI is checking for update. Verify that the UI goes back to the
   network selection screen.
2. Repeat (1) under the non-OOBE scenario. Verify that the UI proceeds
   to the login screen.
==========

to

==========
cros: ensure update check to complete at OOBE before allowing login

This CL updates the CrOS login flow to ensure the update check is
complete at OOBE before allowing the first user to log in. Without a
successful check for update, we can't be sure if there is a critical
update. To play safe and avoid missing any critical update during OOBE,
we modify the UI simply goes back to the network selection screen
instead of proceeding to the login screen. For the non-OOBE flow, we
don't want to block the users while failing to check for update, and
thus preseve the existing UI behavior to proceed to the login screen.

BUG=chromium:683424
TEST=Tested the following:
1. Under the OOBE scenario, interrupt the network connectivity when
   the UI is checking for update. Verify that the UI goes back to the
   network selection screen.
2. Repeat (1) under the non-OOBE scenario. Verify that the UI proceeds
   to the login screen.

Review-Url: https://codereview.chromium.org/2676383002
Cr-Commit-Position: refs/heads/master@{#450428}
Committed:
https://chromium.googlesource.com/chromium/src/+/ec6cb24b6ad420b32f0836b8e3d4...
==========

[commit-bot: I haz the power, 2017-02-14 19:16:45]

Committed patchset #3 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/ec6cb24b6ad420b32f0836b8e3d4...

[achuithb, 2017-02-16 14:15:39]

A revert of this CL (patchset #3 id:80001) has been created in
https://codereview.chromium.org/2699633004/ by achuith@chromium.org.

The reason for reverting is: Believe this is causing a regression in linux
builds.

[achuithb, 2017-02-16 14:20:05]

On 2017/02/13 14:02:12, achuithb wrote:
> On 2017/02/10 16:49:18, Ben Chan wrote:
> > On 2017/02/06 22:44:03, achuithb wrote:
> > > Are you sure this doesn't break developer flow?
> > > 
> > > Have you tried creating an image locally, flashing it onto a DUT and
logging
> > in?
> > > Doesn't the update check fail for this case?
> > 
> > I tried building a local image as you suggested. With a non-official build,
> the
> > update screen could be interrupted/bypassed by pressing ESC key. With an
> > official build (e.g. built with release trybot), the update check went
through
> > as long as there is network connectivity and proceeded to the login screen.
> 
> Also could you please check that local linux builds with chromeos=1 continue
to
> work.

Not sure how this was tested, but it's pretty clearly broken. Clicking on the
'Next' button on the network screen does nothing. This is a pretty serious
blocker for chromeos developers, so I'm going to revert this.