[Sync] Avoid unnecessary DB rewrite for TokenService credentials that were just read from DB

chrome/browser/signin/token_service.cc

 // Copyright (c) 2012 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/token_service.h"
 
 #include "base/basictypes.h"
 #include "base/command_line.h"
 #include "base/string_util.h"
 #include "chrome/browser/prefs/pref_service.h"
@@ skipping 351 matching lines @@
     std::string lsid;
     std::string sid;
 
     if (db_tokens.count(GaiaConstants::kGaiaLsid) > 0)
       lsid = db_tokens.find(GaiaConstants::kGaiaLsid)->second;
 
     if (db_tokens.count(GaiaConstants::kGaiaSid) > 0)
       sid = db_tokens.find(GaiaConstants::kGaiaSid)->second;
 
     if (!lsid.empty() && !sid.empty()) {
-      UpdateCredentials(GaiaAuthConsumer::ClientLoginResult(sid,
-                                                            lsid,
-                                                            std::string(),
-                                                            std::string()));
+      credentials_ = GaiaAuthConsumer::ClientLoginResult(sid,

[Andrew T Wilson (Slow), 2012/07/27 18:01:16]

Are you sure it's safe to skip all the other stuff UpdateCredentials() does
(like cancel running fetchers, sending CredentialsUpdatedNotification, etc)?

[Roger Tawa OOO till Jul 10th, 2012/07/27 18:18:35]

I think it may be safer to change only the persistence and not the rest of the
code.  Maybe write an internal version of UpdateCredendials() that take a bool
flag indicating whether the values should be persisted or not.

[Raghu Simha, 2012/07/27 21:04:35]

Here's my understanding of what's going on in LoadTokensIntoMemory. Please
correct me if I'm wrong :)

UpdateCredentials is meant to be called by other services (like SigninManager)
outside TokenService, to get TokenService to use a new lsid and sid to mint
tokens. It does 4 things:

1) Update the value of the in-memory member variable |credentials _|
2) Write the newly-updated tokens to DB (file)
3) Cancel any fetchers that might be pending
4) Inform the CredentialCacheService that we have new credentials (I added this
code)

LoadTokensIntoMemory (where this change is being made) is only called during
initialization, after tokens are read from DB, before any fetching is done. We
don't need to do #2, because we're reading the same tokens from DB, and it makes
no sense to write them back. We don't need to do #3 because there are no active
fetchers during initialization. We don't need to do #4 because the
CredentialCacheService is only interested in knowing when new tokens are being
written to DB, and we explicitly don't want to re-cache tokens if just read them
from the DB, because they would've gotten cached the last time UpdateCredentials
was called.

My contention is that it's sufficient to do just #1 in this spot, but if you
guys think there's something I'm missing, I'm happy to do more than just #1.

Let me know what you think.

[Roger Tawa OOO till Jul 10th, 2012/07/27 21:21:45]

True that this is called only during init, but it happens asynchronously.  Are
we sure there are no fetches that could be happening?  If you think there isn't,
then lets at least put an assert to check for this, just in case the code
changes in the future.

[Raghu Simha, 2012/07/27 21:37:01]

I stepped through the code with a debugger for sign-in, sign-out and auto-start
cases, and noticed in every case that the fetchers are null at the time of
initialization. I believe there are 2 things that ensure this:

1) TokenService::HasTokenForService protects us by returning false to all
services that ask for a token during initialization (see the comment above line
200 in this file). Therefore, fetchers are not created until all tokens are read
from DB, and then services that need tokens are woken up via notifications.
2) The browser window is not yet created at the time the TokenService is
initialized. Therefore, I believe it is impossible for user activity to create
fetchers during Initialize.

I like your idea of putting in a DCHECK to make sure there are no fetchers
during initialize. Patch coming up shortly.

+                                                         lsid,
+                                                         std::string(),
+                                                         std::string());
     }
   }
 }
 
 void TokenService::LoadSingleTokenIntoMemory(
     const std::map<std::string, std::string>& db_tokens,
     std::map<std::string, std::string>* in_memory_tokens,
     const std::string& service) {
   // OnIssueAuthTokenSuccess should come from the same thread.
   // If a token is already present in the map, it could only have
@@ skipping 15 matching lines @@
 }
 
 void TokenService::Observe(int type,
                            const content::NotificationSource& source,
                            const content::NotificationDetails& details) {
   DCHECK_EQ(type, chrome::NOTIFICATION_TOKEN_UPDATED);
   TokenAvailableDetails* tok_details =
       content::Details<TokenAvailableDetails>(details).ptr();
   OnIssueAuthTokenSuccess(tok_details->service(), tok_details->token());
 }