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

chrome/browser/chromeos/policy/device_local_account_policy_store.cc

Index: chrome/browser/chromeos/policy/device_local_account_policy_store.cc
diff --git a/chrome/browser/chromeos/policy/device_local_account_policy_store.cc b/chrome/browser/chromeos/policy/device_local_account_policy_store.cc
index 013bc40f290580a8c3a5f2577683fa3cb171f312..afb7372fd01b056758892233fa53107c8be9d490 100644
--- a/chrome/browser/chromeos/policy/device_local_account_policy_store.cc
+++ b/chrome/browser/chromeos/policy/device_local_account_policy_store.cc
@@ -43,6 +43,60 @@ void DeviceLocalAccountPolicyStore::Load() {
                  weak_factory_.GetWeakPtr()));
 }
 
+void DeviceLocalAccountPolicyStore::LoadImmediately() {

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

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())?

[Sergey Poromov, 2017/02/28 14:01:17]

Done.

+  // This blocking D-Bus call is in the startup path and will block the UI
+  // thread. This only happens when the Profile is created synchronously, which
+  // on Chrome OS happens whenever the browser is restarted into the same
+  // session. That happens when the browser crashes, or right after signin if

[emaxx, 2017/02/22 17:24:33]

nit: It's a little bit unclear what "that happens" refers here to. Maybe combine
this sentence with the previous one?

[Sergey Poromov, 2017/02/28 14:01:17]

Done.

+  // the user has flags configured in about:flags.
+  // However, on those paths we must load policy synchronously so that the
+  // Profile initialization never sees unmanaged prefs, which would lead to
+  // data loss. http://crbug.com/263061
+
+  std::string policy_blob =

[emaxx, 2017/02/22 17:24:33]

nit: const

[Sergey Poromov, 2017/02/28 14:01:17]

Done.

+      session_manager_client_->BlockingRetrieveDeviceLocalAccountPolicy(
+          account_id_);
+  if (policy_blob.empty()) {
+    status_ = CloudPolicyStore::STATUS_LOAD_ERROR;

[emaxx, 2017/02/22 17:24:33]

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

[Sergey Poromov, 2017/02/28 14:01:17]

Done.

+    NotifyStoreError();
+    return;
+  }
+  std::unique_ptr<em::PolicyFetchResponse> policy(
+      new em::PolicyFetchResponse());
+  if (!policy->ParseFromString(policy_blob)) {
+    status_ = CloudPolicyStore::STATUS_PARSE_ERROR;
+    NotifyStoreError();
+    return;
+  }
+
+  chromeos::DeviceSettingsService::OwnershipStatus ownership_status =
+      device_settings_service_->GetOwnershipStatus();
+
+  DCHECK_NE(chromeos::DeviceSettingsService::OWNERSHIP_UNKNOWN,
+            ownership_status);
+  const em::PolicyData* device_policy_data =
+      device_settings_service_->policy_data();
+  // Note that the key is obtained through the device settings service instead
+  // of using |policy_signature_public_key_| member, as the latter one is
+  // updated only after the successful installation of the policy.
+  scoped_refptr<ownership::PublicKey> key =
+      device_settings_service_->GetPublicKey();
+  if (!key.get() || !key->is_loaded() || !device_policy_data) {
+    status_ = CloudPolicyStore::STATUS_BAD_STATE;
+    NotifyStoreLoaded();
+    return;
+  }
+
+  std::unique_ptr<UserCloudPolicyValidator> validator =
+      CreateValidatorForLoad(false /*valid_timestamp_required*/,
+                             device_policy_data, key, std::move(policy));
+  // Start validation. The Validator will delete itself once validation is
+  // complete.

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

Wait, what? But we have a unique_ptr holding |validator| so if the validator
deletes itself won't we get a double free?

[Sergey Poromov, 2017/02/28 14:01:17]

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

+  validator->RunValidation();
+
+  UpdatePolicy(key->as_string(), validator.get());
+}
+
 void DeviceLocalAccountPolicyStore::Store(
     const em::PolicyFetchResponse& policy) {
   weak_factory_.InvalidateWeakPtrs();
@@ -156,6 +210,20 @@ void DeviceLocalAccountPolicyStore::Validate(
     return;
   }
 
+  std::unique_ptr<UserCloudPolicyValidator> validator =
+      CreateValidatorForLoad(valid_timestamp_required, device_policy_data, key,
+                             std::move(policy_response));
+  // Start validation. The Validator will delete itself once validation is
+  // complete.
+  validator.release()->StartValidation(base::Bind(callback, key->as_string()));
+}
+
+std::unique_ptr<UserCloudPolicyValidator>
+DeviceLocalAccountPolicyStore::CreateValidatorForLoad(
+    bool valid_timestamp_required,
+    const em::PolicyData* device_policy_data,
+    scoped_refptr<ownership::PublicKey> key,
+    std::unique_ptr<em::PolicyFetchResponse> policy_response) {
   std::unique_ptr<UserCloudPolicyValidator> validator(
       UserCloudPolicyValidator::Create(std::move(policy_response),
                                        background_task_runner()));
@@ -182,7 +250,7 @@ void DeviceLocalAccountPolicyStore::Validate(
 
   validator->ValidatePayload();
   validator->ValidateSignature(key->as_string());
-  validator.release()->StartValidation(base::Bind(callback, key->as_string()));
+  return validator;
 }
 
 }  // namespace policy