Revert of [GCM] Fetching OAuth2 tokens periodically in account tracker (patchset #7 id:140001 of https://codereview.chromium.org/631343002/)

Reason for revert:
Reverting since it's causing a crash on canary.

Original issue's description:
> [GCM] Fetching OAuth2 tokens periodically in account tracker
>
> BUG=374969
> R=zea@chromium.org
>
> Committed: https://crrev.com/ede4117a85d34a1c117169d538b297020e69de87
> Cr-Commit-Position: refs/heads/master@{#303340}

TBR=zea@chromium.org,fgorski@chromium.org
NOTREECHECKS=true
NOTRY=true
BUG=374969

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

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

6 files changed, 25 insertions, 200 deletions

diff --git a/chrome/browser/services/gcm/gcm_account_tracker.cc b/chrome/browser/services/gcm/gcm_account_tracker.cc
index 0521688..a2cdba0 100644
--- a/chrome/browser/services/gcm/gcm_account_tracker.cc
+++ b/chrome/browser/services/gcm/gcm_account_tracker.cc
@@ -15,18 +15,11 @@
 namespace gcm {
 
 namespace {
-
-// Scopes needed by the OAuth2 access tokens.
 const char kGCMGroupServerScope[] = "https://www.googleapis.com/auth/gcm";
 const char kGCMCheckinServerScope[] =
     "https://www.googleapis.com/auth/android_checkin";
-// Name of the GCM account tracker for the OAuth2TokenService.
 const char kGCMAccountTrackerName[] = "gcm_account_tracker";
-// Minimum token validity when sending to GCM groups server.
 const int64 kMinimumTokenValidityMs = 500;
-// Token reporting interval, when no account changes are detected.
-const int64 kTokenReportingIntervalMs = 12 * 60 * 60 * 1000;  // 12 hours in ms.
-
 }  // namespace
 
 GCMAccountTracker::AccountInfo::AccountInfo(const std::string& email,
@@ -43,8 +36,7 @@ GCMAccountTracker::GCMAccountTracker(
     : OAuth2TokenService::Consumer(kGCMAccountTrackerName),
       account_tracker_(account_tracker.release()),
       driver_(driver),
-      shutdown_called_(false),
-      reporting_weak_ptr_factory_(this) {
+      shutdown_called_(false) {
 }
 
 GCMAccountTracker::~GCMAccountTracker() {
@@ -64,6 +56,11 @@ void GCMAccountTracker::Start() {
   driver_->AddConnectionObserver(this);
 
   std::vector<gaia::AccountIds> accounts = account_tracker_->GetAccounts();
+  if (accounts.empty()) {
+    CompleteCollectingTokens();
+    return;
+  }
+
   for (std::vector<gaia::AccountIds>::const_iterator iter = accounts.begin();
        iter != accounts.end();
        ++iter) {
@@ -73,22 +70,7 @@ void GCMAccountTracker::Start() {
     }
   }
 
-  if (IsTokenReportingRequired())
-    ReportTokens();
-  else
-    ScheduleReportTokens();
-}
-
-void GCMAccountTracker::ScheduleReportTokens() {
-  DVLOG(1) << "Deferring the token reporting for: "
-           << GetTimeToNextTokenReporting().InSeconds() << " seconds.";
-
-  reporting_weak_ptr_factory_.InvalidateWeakPtrs();
-  base::MessageLoop::current()->PostDelayedTask(
-      FROM_HERE,
-      base::Bind(&GCMAccountTracker::ReportTokens,
-                 reporting_weak_ptr_factory_.GetWeakPtr()),
-      GetTimeToNextTokenReporting());
+  GetAllNeededTokens();
 }
 
 void GCMAccountTracker::OnAccountAdded(const gaia::AccountIds& ids) {
@@ -133,7 +115,7 @@ void GCMAccountTracker::OnGetTokenSuccess(
   }
 
   DeleteTokenRequest(request);
-  ReportTokens();
+  CompleteCollectingTokens();
 }
 
 void GCMAccountTracker::OnGetTokenFailure(
@@ -155,22 +137,21 @@ void GCMAccountTracker::OnGetTokenFailure(
   }
 
   DeleteTokenRequest(request);
-  ReportTokens();
+  CompleteCollectingTokens();
 }
 
 void GCMAccountTracker::OnConnected(const net::IPEndPoint& ip_endpoint) {
-  if (IsTokenReportingRequired())
-    ReportTokens();
+  if (SanitizeTokens())
+    GetAllNeededTokens();
 }
 
 void GCMAccountTracker::OnDisconnected() {
   // We are disconnected, so no point in trying to work with tokens.
 }
 
-void GCMAccountTracker::ReportTokens() {
-  SanitizeTokens();
+void GCMAccountTracker::CompleteCollectingTokens() {
   // Make sure all tokens are valid.
-  if (IsTokenFetchingRequired()) {
+  if (SanitizeTokens()) {
     GetAllNeededTokens();
     return;
   }
@@ -217,14 +198,13 @@ void GCMAccountTracker::ReportTokens() {
   if (!account_tokens.empty() || account_removed) {
     DVLOG(1) << "Reporting the tokens to driver: " << account_tokens.size();
     driver_->SetAccountTokens(account_tokens);
-    driver_->SetLastTokenFetchTime(base::Time::Now());
-    ScheduleReportTokens();
   } else {
     DVLOG(1) << "No tokens and nothing removed. Skipping callback.";
   }
 }
 
-void GCMAccountTracker::SanitizeTokens() {
+bool GCMAccountTracker::SanitizeTokens() {
+  bool tokens_needed = false;
   for (AccountInfos::iterator iter = account_infos_.begin();
        iter != account_infos_.end();
        ++iter) {
@@ -236,43 +216,12 @@ void GCMAccountTracker::SanitizeTokens() {
       iter->second.state = TOKEN_NEEDED;
       iter->second.expiration_time = base::Time();
     }
-  }
-}
-
-bool GCMAccountTracker::IsTokenReportingRequired() const {
-  if (GetTimeToNextTokenReporting() == base::TimeDelta())
-    return true;
-
-  bool reporting_required = false;
-  for (AccountInfos::const_iterator iter = account_infos_.begin();
-       iter != account_infos_.end();
-       ++iter) {
-    if (iter->second.state == ACCOUNT_REMOVED)
-      reporting_required = true;
-  }
 
-  return reporting_required;
-}
-
-bool GCMAccountTracker::IsTokenFetchingRequired() const {
-  bool token_needed = false;
-  for (AccountInfos::const_iterator iter = account_infos_.begin();
-       iter != account_infos_.end();
-       ++iter) {
     if (iter->second.state == TOKEN_NEEDED)
-      token_needed = true;
+      tokens_needed = true;
   }
 
-  return token_needed;
-}
-
-base::TimeDelta GCMAccountTracker::GetTimeToNextTokenReporting() const {
-  base::TimeDelta time_till_next_reporting =
-      driver_->GetLastTokenFetchTime() +
-      base::TimeDelta::FromMilliseconds(kTokenReportingIntervalMs) -
-      base::Time::Now();
-  return time_till_next_reporting < base::TimeDelta() ?
-             base::TimeDelta() : time_till_next_reporting;
+  return tokens_needed;
 }
 
 void GCMAccountTracker::DeleteTokenRequest(
@@ -286,9 +235,6 @@ void GCMAccountTracker::DeleteTokenRequest(
 void GCMAccountTracker::GetAllNeededTokens() {
   // Only start fetching tokens if driver is running, they have a limited
   // validity time and GCM connection is a good indication of network running.
-  // If the GetAllNeededTokens was called as part of periodic schedule, it may
-  // not have network. In that case the next network change will trigger token
-  // fetching.
   if (!driver_->IsConnected())
     return;
 
@@ -336,7 +282,7 @@ void GCMAccountTracker::OnAccountSignedOut(const gaia::AccountIds& ids) {
 
   iter->second.access_token.clear();
   iter->second.state = ACCOUNT_REMOVED;
-  ReportTokens();
+  CompleteCollectingTokens();
 }
 
 OAuth2TokenService* GCMAccountTracker::GetTokenService() {
diff --git a/chrome/browser/services/gcm/gcm_account_tracker.h b/chrome/browser/services/gcm/gcm_account_tracker.h
index 39f357e..0b1c5e8 100644
--- a/chrome/browser/services/gcm/gcm_account_tracker.h
+++ b/chrome/browser/services/gcm/gcm_account_tracker.h
@@ -14,21 +14,12 @@
 #include "google_apis/gaia/account_tracker.h"
 #include "google_apis/gaia/oauth2_token_service.h"
 
-namespace base {
-class Time;
-}
-
 namespace gcm {
 
 class GCMDriver;
 
 // Class for reporting back which accounts are signed into. It is only meant to
 // be used when the user is signed into sync.
-//
-// This class makes a check for tokens periodically, to make sure the user is
-// still logged into the profile, so that in the case that the user is not, we
-// can immediately report that to the GCM and stop messages addressed to that
-// user from ever reaching Chrome.
 class GCMAccountTracker : public gaia::AccountTracker::Observer,
                           public OAuth2TokenService::Consumer,
                           public GCMConnectionObserver {
@@ -85,8 +76,6 @@ class GCMAccountTracker : public gaia::AccountTracker::Observer,
   }
 
  private:
-  friend class GCMAccountTrackerTest;
-
   // Maps account keys to account states. Keyed by account_ids as used by
   // OAuth2TokenService.
   typedef std::map<std::string, AccountInfo> AccountInfos;
@@ -108,22 +97,13 @@ class GCMAccountTracker : public gaia::AccountTracker::Observer,
   void OnConnected(const net::IPEndPoint& ip_endpoint) override;
   void OnDisconnected() override;
 
-  // Schedules token reporting.
-  void ScheduleReportTokens();
   // Report the list of accounts with OAuth2 tokens back using the |callback_|
   // function. If there are token requests in progress, do nothing.
-  void ReportTokens();
+  void CompleteCollectingTokens();
   // Verify that all of the tokens are ready to be passed down to the GCM
   // Driver, e.g. none of them has expired or is missing. Returns true if not
   // all tokens are valid and a fetching yet more tokens is required.
-  void SanitizeTokens();
-  // Indicates whether token reporting is required, either because it is due, or
-  // some of the accounts were removed.
-  bool IsTokenReportingRequired() const;
-  // Indicates whether there are tokens that still need fetching.
-  bool IsTokenFetchingRequired() const;
-  // Gets the time until next token reporting.
-  base::TimeDelta GetTimeToNextTokenReporting() const;
+  bool SanitizeTokens();
   // Deletes a token request. Should be called from OnGetTokenSuccess(..) or
   // OnGetTokenFailure(..).
   void DeleteTokenRequest(const OAuth2TokenService::Request* request);
@@ -153,10 +133,6 @@ class GCMAccountTracker : public gaia::AccountTracker::Observer,
 
   ScopedVector<OAuth2TokenService::Request> pending_token_requests_;
 
-  // Creates weak pointers used to postpone reporting tokens. See
-  // ScheduleReportTokens.
-  base::WeakPtrFactory<GCMAccountTracker> reporting_weak_ptr_factory_;
-
   DISALLOW_COPY_AND_ASSIGN(GCMAccountTracker);
 };
 
diff --git a/chrome/browser/services/gcm/gcm_account_tracker_unittest.cc b/chrome/browser/services/gcm/gcm_account_tracker_unittest.cc
index eb8030a..2211d76 100644
--- a/chrome/browser/services/gcm/gcm_account_tracker_unittest.cc
+++ b/chrome/browser/services/gcm/gcm_account_tracker_unittest.cc
@@ -8,6 +8,8 @@
 #include <string>
 
 #include "base/memory/scoped_ptr.h"
+#include "base/message_loop/message_loop.h"
+#include "base/run_loop.h"
 #include "components/gcm_driver/fake_gcm_driver.h"
 #include "google_apis/gaia/fake_identity_provider.h"
 #include "google_apis/gaia/fake_oauth2_token_service.h"
@@ -74,8 +76,6 @@ class CustomFakeGCMDriver : public FakeGCMDriver {
   void AddConnectionObserver(GCMConnectionObserver* observer) override;
   void RemoveConnectionObserver(GCMConnectionObserver* observer) override;
   bool IsConnected() const override { return connected_; }
-  base::Time GetLastTokenFetchTime() override;
-  void SetLastTokenFetchTime(const base::Time& time) override;
 
   // Test results and helpers.
   void SetConnected(bool connected);
@@ -98,7 +98,6 @@ class CustomFakeGCMDriver : public FakeGCMDriver {
   GCMConnectionObserver* last_connection_observer_;
   GCMConnectionObserver* removed_connection_observer_;
   net::IPEndPoint ip_endpoint_;
-  base::Time last_token_fetch_time_;
 
   DISALLOW_COPY_AND_ASSIGN(CustomFakeGCMDriver);
 };
@@ -142,15 +141,6 @@ void CustomFakeGCMDriver::ResetResults() {
   removed_connection_observer_ = NULL;
 }
 
-
-base::Time CustomFakeGCMDriver::GetLastTokenFetchTime() {
-  return last_token_fetch_time_;
-}
-
-void CustomFakeGCMDriver::SetLastTokenFetchTime(const base::Time& time) {
-  last_token_fetch_time_ = time;
-}
-
 }  // namespace
 
 class GCMAccountTrackerTest : public testing::Test {
@@ -175,11 +165,6 @@ class GCMAccountTrackerTest : public testing::Test {
   GCMAccountTracker* tracker() { return tracker_.get(); }
   CustomFakeGCMDriver* driver() { return &driver_; }
 
-  // Accessors to private methods of account tracker.
-  bool IsFetchingRequired() const;
-  bool IsTokenReportingRequired() const;
-  base::TimeDelta GetTimeToNextTokenReporting() const;
-
  private:
   CustomFakeGCMDriver driver_;
 
@@ -252,18 +237,6 @@ void GCMAccountTrackerTest::IssueError(const std::string& account_key) {
       GoogleServiceAuthError(GoogleServiceAuthError::SERVICE_UNAVAILABLE));
 }
 
-bool GCMAccountTrackerTest::IsFetchingRequired() const {
-  return tracker_->IsTokenFetchingRequired();
-}
-
-bool GCMAccountTrackerTest::IsTokenReportingRequired() const {
-  return tracker_->IsTokenReportingRequired();
-}
-
-base::TimeDelta GCMAccountTrackerTest::GetTimeToNextTokenReporting() const {
-  return tracker_->GetTimeToNextTokenReporting();
-}
-
 TEST_F(GCMAccountTrackerTest, NoAccounts) {
   EXPECT_FALSE(driver()->update_accounts_called());
   tracker()->Start();
@@ -430,7 +403,7 @@ TEST_F(GCMAccountTrackerTest, PostponeTokenFetchingUntilConnected) {
   EXPECT_EQ(1UL, tracker()->get_pending_token_request_count());
 }
 
-TEST_F(GCMAccountTrackerTest, InvalidateExpiredTokens) {
+TEST_F(GCMAccountTrackerTest, IvalidateExpiredTokens) {
   StartAccountSignIn(kAccountId1);
   StartAccountSignIn(kAccountId2);
   tracker()->Start();
@@ -448,63 +421,6 @@ TEST_F(GCMAccountTrackerTest, InvalidateExpiredTokens) {
   EXPECT_EQ(1UL, tracker()->get_pending_token_request_count());
 }
 
-// Testing for whether there are still more tokens to be fetched. Typically the
-// need for token fetching triggers immediate request, unless there is no
-// connection, that is why connection is set on and off in this test.
-TEST_F(GCMAccountTrackerTest, IsTokenFetchingRequired) {
-  tracker()->Start();
-  driver()->SetConnected(false);
-  EXPECT_FALSE(IsFetchingRequired());
-  StartAccountSignIn(kAccountId1);
-  FinishAccountSignIn(kAccountId1);
-  EXPECT_TRUE(IsFetchingRequired());
-
-  driver()->SetConnected(true);
-  EXPECT_FALSE(IsFetchingRequired());  // Indicates that fetching has started.
-  IssueAccessToken(kAccountId1);
-  EXPECT_FALSE(IsFetchingRequired());
-
-  driver()->SetConnected(false);
-  StartAccountSignIn(kAccountId2);
-  FinishAccountSignIn(kAccountId2);
-  EXPECT_TRUE(IsFetchingRequired());
-
-  IssueExpiredAccessToken(kAccountId2);
-  // Make sure that if the token was expired it is still needed.
-  EXPECT_TRUE(IsFetchingRequired());
-}
-
-// Tests what is the expected time to the next token fetching.
-TEST_F(GCMAccountTrackerTest, GetTimeToNextTokenReporting) {
-  tracker()->Start();
-  // At this point the last token fetch time is never.
-  EXPECT_EQ(base::TimeDelta(), GetTimeToNextTokenReporting());
-
-  driver()->SetLastTokenFetchTime(base::Time::Now());
-  EXPECT_TRUE(GetTimeToNextTokenReporting() <=
-                  base::TimeDelta::FromSeconds(12 * 60 * 60));
-}
-
-// Tests conditions when token reporting is required.
-TEST_F(GCMAccountTrackerTest, IsTokenReportingRequired) {
-  tracker()->Start();
-  // Required because it is overdue.
-  EXPECT_TRUE(IsTokenReportingRequired());
-
-  // Not required because it just happened.
-  driver()->SetLastTokenFetchTime(base::Time::Now());
-  EXPECT_FALSE(IsTokenReportingRequired());
-
-  SignInAccount(kAccountId1);
-  IssueAccessToken(kAccountId1);
-  driver()->ResetResults();
-  // Reporting was triggered, which means testing for required will give false,
-  // but we have the update call.
-  SignOutAccount(kAccountId1);
-  EXPECT_TRUE(driver()->update_accounts_called());
-  EXPECT_FALSE(IsTokenReportingRequired());
-}
-
 // TODO(fgorski): Add test for adding account after removal >> make sure it does
 // not mark removal.
 
diff --git a/components/gcm_driver/gcm_client_impl.cc b/components/gcm_driver/gcm_client_impl.cc
index 4a68dc0..8fe3df7 100644
--- a/components/gcm_driver/gcm_client_impl.cc
+++ b/components/gcm_driver/gcm_client_impl.cc
@@ -474,7 +474,7 @@ void GCMClientImpl::RemoveAccountMapping(const std::string& account_id) {
 void GCMClientImpl::SetLastTokenFetchTime(const base::Time& time) {
   gcm_store_->SetLastTokenFetchTime(
       time,
-      base::Bind(&GCMClientImpl::IgnoreWriteResultCallback,
+      base::Bind(&GCMClientImpl::DefaultStoreCallback,
                  weak_ptr_factory_.GetWeakPtr()));
 }
 
@@ -597,11 +597,6 @@ void GCMClientImpl::DefaultStoreCallback(bool success) {
   DCHECK(success);
 }
 
-void GCMClientImpl::IgnoreWriteResultCallback(bool success) {
-  // TODO(fgorski): Ignoring the write result for now to make sure
-  // sync_intergration_tests are not broken.
-}
-
 void GCMClientImpl::Stop() {
   DVLOG(1) << "Stopping the GCM Client";
   weak_ptr_factory_.InvalidateWeakPtrs();
diff --git a/components/gcm_driver/gcm_client_impl.h b/components/gcm_driver/gcm_client_impl.h
index c27bb7b..b6a1ab5 100644
--- a/components/gcm_driver/gcm_client_impl.h
+++ b/components/gcm_driver/gcm_client_impl.h
@@ -230,9 +230,6 @@ class GCMClientImpl
   // |gcm_store_| fails.
   void DefaultStoreCallback(bool success);
 
-  // Callback for store operation where result does not matter.
-  void IgnoreWriteResultCallback(bool success);
-
   // Completes the registration request.
   void OnRegisterCompleted(const std::string& app_id,
                            const std::vector<std::string>& sender_ids,
diff --git a/components/gcm_driver/gcm_driver_desktop.cc b/components/gcm_driver/gcm_driver_desktop.cc
index 352007a..6902efb 100644
--- a/components/gcm_driver/gcm_driver_desktop.cc
+++ b/components/gcm_driver/gcm_driver_desktop.cc
@@ -363,10 +363,6 @@ GCMDriverDesktop::GCMDriverDesktop(
       gcm_enabled_(true),
       connected_(false),
       account_mapper_(new GCMAccountMapper(this)),
-      // Setting to max, to make sure it does not prompt for token reporting
-      // Before reading a reasonable value from the DB, which might be never,
-      // in which case the fetching will be triggered.
-      last_token_fetch_time_(base::Time::Max()),
       ui_thread_(ui_thread),
       io_thread_(io_thread),
       weak_ptr_factory_(this) {
@@ -776,10 +772,9 @@ void GCMDriverDesktop::GCMClientReady(
     const base::Time& last_token_fetch_time) {
   DCHECK(ui_thread_->RunsTasksOnCurrentThread());
 
-  last_token_fetch_time_ = last_token_fetch_time;
-
   GCMDriver::AddAppHandler(kGCMAccountMapperAppId, account_mapper_.get());
   account_mapper_->Initialize(account_mappings);
+  last_token_fetch_time_ = last_token_fetch_time;
 
   delayed_task_controller_->SetReady();
 }