UserPolicyManagerFactoryChromeOS: Elaborate comment

UserPolicyManagerFactoryChromeOS: Elaborate comment

BUG=722374
Review-Url: https://codereview.chromium.org/2891933002
Cr-Commit-Position: refs/heads/master@{#478598}
Committed: https://chromium.googlesource.com/chromium/src/+/bf77209685ba07195bf0c0ba84c2e1d1a78f4491

[Thiemo Nagel, 2017-05-18 15:12:33]

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

[Thiemo Nagel, 2017-05-18 15:12:50]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2017-05-18 15:13:04]

Dry run: CQ is trying da patch.

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

[Thiemo Nagel, 2017-05-18 15:14:46]

Description was changed from

==========
UserPolicyManagerFactoryChromeOS: Prevent unknown account types

BUG=722374
==========

to

==========
UserPolicyManagerFactoryChromeOS: Prevent unknown account types

At the time of login, account time must not be unknown.  Adding CHECK()
to rule out the possibility of a policy escape.

BUG=722374
==========

[Thiemo Nagel, 2017-05-18 15:15:02]

Description was changed from

==========
UserPolicyManagerFactoryChromeOS: Prevent unknown account types

At the time of login, account time must not be unknown.  Adding CHECK()
to rule out the possibility of a policy escape.

BUG=722374
==========

to

==========
UserPolicyManagerFactoryChromeOS: Prevent unknown account types

At the time of login, account time must not be unknown.  Adding CHECK()
to rule out the possibility of a policy escape and to clarify the code.

BUG=722374
==========

[Thiemo Nagel, 2017-05-18 15:18:46]

tnagel@chromium.org changed reviewers:
+ atwilson@chromium.org, rsorokin@google.com

[Thiemo Nagel, 2017-05-18 15:18:47]

Roman, Drew, could you please take a look?

Thank you,
Thiemo

[Andrew T Wilson (Slow), 2017-05-18 15:26:03]

https://codereview.chromium.org/2891933002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/user_policy_manager_factory_chromeos.cc
(right):

https://codereview.chromium.org/2891933002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_policy_manager_factory_chromeos.cc:180:
CHECK(false);
Is CHECK the right thing here, nor AttemptUserExit() or NOTREACHED? I'm somewhat
unclear as to what NOTREACHED does in production code.

Is there any chance of a reboot loop if you just CHECK here?

[commit-bot: I haz the power, 2017-05-18 15:51:37]

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

[commit-bot: I haz the power, 2017-05-18 15:51:38]

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_...)

[Thiemo Nagel, 2017-05-18 16:00:25]

https://codereview.chromium.org/2891933002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/user_policy_manager_factory_chromeos.cc
(right):

https://codereview.chromium.org/2891933002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_policy_manager_factory_chromeos.cc:180:
CHECK(false);
On 2017/05/18 15:26:03, Andrew T Wilson (Slow) wrote:
> Is CHECK the right thing here, nor AttemptUserExit() or NOTREACHED?

I guess AttemptUserExit() is just as good, except maybe that CHECK() acts with
greater prejudice (I'm slightly put off by the "Attempt" wording) and also
conceptually this is something that (supposedly) is prevented by program logic
and thus CHECK() would seem like the correct way to express that.

> I'm somewhat unclear as to what NOTREACHED does in production code.

In production it just logs an error.
 
> Is there any chance of a reboot loop if you just CHECK here?

I don't think so because for the login profile we bail above.

[rsorokin_google.com, 2017-05-18 16:01:16]

rsorokin@google.com changed reviewers:
+ rsorokin@chromium.org
- rsorokin@google.com

[Roman Sorokin (ftl), 2017-05-18 16:18:56]

lgtm

[Thiemo Nagel, 2017-05-29 09:49:16]

Drew, friendly ping?

[Andrew T Wilson (Slow), 2017-05-29 12:07:10]

lgtm

[Thiemo Nagel, 2017-06-02 11:58:34]

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

[commit-bot: I haz the power, 2017-06-02 11:58: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-06-02 12:47:28]

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

[commit-bot: I haz the power, 2017-06-02 12:47:29]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Thiemo Nagel, 2017-06-09 12:29:21]

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

[commit-bot: I haz the power, 2017-06-09 12:29:26]

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-06-09 13:47:30]

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

[commit-bot: I haz the power, 2017-06-09 13:47:31]

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_...)

[Thiemo Nagel, 2017-06-09 14:11:24]

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

[commit-bot: I haz the power, 2017-06-09 14:11:37]

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-06-09 15:19:46]

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

[commit-bot: I haz the power, 2017-06-09 15:19:48]

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_...)

[Thiemo Nagel, 2017-06-12 09:09:45]

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

[commit-bot: I haz the power, 2017-06-12 09:09:59]

Dry run: CQ is trying da patch.

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

[Thiemo Nagel, 2017-06-12 09:09:59]

Patchset #2 (id:40001) has been deleted

[Thiemo Nagel, 2017-06-12 09:10:10]

Patchset #2 (id:60001) has been deleted

[Thiemo Nagel, 2017-06-12 09:10:21]

Patchset #2 (id:80001) has been deleted

[Thiemo Nagel, 2017-06-12 09:12:07]

Roman, ptal.

It turns out that KioskAppManager hasn't been migrated yet.  Thus right now, we
can't CHECK() on unknown account types.

[Thiemo Nagel, 2017-06-12 09:24:59]

Description was changed from

==========
UserPolicyManagerFactoryChromeOS: Prevent unknown account types

At the time of login, account time must not be unknown.  Adding CHECK()
to rule out the possibility of a policy escape and to clarify the code.

BUG=722374
==========

to

==========
UserPolicyManagerFactoryChromeOS: Elaborate comment

BUG=722374
==========

[commit-bot: I haz the power, 2017-06-12 09:39:44]

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

[commit-bot: I haz the power, 2017-06-12 09:39:45]

Dry run: This issue passed the CQ dry run.

[Roman Sorokin (ftl), 2017-06-12 12:29:33]

lgtm

[Thiemo Nagel, 2017-06-12 13:01:32]

The CQ bit was checked by tnagel@chromium.org

[Thiemo Nagel, 2017-06-12 13:01:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
atwilson@chromium.org
Link to the patchset: https://codereview.chromium.org/2891933002/#ps100001
(title: "Revert CHECK()")

[commit-bot: I haz the power, 2017-06-12 13:01:44]

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-06-12 13:06:33]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1497272492168860,
"parent_rev": "d59edeefb6bfb8e4abe9c2c2ff313baf5f47b513", "commit_rev":
"bf77209685ba07195bf0c0ba84c2e1d1a78f4491"}

[commit-bot: I haz the power, 2017-06-12 13:06:44]

Description was changed from

==========
UserPolicyManagerFactoryChromeOS: Elaborate comment

BUG=722374
==========

to

==========
UserPolicyManagerFactoryChromeOS: Elaborate comment

BUG=722374

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

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

Committed patchset #2 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/bf77209685ba07195bf0c0ba84c2...