Delay CookieManagerService calls until network is available. This affects AddAccount, LogOut and ListAccounts. Note that ChromeOS will pass the call to the ChromeOS specific implementation of DelayNetworkCall; other platforms just wait for connection availability. This integrates with the existing backoff/retry.

BUG=466799

Review URL: https://codereview.chromium.org/1108393004

Cr-Commit-Position: refs/heads/master@{#328401}

9 files changed, 237 insertions, 19 deletions

diff --git a/chrome/browser/signin/chrome_signin_client.cc b/chrome/browser/signin/chrome_signin_client.cc
index 88cb729..c4b839a 100644
--- a/chrome/browser/signin/chrome_signin_client.cc
+++ b/chrome/browser/signin/chrome_signin_client.cc
@@ -32,6 +32,7 @@
 #endif
 
 #if defined(OS_CHROMEOS)
+#include "chrome/browser/chromeos/net/delay_network_call.h"
 #include "components/user_manager/user_manager.h"
 #endif
 
@@ -44,12 +45,21 @@ ChromeSigninClient::ChromeSigninClient(
     : profile_(profile),
       signin_error_controller_(signin_error_controller) {
   signin_error_controller_->AddObserver(this);
+#if !defined(OS_CHROMEOS)
+  net::NetworkChangeNotifier::AddNetworkChangeObserver(this);
+#endif
 }
 
 ChromeSigninClient::~ChromeSigninClient() {
   signin_error_controller_->RemoveObserver(this);
 }
 
+void ChromeSigninClient::Shutdown() {
+#if !defined(OS_CHROMEOS)
+  net::NetworkChangeNotifier::RemoveNetworkChangeObserver(this);
+#endif
+}
+
 // static
 bool ChromeSigninClient::ProfileAllowsSigninCookies(Profile* profile) {
   CookieSettings* cookie_settings =
@@ -225,3 +235,32 @@ void ChromeSigninClient::OnErrorChanged() {
   cache.SetProfileIsAuthErrorAtIndex(index,
       signin_error_controller_->HasError());
 }
+
+#if !defined(OS_CHROMEOS)
+void ChromeSigninClient::OnNetworkChanged(
+    net::NetworkChangeNotifier::ConnectionType type) {
+  if (type >= net::NetworkChangeNotifier::ConnectionType::CONNECTION_NONE)
+    return;
+
+  for (const base::Closure& callback : delayed_callbacks_)
+    callback.Run();
+
+  delayed_callbacks_.clear();
+}
+#endif
+
+void ChromeSigninClient::DelayNetworkCall(const base::Closure& callback) {
+#if defined(OS_CHROMEOS)
+  chromeos::DelayNetworkCall(
+      base::TimeDelta::FromMilliseconds(chromeos::kDefaultNetworkRetryDelayMS),
+      callback);
+  return;
+#else
+  // Don't bother if we don't have any kind of network connection.
+  if (net::NetworkChangeNotifier::IsOffline()) {
+    delayed_callbacks_.push_back(callback);
+  } else {
+    callback.Run();
+  }
+#endif
+}
diff --git a/chrome/browser/signin/chrome_signin_client.h b/chrome/browser/signin/chrome_signin_client.h
index d2067d7..24da63da 100644
--- a/chrome/browser/signin/chrome_signin_client.h
+++ b/chrome/browser/signin/chrome_signin_client.h
@@ -10,15 +10,24 @@
 #include "components/signin/core/browser/signin_client.h"
 #include "components/signin/core/browser/signin_error_controller.h"
 
+#if !defined(OS_CHROMEOS)
+#include "net/base/network_change_notifier.h"
+#endif
+
 class CookieSettings;
 class Profile;
 
-class ChromeSigninClient : public SigninClient,
-                           public SigninErrorController::Observer {
+class ChromeSigninClient
+    : public SigninClient,
+#if !defined(OS_CHROMEOS)
+      public net::NetworkChangeNotifier::NetworkChangeObserver,
+#endif
+      public SigninErrorController::Observer {
  public:
   explicit ChromeSigninClient(
       Profile* profile, SigninErrorController* signin_error_controller);
   ~ChromeSigninClient() override;
+  void Shutdown() override;
 
   // Utility methods.
   static bool ProfileAllowsSigninCookies(Profile* profile);
@@ -39,6 +48,7 @@ class ChromeSigninClient : public SigninClient,
       content_settings::Observer* observer) override;
   void RemoveContentSettingsObserver(
       content_settings::Observer* observer) override;
+  void DelayNetworkCall(const base::Closure& callback) override;
 
   // Returns a string describing the chrome version environment. Version format:
   // <Build Info> <OS> <Version number> (<Last change>)<channel or "-devel">
@@ -60,10 +70,19 @@ class ChromeSigninClient : public SigninClient,
   // SigninErrorController::Observer implementation.
   void OnErrorChanged() override;
 
+#if !defined(OS_CHROMEOS)
+  // net::NetworkChangeController::NetworkChangeObserver implementation.
+  void OnNetworkChanged(net::NetworkChangeNotifier::ConnectionType type)
+      override;
+#endif
+
  private:
   Profile* profile_;
 
   SigninErrorController* signin_error_controller_;
+#if !defined(OS_CHROMEOS)
+  std::list<base::Closure> delayed_callbacks_;
+#endif
 
   DISALLOW_COPY_AND_ASSIGN(ChromeSigninClient);
 };
diff --git a/chrome/browser/signin/chrome_signin_client_unittest.cc b/chrome/browser/signin/chrome_signin_client_unittest.cc
new file mode 100644
index 0000000..b6e24c3
--- /dev/null
+++ b/chrome/browser/signin/chrome_signin_client_unittest.cc
@@ -0,0 +1,119 @@
+// Copyright 2015 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/signin/chrome_signin_client.h"
+
+#include "base/bind.h"
+#include "base/message_loop/message_loop.h"
+#include "base/run_loop.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/test/base/testing_profile.h"
+#include "content/public/test/test_browser_thread_bundle.h"
+#include "net/base/network_change_notifier.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+// ChromeOS has its own network delay logic.
+#if !defined(OS_CHROMEOS)
+
+namespace {
+
+class MockNetworkChangeNotifierNeverOffline :
+    public net::NetworkChangeNotifier {
+ public:
+  net::NetworkChangeNotifier::ConnectionType GetCurrentConnectionType() const
+      override {
+    return NetworkChangeNotifier::CONNECTION_3G;
+  }
+};
+
+class MockNetworkChangeNotifierOfflineUntilChange :
+    public net::NetworkChangeNotifier {
+ public:
+  MockNetworkChangeNotifierOfflineUntilChange() : online_(false) {}
+  net::NetworkChangeNotifier::ConnectionType GetCurrentConnectionType() const
+      override {
+    return online_ ? net::NetworkChangeNotifier::CONNECTION_3G
+                   : net::NetworkChangeNotifier::CONNECTION_NONE;
+  }
+
+  void GoOnline() {
+    online_ = true;
+    net::NetworkChangeNotifier::NotifyObserversOfNetworkChangeForTests(
+        net::NetworkChangeNotifier::CONNECTION_3G);
+  }
+
+ private:
+  bool online_;
+};
+
+class CallbackTester {
+ public:
+  CallbackTester() : called_(0) {}
+
+  void Increment();
+  bool WasCalledExactlyOnce();
+
+ private:
+  int called_;
+};
+
+void CallbackTester::Increment() {
+  called_++;
+}
+
+bool CallbackTester::WasCalledExactlyOnce() {
+  return called_ == 1;
+}
+
+}  // namespace
+
+class ChromeSigninClientTest : public testing::Test {
+ public:
+  ChromeSigninClientTest() {}
+  void SetUp() override;
+
+  Profile* profile() { return profile_.get(); }
+  ChromeSigninClient* signin_client() { return signin_client_.get(); }
+
+ private:
+  scoped_ptr<Profile> profile_;
+  scoped_ptr<ChromeSigninClient> signin_client_;
+  SigninErrorController signin_error_controller_;
+  content::TestBrowserThreadBundle thread_bundle_;
+};
+
+void ChromeSigninClientTest::SetUp() {
+  // Create a signed-in profile.
+  TestingProfile::Builder builder;
+  profile_ = builder.Build();
+
+  signin_client_.reset(new ChromeSigninClient(profile(),
+                                              &signin_error_controller_));
+}
+
+TEST_F(ChromeSigninClientTest, DelayNetworkCallRunsImmediatelyWithNetwork) {
+  scoped_ptr<net::NetworkChangeNotifier>
+      mock(new MockNetworkChangeNotifierNeverOffline);
+  CallbackTester tester;
+  signin_client()->DelayNetworkCall(base::Bind(&CallbackTester::Increment,
+                                               base::Unretained(&tester)));
+  ASSERT_TRUE(tester.WasCalledExactlyOnce());
+}
+
+TEST_F(ChromeSigninClientTest, DelayNetworkCallRunsAfterNetworkFound) {
+  scoped_ptr<MockNetworkChangeNotifierOfflineUntilChange>
+      mock(new MockNetworkChangeNotifierOfflineUntilChange());
+  // Install a SigninClient after the NetworkChangeNotifier's created.
+  SetUp();
+
+  CallbackTester tester;
+  signin_client()->DelayNetworkCall(base::Bind(&CallbackTester::Increment,
+                                               base::Unretained(&tester)));
+  ASSERT_FALSE(tester.WasCalledExactlyOnce());
+  mock->GoOnline();
+  base::RunLoop().RunUntilIdle();
+  ASSERT_TRUE(tester.WasCalledExactlyOnce());
+}
+
+#endif
diff --git a/chrome/chrome_tests_unit.gypi b/chrome/chrome_tests_unit.gypi
index 424d27d..c4649f7 100644
--- a/chrome/chrome_tests_unit.gypi
+++ b/chrome/chrome_tests_unit.gypi
@@ -240,6 +240,7 @@
       'browser/shell_integration_win_unittest.cc',
       'browser/signin/account_reconcilor_unittest.cc',
       'browser/signin/account_service_flag_fetcher_unittest.cc',
+      'browser/signin/chrome_signin_client_unittest.cc',
       'browser/signin/local_auth_unittest.cc',
       'browser/signin/signin_global_error_unittest.cc',
       'browser/signin/signin_manager_unittest.cc',
diff --git a/components/signin/core/browser/gaia_cookie_manager_service.cc b/components/signin/core/browser/gaia_cookie_manager_service.cc
index e1728c8..7eb1c28 100644
--- a/components/signin/core/browser/gaia_cookie_manager_service.cc
+++ b/components/signin/core/browser/gaia_cookie_manager_service.cc
@@ -318,8 +318,11 @@ void GaiaCookieManagerService::AddAccountToCookie(
   DCHECK(!account_id.empty());
   VLOG(1) << "GaiaCookieManagerService::AddAccountToCookie: " << account_id;
   requests_.push_back(GaiaCookieRequest::CreateAddAccountRequest(account_id));
-  if (requests_.size() == 1)
-    StartFetchingUbertoken();
+  if (requests_.size() == 1) {
+    signin_client_->DelayNetworkCall(
+        base::Bind(&GaiaCookieManagerService::StartFetchingUbertoken,
+                   base::Unretained(this)));
+  }
 }
 
 bool GaiaCookieManagerService::ListAccounts(
@@ -336,7 +339,9 @@ bool GaiaCookieManagerService::ListAccounts(
   if (!list_accounts_fetched_once_) {
     fetcher_retries_ = 0;
     requests_.push_back(GaiaCookieRequest::CreateListAccountsRequest());
-    StartFetchingListAccounts();
+    signin_client_->DelayNetworkCall(
+        base::Bind(&GaiaCookieManagerService::StartFetchingListAccounts,
+                   base::Unretained(this)));
     return false;
   }
 
@@ -386,7 +391,9 @@ void GaiaCookieManagerService::LogOutAllAccounts() {
     requests_.push_back(GaiaCookieRequest::CreateLogOutRequest());
     if (requests_.size() == 1) {
       fetcher_retries_ = 0;
-      StartLogOutUrlFetch();
+      signin_client_->DelayNetworkCall(
+          base::Bind(&GaiaCookieManagerService::StartLogOutUrlFetch,
+                     base::Unretained(this)));
     }
   }
 }
@@ -418,7 +425,9 @@ void GaiaCookieManagerService::OnCookieChanged(
   if (requests_.empty()) {
     requests_.push_back(GaiaCookieRequest::CreateListAccountsRequest());
     fetcher_retries_ = 0;
-    StartFetchingListAccounts();
+    signin_client_->DelayNetworkCall(
+        base::Bind(&GaiaCookieManagerService::StartFetchingListAccounts,
+                   base::Unretained(this)));
   } else {
     // Remove all pending ListAccount calls; for efficiency, only call
     // after all pending requests are processed.
@@ -479,7 +488,9 @@ void GaiaCookieManagerService::OnUbertokenSuccess(
     return;
   }
 
-  StartFetchingMergeSession();
+  signin_client_->DelayNetworkCall(
+      base::Bind(&GaiaCookieManagerService::StartFetchingMergeSession,
+                 base::Unretained(this)));
 }
 
 void GaiaCookieManagerService::OnUbertokenFailure(
@@ -516,8 +527,12 @@ void GaiaCookieManagerService::OnMergeSessionFailure(
   if (++fetcher_retries_ < kMaxFetcherRetries && IsTransientError(error)) {
     fetcher_backoff_.InformOfRequest(false);
     fetcher_timer_.Start(
-        FROM_HERE, fetcher_backoff_.GetTimeUntilRelease(), this,
-        &GaiaCookieManagerService::StartFetchingMergeSession);
+        FROM_HERE, fetcher_backoff_.GetTimeUntilRelease(),
+        base::Bind(&SigninClient::DelayNetworkCall,
+                   base::Unretained(signin_client_),
+                   base::Bind(
+                       &GaiaCookieManagerService::StartFetchingMergeSession,
+                       base::Unretained(this))));
     return;
   }
 
@@ -560,8 +575,12 @@ void GaiaCookieManagerService::OnListAccountsFailure(
   if (++fetcher_retries_ < kMaxFetcherRetries && IsTransientError(error)) {
     fetcher_backoff_.InformOfRequest(false);
     fetcher_timer_.Start(
-        FROM_HERE, fetcher_backoff_.GetTimeUntilRelease(), this,
-        &GaiaCookieManagerService::StartFetchingListAccounts);
+        FROM_HERE, fetcher_backoff_.GetTimeUntilRelease(),
+        base::Bind(&SigninClient::DelayNetworkCall,
+                   base::Unretained(signin_client_),
+                   base::Bind(
+                       &GaiaCookieManagerService::StartFetchingListAccounts,
+                       base::Unretained(this))));
     return;
   }
 
@@ -608,8 +627,12 @@ void GaiaCookieManagerService::OnURLFetchComplete(
       ++fetcher_retries_ < kMaxFetcherRetries) {
     fetcher_backoff_.InformOfRequest(false);
     fetcher_timer_.Start(
-        FROM_HERE, fetcher_backoff_.GetTimeUntilRelease(), this,
-        &GaiaCookieManagerService::StartLogOutUrlFetch);
+        FROM_HERE, fetcher_backoff_.GetTimeUntilRelease(),
+        base::Bind(&SigninClient::DelayNetworkCall,
+                   base::Unretained(signin_client_),
+                   base::Bind(
+                       &GaiaCookieManagerService::StartLogOutUrlFetch,
+                       base::Unretained(this))));
     return;
   }
 
@@ -639,14 +662,20 @@ void GaiaCookieManagerService::HandleNextRequest() {
   } else {
     switch (requests_.front().request_type()) {
       case GaiaCookieRequestType::ADD_ACCOUNT:
-        StartFetchingUbertoken();
+        signin_client_->DelayNetworkCall(
+            base::Bind(&GaiaCookieManagerService::StartFetchingUbertoken,
+                       base::Unretained(this)));
         break;
       case GaiaCookieRequestType::LOG_OUT:
-        StartLogOutUrlFetch();
+        signin_client_->DelayNetworkCall(
+            base::Bind(&GaiaCookieManagerService::StartLogOutUrlFetch,
+                       base::Unretained(this)));
         break;
       case GaiaCookieRequestType::LIST_ACCOUNTS:
         uber_token_fetcher_.reset();
-        StartFetchingListAccounts();
+        signin_client_->DelayNetworkCall(
+            base::Bind(&GaiaCookieManagerService::StartFetchingListAccounts,
+                       base::Unretained(this)));
         break;
     };
   }
diff --git a/components/signin/core/browser/gaia_cookie_manager_service.h b/components/signin/core/browser/gaia_cookie_manager_service.h
index d5b6c51..7349809 100644
--- a/components/signin/core/browser/gaia_cookie_manager_service.h
+++ b/components/signin/core/browser/gaia_cookie_manager_service.h
@@ -239,9 +239,12 @@ class GaiaCookieManagerService : public KeyedService,
   scoped_ptr<UbertokenFetcher> uber_token_fetcher_;
   ExternalCcResultFetcher external_cc_result_fetcher_;
 
-  // If the GaiaAuthFetcher or URLFetcher fails, retry with exponential backoff.
+  // If the GaiaAuthFetcher or URLFetcher fails, retry with exponential backoff
+  // and network delay.
   net::BackoffEntry fetcher_backoff_;
-  base::OneShotTimer<GaiaCookieManagerService> fetcher_timer_;
+  // We can safely depend on the SigninClient here because there is an explicit
+  // dependency, as noted in the GaiaCookieManagerServiceFactory.
+  base::OneShotTimer<SigninClient> fetcher_timer_;
   int fetcher_retries_;
 
   // The last fetched ubertoken, for use in MergeSession retries.
diff --git a/components/signin/core/browser/signin_client.h b/components/signin/core/browser/signin_client.h
index 2575e5d..2ccf7cd 100644
--- a/components/signin/core/browser/signin_client.h
+++ b/components/signin/core/browser/signin_client.h
@@ -123,6 +123,9 @@ class SigninClient : public KeyedService {
   // the core SigninClient.
   virtual ios::ProfileOAuth2TokenServiceIOSProvider* GetIOSProvider() = 0;
 #endif
+
+  // Execute |callback| if and when there is a network connection.
+  virtual void DelayNetworkCall(const base::Closure& callback) = 0;
 };
 
 #endif  // COMPONENTS_SIGNIN_CORE_BROWSER_SIGNIN_CLIENT_H_
diff --git a/components/signin/core/browser/test_signin_client.cc b/components/signin/core/browser/test_signin_client.cc
index 707a850..dd52bb2 100644
--- a/components/signin/core/browser/test_signin_client.cc
+++ b/components/signin/core/browser/test_signin_client.cc
@@ -128,3 +128,7 @@ void TestSigninClient::AddContentSettingsObserver(
 void TestSigninClient::RemoveContentSettingsObserver(
     content_settings::Observer* observer) {
 }
+
+void TestSigninClient::DelayNetworkCall(const base::Closure& callback) {
+  callback.Run();
+}
diff --git a/components/signin/core/browser/test_signin_client.h b/components/signin/core/browser/test_signin_client.h
index e1e3a47..50353b4 100644
--- a/components/signin/core/browser/test_signin_client.h
+++ b/components/signin/core/browser/test_signin_client.h
@@ -98,6 +98,7 @@ class TestSigninClient : public SigninClient {
       content_settings::Observer* observer) override;
   void RemoveContentSettingsObserver(
       content_settings::Observer* observer) override;
+  void DelayNetworkCall(const base::Closure& callback) override;
 
  private:
   // Loads the token database.