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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 months, 1 week ago by khmel
Modified:
6 months, 1 week 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 #

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 ...
6 months, 1 week 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: > ...
6 months, 1 week 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: > ...
6 months, 1 week 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: > ...
6 months, 1 week 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 > ...
6 months, 1 week 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: > ...
6 months, 1 week 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: > ...
6 months, 1 week 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 ...
6 months, 1 week 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. ...
6 months, 1 week 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 ...
6 months, 1 week 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 ...
6 months, 1 week ago (2017-03-16 16:41:57 UTC) #13
msarda
Oh, I forgot the LGTM. Sorry LGTM.
6 months, 1 week 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 ...
6 months, 1 week ago (2017-03-16 17:06:37 UTC) #16
Ilya Sherman
Metrics LGTM
6 months, 1 week ago (2017-03-16 22:24:11 UTC) #17
khmel
Thank you all for review and ideas!
6 months, 1 week 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.
6 months, 1 week 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
6 months, 1 week 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. ...
6 months, 1 week 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
6 months, 1 week ago (2017-03-16 22:45:49 UTC) #28
commit-bot: I haz the power
6 months, 1 week 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...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld b40b6558b