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

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

Created:
1 year, 1 month ago by khmel
Modified:
1 year, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month ago (2017-03-16 16:41:57 UTC) #13
msarda
Oh, I forgot the LGTM. Sorry LGTM.
1 year, 1 month 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, 1 month ago (2017-03-16 17:06:37 UTC) #16
Ilya Sherman
Metrics LGTM
1 year, 1 month ago (2017-03-16 22:24:11 UTC) #17
khmel
Thank you all for review and ideas!
1 year, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month ago (2017-03-16 22:45:49 UTC) #28
commit-bot: I haz the power
1 year, 1 month 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