Chromad: Refresh policy every 90 minutes

chrome/browser/chromeos/policy/active_directory_policy_manager.cc

Index: chrome/browser/chromeos/policy/active_directory_policy_manager.cc
diff --git a/chrome/browser/chromeos/policy/active_directory_policy_manager.cc b/chrome/browser/chromeos/policy/active_directory_policy_manager.cc
index fd358156d9068a733f005805ddfc827e0f3d1b75..6ec0df76d0262b5ff4080246f02f54110aa7265a 100644
--- a/chrome/browser/chromeos/policy/active_directory_policy_manager.cc
+++ b/chrome/browser/chromeos/policy/active_directory_policy_manager.cc
@@ -4,17 +4,31 @@
 
 #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() {}
@@ -47,7 +61,7 @@ void ActiveDirectoryPolicyManager::Init(SchemaRegistry* registry) {
   // Does nothing if |store_| hasn't yet initialized.
   PublishPolicy();
 
-  RefreshPolicies();
+  ScheduleRefresh();
 }
 
 void ActiveDirectoryPolicyManager::Shutdown() {
@@ -57,8 +71,9 @@ void ActiveDirectoryPolicyManager::Shutdown() {
 
 bool ActiveDirectoryPolicyManager::IsInitializationComplete(
     PolicyDomain domain) const {
-  if (domain == POLICY_DOMAIN_CHROME)
+  if (domain == POLICY_DOMAIN_CHROME) {
     return store_->is_initialized();
+  }
   return true;
 }
 
@@ -69,6 +84,8 @@ void ActiveDirectoryPolicyManager::RefreshPolicies() {
   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,
@@ -85,6 +102,10 @@ 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(
@@ -94,6 +115,10 @@ void ActiveDirectoryPolicyManager::OnStoreError(
   // 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(
@@ -101,6 +126,9 @@ ActiveDirectoryPolicyManager::ActiveDirectoryPolicyManager(
     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() {
@@ -129,4 +157,37 @@ void ActiveDirectoryPolicyManager::OnPolicyRefreshed(bool success) {
   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