Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2672603003:
Avoid loading an empty token when decrypt failed (Closed)
DescriptionAvoid 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}
Committed: https://chromium.googlesource.com/chromium/src/+/ee20ae90108326aab2178aa1d102b18bf9081d93
Patch Set 1 #Patch Set 2 : Nit #Patch Set 3 : Fix compile #
Total comments: 2
Patch Set 4 : Histograms #
Total comments: 6
Patch Set 5 : Fix histogram names #
Messages
Total messages: 30 (11 generated)
msarda@chromium.org changed reviewers: + jochen@chromium.org, rogerta@chromium.org
Please take a look.
msarda@chromium.org changed reviewers: + asvitkine@chromium.org
Alexei: Please take a look at the changes in actions.xml. Thank you.
lgtm, thx
https://codereview.chromium.org/2672603003/diff/40001/components/signin/core/... File components/signin/core/browser/webdata/token_service_table.cc (right): https://codereview.chromium.org/2672603003/diff/40001/components/signin/core/... components/signin/core/browser/webdata/token_service_table.cc:100: base::UserMetricsAction("Signin_TokenTable_GetAllTokensInvalidSql")); These don't look like actions initiated by a user. You should use an enumerated histogram instead.
With this change, if you have account consistency enabled on desktop and you have a problem decrypting a token, does the account just disappear or do you get a re-auth error?
Patchset #4 (id:60001) has been deleted
The CQ bit was checked by msarda@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/02/02 19:03:44, Roger Tawa wrote: > With this change, if you have account consistency enabled on desktop and you > have a problem decrypting a token, does the account just disappear or do you get > a re-auth error? I've tested this and there are the following cases: 1. Before my change (if we fail to decrypt the token): a. We hit the DCHECK explained in the b. If we fail to decrypt the primary account, then we get an auth error. c. If we fail to decrypt a secondary account, then we get an auth error - see screenshot https://drive.google.com/open?id=0Bw1MJ8m7U5kbcFJSNEluYXQ1ZEE . The message in this auth error is wrong though, as sync is still working. 2. After my change (if we fail to decrypt the token): a. If we fail to decrypt the primary account, then we get an auth error. b. If we fail to decrypt a secondary account, then the seconday account is simply removed from the cookies. I am not 100% sure I know what the right behavior would be here. If we want to keep the existing behavior, then it would be wise to remove the DCHECK
https://codereview.chromium.org/2672603003/diff/40001/components/signin/core/... File components/signin/core/browser/webdata/token_service_table.cc (right): https://codereview.chromium.org/2672603003/diff/40001/components/signin/core/... components/signin/core/browser/webdata/token_service_table.cc:100: base::UserMetricsAction("Signin_TokenTable_GetAllTokensInvalidSql")); On 2017/02/02 17:22:13, Alexei Svitkine (very slow) wrote: > These don't look like actions initiated by a user. You should use an enumerated > histogram instead. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I've tested this and there are the following cases: 1. Before my change (if we fail to decrypt the token): a. We hit the DCHECK explained in the b. If we fail to decrypt the primary account, then we get an auth error. c. If we fail to decrypt a secondary account, then we get an auth error - see screenshot https://drive.google.com/open?id=0Bw1MJ8m7U5kbcFJSNEluYXQ1ZEE . The message in this auth error is wrong though, as sync is still working. 2. After my change (if we fail to decrypt the token): a. If we fail to decrypt the primary account, then we get an auth error. b. If we fail to decrypt a secondary account, then the seconday account is simply removed from the cookies. I am not 100% sure I know what the right behavior would be here. If we want to keep the existing behavior, then it would be wise to remove the DCHECK
On 2017/02/06 13:04:43, msarda wrote: > I've tested this and there are the following cases: > > 1. Before my change (if we fail to decrypt the token): > a. We hit the DCHECK explained in the > b. If we fail to decrypt the primary account, then we get an auth error. > c. If we fail to decrypt a secondary account, then we get an auth error - see > screenshot https://drive.google.com/open?id=0Bw1MJ8m7U5kbcFJSNEluYXQ1ZEE . The > message in this auth error is wrong though, as sync is still working. > > 2. After my change (if we fail to decrypt the token): > a. If we fail to decrypt the primary account, then we get an auth error. > b. If we fail to decrypt a secondary account, then the seconday account is > simply removed from the cookies. > > I am not 100% sure I know what the right behavior would be here. If we want to > keep the existing behavior, then it would be wise to remove the DCHECK OK. I believe 2b is not correct behaviour, at least that's not what I would expect from account consistency.
On 2017/02/06 14:42:10, Roger Tawa wrote: > On 2017/02/06 13:04:43, msarda wrote: > > I've tested this and there are the following cases: > > > > 1. Before my change (if we fail to decrypt the token): > > a. We hit the DCHECK explained in the > > b. If we fail to decrypt the primary account, then we get an auth error. > > c. If we fail to decrypt a secondary account, then we get an auth error - see > > screenshot https://drive.google.com/open?id=0Bw1MJ8m7U5kbcFJSNEluYXQ1ZEE . The > > message in this auth error is wrong though, as sync is still working. > > > > 2. After my change (if we fail to decrypt the token): > > a. If we fail to decrypt the primary account, then we get an auth error. > > b. If we fail to decrypt a secondary account, then the seconday account is > > simply removed from the cookies. > > > > I am not 100% sure I know what the right behavior would be here. If we want to > > keep the existing behavior, then it would be wise to remove the DCHECK > > OK. I believe 2b is not correct behaviour, at least that's not what I would > expect from account consistency. I suppose then that I would need to add a fake string (maybe something like "token_cannot_be_decrypted") or leave the token field empty and remove the DCHECK. Would that make sense?
https://codereview.chromium.org/2672603003/diff/80001/components/signin/core/... File components/signin/core/browser/webdata/token_service_table.cc (right): https://codereview.chromium.org/2672603003/diff/80001/components/signin/core/... components/signin/core/browser/webdata/token_service_table.cc:138: READ_ONE_TOKEN_MAX_VALUE); Please refactor the code so that this macro only appears once for this histogram. Either make a helper function or use a local var here and log it below. Each macro expands to a bunch of code, so extra macros for the same histogram cause unnecessary bloat. https://codereview.chromium.org/2672603003/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2672603003/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:62705: +<histogram name="Signin.TokenTable.GetAllTokensSqlStatement" Nit: Make the histogram name refer to what it's actually logging - i.e. the name doesn't imply that it's logging whether it's valid... https://codereview.chromium.org/2672603003/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:62713: +<histogram name="Signin.TokenTable.ReadTokenFromDB" Nit: Same comment as above. e.g. maybe ReadTokenFromDB.Result?
On 2017/02/06 15:35:51, Alexei Svitkine (very slow) wrote: > https://codereview.chromium.org/2672603003/diff/80001/components/signin/core/... > File components/signin/core/browser/webdata/token_service_table.cc (right): > > https://codereview.chromium.org/2672603003/diff/80001/components/signin/core/... > components/signin/core/browser/webdata/token_service_table.cc:138: > READ_ONE_TOKEN_MAX_VALUE); > Please refactor the code so that this macro only appears once for this > histogram. > > Either make a helper function or use a local var here and log it below. > > Each macro expands to a bunch of code, so extra macros for the same histogram > cause unnecessary bloat. > > https://codereview.chromium.org/2672603003/diff/80001/tools/metrics/histogram... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/2672603003/diff/80001/tools/metrics/histogram... > tools/metrics/histograms/histograms.xml:62705: +<histogram > name="Signin.TokenTable.GetAllTokensSqlStatement" > Nit: Make the histogram name refer to what it's actually logging - i.e. the name > doesn't imply that it's logging whether it's valid... > > https://codereview.chromium.org/2672603003/diff/80001/tools/metrics/histogram... > tools/metrics/histograms/histograms.xml:62713: +<histogram > name="Signin.TokenTable.ReadTokenFromDB" > Nit: Same comment as above. e.g. maybe ReadTokenFromDB.Result? lgtm Sorry, my bad Mihai. I re-read your message and you said "removed from cookie". I had read "removed from chrome". Removed from cookie is correct and what I would expect.
https://codereview.chromium.org/2672603003/diff/80001/components/signin/core/... File components/signin/core/browser/webdata/token_service_table.cc (right): https://codereview.chromium.org/2672603003/diff/80001/components/signin/core/... components/signin/core/browser/webdata/token_service_table.cc:138: READ_ONE_TOKEN_MAX_VALUE); On 2017/02/06 15:35:51, Alexei Svitkine (very slow) wrote: > Please refactor the code so that this macro only appears once for this > histogram. > > Either make a helper function or use a local var here and log it below. > > Each macro expands to a bunch of code, so extra macros for the same histogram > cause unnecessary bloat. Done. https://codereview.chromium.org/2672603003/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2672603003/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:62705: +<histogram name="Signin.TokenTable.GetAllTokensSqlStatement" On 2017/02/06 15:35:51, Alexei Svitkine (very slow) wrote: > Nit: Make the histogram name refer to what it's actually logging - i.e. the name > doesn't imply that it's logging whether it's valid... Done. https://codereview.chromium.org/2672603003/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:62713: +<histogram name="Signin.TokenTable.ReadTokenFromDB" On 2017/02/06 15:35:51, Alexei Svitkine (very slow) wrote: > Nit: Same comment as above. e.g. maybe ReadTokenFromDB.Result? Done.
lgtm
Roger: My latest patch has a small behavior change (see https://codereview.chromium.org/2672603003/diff2/80001:100001/components/sign... ). On the bad entry branch, I do not return false on the first bad entry, but continue to process all the entries. I think this is better as one bad entry should not block all the processing.
lgtm That makes sense Mihai.
The CQ bit was checked by msarda@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org Link to the patchset: https://codereview.chromium.org/2672603003/#ps100001 (title: "Fix histogram names")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1486469422502050,
"parent_rev": "a8e8f1139422ceeef5ff8e0901a87eeafeb81f65", "commit_rev":
"ee20ae90108326aab2178aa1d102b18bf9081d93"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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} Committed: https://chromium.googlesource.com/chromium/src/+/ee20ae90108326aab2178aa1d102... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001) as https://chromium.googlesource.com/chromium/src/+/ee20ae90108326aab2178aa1d102... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
