Chromad: Refresh policy every 90 minutes

chrome/browser/chromeos/policy/active_directory_policy_manager.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/chromeos/policy/active_directory_policy_manager.h"
 
+#include <algorithm>
 #include <string>
 #include <utility>
 
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
+#include "base/threading/thread_task_runner_handle.h"
 #include "chromeos/dbus/auth_policy_client.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "components/policy/core/common/policy_bundle.h"
 #include "components/policy/core/common/policy_types.h"
 #include "components/policy/policy_constants.h"
 
+namespace {
+
+// Refresh policy every 90 minutes which matches the Windows default:
+// https://technet.microsoft.com/en-us/library/cc940895.aspx
+constexpr int kRefreshIntervalSeconds = 90 * 60;
+
+// Minimum delay when scheduling a policy refresh task (to rule out the

[emaxx, 2017/01/24 15:54:26]

Have I understood correctly that this constant is mainly for preventing task
"storm" in case of a future bug introduced into the code? I'm not sure then that
complicating the code with this constant is really helpful against this
potential issue.

[Thiemo Nagel, 2017/01/26 20:14:17]

I also used it for re-scheduling when refresh_in_progress_.  Renamed it.

+// possibility of a re-scheduling storm).
+constexpr int kMinSchedulingDelaySeconds = 1;
+
+}  // namespace
+
 namespace policy {
 
 ActiveDirectoryPolicyManager::~ActiveDirectoryPolicyManager() {}
 
 // static
 std::unique_ptr<ActiveDirectoryPolicyManager>
 ActiveDirectoryPolicyManager::CreateForDevicePolicy(
     std::unique_ptr<CloudPolicyStore> store) {
   return base::WrapUnique(
       new ActiveDirectoryPolicyManager(EmptyAccountId(), std::move(store)));
@@ skipping 12 matching lines @@
   ConfigurationPolicyProvider::Init(registry);
 
   store_->AddObserver(this);
   if (!store_->is_initialized()) {
     store_->Load();
   }
 
   // Does nothing if |store_| hasn't yet initialized.
   PublishPolicy();
 
-  RefreshPolicies();
+  ScheduleRefresh();
 }
 
 void ActiveDirectoryPolicyManager::Shutdown() {
   store_->RemoveObserver(this);
   ConfigurationPolicyProvider::Shutdown();
 }
 
 bool ActiveDirectoryPolicyManager::IsInitializationComplete(
     PolicyDomain domain) const {
-  if (domain == POLICY_DOMAIN_CHROME)
+  if (domain == POLICY_DOMAIN_CHROME) {
     return store_->is_initialized();
+  }
   return true;
 }
 
 void ActiveDirectoryPolicyManager::RefreshPolicies() {
   chromeos::DBusThreadManager* thread_manager =
       chromeos::DBusThreadManager::Get();
   DCHECK(thread_manager);
   chromeos::AuthPolicyClient* auth_policy_client =
       thread_manager->GetAuthPolicyClient();
   DCHECK(auth_policy_client);
+
+  refresh_in_progress_ = true;
   if (account_id_ == EmptyAccountId()) {
     auth_policy_client->RefreshDevicePolicy(
         base::Bind(&ActiveDirectoryPolicyManager::OnPolicyRefreshed,
                    weak_ptr_factory_.GetWeakPtr()));
   } else {
     auth_policy_client->RefreshUserPolicy(
         account_id_,
         base::Bind(&ActiveDirectoryPolicyManager::OnPolicyRefreshed,
                    weak_ptr_factory_.GetWeakPtr()));
   }
 }
 
 void ActiveDirectoryPolicyManager::OnStoreLoaded(
     CloudPolicyStore* cloud_policy_store) {
   DCHECK_EQ(store_.get(), cloud_policy_store);
   PublishPolicy();
+
+  refresh_in_progress_ = false;

[emaxx, 2017/01/24 15:54:26]

Shouldn't this be done in OnPolicyRefreshed instead? It looks like the
assignments to refresh_in_progress_ are not paired up currently (for instance,
in case of a user-initiated policy fetch).

[Thiemo Nagel, 2017/01/26 20:14:17]

I think it's paired correctly for user-initiated fetch, but not in case
store->Load() is triggered, as it is e.g. in Init().  Thus I'm foregoing the
minor benefit of having store->Load() inside the protected section for greater
reliability and change the code to release the lock in OnPolicyRefreshed().

+  last_refresh_ = base::TimeTicks::Now();
+  ScheduleRefresh();
 }
 
 void ActiveDirectoryPolicyManager::OnStoreError(
     CloudPolicyStore* cloud_policy_store) {
   DCHECK_EQ(store_.get(), cloud_policy_store);
   // Publish policy (even though it hasn't changed) in order to signal load
   // complete on the ConfigurationPolicyProvider interface. Technically, this is
   // only required on the first load, but doesn't hurt in any case.
   PublishPolicy();
+
+  refresh_in_progress_ = false;
+  last_refresh_ = base::TimeTicks::Now();
+  ScheduleRefresh();
 }
 
 ActiveDirectoryPolicyManager::ActiveDirectoryPolicyManager(
     const AccountId& account_id,
     std::unique_ptr<CloudPolicyStore> store)
     : account_id_(account_id),
       store_(std::move(store)),
+      refresh_interval_(base::TimeDelta::FromSeconds(kRefreshIntervalSeconds)),
+      min_scheduling_delay_(
+          base::TimeDelta::FromSeconds(kMinSchedulingDelaySeconds)),
       weak_ptr_factory_(this) {}
 
 void ActiveDirectoryPolicyManager::PublishPolicy() {
   if (!store_->is_initialized()) {
     return;
   }
   std::unique_ptr<PolicyBundle> bundle = base::MakeUnique<PolicyBundle>();
   PolicyMap& policy_map =
       bundle->Get(PolicyNamespace(POLICY_DOMAIN_CHROME, std::string()));
   policy_map.CopyFrom(store_->policy_map());
 
   // Overwrite the source which is POLICY_SOURCE_CLOUD by default.
   // TODO(tnagel): Rename CloudPolicyStore to PolicyStore and make the source
   // configurable, then drop PolicyMap::SetSourceForAll().
   policy_map.SetSourceForAll(POLICY_SOURCE_ACTIVE_DIRECTORY);
   SetEnterpriseUsersDefaults(&policy_map);
   UpdatePolicy(std::move(bundle));
 }
 
 void ActiveDirectoryPolicyManager::OnPolicyRefreshed(bool success) {
   if (!success) {
     LOG(ERROR) << "Active Directory policy refresh failed.";
   }
   // Load independently of success or failure to keep up to date with whatever
   // has happened on the authpolicyd / session manager side.
   store_->Load();
 }
 
+void ActiveDirectoryPolicyManager::ScheduleRefresh() {
+  const base::TimeTicks now(base::TimeTicks::Now());
+  base::TimeDelta delay(min_scheduling_delay_);
+  if (now < last_refresh_ + refresh_interval_) {

[emaxx, 2017/01/24 15:54:26]

I'm feeling a bit uncomfortable with the possibility to do maths here with a
default-initialized TimeTicks last_refresh_.
Maybe add a check against is_null into this condition?

[Thiemo Nagel, 2017/01/26 20:14:18]

This calculation is written with default-initialized (0) TimeTicks in mind. 
Shall I make it explicit by adding a comment?

+    delay = std::max(delay, last_refresh_ + refresh_interval_ - now);
+  }
+  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
+      FROM_HERE, base::Bind(&ActiveDirectoryPolicyManager::RunScheduledRefresh,
+                            weak_ptr_factory_.GetWeakPtr(), ++task_number_),

[emaxx, 2017/01/24 15:54:26]

Maybe do the increment before the call? It's a bit difficult to see it here.
Typically these long "Bind" expressions contain only passing the parameters, so
one may not expect a piece of logic there.

[Thiemo Nagel, 2017/01/26 20:14:17]

Thanks, that's obsolete now.

+      delay);
+}
+
+void ActiveDirectoryPolicyManager::RunScheduledRefresh(int task_number) {
+  // There may be multiple tasks in the queue at the same time (e.g. after

[emaxx, 2017/01/24 15:54:26]

Hmm... I was going to suggest using base::CancelableClosure (like
CloudPolicyRefreshScheduler does), as in principle it allows to omit tracking
this task_number.

However, I realized then that it's probably unsafe to call
CancelableClosure::Cancel/CancelableClosure::Reset from inside the callback
itself, as this could probably invalidate weak pointers and destroy this...

But if you think that this case is impossible, then using CancelableClosure
would simplify the code.

[Thiemo Nagel, 2017/01/26 20:14:17]

Seems like a good idea!  Posting ScheduleRefresh() to the loop should avoid
self-cancellation.

+  // manual policy refresh) but only the one that carries the current
+  // |task_number_| is valid.
+  if (task_number != task_number_) {
+    return;
+  }
+
+  // Re-schedule in case the intended refresh interval has not yet been reached

[emaxx, 2017/01/24 15:54:26]

Why is this really necessary? Shouldn't all the cases described here end up in
OnStoreLoaded/OnStoreError, which do the re-scheduling themselves?

[Thiemo Nagel, 2017/01/26 20:14:17]

Seems a valid argument.  ;)

+  // (e.g. because of a manual policy fetch in between two scheduled fetches) or
+  // when a refresh is currently in progress (to avoid refresh jobs piling up
+  // behind each other which could possibly lead to resource exhaustion).
+  if (base::TimeTicks::Now() - last_refresh_ < refresh_interval_ ||
+      refresh_in_progress_) {
+    ScheduleRefresh();
+    return;
+  }
+
+  RefreshPolicies();
+}
+
 }  // namespace policy