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

chrome/browser/signin/token_service.cc

Index: chrome/browser/signin/token_service.cc
diff --git a/chrome/browser/signin/token_service.cc b/chrome/browser/signin/token_service.cc
index 536dcfa85d6dd65ed41bfd19e84204c1950cd31c..e585989da5d8375806add65b581b2faeed25be09 100644
--- a/chrome/browser/signin/token_service.cc
+++ b/chrome/browser/signin/token_service.cc
@@ -369,10 +369,10 @@ void TokenService::LoadTokensIntoMemory(
       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());
     }
   }
 }