Load DeviceLocalAccount policy and DeviceSettings immediately on restore after Chrome crash.

Load DeviceLocalAccount policy and DeviceSettings immediately on restore after Chrome crash.

UserCloudPolicyManagerChromeOS and UserCloudPolicyManager are loading user policy immediately
 after Chrome crash as profile is created synchronously so that the Profile initialization
 never sees unmanaged prefs.

However, it was not true for device local accounts, such as public sessions and ARC Kiosk session,
 that means having Profile with uninitialized prefs, that particularly leads to crash/data loss
 for ARC kiosk.

This change forces policies for these accounts to also be loaded immediately on crash with blocking
 call to D-Bus, similar to loading policies for users.

note: While this change fixes the described issue, the better way is to get rid of synchronous profile
 creation, but still ensure that policy is loaded before end of profile creation. See crbug.com/160522

BUG=679810
BUG=694546
TEST=Ran tryjobs, start and restore after crash for: ARC Kiosk, Public Session, User Session.

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

[Sergey Poromov, 2017-02-22 12:58:31]

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

[commit-bot: I haz the power, 2017-02-22 12:58:44]

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

[Sergey Poromov, 2017-02-22 13:05:00]

poromov@chromium.org changed reviewers:
+ atwilson@chromium.org, emaxx@chromium.org, erg@chromium.org,
stevenjb@chromium.org, tnagel@chromium.org

[Sergey Poromov, 2017-02-22 13:05:00]

erg@chromium.org: Please review changes in 
 chrome/browser/profiles/profile_impl.cc

stevenjb@chromium.org: Please review changes in 
 chromeos/dbus/*

tnagel@chromium.org: Please review changes in
 chrome/browser/chromeos/policy/*
 chrome/browser/chromeos/settings/*
 chrome/browser/policy/*

emaxx@chromium.org, atwilson@chromium.org: Please review the change in general

[commit-bot: I haz the power, 2017-02-22 13:48:23]

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

[commit-bot: I haz the power, 2017-02-22 13:48:24]

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

[Sergey Poromov, 2017-02-22 14:21:19]

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

[commit-bot: I haz the power, 2017-02-22 14:21:34]

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-22 15:52:48]

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

[commit-bot: I haz the power, 2017-02-22 15:52:49]

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

[stevenjb, 2017-02-22 16:40:19]

chromeos/dbus owner lgtm w/ caveat.

https://codereview.chromium.org/2714493002/diff/20001/chromeos/dbus/session_m...
File chromeos/dbus/session_manager_client.h (right):

https://codereview.chromium.org/2714493002/diff/20001/chromeos/dbus/session_m...
chromeos/dbus/session_manager_client.h:160: virtual std::string
BlockingRetrieveDevicePolicy() = 0;
Since there is precedence for this already for user policy, I won't push back
against this, but can we reference crbug.com/160522 here and for all blocking
calls (or at the top of this file)? We really need a better solution.

https://codereview.chromium.org/2714493002/diff/20001/chromeos/dbus/session_m...
chromeos/dbus/session_manager_client.h:187: // call fails.
combine lines

[emaxx, 2017-02-22 17:24:34]

Could you please add some tests for the new synchronous load code?

https://codereview.chromium.org/2714493002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/device_local_account_policy_service.h
(right):

https://codereview.chromium.org/2714493002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_local_account_policy_service.h:72: //
Initialize the broker, loading its |store_|.
Worth mentioning explicitly that this actually just initiates an asynchronous
load of the store.

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

https://codereview.chromium.org/2714493002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_local_account_policy_store.cc:50: //
session. That happens when the browser crashes, or right after signin if
nit: It's a little bit unclear what "that happens" refers here to. Maybe combine
this sentence with the previous one?

https://codereview.chromium.org/2714493002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_local_account_policy_store.cc:56:
std::string policy_blob =
nit: const

https://codereview.chromium.org/2714493002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_local_account_policy_store.cc:60: status_
= CloudPolicyStore::STATUS_LOAD_ERROR;
Isn't it possible to get rid of this code duplication by sharing the common
validation code with the asynchronous load path?
They must be pretty similar to each other, except that async calls (to D-Bus, to
validator) have to be replaced with sync analogs.
I think this can be achieved by passing a boolean flag along the chain of calls,
starting, for instance, from ValidateLoadedPolicyBlob.
(BTW, this is what is done in the analogous case for
UserCloudPolicyStore::PolicyLoaded()).

https://codereview.chromium.org/2714493002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/settings/device_settings_service.cc (right):

https://codereview.chromium.org/2714493002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/settings/device_settings_service.cc:134:
std::unique_ptr<SessionManagerOperation> operation(new LoadSettingsOperation(
Should the same check for "device_mode_ == policy::DEVICE_MODE_ENTERPRISE_AD" be
done here as in DeviceSettingsService::EnqueueLoad()?

https://codereview.chromium.org/2714493002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/settings/device_settings_service.cc:313: if
(!loading_immediately_) {
Could we have a separate method for the async operation instead?
It would call the method with the common functionality (which is in lines
274..311 currently) and additionally do everything that is needed for the
asynchronous case.

https://codereview.chromium.org/2714493002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/settings/session_manager_operation.cc (right):

https://codereview.chromium.org/2714493002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/settings/session_manager_operation.cc:77: if
(public_key_.get() && public_key_->is_loaded()) {
I believe you have to honor the force_key_load_ flag too.
(BTW, the async load path also contains a bug around this flag, and the fix is
in the stalled CL: http://crrev.com/2558543003. But let's not duplicate that bug
here too.)

And also the same comment on the code duplication as above: couldn't you reuse
the existing code on this synchronous load path?
E.g. LoadPublicKey() does almost the same as you need here, and it's doing it
already synchronously.

https://codereview.chromium.org/2714493002/diff/20001/chromeos/dbus/session_m...
File chromeos/dbus/session_manager_client.cc (right):

https://codereview.chromium.org/2714493002/diff/20001/chromeos/dbus/session_m...
chromeos/dbus/session_manager_client.cc:879:
owner_key_path.DirName().AppendASCII("stub_device_policy");
nit: Maybe define a constant for this string? Or even make a separate function
for obtaining the device policy path?
(Also would be nice to resolve the duplication of "stub_policy" below too.)

https://codereview.chromium.org/2714493002/diff/20001/chromeos/dbus/session_m...
File chromeos/dbus/session_manager_client.h (right):

https://codereview.chromium.org/2714493002/diff/20001/chromeos/dbus/session_m...
chromeos/dbus/session_manager_client.h:185: // received, and
nit: Please reformat the comment.

[Elliot Glaysher, 2017-02-22 18:19:00]

profiles lgtm

[Andrew T Wilson (Slow), 2017-02-23 12:05:48]

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

https://codereview.chromium.org/2714493002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_local_account_policy_provider.cc:94:
provider->GetBroker()->LoadImmediately();
See my comment in Store->LoadImmediately() - it's a bit weird that we're issuing
both an asynchronous *and* a synchronous store load. Ideally we'd avoid the
asynchronous load if we're gonna do a sync load, or at least ignore the result
from the async load.

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

https://codereview.chromium.org/2714493002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_local_account_policy_store.cc:46: void
DeviceLocalAccountPolicyStore::LoadImmediately() {
In general, there will also be an asynchronous load in progress - do we need to
cancel that (perhaps via a call to weak_factory_.InvalidateWeakPtrs())?

https://codereview.chromium.org/2714493002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_local_account_policy_store.cc:94: //
complete.
Wait, what? But we have a unique_ptr holding |validator| so if the validator
deletes itself won't we get a double free?

https://codereview.chromium.org/2714493002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/settings/device_settings_service.cc (right):

https://codereview.chromium.org/2714493002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/settings/device_settings_service.cc:133:
loading_immediately_ = true;
Can we get rid of this data member and instead pass this in as a flag to
HandleCompletedOperation (bind it in the callback below)? It's kind of weird to
have this state flag hanging around.

https://codereview.chromium.org/2714493002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/settings/device_settings_service.cc:313: if
(!loading_immediately_) {
On 2017/02/22 17:24:33, emaxx wrote:
> Could we have a separate method for the async operation instead?
> It would call the method with the common functionality (which is in lines
> 274..311 currently) and additionally do everything that is needed for the
> asynchronous case.

Yes, I'd either do this, or else bind a boolean into this callback. But agreed
it's probably cleanest to have two callbacks.

[Sergey Poromov, 2017-02-23 16:11:24]

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

[commit-bot: I haz the power, 2017-02-23 16:11:57]

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-23 17:07:29]

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

[commit-bot: I haz the power, 2017-02-23 17:07:31]

Dry run: This issue passed the CQ dry run.

[Sergey Poromov, 2017-02-23 18:00:04]

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

[commit-bot: I haz the power, 2017-02-23 18:00:29]

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-23 19:25:55]

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

[commit-bot: I haz the power, 2017-02-23 19:25:56]

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

[Sergey Poromov, 2017-02-24 11:41:43]

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

[commit-bot: I haz the power, 2017-02-24 11:42:01]

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-24 11:44:32]

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

[commit-bot: I haz the power, 2017-02-24 11:44:33]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[Sergey Poromov, 2017-02-24 11:50:58]

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

[commit-bot: I haz the power, 2017-02-24 11:51:12]

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-24 12:30:35]

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

[commit-bot: I haz the power, 2017-02-24 12:30:36]

Dry run: This issue passed the CQ dry run.

[Sergey Poromov, 2017-02-24 13:26:35]

The CQ bit was checked by poromov@chromium.org

[Sergey Poromov, 2017-02-24 13:26:37]

The patchset sent to the CQ was uploaded after l-g-t-m from erg@chromium.org,
stevenjb@chromium.org
Link to the patchset: https://codereview.chromium.org/2714493002/#ps130001
(title: "rebase")

[commit-bot: I haz the power, 2017-02-24 13:26:59]

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-24 13:33:58]

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

[commit-bot: I haz the power, 2017-02-24 13:33:59]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Sergey Poromov, 2017-02-28 13:57:29]

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

[commit-bot: I haz the power, 2017-02-28 13:57:46]

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

[Sergey Poromov, 2017-02-28 14:01:18]

https://codereview.chromium.org/2714493002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/device_local_account_policy_service.h
(right):

https://codereview.chromium.org/2714493002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_local_account_policy_service.h:72: //
Initialize the broker, loading its |store_|.
On 2017/02/22 17:24:33, emaxx wrote:
> Worth mentioning explicitly that this actually just initiates an asynchronous
> load of the store.

Done.

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

https://codereview.chromium.org/2714493002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_local_account_policy_store.cc:46: void
DeviceLocalAccountPolicyStore::LoadImmediately() {
On 2017/02/23 12:05:48, Andrew T Wilson (Slow) wrote:
> In general, there will also be an asynchronous load in progress - do we need
to
> cancel that (perhaps via a call to weak_factory_.InvalidateWeakPtrs())?

Done.

https://codereview.chromium.org/2714493002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_local_account_policy_store.cc:50: //
session. That happens when the browser crashes, or right after signin if
On 2017/02/22 17:24:33, emaxx wrote:
> nit: It's a little bit unclear what "that happens" refers here to. Maybe
combine
> this sentence with the previous one?

Done.

https://codereview.chromium.org/2714493002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_local_account_policy_store.cc:56:
std::string policy_blob =
On 2017/02/22 17:24:33, emaxx wrote:
> nit: const

Done.

https://codereview.chromium.org/2714493002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_local_account_policy_store.cc:60: status_
= CloudPolicyStore::STATUS_LOAD_ERROR;
On 2017/02/22 17:24:33, emaxx wrote:
> Isn't it possible to get rid of this code duplication by sharing the common
> validation code with the asynchronous load path?
> They must be pretty similar to each other, except that async calls (to D-Bus,
to
> validator) have to be replaced with sync analogs.
> I think this can be achieved by passing a boolean flag along the chain of
calls,
> starting, for instance, from ValidateLoadedPolicyBlob.
> (BTW, this is what is done in the analogous case for
> UserCloudPolicyStore::PolicyLoaded()).

Done.

https://codereview.chromium.org/2714493002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_local_account_policy_store.cc:94: //
complete.
On 2017/02/23 12:05:48, Andrew T Wilson (Slow) wrote:
> Wait, what? But we have a unique_ptr holding |validator| so if the validator
> deletes itself won't we get a double free?

You are right, that was true only for StartValidation()

https://codereview.chromium.org/2714493002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/settings/device_settings_service.cc (right):

https://codereview.chromium.org/2714493002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/settings/device_settings_service.cc:133:
loading_immediately_ = true;
On 2017/02/23 12:05:48, Andrew T Wilson (Slow) wrote:
> Can we get rid of this data member and instead pass this in as a flag to
> HandleCompletedOperation (bind it in the callback below)? It's kind of weird
to
> have this state flag hanging around.

Done.

https://codereview.chromium.org/2714493002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/settings/device_settings_service.cc:134:
std::unique_ptr<SessionManagerOperation> operation(new LoadSettingsOperation(
On 2017/02/22 17:24:33, emaxx wrote:
> Should the same check for "device_mode_ == policy::DEVICE_MODE_ENTERPRISE_AD"
be
> done here as in DeviceSettingsService::EnqueueLoad()?

Expect so, but needs review from tnagel@ for ActiveDirectory part of code.

https://codereview.chromium.org/2714493002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/settings/device_settings_service.cc:313: if
(!loading_immediately_) {
On 2017/02/23 12:05:48, Andrew T Wilson (Slow) wrote:
> On 2017/02/22 17:24:33, emaxx wrote:
> > Could we have a separate method for the async operation instead?
> > It would call the method with the common functionality (which is in lines
> > 274..311 currently) and additionally do everything that is needed for the
> > asynchronous case.
> 
> Yes, I'd either do this, or else bind a boolean into this callback. But agreed
> it's probably cleanest to have two callbacks.

Done.

https://codereview.chromium.org/2714493002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/settings/device_settings_service.cc:313: if
(!loading_immediately_) {
On 2017/02/22 17:24:33, emaxx wrote:
> Could we have a separate method for the async operation instead?
> It would call the method with the common functionality (which is in lines
> 274..311 currently) and additionally do everything that is needed for the
> asynchronous case.

Done.

https://codereview.chromium.org/2714493002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/settings/session_manager_operation.cc (right):

https://codereview.chromium.org/2714493002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/settings/session_manager_operation.cc:77: if
(public_key_.get() && public_key_->is_loaded()) {
On 2017/02/22 17:24:33, emaxx wrote:
> I believe you have to honor the force_key_load_ flag too.
> (BTW, the async load path also contains a bug around this flag, and the fix is
> in the stalled CL: http://crrev.com/2558543003. But let's not duplicate that
bug
> here too.)
> 
> And also the same comment on the code duplication as above: couldn't you reuse
> the existing code on this synchronous load path?
> E.g. LoadPublicKey() does almost the same as you need here, and it's doing it
> already synchronously.

Done.

https://codereview.chromium.org/2714493002/diff/20001/chromeos/dbus/session_m...
File chromeos/dbus/session_manager_client.cc (right):

https://codereview.chromium.org/2714493002/diff/20001/chromeos/dbus/session_m...
chromeos/dbus/session_manager_client.cc:879:
owner_key_path.DirName().AppendASCII("stub_device_policy");
On 2017/02/22 17:24:34, emaxx wrote:
> nit: Maybe define a constant for this string? Or even make a separate function
> for obtaining the device policy path?
> (Also would be nice to resolve the duplication of "stub_policy" below too.)

Done.

https://codereview.chromium.org/2714493002/diff/20001/chromeos/dbus/session_m...
File chromeos/dbus/session_manager_client.h (right):

https://codereview.chromium.org/2714493002/diff/20001/chromeos/dbus/session_m...
chromeos/dbus/session_manager_client.h:160: virtual std::string
BlockingRetrieveDevicePolicy() = 0;
On 2017/02/22 16:40:19, stevenjb wrote:
> Since there is precedence for this already for user policy, I won't push back
> against this, but can we reference crbug.com/160522 here and for all blocking
> calls (or at the top of this file)? We really need a better solution.

Done.

https://codereview.chromium.org/2714493002/diff/20001/chromeos/dbus/session_m...
chromeos/dbus/session_manager_client.h:185: // received, and
On 2017/02/22 17:24:34, emaxx wrote:
> nit: Please reformat the comment.

Done.

https://codereview.chromium.org/2714493002/diff/20001/chromeos/dbus/session_m...
chromeos/dbus/session_manager_client.h:187: // call fails.
On 2017/02/22 16:40:19, stevenjb wrote:
> combine lines

Done.

[emaxx, 2017-02-28 14:31:32]

https://codereview.chromium.org/2714493002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/settings/device_settings_service.cc (right):

https://codereview.chromium.org/2714493002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/settings/device_settings_service.cc:134:
std::unique_ptr<SessionManagerOperation> operation(new LoadSettingsOperation(
On 2017/02/28 14:01:17, Sergey Poromov wrote:
> On 2017/02/22 17:24:33, emaxx wrote:
> > Should the same check for "device_mode_ ==
policy::DEVICE_MODE_ENTERPRISE_AD"
> be
> > done here as in DeviceSettingsService::EnqueueLoad()?
> 
> Expect so, but needs review from tnagel@ for ActiveDirectory part of code.

Yes, please ask Thiemo to pay special attention to this piece of code.

https://codereview.chromium.org/2714493002/diff/150001/chrome/browser/chromeo...
File chrome/browser/chromeos/settings/session_manager_operation.cc (right):

https://codereview.chromium.org/2714493002/diff/150001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/session_manager_operation.cc:208:
ReportValidatorStatus(validator);
Isn't the validator leaking in this case?
If so, then suggesting to store it in std::unique_ptr<> (and use .release() in
second branch) to show the memory management more explicit in the code.

[commit-bot: I haz the power, 2017-02-28 14:54:59]

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

[commit-bot: I haz the power, 2017-02-28 14:55:00]

Dry run: This issue passed the CQ dry run.

[Sergey Poromov, 2017-02-28 15:22:42]

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

[commit-bot: I haz the power, 2017-02-28 15:22:55]

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-02-28 15:41:05]

https://codereview.chromium.org/2714493002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/settings/device_settings_service.cc (right):

https://codereview.chromium.org/2714493002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/settings/device_settings_service.cc:134:
std::unique_ptr<SessionManagerOperation> operation(new LoadSettingsOperation(
> > On 2017/02/22 17:24:33, emaxx wrote:
> > > Should the same check for "device_mode_ ==
> policy::DEVICE_MODE_ENTERPRISE_AD"
> > be
> > > done here as in DeviceSettingsService::EnqueueLoad()?

Yes!

https://codereview.chromium.org/2714493002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/settings/device_settings_service.cc:246:
request_key_load, cloud_validations, false /*force_immediate_load*/,
Nit: pls add blanks: /* force_immediate_load */

[Sergey Poromov, 2017-02-28 15:47:37]

https://codereview.chromium.org/2714493002/diff/150001/chrome/browser/chromeo...
File chrome/browser/chromeos/settings/session_manager_operation.cc (right):

https://codereview.chromium.org/2714493002/diff/150001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/session_manager_operation.cc:208:
ReportValidatorStatus(validator);
On 2017/02/28 14:31:31, emaxx wrote:
> Isn't the validator leaking in this case?
> If so, then suggesting to store it in std::unique_ptr<> (and use .release() in
> second branch) to show the memory management more explicit in the code.

Done.

[emaxx, 2017-02-28 15:52:36]

LGTM

[Sergey Poromov, 2017-02-28 16:25:20]

The CQ bit was checked by poromov@chromium.org

[Sergey Poromov, 2017-02-28 16:25:21]

The patchset sent to the CQ was uploaded after l-g-t-m from erg@chromium.org,
stevenjb@chromium.org
Link to the patchset: https://codereview.chromium.org/2714493002/#ps170001
(title: "wrap Validator in unique_ptr")

[commit-bot: I haz the power, 2017-02-28 16:25:28]

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-28 16:38:15]

CQ is committing da patch.

Bot data: {"patchset_id": 170001, "attempt_start_ts": 1488299120799520,
"parent_rev": "d10f5767cbf8050a4d39a3f42306d58eab351cf7", "commit_rev":
"6944c7931a56d730fcbf31f7f61ceb94fc04d1df"}

[commit-bot: I haz the power, 2017-02-28 16:42:33]

CQ is committing da patch.

Bot data: {"patchset_id": 170001, "attempt_start_ts": 1488299120799520,
"parent_rev": "aaec153c7362dc18e576db2f64f6dc99934fd6f1", "commit_rev":
"afc9d60e96d31a11f756d5c56c2be54a28130960"}

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

Description was changed from

==========
Load DeviceLocalAccount policy and DeviceSettings immediately on restore after
Chrome crash.

UserCloudPolicyManagerChromeOS and UserCloudPolicyManager are loading user
policy immediately
 after Chrome crash as profile is created synchronously so that the Profile
initialization
 never sees unmanaged prefs.

However, it was not true for device local accounts, such as public sessions and
ARC Kiosk session,
 that means having Profile with uninitialized prefs, that particularly leads to
crash/data loss
 for ARC kiosk.

This change forces policies for these accounts to also be loaded immediately on
crash with blocking
 call to D-Bus, similar to loading policies for users.

note: While this change fixes the described issue, the better way is to get rid
of synchronous profile
 creation, but still ensure that policy is loaded before end of profile
creation. See crbug.com/160522

BUG=679810
BUG=694546

TEST=Ran tryjobs, start and restore after crash for: ARC Kiosk, Public Session,
User Session.
==========

to

==========
Load DeviceLocalAccount policy and DeviceSettings immediately on restore after
Chrome crash.

UserCloudPolicyManagerChromeOS and UserCloudPolicyManager are loading user
policy immediately
 after Chrome crash as profile is created synchronously so that the Profile
initialization
 never sees unmanaged prefs.

However, it was not true for device local accounts, such as public sessions and
ARC Kiosk session,
 that means having Profile with uninitialized prefs, that particularly leads to
crash/data loss
 for ARC kiosk.

This change forces policies for these accounts to also be loaded immediately on
crash with blocking
 call to D-Bus, similar to loading policies for users.

note: While this change fixes the described issue, the better way is to get rid
of synchronous profile
 creation, but still ensure that policy is loaded before end of profile
creation. See crbug.com/160522

BUG=679810
BUG=694546

TEST=Ran tryjobs, start and restore after crash for: ARC Kiosk, Public Session,
User Session.

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

[commit-bot: I haz the power, 2017-02-28 16:44:18]

Committed patchset #10 (id:170001) as
https://chromium.googlesource.com/chromium/src/+/afc9d60e96d31a11f756d5c56c2b...