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

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

When the token service is initialized, tokens are read from disk via LoadTokensFromDB. The callback for this function after the DB read completes is LoadTokensIntoMemory.

When LoadTokensIntoMemory is run, it calls UpdateCredentials, which implicitly rewrites the just-read credentials back to the DB. This DB write is unnecessary. An unintended side effect is that the credential caching service does an unnecessary file-write to "Sync Credentials" every single time Chrome is restarted, even though the sync credentials haven't changed in any way.

This patch modifies LoadTokensIntoMemory to merely update |credentials_|, the in-memory credentials object, without updating the DB too.

BUG=139263
TEST=unit_tests; Sign in to sync, restart chrome, and make sure TokenService credentials are not re-written to DB during initialization.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=148863

[Raghu Simha, 2012-07-27 00:48:40]

Roger, please review. Thanks!

[Raghu Simha, 2012-07-27 17:50:24]

DIdn't see Roger in today. Drew, could you take a look, please?

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

http://codereview.chromium.org/10829051/diff/1/chrome/browser/signin/token_se...
File chrome/browser/signin/token_service.cc (right):

http://codereview.chromium.org/10829051/diff/1/chrome/browser/signin/token_se...
chrome/browser/signin/token_service.cc:372: credentials_ =
GaiaAuthConsumer::ClientLoginResult(sid,
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]

http://codereview.chromium.org/10829051/diff/1/chrome/browser/signin/token_se...
File chrome/browser/signin/token_service.cc (right):

http://codereview.chromium.org/10829051/diff/1/chrome/browser/signin/token_se...
chrome/browser/signin/token_service.cc:372: credentials_ =
GaiaAuthConsumer::ClientLoginResult(sid,
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.

[Andrew T Wilson (Slow), 2012-07-27 18:20:33]

Actually, can we just change UpdateCredentials to do nothing if the credentials
themselves have not changed (the new values are the same as the current values
in memory)? That seems like the very cleanest fix.

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

On 2012/07/27 18:20:33, Andrew T Wilson wrote:
> Actually, can we just change UpdateCredentials to do nothing if the
credentials
> themselves have not changed (the new values are the same as the current values
> in memory)? That seems like the very cleanest fix.

This sounds good, and I'm happy to do so. But I don't think this will solve the
original problem of re-writing tokens to the DB immediately after we read them,
because |credentials_| is going to be empty at the start of
TokenService::Initialize, right?

http://codereview.chromium.org/10829051/diff/1/chrome/browser/signin/token_se...
File chrome/browser/signin/token_service.cc (right):

http://codereview.chromium.org/10829051/diff/1/chrome/browser/signin/token_se...
chrome/browser/signin/token_service.cc:372: credentials_ =
GaiaAuthConsumer::ClientLoginResult(sid,
On 2012/07/27 18:18:35, Roger Tawa wrote:
> 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.

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]

http://r885-1d6aae961b36-rogerta.chromiumcodereview-hr.appspot.com/10829051/d...
File chrome/browser/signin/token_service.cc (right):

http://r885-1d6aae961b36-rogerta.chromiumcodereview-hr.appspot.com/10829051/d...
chrome/browser/signin/token_service.cc:372: credentials_ =
GaiaAuthConsumer::ClientLoginResult(sid,
On 2012/07/27 21:04:35, rsimha wrote:
> On 2012/07/27 18:18:35, Roger Tawa wrote:
> > 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.
> 
> 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.

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.

[Andrew T Wilson (Slow), 2012-07-27 21:35:39]

LGTM with Roger's suggestion of an assert.

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

http://codereview.chromium.org/10829051/diff/1/chrome/browser/signin/token_se...
File chrome/browser/signin/token_service.cc (right):

http://codereview.chromium.org/10829051/diff/1/chrome/browser/signin/token_se...
chrome/browser/signin/token_service.cc:372: credentials_ =
GaiaAuthConsumer::ClientLoginResult(sid,
On 2012/07/27 21:21:45, Roger Tawa wrote:
> 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.

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.

[commit-bot: I haz the power, 2012-07-27 23:14:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsimha@chromium.org/10829051/9002

[commit-bot: I haz the power, 2012-07-28 01:20:50]

Change committed as 148863