This is very strange - I do not think we have change that piece of code. It looks like there was a problem reading the refresh tokens from disk. Did you have any other problem related to other data from the profile not being loaded?

Note that this DCHECK is useful as we want to make sure that refesh tokens are not lost by accident as then the user is silently signed out.

the profile itself doesn't load because of the version mismatch.

How does a DCHECK help with users? It only affects developers.

if we can't open SQLite databases in the profile, we show a dialog and ask the user to send a feedback report (search for IDS_PROFILE_TOO_NEW_ERROR). I think that'd be way better than a DCHECK & silent error in release

CC+ rogerta@

I've debugged this a bit more on Windows. Here is my understanding on what is going on:
1. Initializing the token web data db is done at the same time as the other SQL db and should display a send a feedback report if there is an error when this DB is loaded - see
https://cs.chromium.org/chromium/src/components/webdata_services/web_data_service_wrapper.cc?rcl=bf0871ef36416a7ac4bafba5bea3bdd8ad733f0c&l=108

2. If there is an error reading the token data from the SQL, then this error is silently ignores:
2.a. The method bool |TokenServiceTable::GetAllTokens| returns false if reading the tokens from the BD fails (see https://cs.chromium.org/chromium/src/components/signin/core/browser/webdata/token_service_table.cc?rcl=bf0871ef36416a7ac4bafba5bea3bdd8ad733f0c&l=91 )
2.b. The method |TokenWebDataBackend::GetAllTokens| ignores the return value of the GetAllTokens (see https://cs.chromium.org/chromium/src/components/signin/core/browser/webdata/token_web_data.cc?rcl=bf0871ef36416a7ac4bafba5bea3bdd8ad733f0c&l=51 )

I guess that if the SQL statement is invalid, then the tokens will not be loaded.

jochen@:
1. I do not think that we should present to the user a profile error dialog as in this case we successfully loaded the token DB, but fail to execute the SQL to read refresh tokens.
2. The DCHECK in the token service is added to defend against cases when we consistently fail to load the tokens (catch refactoring, incorrect data modal changes).

I was looking for other data types and it looks like we're ignoring the SQL statement return value as well - see https://cs.chromium.org/chromium/src/components/autofill/core/browser/webdata/autofill_webdata_backend_impl.cc?rcl=bf0871ef36416a7ac4bafba5bea3bdd8ad733f0c&l=241
https://cs.chromium.org/chromium/src/components/autofill/core/browser/autocomplete_history_manager.cc?rcl=bf0871ef36416a7ac4bafba5bea3bdd8ad733f0c&l=153

Note that the fix for the autocompletion was to keep the DCHECK and also return early in case of sql failures:
https://cs.chromium.org/chromium/src/components/autofill/core/browser/autocomplete_history_manager.cc?rcl=bf0871ef36416a7ac4bafba5bea3bdd8ad733f0c&l=157

Just to validate my assumptions here, when this happens again, may I ask you to add a breakpoint in |TokenServiceTable::GetAllTokens| and see if the SQL statement fails to execute.

Wrt comment #1, the user should not be signed out, they should be in an auth error state, which is the correct behaviour on desktop.  The auto-sign out thing is an iOS specific behaviour I believe.

Wrt comment #4, errors are ignored, but I don't think this code can do anything meaningful anyway.  Note the user will be notified something went wrong because they be in an auth error state, as mentioned above.

I've looked again and the DCHECK(!refresh_token.empty()) makes sense to me (this one https://cs.chromium.org/chromium/src/chrome/browser/signin/mutable_profile_oauth2_token_service_delegate.cc?rcl=ea5098bb3372baa8d32b117f1b687e739e075988&l=343)

Let me explain a bit more:
1. This DCHECK is hit when there is an entry in the web token data that has a an account id (named "service" in the TokenServiceTable class) but no refresh token for that.
2. Based on the code https://cs.chromium.org/chromium/src/components/signin/core/browser/webdata/token_service_table.cc?rcl=bf0871ef36416a7ac4bafba5bea3bdd8ad733f0c&l=91 , it looks like the only case when the service is non-nul and the token data is null is when we fail to decrypt the token.

As a conclusion, we hit this state in the following case:
* service (this actually value for this is "AcccountId-<gaia_id>") + encrypted token data (maybe empty) is successfully read from SQL DB for at least one account
* if the encrypted token data is empty or if we fail to decrypt it, then we'll add a service + empty token to the map of tokens - see https://cs.chromium.org/chromium/src/components/signin/core/browser/webdata/token_service_table.cc?rcl=bf0871ef36416a7ac4bafba5bea3bdd8ad733f0c&l=107

I think we should probably avoid adding a refresh token to the map of tokens if we fail to decrypt it.

I just ran this in gdb, and it seems that the SQL query works, but then we fail to decrypt the token.

Not adding the token would be great.

Roger: Do you have any idea why Chrome fails to decrypt a token on Windows? This seems like a bug on the windows API to crypt and decrypt APIs.

uh, sorry, I didn't notice that crbug helpfully marked this as a windows bug, I actually have this problem on Linux

I can step into the function if that helps

That would be great. I'd like to understand why this occurs as this bug may lead to people being signed out of Chrome (which is really bad).

Just to repeat: on desktop, this bug should never lead to people being signed out of chrome.  This should lead them to being in an auth error state, which is corrected by re-auth.

On windows, I'm not sure I would attribute the error to the api.  Chrome asks windows to encrypt the data then chrome persists the data separately.

- If the data becomes corrupt, it cannot be decrypted by windows.
- The user may have a way to rotate the keys used for encryption (I'm not actually sure though).  Chrome does not have any support for that.

On linux, I believe the tokens are stored in the keychain.  If you change your login password then chrome does not have access to the tokens.  There may also be others that the keychain becomes inaccessible.

Not adding invalid tokens to the map might be OK.  Would need to double check code that enumerates accounts in the profile via OAuth2TokenService::GetAccounts() to make sure they still work.  That is, see if they do drastically different things when there is no account vs an account in auth error state.

You are right, Roger, when I ment sign out of Chrome I was referring to auth error state. Thank you for clarifying that.

From the code, I think we always save the token data in SQL all the time (see https://cs.chromium.org/chromium/src/components/signin/core/browser/webdata/token_service_table.cc ), using the OSCrypt::EncryptString and OSCrypt::DecryptString functions. I think we're justing per-platform implementation of this fucctions (see https://cs.chromium.org/chromium/src/components/os_crypt/os_crypt_linux.cc for linux and https://cs.chromium.org/chromium/src/components/os_crypt/os_crypt_mac.mm for macOS). However, I only debugged this code on windows, but I'll double check tomorrow on macOS to make sure.

It fails here: https://cs.chromium.org/chromium/src/third_party/boringssl/src/crypto/cipher/cipher.c?rcl=358baeb9a4df81ccb916a5b8cf0ed059087645bd&l=461

n is 184, b is 16, ctx->buf_len is 0, ctx->final_used is 1

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ee20ae90108326aab2178aa1d102b18bf9081d93

commit ee20ae90108326aab2178aa1d102b18bf9081d93
Author: msarda <msarda@chromium.org>
Date: Tue Feb 07 13:13:53 2017

Avoid loading an empty token when decrypt failed

This CL avoids loading an empty token when the decrypt fails. It also
add logs and user metrics for errors during the load of the tokens
from the DB in order to measure if there are platforms on which this
occurs repeatedly.

BUG= 686485 

Review-Url: https://codereview.chromium.org/2672603003
Cr-Commit-Position: refs/heads/master@{#448609}

[modify] https://crrev.com/ee20ae90108326aab2178aa1d102b18bf9081d93/chrome/browser/signin/mutable_profile_oauth2_token_service_delegate.cc
[modify] https://crrev.com/ee20ae90108326aab2178aa1d102b18bf9081d93/components/signin/core/browser/webdata/token_service_table.cc
[modify] https://crrev.com/ee20ae90108326aab2178aa1d102b18bf9081d93/tools/metrics/histograms/histograms.xml

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fd932ab936187fa497e6744b9a299755a7311af5

commit fd932ab936187fa497e6744b9a299755a7311af5
Author: msarda <msarda@chromium.org>
Date: Thu Feb 09 14:57:13 2017

[Desktop] Add diagnostics about loading the refresh tokens.

This CL adds a new line in the sign-in internals that logs the state
of the last load of credentials. This would enable debugging cases
when the profiles enters an invalid credentials auth error state.

Screenshot:
https://drive.google.com/open?id=0Bw1MJ8m7U5kbOV9Yb2hUTVpwMjA

TBR=pkasting@chromium.org

BUG= 686485 

Review-Url: https://codereview.chromium.org/2672833002
Cr-Commit-Position: refs/heads/master@{#449290}

[modify] https://crrev.com/fd932ab936187fa497e6744b9a299755a7311af5/chrome/browser/signin/mutable_profile_oauth2_token_service_delegate.cc
[modify] https://crrev.com/fd932ab936187fa497e6744b9a299755a7311af5/chrome/browser/signin/mutable_profile_oauth2_token_service_delegate.h
[modify] https://crrev.com/fd932ab936187fa497e6744b9a299755a7311af5/chrome/browser/signin/mutable_profile_oauth2_token_service_delegate_unittest.cc
[modify] https://crrev.com/fd932ab936187fa497e6744b9a299755a7311af5/components/signin/core/browser/about_signin_internals.cc
[modify] https://crrev.com/fd932ab936187fa497e6744b9a299755a7311af5/components/signin/core/browser/about_signin_internals.h
[modify] https://crrev.com/fd932ab936187fa497e6744b9a299755a7311af5/components/signin/core/browser/signin_internals_util.h
[modify] https://crrev.com/fd932ab936187fa497e6744b9a299755a7311af5/components/signin/core/browser/webdata/token_service_table.cc
[modify] https://crrev.com/fd932ab936187fa497e6744b9a299755a7311af5/components/signin/core/browser/webdata/token_service_table.h
[modify] https://crrev.com/fd932ab936187fa497e6744b9a299755a7311af5/components/signin/core/browser/webdata/token_web_data.cc
[modify] https://crrev.com/fd932ab936187fa497e6744b9a299755a7311af5/components/signin/core/browser/webdata/token_web_data.h
[modify] https://crrev.com/fd932ab936187fa497e6744b9a299755a7311af5/components/webdata/common/web_data_results.h
[modify] https://crrev.com/fd932ab936187fa497e6744b9a299755a7311af5/google_apis/gaia/oauth2_token_service_delegate.cc
[modify] https://crrev.com/fd932ab936187fa497e6744b9a299755a7311af5/google_apis/gaia/oauth2_token_service_delegate.h

FYI - just checking out the data from 58 beta so far, looks like the decryption fails about .5% of the time: https://uma.googleplex.com/p/chrome/histograms/?endDate=latest&dayCount=7&histograms=Signin.TokenTable.GetAllTokensSqlStatementValidity%2CSignin.TokenTable.ReadTokenFromDBResult&fixupData=true&showMax=true&filters=channel%2Ceq%2C3%2Cplatform%2Cone_of%2CW%7CM%7CL%2Cisofficial%2Ceq%2CTrue&implicitFilters=isofficial

Mihai, is this roughly what you'd expect?

I had no expectations - I wanted to get this information to see how many of our users are in this unexpected state. 0.5% do not see too much, but they are still an important number.

Let's check this also on stable.

FYI still looks like 0.5% on Stable: https://uma.googleplex.com/p/chrome/histograms/?endDate=latest&dayCount=7&histograms=Signin.TokenTable.GetAllTokensSqlStatementValidity%2CSignin.TokenTable.ReadTokenFromDBResult&fixupData=true&showMax=true&filters=channel%2Ceq%2C4%2Cplatform%2Cone_of%2CW%7CM%7CL%2Cisofficial%2Ceq%2CTrue&implicitFilters=isofficial.

Seems pretty low-ish to me, but I'm curious why we fail to load/decrypt the token 0.5% of the time.