Put a hard limit on initial policy fetch wait time.

This assures that we're not hanging for a long time at login on the
initial policy fetch for Chrome OS user policy to happen.

BUG=chromium:285694
TEST=See bug, unit tests.

Review URL: https://chromiumcodereview.appspot.com/23450022

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@221723 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos.cc

@@ -51,13 +51,22 @@ const char kUMAInitialFetchOAuth2NetworkError[] =
 UserCloudPolicyManagerChromeOS::UserCloudPolicyManagerChromeOS(
     scoped_ptr<CloudPolicyStore> store,
     scoped_ptr<ResourceCache> resource_cache,
-    bool wait_for_policy_fetch)
+    bool wait_for_policy_fetch,
+    base::TimeDelta initial_policy_fetch_timeout)
     : CloudPolicyManager(
           PolicyNamespaceKey(dm_protocol::kChromeUserPolicyType, std::string()),
           store.get()),
       store_(store.Pass()),
-      wait_for_policy_fetch_(wait_for_policy_fetch) {
+      wait_for_policy_fetch_(wait_for_policy_fetch),
+      policy_fetch_timeout_(false, false) {
   time_init_started_ = base::Time::Now();
+  if (wait_for_policy_fetch_) {
+    policy_fetch_timeout_.Start(
+        FROM_HERE,
+        initial_policy_fetch_timeout,
+        base::Bind(&UserCloudPolicyManagerChromeOS::CancelWaitForPolicyFetch,
+                   base::Unretained(this)));
+  }
   if (resource_cache) {
     // TODO(joaodasilva): Move the backend from the FILE thread to the blocking
     // pool.
@@ -306,6 +315,9 @@ void UserCloudPolicyManagerChromeOS::OnInitialPolicyFetchComplete(
 }
 
 void UserCloudPolicyManagerChromeOS::CancelWaitForPolicyFetch() {
+  if (!wait_for_policy_fetch_)
+    return;
+
   wait_for_policy_fetch_ = false;
   CheckAndPublishPolicy();
   // Now that |wait_for_policy_fetch_| is guaranteed to be false, the scheduler

chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos.h

@@ -12,6 +12,7 @@
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/time/time.h"
+#include "base/timer/timer.h"
 #include "chrome/browser/policy/cloud/cloud_policy_client.h"
 #include "chrome/browser/policy/cloud/cloud_policy_constants.h"
 #include "chrome/browser/policy/cloud/cloud_policy_manager.h"
@@ -46,7 +47,8 @@ class UserCloudPolicyManagerChromeOS
   UserCloudPolicyManagerChromeOS(
       scoped_ptr<CloudPolicyStore> store,
       scoped_ptr<ResourceCache> resource_cache,
-      bool wait_for_policy_fetch);
+      bool wait_for_policy_fetch,
+      base::TimeDelta initial_policy_fetch_timeout);
   virtual ~UserCloudPolicyManagerChromeOS();
 
   // Initializes the cloud connection. |local_state| and
@@ -124,6 +126,10 @@ class UserCloudPolicyManagerChromeOS
   // IsInitializationComplete().
   bool wait_for_policy_fetch_;
 
+  // A timer that puts a hard limit on the maximum time to wait for the intial
+  // policy fetch.
+  base::Timer policy_fetch_timeout_;
+
   // The pref service to pass to the refresh scheduler on initialization.
   PrefService* local_state_;
 

chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_unittest.cc

@@ -15,6 +15,7 @@
 #include "base/run_loop.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/time/time.h"
 #include "chrome/browser/chromeos/policy/user_cloud_policy_token_forwarder.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "chrome/browser/policy/cloud/cloud_policy_constants.h"
@@ -143,13 +144,14 @@ class UserCloudPolicyManagerChromeOSTest : public testing::Test {
     profile_manager_->DeleteTestingProfile(chrome::kInitialProfile);
   }
 
-  void CreateManager(bool wait_for_fetch) {
+  void CreateManager(bool wait_for_fetch, int fetch_timeout) {
     store_ = new MockCloudPolicyStore();
     EXPECT_CALL(*store_, Load());
     manager_.reset(new UserCloudPolicyManagerChromeOS(
         scoped_ptr<CloudPolicyStore>(store_),
         scoped_ptr<ResourceCache>(),
-        wait_for_fetch));
+        wait_for_fetch,
+        base::TimeDelta::FromSeconds(fetch_timeout)));
     manager_->Init();
     manager_->AddObserver(&observer_);
     manager_->Connect(&prefs_, &device_management_service_, NULL,
@@ -306,7 +308,7 @@ const char UserCloudPolicyManagerChromeOSTest::kSigninProfile[] =
 TEST_F(UserCloudPolicyManagerChromeOSTest, BlockingFirstFetch) {
   // Tests the initialization of a manager whose Profile is waiting for the
   // initial fetch, when the policy cache is empty.
-  ASSERT_NO_FATAL_FAILURE(CreateManager(true));
+  ASSERT_NO_FATAL_FAILURE(CreateManager(true, 1000));
 
   // Initialize the CloudPolicyService without any stored data.
   EXPECT_FALSE(manager_->core()->service()->IsInitializationComplete());
@@ -329,7 +331,7 @@ TEST_F(UserCloudPolicyManagerChromeOSTest, BlockingFirstFetch) {
 TEST_F(UserCloudPolicyManagerChromeOSTest, BlockingRefreshFetch) {
   // Tests the initialization of a manager whose Profile is waiting for the
   // initial fetch, when a previously cached policy and DMToken already exist.
-  ASSERT_NO_FATAL_FAILURE(CreateManager(true));
+  ASSERT_NO_FATAL_FAILURE(CreateManager(true, 1000));
 
   // Set the initially cached data and initialize the CloudPolicyService.
   // The initial policy fetch is issued using the cached DMToken.
@@ -341,7 +343,7 @@ TEST_F(UserCloudPolicyManagerChromeOSTest, BlockingRefreshFetch) {
 TEST_F(UserCloudPolicyManagerChromeOSTest, BlockingFetchStoreError) {
   // Tests the initialization of a manager whose Profile is waiting for the
   // initial fetch, when the initial store load fails.
-  ASSERT_NO_FATAL_FAILURE(CreateManager(true));
+  ASSERT_NO_FATAL_FAILURE(CreateManager(true, 1000));
 
   // Initialize the CloudPolicyService without any stored data.
   EXPECT_FALSE(manager_->core()->service()->IsInitializationComplete());
@@ -364,7 +366,7 @@ TEST_F(UserCloudPolicyManagerChromeOSTest, BlockingFetchStoreError) {
 TEST_F(UserCloudPolicyManagerChromeOSTest, BlockingFetchOAuthError) {
   // Tests the initialization of a manager whose Profile is waiting for the
   // initial fetch, when the OAuth2 token fetch fails.
-  ASSERT_NO_FATAL_FAILURE(CreateManager(true));
+  ASSERT_NO_FATAL_FAILURE(CreateManager(true, 1000));
 
   // Initialize the CloudPolicyService without any stored data.
   EXPECT_FALSE(manager_->core()->service()->IsInitializationComplete());
@@ -393,7 +395,7 @@ TEST_F(UserCloudPolicyManagerChromeOSTest, BlockingFetchOAuthError) {
 TEST_F(UserCloudPolicyManagerChromeOSTest, BlockingFetchRegisterError) {
   // Tests the initialization of a manager whose Profile is waiting for the
   // initial fetch, when the device management registration fails.
-  ASSERT_NO_FATAL_FAILURE(CreateManager(true));
+  ASSERT_NO_FATAL_FAILURE(CreateManager(true, 1000));
 
   // Initialize the CloudPolicyService without any stored data.
   EXPECT_FALSE(manager_->core()->service()->IsInitializationComplete());
@@ -419,7 +421,7 @@ TEST_F(UserCloudPolicyManagerChromeOSTest, BlockingFetchRegisterError) {
 TEST_F(UserCloudPolicyManagerChromeOSTest, BlockingFetchPolicyFetchError) {
   // Tests the initialization of a manager whose Profile is waiting for the
   // initial fetch, when the policy fetch request fails.
-  ASSERT_NO_FATAL_FAILURE(CreateManager(true));
+  ASSERT_NO_FATAL_FAILURE(CreateManager(true, 1000));
 
   // Initialize the CloudPolicyService without any stored data.
   EXPECT_FALSE(manager_->core()->service()->IsInitializationComplete());
@@ -457,9 +459,29 @@ TEST_F(UserCloudPolicyManagerChromeOSTest, BlockingFetchPolicyFetchError) {
   EXPECT_TRUE(PolicyBundle().Equals(manager_->policies()));
 }
 
+TEST_F(UserCloudPolicyManagerChromeOSTest, BlockingFetchTimeout) {
+  // The blocking fetch should be abandoned after the timeout.
+  ASSERT_NO_FATAL_FAILURE(CreateManager(true, 0));
+
+  // Initialize the CloudPolicyService without any stored data.
+  EXPECT_FALSE(manager_->core()->service()->IsInitializationComplete());
+  store_->NotifyStoreLoaded();
+  EXPECT_TRUE(manager_->core()->service()->IsInitializationComplete());
+  EXPECT_FALSE(manager_->core()->client()->is_registered());
+
+  // Running the message loop should trigger the timeout.
+  EXPECT_CALL(observer_, OnUpdatePolicy(manager_.get())).Times(AtLeast(1));
+  EXPECT_FALSE(manager_->IsInitializationComplete(POLICY_DOMAIN_CHROME));
+  base::RunLoop().RunUntilIdle();
+  Mock::VerifyAndClearExpectations(&observer_);
+  EXPECT_TRUE(manager_->IsInitializationComplete(POLICY_DOMAIN_CHROME));
+  EXPECT_TRUE(PolicyBundle().Equals(manager_->policies()));
+}
+
+
 TEST_F(UserCloudPolicyManagerChromeOSTest, NonBlockingFirstFetch) {
   // Tests the first policy fetch request by a Profile that isn't managed.
-  ASSERT_NO_FATAL_FAILURE(CreateManager(false));
+  ASSERT_NO_FATAL_FAILURE(CreateManager(false, 1000));
 
   // Initialize the CloudPolicyService without any stored data. Since the
   // manager is not waiting for the initial fetch, it will become initialized
@@ -502,7 +524,7 @@ TEST_F(UserCloudPolicyManagerChromeOSTest, NonBlockingFirstFetch) {
 TEST_F(UserCloudPolicyManagerChromeOSTest, NonBlockingRefreshFetch) {
   // Tests a non-blocking initial policy fetch for a Profile that already has
   // a cached DMToken.
-  ASSERT_NO_FATAL_FAILURE(CreateManager(false));
+  ASSERT_NO_FATAL_FAILURE(CreateManager(false, 1000));
 
   // Set the initially cached data and initialize the CloudPolicyService.
   // The initial policy fetch is issued using the cached DMToken.

chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.cc

@@ -8,6 +8,7 @@
 #include "base/files/file_path.h"
 #include "base/logging.h"
 #include "base/path_service.h"
+#include "base/time/time.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chromeos/login/user.h"
 #include "chrome/browser/chromeos/login/user_manager.h"
@@ -42,6 +43,10 @@ const base::FilePath::CharType kPolicy[] = FILE_PATH_LITERAL("Policy");
 // resources are stored.
 const base::FilePath::CharType kResourceDir[] = FILE_PATH_LITERAL("Resources");
 
+// Timeout in seconds after which to abandon the initial policy fetch and start
+// the session regardless.
+const int kInitialPolicyFetchTimeoutSeconds = 10;
+
 }  // namespace
 
 // static
@@ -147,9 +152,11 @@ scoped_ptr<UserCloudPolicyManagerChromeOS>
     resource_cache.reset(new ResourceCache(resource_cache_dir));
 
   scoped_ptr<UserCloudPolicyManagerChromeOS> manager(
-      new UserCloudPolicyManagerChromeOS(store.PassAs<CloudPolicyStore>(),
+      new UserCloudPolicyManagerChromeOS(
+          store.PassAs<CloudPolicyStore>(),
           resource_cache.Pass(),
-                                         wait_for_initial_policy));
+          wait_for_initial_policy,
+          base::TimeDelta::FromSeconds(kInitialPolicyFetchTimeoutSeconds)));
   manager->Init();
   manager->Connect(g_browser_process->local_state(),
                    device_management_service,