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..51e87fc69daf4f74480d1696c9d56cabcca20c6d 100644
--- a/chrome/browser/chromeos/policy/active_directory_policy_manager.cc
+++ b/chrome/browser/chromeos/policy/active_directory_policy_manager.cc
@@ -4,17 +4,34 @@
 
 #include "chrome/browser/chromeos/policy/active_directory_policy_manager.h"
 
+#include <algorithm>

[emaxx, 2017/01/26 20:54:02]

nit: not needed?

[Thiemo Nagel, 2017/01/27 10:55:35]

Done.

 #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;
+
+// After startup, delay initial policy fetch to keep authpolicyd free for more
+// important tasks like user auth.
+constexpr int kInitialFetchDelaySeconds = 60;
+
+// Retry delay in case of |refresh_in_progress_|.
+constexpr int kBusyRetryDelaySeconds = 1;
+
+}  // namespace
+
 namespace policy {
 
 ActiveDirectoryPolicyManager::~ActiveDirectoryPolicyManager() {}
@@ -47,7 +64,7 @@ void ActiveDirectoryPolicyManager::Init(SchemaRegistry* registry) {
   // Does nothing if |store_| hasn't yet initialized.
   PublishPolicy();
 
-  RefreshPolicies();
+  ScheduleAutomaticRefresh();
 }
 
 void ActiveDirectoryPolicyManager::Shutdown() {
@@ -57,28 +74,14 @@ 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;
 }
 
 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);
-  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()));
-  }
+  ScheduleRefresh(base::TimeDelta());
 }
 
 void ActiveDirectoryPolicyManager::OnStoreLoaded(
@@ -127,6 +130,72 @@ void ActiveDirectoryPolicyManager::OnPolicyRefreshed(bool success) {
   // Load independently of success or failure to keep up to date with whatever
   // has happened on the authpolicyd / session manager side.
   store_->Load();
+
+  refresh_in_progress_ = false;
+  last_refresh_ = base::TimeTicks::Now();
+  ScheduleAutomaticRefresh();
+}
+
+void ActiveDirectoryPolicyManager::ScheduleRefresh(base::TimeDelta delay) {
+  if (refresh_task_) {
+    refresh_task_->Cancel();
+  }
+  refresh_task_ = base::MakeUnique<base::CancelableClosure>(
+      base::Bind(&ActiveDirectoryPolicyManager::RunScheduledRefresh,
+                 weak_ptr_factory_.GetWeakPtr()));
+  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
+      FROM_HERE, refresh_task_->callback(), delay);
+}
+
+void ActiveDirectoryPolicyManager::ScheduleAutomaticRefresh() {
+  base::TimeTicks baseline;
+  base::TimeDelta interval;
+  if (last_refresh_ == base::TimeTicks()) {
+    baseline = startup_;
+    interval = base::TimeDelta::FromSeconds(kInitialFetchDelaySeconds);
+  } else {
+    baseline = last_refresh_;
+    interval = base::TimeDelta::FromSeconds(kRefreshIntervalSeconds);
+  }
+  base::TimeTicks now(base::TimeTicks::Now());
+  base::TimeDelta delay;
+  if (now < baseline + interval) {
+    delay = baseline + interval - now;
+  }
+  ScheduleRefresh(delay);
+}
+
+void ActiveDirectoryPolicyManager::RunScheduledRefresh() {
+  // Re-schedule when a refresh is currently in progress (to avoid D-Bus jobs

[emaxx, 2017/01/26 20:54:02]

Sorry, I'm still a bit puzzled about why this re-scheduling is necessary.
If there is a refresh happening right now, then, eventually, we will end up in
OnPolicyRefreshed, that, by calling ScheduleAutomaticRefresh, will anyway
forcibly schedule the next refresh in kRefreshIntervalSeconds.
So the re-scheduling here shouldn't bring much value - or am I overlooking some
case here?

[Thiemo Nagel, 2017/01/27 10:55:35]

Your reasoning is sound.  I wanted to a manual refresh to override the usual
refresh interval, but it's not possible in this way.  I'm adding a note.

+  // piling up behind each other).
+  if (refresh_in_progress_) {
+    // Don't call ScheduleRefresh() directly as that would cancel the task
+    // that's currently running.
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE,
+        base::Bind(&ActiveDirectoryPolicyManager::ScheduleRefresh,
+                   weak_ptr_factory_.GetWeakPtr(),
+                   base::TimeDelta::FromSeconds(kBusyRetryDelaySeconds)));
+    return;
+  }
+
+  refresh_in_progress_ = true;
+  chromeos::DBusThreadManager* thread_manager =
+      chromeos::DBusThreadManager::Get();
+  DCHECK(thread_manager);
+  chromeos::AuthPolicyClient* auth_policy_client =
+      thread_manager->GetAuthPolicyClient();
+  DCHECK(auth_policy_client);
+  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()));
+  }
 }
 
 }  // namespace policy