[signin] Delay token revocation until network is available

Token revocation is only best effort and has no guarantee to go through.
However existing code was just giving up if the network was down.
This CL improves the behavior, by waiting until the network is available
before sending the revocation request.

Bug: 824791
Change-Id: I1bed2b1d28e5e827a07be0797fe6a56a38778a29
Reviewed-on: https://chromium-review.googlesource.com/1004616
Commit-Queue: David Roger <droger@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550576}

chrome/browser/signin/mutable_profile_oauth2_token_service_delegate.cc

@@ -9,7 +9,9 @@
 #include <map>
 #include <string>
 
+#include "base/bind.h"
 #include "base/macros.h"
+#include "base/memory/weak_ptr.h"
 #include "base/metrics/histogram_macros.h"
 #include "components/pref_registry/pref_registry_syncable.h"
 #include "components/prefs/pref_service.h"
@@ -200,15 +202,21 @@ class MutableProfileOAuth2TokenServiceDelegate::RevokeServerRefreshToken
  public:
   RevokeServerRefreshToken(
       MutableProfileOAuth2TokenServiceDelegate* token_service_delegate,
-      const std::string& account_id);
+      SigninClient* client,
+      const std::string& refresh_token);
   ~RevokeServerRefreshToken() override;
 
+  // Starts the network request.
+  static void Start(base::WeakPtr<RevokeServerRefreshToken> rsrt,
+                    const std::string& refresh_token);
+
  private:
   // GaiaAuthConsumer overrides:
   void OnOAuth2RevokeTokenCompleted() override;
 
   MutableProfileOAuth2TokenServiceDelegate* token_service_delegate_;
   GaiaAuthFetcher fetcher_;
+  base::WeakPtrFactory<RevokeServerRefreshToken> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(RevokeServerRefreshToken);
 };
@@ -216,12 +224,26 @@ class MutableProfileOAuth2TokenServiceDelegate::RevokeServerRefreshToken
 MutableProfileOAuth2TokenServiceDelegate::RevokeServerRefreshToken::
     RevokeServerRefreshToken(
         MutableProfileOAuth2TokenServiceDelegate* token_service_delegate,
+        SigninClient* client,
         const std::string& refresh_token)
     : token_service_delegate_(token_service_delegate),
       fetcher_(this,
                GaiaConstants::kChromeSource,
-               token_service_delegate_->GetRequestContext()) {
-  fetcher_.StartRevokeOAuth2Token(refresh_token);
+               token_service_delegate_->GetRequestContext()),
+      weak_ptr_factory_(this) {
+  client->DelayNetworkCall(
+      base::Bind(&MutableProfileOAuth2TokenServiceDelegate::
+                     RevokeServerRefreshToken::Start,
+                 weak_ptr_factory_.GetWeakPtr(), refresh_token));
+}
+
+// static
+void MutableProfileOAuth2TokenServiceDelegate::RevokeServerRefreshToken::Start(
+    base::WeakPtr<RevokeServerRefreshToken> rsrt,
+    const std::string& refresh_token) {
+  if (!rsrt)
+    return;
+  rsrt->fetcher_.StartRevokeOAuth2Token(refresh_token);
 }
 
 MutableProfileOAuth2TokenServiceDelegate::RevokeServerRefreshToken::
@@ -779,7 +801,7 @@ void MutableProfileOAuth2TokenServiceDelegate::RevokeCredentialsOnServer(
   // Keep track or all server revoke requests.  This way they can be deleted
   // before the token service is shutdown and won't outlive the profile.
   server_revokes_.push_back(
-      std::make_unique<RevokeServerRefreshToken>(this, refresh_token));
+      std::make_unique<RevokeServerRefreshToken>(this, client_, refresh_token));
 }
 
 void MutableProfileOAuth2TokenServiceDelegate::CancelWebTokenFetch() {

chrome/browser/signin/mutable_profile_oauth2_token_service_delegate.h

@@ -126,6 +126,10 @@ class MutableProfileOAuth2TokenServiceDelegate
                            PersistenceLoadCredentials);
   FRIEND_TEST_ALL_PREFIXES(MutableProfileOAuth2TokenServiceDelegateTest,
                            RevokeOnUpdate);
+  FRIEND_TEST_ALL_PREFIXES(MutableProfileOAuth2TokenServiceDelegateTest,
+                           DelayedRevoke);
+  FRIEND_TEST_ALL_PREFIXES(MutableProfileOAuth2TokenServiceDelegateTest,
+                           ShutdownDuringRevoke);
   FRIEND_TEST_ALL_PREFIXES(MutableProfileOAuth2TokenServiceDelegateTest,
                            UpdateInvalidToken);
   FRIEND_TEST_ALL_PREFIXES(MutableProfileOAuth2TokenServiceDelegateTest,

chrome/browser/signin/mutable_profile_oauth2_token_service_delegate_unittest.cc

@@ -706,6 +706,41 @@ TEST_F(MutableProfileOAuth2TokenServiceDelegateTest, RevokeOnUpdate) {
   EXPECT_TRUE(oauth2_service_delegate_->server_revokes_.empty());
 }
 
+TEST_F(MutableProfileOAuth2TokenServiceDelegateTest, DelayedRevoke) {
+  client_->SetNetworkCallsDelayed(true);
+  CreateOAuth2ServiceDelegate(signin::AccountConsistencyMethod::kDisabled);
+  oauth2_service_delegate_->UpdateCredentials("account_id", "refresh_token");
+  EXPECT_TRUE(oauth2_service_delegate_->server_revokes_.empty());
+  oauth2_service_delegate_->RevokeCredentials("account_id");
+
+  // The revoke does not start until network calls are unblocked.
+  EXPECT_EQ(1u, oauth2_service_delegate_->server_revokes_.size());
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(1u, oauth2_service_delegate_->server_revokes_.size());
+
+  // Unblock network calls, and check that the revocation goes through.
+  client_->SetNetworkCallsDelayed(false);
+  base::RunLoop().RunUntilIdle();
+  EXPECT_TRUE(oauth2_service_delegate_->server_revokes_.empty());
+}
+
+TEST_F(MutableProfileOAuth2TokenServiceDelegateTest, ShutdownDuringRevoke) {
+  // Shutdown cancels the revocation.
+  client_->SetNetworkCallsDelayed(true);
+  CreateOAuth2ServiceDelegate(signin::AccountConsistencyMethod::kDisabled);
+  oauth2_service_delegate_->UpdateCredentials("account_id", "refresh_token");
+  oauth2_service_delegate_->RevokeCredentials("account_id");
+  EXPECT_EQ(1u, oauth2_service_delegate_->server_revokes_.size());
+
+  // Shutdown.
+  oauth2_service_delegate_->Shutdown();
+  EXPECT_TRUE(oauth2_service_delegate_->server_revokes_.empty());
+
+  // Unblocking network calls after shutdown does not crash.
+  client_->SetNetworkCallsDelayed(false);
+  base::RunLoop().RunUntilIdle();
+}
+
 TEST_F(MutableProfileOAuth2TokenServiceDelegateTest, UpdateInvalidToken) {
   // Add the invalid token.
   CreateOAuth2ServiceDelegate(signin::AccountConsistencyMethod::kDisabled);
@@ -868,7 +903,7 @@ TEST_F(MutableProfileOAuth2TokenServiceDelegateTest, RetryBackoff) {
   EXPECT_EQ(1, access_token_failure_count_);
   // Expect a positive backoff time.
   EXPECT_GT(oauth2_service_delegate_->backoff_entry_.GetTimeUntilRelease(),
-      TimeDelta());
+            TimeDelta());
 
   // Pretend that backoff has expired and try again.
   oauth2_service_delegate_->backoff_entry_.SetCustomReleaseTime(

components/signin/core/browser/test_signin_client.cc

@@ -14,7 +14,9 @@
 #include "testing/gtest/include/gtest/gtest.h"
 
 TestSigninClient::TestSigninClient(PrefService* pref_service)
-    : pref_service_(pref_service), are_signin_cookies_allowed_(true) {}
+    : pref_service_(pref_service),
+      are_signin_cookies_allowed_(true),
+      network_calls_delayed_(false) {}
 
 TestSigninClient::~TestSigninClient() {}
 
@@ -79,6 +81,16 @@ TestSigninClient::AddCookieChangeCallback(const GURL& url,
   return std::make_unique<SigninClient::CookieChangeSubscription>();
 }
 
+void TestSigninClient::SetNetworkCallsDelayed(bool value) {
+  network_calls_delayed_ = value;
+
+  if (!network_calls_delayed_) {
+    for (base::OnceClosure& call : delayed_network_calls_)
+      std::move(call).Run();
+    delayed_network_calls_.clear();
+  }
+}
+
 bool TestSigninClient::IsFirstRun() const {
   return false;
 }
@@ -100,7 +112,11 @@ void TestSigninClient::RemoveContentSettingsObserver(
 }
 
 void TestSigninClient::DelayNetworkCall(const base::Closure& callback) {
-  callback.Run();
+  if (network_calls_delayed_) {
+    delayed_network_calls_.push_back(callback);
+  } else {
+    callback.Run();
+  }
 }
 
 std::unique_ptr<GaiaAuthFetcher> TestSigninClient::CreateGaiaAuthFetcher(

components/signin/core/browser/test_signin_client.h

@@ -6,7 +6,10 @@
 #define COMPONENTS_SIGNIN_CORE_BROWSER_TEST_SIGNIN_CLIENT_H_
 
 #include <memory>
+#include <string>
+#include <vector>
 
+#include "base/callback_forward.h"
 #include "base/compiler_specific.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/macros.h"
@@ -78,6 +81,12 @@ class TestSigninClient : public SigninClient {
     are_signin_cookies_allowed_ = value;
   }
 
+  // When |value| is true, network calls posted through DelayNetworkCall() are
+  // delayed indefinitely.
+  // When |value| is false, all pending calls are unblocked, and new calls are
+  // executed immediately.
+  void SetNetworkCallsDelayed(bool value);
+
   // SigninClient overrides:
   bool IsFirstRun() const override;
   base::Time GetInstallDate() override;
@@ -102,6 +111,8 @@ class TestSigninClient : public SigninClient {
   scoped_refptr<TokenWebData> database_;
   PrefService* pref_service_;
   bool are_signin_cookies_allowed_;
+  bool network_calls_delayed_;
+  std::vector<base::OnceClosure> delayed_network_calls_;
 
   // Pointer to be filled by PostSignedIn.
   std::string signed_in_password_;