Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2752873002:
Fix refresh token is not available after Chrome restart on crash. (Closed)
|
Created:
3 years, 9 months ago by khmel Modified:
3 years, 9 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. |
DescriptionFix 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@chromium.org changed reviewers: + xiyuan@chromium.org
Hi Xiyuan, PTAL Thanks https://codereview.chromium.org/2752873002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_auth_context.cc (left): https://codereview.chromium.org/2752873002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_auth_context.cc:44: account_id_ = signin_manager->GetAuthenticatedAccountId(); This is race condition on Chrome restart on crash. account_id_ is empty at time of context creation. Access account_id on time of use.
https://codereview.chromium.org/2752873002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_auth_context.cc (left): https://codereview.chromium.org/2752873002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_auth_context.cc:44: account_id_ = signin_manager->GetAuthenticatedAccountId(); On 2017/03/15 18:12:22, khmel wrote: > This is race condition on Chrome restart on crash. account_id_ is empty at time > of context creation. Access account_id on time of use. Not sure I understand why there is a race. SigninManagerBase should store the account info in prefs and load it in SigninManagerBase::Initialize. On a crash-n-restart case, the account id should still be available. The store code after login is in UserSessionManager::InitProfilePreferences [1]. And SigninManagerBase::Initialize should read it out in Line 92 and 154 [2]. If this is broken, we would have other problems too. Can you double check? [1]: https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/session/us... [2]: https://cs.chromium.org/chromium/src/components/signin/core/browser/signin_ma...
https://codereview.chromium.org/2752873002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_auth_context.cc (left): https://codereview.chromium.org/2752873002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_auth_context.cc:44: account_id_ = signin_manager->GetAuthenticatedAccountId(); On 2017/03/15 19:34:30, xiyuan wrote: > On 2017/03/15 18:12:22, khmel wrote: > > This is race condition on Chrome restart on crash. account_id_ is empty at > time > > of context creation. Access account_id on time of use. > > Not sure I understand why there is a race. SigninManagerBase should store the > account info in prefs and load it in SigninManagerBase::Initialize. On a > crash-n-restart case, the account id should still be available. > > The store code after login is in UserSessionManager::InitProfilePreferences [1]. > And SigninManagerBase::Initialize should read it out in Line 92 and 154 [2]. > > If this is broken, we would have other problems too. Can you double check? > > [1]: > https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/session/us... > > [2]: > https://cs.chromium.org/chromium/src/components/signin/core/browser/signin_ma... InitProfilePreferences is not called in case of crash_and_restart (at least at scenario I tested) because OnProfileCreated is not called.
https://codereview.chromium.org/2752873002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_auth_context.cc (left): https://codereview.chromium.org/2752873002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_auth_context.cc:44: account_id_ = signin_manager->GetAuthenticatedAccountId(); On 2017/03/15 19:39:05, khmel wrote: > On 2017/03/15 19:34:30, xiyuan wrote: > > On 2017/03/15 18:12:22, khmel wrote: > > > This is race condition on Chrome restart on crash. account_id_ is empty at > > time > > > of context creation. Access account_id on time of use. > > > > Not sure I understand why there is a race. SigninManagerBase should store the > > account info in prefs and load it in SigninManagerBase::Initialize. On a > > crash-n-restart case, the account id should still be available. > > > > The store code after login is in UserSessionManager::InitProfilePreferences > [1]. > > And SigninManagerBase::Initialize should read it out in Line 92 and 154 [2]. > > > > If this is broken, we would have other problems too. Can you double check? > > > > [1]: > > > https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/session/us... > > > > [2]: > > > https://cs.chromium.org/chromium/src/components/signin/core/browser/signin_ma... > > InitProfilePreferences is not called in case of crash_and_restart (at least at > scenario I tested) because OnProfileCreated is not called. InitProfilePreferences stores the info in SigninManagerBase. On crash-n-restart case, it does not need to be called. SigninManagerBase should get the info in prefs. Can you track SigninManagerBase::Initialize and see why this does not happen?
On 2017/03/15 19:53:16, xiyuan wrote: > https://codereview.chromium.org/2752873002/diff/1/chrome/browser/chromeos/arc... > File chrome/browser/chromeos/arc/arc_auth_context.cc (left): > > https://codereview.chromium.org/2752873002/diff/1/chrome/browser/chromeos/arc... > chrome/browser/chromeos/arc/arc_auth_context.cc:44: account_id_ = > signin_manager->GetAuthenticatedAccountId(); > On 2017/03/15 19:39:05, khmel wrote: > > On 2017/03/15 19:34:30, xiyuan wrote: > > > On 2017/03/15 18:12:22, khmel wrote: > > > > This is race condition on Chrome restart on crash. account_id_ is empty at > > > time > > > > of context creation. Access account_id on time of use. > > > > > > Not sure I understand why there is a race. SigninManagerBase should store > the > > > account info in prefs and load it in SigninManagerBase::Initialize. On a > > > crash-n-restart case, the account id should still be available. > > > > > > The store code after login is in UserSessionManager::InitProfilePreferences > > [1]. > > > And SigninManagerBase::Initialize should read it out in Line 92 and 154 [2]. > > > > > > If this is broken, we would have other problems too. Can you double check? > > > > > > [1]: > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/session/us... > > > > > > [2]: > > > > > > https://cs.chromium.org/chromium/src/components/signin/core/browser/signin_ma... > > > > InitProfilePreferences is not called in case of crash_and_restart (at least at > > scenario I tested) because OnProfileCreated is not called. > > InitProfilePreferences stores the info in SigninManagerBase. On crash-n-restart > case, it does not need to be called. SigninManagerBase should get the info in > prefs. Can you track SigninManagerBase::Initialize and see why this does not > happen? Sure, WillTAL
https://codereview.chromium.org/2752873002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_auth_context.cc (left): https://codereview.chromium.org/2752873002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_auth_context.cc:44: account_id_ = signin_manager->GetAuthenticatedAccountId(); On 2017/03/15 19:53:16, xiyuan wrote: > On 2017/03/15 19:39:05, khmel wrote: > > On 2017/03/15 19:34:30, xiyuan wrote: > > > On 2017/03/15 18:12:22, khmel wrote: > > > > This is race condition on Chrome restart on crash. account_id_ is empty at > > > time > > > > of context creation. Access account_id on time of use. > > > > > > Not sure I understand why there is a race. SigninManagerBase should store > the > > > account info in prefs and load it in SigninManagerBase::Initialize. On a > > > crash-n-restart case, the account id should still be available. > > > > > > The store code after login is in UserSessionManager::InitProfilePreferences > > [1]. > > > And SigninManagerBase::Initialize should read it out in Line 92 and 154 [2]. > > > > > > If this is broken, we would have other problems too. Can you double check? > > > > > > [1]: > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/session/us... > > > > > > [2]: > > > > > > https://cs.chromium.org/chromium/src/components/signin/core/browser/signin_ma... > > > > InitProfilePreferences is not called in case of crash_and_restart (at least at > > scenario I tested) because OnProfileCreated is not called. > > InitProfilePreferences stores the info in SigninManagerBase. On crash-n-restart > case, it does not need to be called. SigninManagerBase should get the info in > prefs. Can you track SigninManagerBase::Initialize and see why this does not > happen? So, I investigated this case and can confirm my suspicion. /home/chronos/XXXX/Preferences has commit interval 10 seconds. If crash happens between commit and setting account id to prefs then it could not be restored after crash. And this is the reason of RefreshTokenProblem. WDYT?
https://codereview.chromium.org/2752873002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_auth_context.cc (left): https://codereview.chromium.org/2752873002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_auth_context.cc:44: account_id_ = signin_manager->GetAuthenticatedAccountId(); On 2017/03/16 02:07:24, khmel wrote: > On 2017/03/15 19:53:16, xiyuan wrote: > > On 2017/03/15 19:39:05, khmel wrote: > > > On 2017/03/15 19:34:30, xiyuan wrote: > > > > On 2017/03/15 18:12:22, khmel wrote: > > > > > This is race condition on Chrome restart on crash. account_id_ is empty > at > > > > time > > > > > of context creation. Access account_id on time of use. > > > > > > > > Not sure I understand why there is a race. SigninManagerBase should store > > the > > > > account info in prefs and load it in SigninManagerBase::Initialize. On a > > > > crash-n-restart case, the account id should still be available. > > > > > > > > The store code after login is in > UserSessionManager::InitProfilePreferences > > > [1]. > > > > And SigninManagerBase::Initialize should read it out in Line 92 and 154 > [2]. > > > > > > > > If this is broken, we would have other problems too. Can you double check? > > > > > > > > [1]: > > > > > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/session/us... > > > > > > > > [2]: > > > > > > > > > > https://cs.chromium.org/chromium/src/components/signin/core/browser/signin_ma... > > > > > > InitProfilePreferences is not called in case of crash_and_restart (at least > at > > > scenario I tested) because OnProfileCreated is not called. > > > > InitProfilePreferences stores the info in SigninManagerBase. On > crash-n-restart > > case, it does not need to be called. SigninManagerBase should get the info in > > prefs. Can you track SigninManagerBase::Initialize and see why this does not > > happen? > > So, I investigated this case and can confirm my suspicion. > /home/chronos/XXXX/Preferences has commit interval 10 seconds. If crash happens > between commit and setting account id to prefs then it could not be restored > after crash. And this is the reason of RefreshTokenProblem. > WDYT? Thanks for looking into this. I would prefer to add a client_->GetPrefs()->CommitPendingWrite() at the end of SigninManagerBase::SetAuthenticatedAccountId() to flush. Would this work?
PTAL updated version Thanks! https://codereview.chromium.org/2752873002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_auth_context.cc (left): https://codereview.chromium.org/2752873002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_auth_context.cc:44: account_id_ = signin_manager->GetAuthenticatedAccountId(); On 2017/03/16 04:37:42, xiyuan wrote: > On 2017/03/16 02:07:24, khmel wrote: > > On 2017/03/15 19:53:16, xiyuan wrote: > > > On 2017/03/15 19:39:05, khmel wrote: > > > > On 2017/03/15 19:34:30, xiyuan wrote: > > > > > On 2017/03/15 18:12:22, khmel wrote: > > > > > > This is race condition on Chrome restart on crash. account_id_ is > empty > > at > > > > > time > > > > > > of context creation. Access account_id on time of use. > > > > > > > > > > Not sure I understand why there is a race. SigninManagerBase should > store > > > the > > > > > account info in prefs and load it in SigninManagerBase::Initialize. On a > > > > > crash-n-restart case, the account id should still be available. > > > > > > > > > > The store code after login is in > > UserSessionManager::InitProfilePreferences > > > > [1]. > > > > > And SigninManagerBase::Initialize should read it out in Line 92 and 154 > > [2]. > > > > > > > > > > If this is broken, we would have other problems too. Can you double > check? > > > > > > > > > > [1]: > > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/session/us... > > > > > > > > > > [2]: > > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/components/signin/core/browser/signin_ma... > > > > > > > > InitProfilePreferences is not called in case of crash_and_restart (at > least > > at > > > > scenario I tested) because OnProfileCreated is not called. > > > > > > InitProfilePreferences stores the info in SigninManagerBase. On > > crash-n-restart > > > case, it does not need to be called. SigninManagerBase should get the info > in > > > prefs. Can you track SigninManagerBase::Initialize and see why this does not > > > happen? > > > > So, I investigated this case and can confirm my suspicion. > > /home/chronos/XXXX/Preferences has commit interval 10 seconds. If crash > happens > > between commit and setting account id to prefs then it could not be restored > > after crash. And this is the reason of RefreshTokenProblem. > > WDYT? > > Thanks for looking into this. > > I would prefer to add a client_->GetPrefs()->CommitPendingWrite() at the end of > SigninManagerBase::SetAuthenticatedAccountId() to flush. Would this work? Yes, this works.
xiyuan@chromium.org changed reviewers: + msarda@chromium.org
lgtm Cool. Thank you for bearing with me. msarda@, please help with owner review. Thanks. https://codereview.chromium.org/2752873002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/session/user_session_manager.cc (right): https://codereview.chromium.org/2752873002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/session/user_session_manager.cc:555: LOG(ERROR) << "No account is associated with sign-in manager on restore."; nit: LOG_IF(ERROR, !signin_manager || signin_manager->GetAuthenticatedAccountId().empty()) << "..."; https://codereview.chromium.org/2752873002/diff/20001/components/signin/core/... File components/signin/core/browser/signin_manager_base.cc (right): https://codereview.chromium.org/2752873002/diff/20001/components/signin/core/... components/signin/core/browser/signin_manager_base.cc:220: client_->GetPrefs()->CommitPendingWrite(); nit: Let's add a comment of why this is needed, e.g. // Commit authenticated account info immediately so that it does // not get lost if chrome crashes before the next commit interval.
Thanks Xiyuan for review! https://codereview.chromium.org/2752873002/diff/20001/components/signin/core/... File components/signin/core/browser/signin_manager_base.cc (right): https://codereview.chromium.org/2752873002/diff/20001/components/signin/core/... components/signin/core/browser/signin_manager_base.cc:220: client_->GetPrefs()->CommitPendingWrite(); On 2017/03/16 15:59:02, xiyuan wrote: > nit: Let's add a comment of why this is needed, > > e.g. > // Commit authenticated account info immediately so that it does > // not get lost if chrome crashes before the next commit interval. Thanks, done
https://codereview.chromium.org/2752873002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/login/session/user_session_manager.cc (right): https://codereview.chromium.org/2752873002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/login/session/user_session_manager.cc:555: LOG(ERROR) << "No account is associated with sign-in manager on restore."; Suggestion: Consider adding a UMA histogram for this to be able to monitor how often this event occurs.
Oh, I forgot the LGTM. Sorry LGTM.
khmel@chromium.org changed reviewers: + isherman@chromium.org
Thank you for review! Adding isherman@ for UMA review PTAL https://codereview.chromium.org/2752873002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/login/session/user_session_manager.cc (right): https://codereview.chromium.org/2752873002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/login/session/user_session_manager.cc:555: LOG(ERROR) << "No account is associated with sign-in manager on restore."; On 2017/03/16 16:41:57, msarda wrote: > Suggestion: Consider adding a UMA histogram for this to be able to monitor how > often this event occurs. Nice point!
Metrics LGTM
Thank you all for review and ideas!
The CQ bit was checked by khmel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org, msarda@chromium.org Link to the patchset: https://codereview.chromium.org/2752873002/#ps60001 (title: "UMA added")
lhchavez@chromium.org changed reviewers: + lhchavez@chromium.org
drive-by nit: s/chrash/crash/ on title and commit message.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by khmel@chromium.org
Description was changed from
==========
Fix refresh token is not available after Chrome restart on chrash.
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
==========
to
==========
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
==========
On 2017/03/16 22:37:36, commit-bot: I haz the power wrote: > CQ is trying da patch. Follow status at > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... Thanks Luis
The CQ bit was checked by khmel@chromium.org
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": 60001, "attempt_start_ts": 1489704311288110,
"parent_rev": "efd3d31152e48d1089175b9256c56bccbe6d9792", "commit_rev":
"f0e2fb482636b0c1aed01f2d672ec5619bff9eb9"}
Message was sent while issue was closed.
Description was changed from
==========
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
==========
to
==========
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/+/f0e2fb482636b0c1aed01f2d672e...
==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/f0e2fb482636b0c1aed01f2d672e... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
