Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(154)

Issue 2752873002: Fix refresh token is not available after Chrome restart on crash. (Closed)

Created:
1 year, 3 months ago by khmel
Modified:
1 year, 3 months ago
CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, alemate+watch_chromium.org, oshima+watch_chromium.org, yusukes+watch_chromium.org, hidehiko+watch_chromium.org, achuith+watch_chromium.org, khmel+watch_chromium.org, lhchavez+watch_chromium.org, victorhsieh+watch_chromium.org, davemoore+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Fix refresh token is not available after Chrome restart on crash. In case Chrome crashed and restarted, refresh token was not reloaded. This caused several problems, including random user image profile and impossibility to opt in ARC. TEST=Manually simulated crash. On Chrome restart profile image is correct and ARC can be opted in. Also logs show that token is loaded correctly. BUG=701866 Review-Url: https://codereview.chromium.org/2752873002 Cr-Commit-Position: refs/heads/master@{#457619} Committed: https://chromium.googlesource.com/chromium/src/+/f0e2fb482636b0c1aed01f2d672ec5619bff9eb9

Patch Set 1 #

Total comments: 7

Patch Set 2 : alternative solution #

Total comments: 3

Patch Set 3 : comment added #

Total comments: 2

Patch Set 4 : UMA added #

Unified diffs Side-by-side diffs Delta from patch set Stats (+22 lines, -0 lines) Patch
M chrome/browser/chromeos/login/session/user_session_manager.cc View 1 2 3 1 chunk +10 lines, -0 lines 0 comments Download
M components/signin/core/browser/signin_manager_base.cc View 1 2 1 chunk +4 lines, -0 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 2 3 1 chunk +8 lines, -0 lines 0 comments Download

Messages

Total messages: 31 (11 generated)
khmel
Hi Xiyuan, PTAL Thanks https://codereview.chromium.org/2752873002/diff/1/chrome/browser/chromeos/arc/arc_auth_context.cc File chrome/browser/chromeos/arc/arc_auth_context.cc (left): https://codereview.chromium.org/2752873002/diff/1/chrome/browser/chromeos/arc/arc_auth_context.cc#oldcode44 chrome/browser/chromeos/arc/arc_auth_context.cc:44: account_id_ = signin_manager->GetAuthenticatedAccountId(); This is ...
1 year, 3 months ago (2017-03-15 18:12:22 UTC) #2
xiyuan
https://codereview.chromium.org/2752873002/diff/1/chrome/browser/chromeos/arc/arc_auth_context.cc File chrome/browser/chromeos/arc/arc_auth_context.cc (left): https://codereview.chromium.org/2752873002/diff/1/chrome/browser/chromeos/arc/arc_auth_context.cc#oldcode44 chrome/browser/chromeos/arc/arc_auth_context.cc:44: account_id_ = signin_manager->GetAuthenticatedAccountId(); On 2017/03/15 18:12:22, khmel wrote: > ...
1 year, 3 months ago (2017-03-15 19:34:31 UTC) #3
khmel
https://codereview.chromium.org/2752873002/diff/1/chrome/browser/chromeos/arc/arc_auth_context.cc File chrome/browser/chromeos/arc/arc_auth_context.cc (left): https://codereview.chromium.org/2752873002/diff/1/chrome/browser/chromeos/arc/arc_auth_context.cc#oldcode44 chrome/browser/chromeos/arc/arc_auth_context.cc:44: account_id_ = signin_manager->GetAuthenticatedAccountId(); On 2017/03/15 19:34:30, xiyuan wrote: > ...
1 year, 3 months ago (2017-03-15 19:39:05 UTC) #4
xiyuan
https://codereview.chromium.org/2752873002/diff/1/chrome/browser/chromeos/arc/arc_auth_context.cc File chrome/browser/chromeos/arc/arc_auth_context.cc (left): https://codereview.chromium.org/2752873002/diff/1/chrome/browser/chromeos/arc/arc_auth_context.cc#oldcode44 chrome/browser/chromeos/arc/arc_auth_context.cc:44: account_id_ = signin_manager->GetAuthenticatedAccountId(); On 2017/03/15 19:39:05, khmel wrote: > ...
1 year, 3 months ago (2017-03-15 19:53:16 UTC) #5
khmel
On 2017/03/15 19:53:16, xiyuan wrote: > https://codereview.chromium.org/2752873002/diff/1/chrome/browser/chromeos/arc/arc_auth_context.cc > File chrome/browser/chromeos/arc/arc_auth_context.cc (left): > > https://codereview.chromium.org/2752873002/diff/1/chrome/browser/chromeos/arc/arc_auth_context.cc#oldcode44 > ...
1 year, 3 months ago (2017-03-15 19:54:25 UTC) #6
khmel
https://codereview.chromium.org/2752873002/diff/1/chrome/browser/chromeos/arc/arc_auth_context.cc File chrome/browser/chromeos/arc/arc_auth_context.cc (left): https://codereview.chromium.org/2752873002/diff/1/chrome/browser/chromeos/arc/arc_auth_context.cc#oldcode44 chrome/browser/chromeos/arc/arc_auth_context.cc:44: account_id_ = signin_manager->GetAuthenticatedAccountId(); On 2017/03/15 19:53:16, xiyuan wrote: > ...
1 year, 3 months ago (2017-03-16 02:07:24 UTC) #7
xiyuan
https://codereview.chromium.org/2752873002/diff/1/chrome/browser/chromeos/arc/arc_auth_context.cc File chrome/browser/chromeos/arc/arc_auth_context.cc (left): https://codereview.chromium.org/2752873002/diff/1/chrome/browser/chromeos/arc/arc_auth_context.cc#oldcode44 chrome/browser/chromeos/arc/arc_auth_context.cc:44: account_id_ = signin_manager->GetAuthenticatedAccountId(); On 2017/03/16 02:07:24, khmel wrote: > ...
1 year, 3 months ago (2017-03-16 04:37:42 UTC) #8
khmel
PTAL updated version Thanks! https://codereview.chromium.org/2752873002/diff/1/chrome/browser/chromeos/arc/arc_auth_context.cc File chrome/browser/chromeos/arc/arc_auth_context.cc (left): https://codereview.chromium.org/2752873002/diff/1/chrome/browser/chromeos/arc/arc_auth_context.cc#oldcode44 chrome/browser/chromeos/arc/arc_auth_context.cc:44: account_id_ = signin_manager->GetAuthenticatedAccountId(); On 2017/03/16 ...
1 year, 3 months ago (2017-03-16 15:41:42 UTC) #9
xiyuan
lgtm Cool. Thank you for bearing with me. msarda@, please help with owner review. Thanks. ...
1 year, 3 months ago (2017-03-16 15:59:03 UTC) #11
khmel
Thanks Xiyuan for review! https://codereview.chromium.org/2752873002/diff/20001/components/signin/core/browser/signin_manager_base.cc File components/signin/core/browser/signin_manager_base.cc (right): https://codereview.chromium.org/2752873002/diff/20001/components/signin/core/browser/signin_manager_base.cc#newcode220 components/signin/core/browser/signin_manager_base.cc:220: client_->GetPrefs()->CommitPendingWrite(); On 2017/03/16 15:59:02, xiyuan ...
1 year, 3 months ago (2017-03-16 16:19:43 UTC) #12
msarda
https://codereview.chromium.org/2752873002/diff/40001/chrome/browser/chromeos/login/session/user_session_manager.cc File chrome/browser/chromeos/login/session/user_session_manager.cc (right): https://codereview.chromium.org/2752873002/diff/40001/chrome/browser/chromeos/login/session/user_session_manager.cc#newcode555 chrome/browser/chromeos/login/session/user_session_manager.cc:555: LOG(ERROR) << "No account is associated with sign-in manager ...
1 year, 3 months ago (2017-03-16 16:41:57 UTC) #13
msarda
Oh, I forgot the LGTM. Sorry LGTM.
1 year, 3 months ago (2017-03-16 16:44:53 UTC) #14
khmel
Thank you for review! Adding isherman@ for UMA review PTAL https://codereview.chromium.org/2752873002/diff/40001/chrome/browser/chromeos/login/session/user_session_manager.cc File chrome/browser/chromeos/login/session/user_session_manager.cc (right): https://codereview.chromium.org/2752873002/diff/40001/chrome/browser/chromeos/login/session/user_session_manager.cc#newcode555 ...
1 year, 3 months ago (2017-03-16 17:06:37 UTC) #16
Ilya Sherman
Metrics LGTM
1 year, 3 months ago (2017-03-16 22:24:11 UTC) #17
khmel
Thank you all for review and ideas!
1 year, 3 months ago (2017-03-16 22:36:49 UTC) #18
Luis Héctor Chávez
drive-by nit: s/chrash/crash/ on title and commit message.
1 year, 3 months ago (2017-03-16 22:37:26 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2752873002/60001
1 year, 3 months ago (2017-03-16 22:37:36 UTC) #23
khmel
On 2017/03/16 22:37:36, commit-bot: I haz the power wrote: > CQ is trying da patch. ...
1 year, 3 months ago (2017-03-16 22:45:03 UTC) #26
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2752873002/60001
1 year, 3 months ago (2017-03-16 22:45:49 UTC) #28
commit-bot: I haz the power
1 year, 3 months ago (2017-03-17 00:17:00 UTC) #31
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/f0e2fb482636b0c1aed01f2d672e...

Powered by Google App Engine
This is Rietveld 408576698