Settings signout crashing the browser. |
|||||||
Issue description[45125:45125:0703/125909.765883:ERROR:account_info_fetcher.cc(58)] OnGetTokenFailure: Invalid credentials (3). [45125:45125:0703/125916.776830:FATAL:identity_manager.cc(262)] Check failed: iterator != accounts_with_refresh_tokens_.end(). #0 0x7f125affa3cc base::debug::StackTrace::StackTrace() #1 0x7f125af421cb logging::LogMessage::~LogMessage() #2 0x557b1fd35cea identity::IdentityManager::WillFireOnRefreshTokenRevoked() #3 0x557b20844bc6 ProfileOAuth2TokenService::OnRefreshTokenRevoked() #4 0x557b1fc90a25 OAuth2TokenServiceDelegate::FireRefreshTokenRevoked() #5 0x557b203f5db6 MutableProfileOAuth2TokenServiceDelegate::RevokeCredentials() #6 0x557b203f589c MutableProfileOAuth2TokenServiceDelegate::RevokeAllCredentials() #7 0x557b2084bcbb SigninManager::DoSignOut() #8 0x557b1fe568dc extensions::NullExtensionCache::Start() #9 0x557b203ecd31 ChromeSigninClient::PreSignOut() #10 0x557b2084b512 SigninManager::SignOutAndRemoveAllAccounts() #11 0x557b2145b648 settings::PeopleHandler::HandleSignout() #12 0x7f12592d7205 content::WebUIImpl::ProcessWebUIMessage() #13 0x7f12592d63d4 content::WebUIImpl::OnWebUISend() #14 0x7f12592d617c _ZN3IPC8MessageTI27FrameHostMsg_WebUISend_MetaNSt3__15tupleIJNS2_12basic_stringIcNS2_11char_traitsIcEENS2_9allocatorIcEEEEN4base9ListValueEEEEvE8DispatchIN7content9WebUIImplESG_NSF_15RenderFrameHostEMSG_FvPSH_RKS9_RKSB_EEEbPKNS_7MessageEPT_PT0_PT1_T2_ #15 0x7f12592d60a9 content::WebUIImpl::OnMessageReceived() #16 0x7f125928b551 content::WebContentsImpl::OnMessageReceived() #17 0x7f1258f01745 content::RenderFrameHostImpl::OnMessageReceived() #18 0x7f125913369b content::RenderProcessHostImpl::OnMessageReceived() #19 0x7f125b106311 IPC::ChannelProxy::Context::OnDispatchMessage() #20 0x7f125b109228 _ZN4base8internal7InvokerINS0_9BindStateIMN3IPC12ChannelProxy7ContextEFvRKNS3_7MessageEEJ13scoped_refptrIS5_ES6_EEEFvvEE3RunEPNS0_13BindStateBaseE #21 0x7f125af22dfd base::debug::TaskAnnotator::RunTask() #22 0x7f125af4e776 base::internal::IncomingTaskQueue::RunTask() #23 0x7f125af52757 base::MessageLoop::RunTask() #24 0x7f125af52b6a base::MessageLoop::DeferOrRunPendingTask() #25 0x7f125af52eac base::MessageLoop::DoWork() #26 0x7f125af5602f base::(anonymous namespace)::WorkSourceDispatch() #27 0x7f124ef16f07 g_main_context_dispatch #28 0x7f124ef17138 <unknown> #29 0x7f124ef171cc g_main_context_iteration #30 0x7f125af55d62 base::MessagePumpGlib::Run() #31 0x7f125af52081 base::MessageLoop::Run() #32 0x7f125af86536 base::RunLoop::Run() #33 0x557b202260a8 ChromeBrowserMainParts::MainMessageLoopRun() #34 0x7f1258d63797 content::BrowserMainLoop::RunMainMessageLoopParts() #35 0x7f1258d66a43 content::BrowserMainRunnerImpl::Run() #36 0x7f1258d5f644 content::BrowserMain() #37 0x7f12598ae025 content::RunBrowserProcessMain() #38 0x7f12598af001 content::ContentMainRunnerImpl::Run() #39 0x7f125b26b877 service_manager::Main() #40 0x7f12598acf94 content::ContentMain() #41 0x557b1fc471b3 ChromeMain #42 0x7f124d6082b1 __libc_start_main #43 0x557b1fc4702a _start
,
Jul 3
,
Jul 3
,
Jul 3
,
Jul 4
Colin: this seems related to bug 859377
,
Jul 4
The offending CL has been reverted, so I'm going to close this bug. However we need to take this scenario into account before relanding. Thanks for the report Demetrios!
,
Jul 5
Thank you, Demetrios! Reopening the bug as the CL has relanded on trunk with adjustments that wouldn't fix this case, as we weren't aware of this bug report at the time of relanding the CL. David and I investigated and we believe that the crash is most likely from the following flow: - When Chrome started up the user was signed in but the token service did not find a token for the primary account in the on-disk database for whatever reason (e.g., it couldn't decrypt the database). - In this case, it inserts an invalid token but *doesn't inform observers* via the OnRefreshTokenAvailable() callback. - When the user later signed out, IdentityManager's expectation that it had previously been made aware of the token now being revoked is violated. I manually altered MutablePO2TSDelegate to drop the token for the primary account on the floor during token loading and verified that if I then signed out via the settings page I got the crash that was described in the OP. Obviously this doesn't show that the OP definitively *was* going through this flow, but it shows that this flow leads to this crash. https://chromium-review.googlesource.com/c/chromium/src/+/1126861 is out for review to fix this case. It should land today, at which point I'll tentatively mark this bug fixed. If you see similar problems when running on trunk past the point of that fix landing, please let me know ASAP. Thanks, Colin
,
Jul 12
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d373e8d468f12b8ca7bbf6b979961e3a3f29cc73 commit d373e8d468f12b8ca7bbf6b979961e3a3f29cc73 Author: Colin Blundell <blundell@chromium.org> Date: Thu Jul 12 13:40:21 2018 MutablePO2TS: Fire token available when adding invalid token at startup ProfileOAuth2TokenService maintains the invariant that if the user is signed in at startup, the primary account is always present in PO2TS::GetAccounts(). To maintain this invariant, MutableProfileOAuth2TokenServiceDelegate performs the following action when the load of tokens from the on-disk database is complete: if the user is signed in, it checks whether there is a token present for the primary account and if not, inserts an invalid token for that account. (The token can be absent due to e.g. a failure to decrypt the database on disk for whatever reason). However, MutablePO2TSDelegate does *not* fire the OnRefreshTokenAvailable() callback in that case. This breaks the implicit expectation of clients that when ProfileOAuth2TokenService adds an account, it will always fire OnRefreshTokenAvailable(). It is also inconsistent with the behavior if the primary account happens to have an invalid token on disk that is successfully decrypted: in that case, MutablePO2TSDelegate will fire the observer callback, but of course, from clients' POV there is no difference whatsoever between these two cases. In particular, the failure of this invariant can cause IdentityManager to crash if a user in this situation later signs out, as the token being revoked violates IdentityManager's expectation that it had previously been made aware of that token via a notification that it was available. This CL remedies the problem by firing OnRefreshTokenAvailable() in this case. It also extends the relevant MutablePO2TSDelegate unittest to fail without this change and succeed with this change. Bug: 860011 Change-Id: I66cebda29ec56d79f209f6b2c93dff67797c57b4 Reviewed-on: https://chromium-review.googlesource.com/1126861 Commit-Queue: Colin Blundell <blundell@chromium.org> Reviewed-by: David Roger <droger@chromium.org> Cr-Commit-Position: refs/heads/master@{#574550} [modify] https://crrev.com/d373e8d468f12b8ca7bbf6b979961e3a3f29cc73/chrome/browser/signin/mutable_profile_oauth2_token_service_delegate.cc [modify] https://crrev.com/d373e8d468f12b8ca7bbf6b979961e3a3f29cc73/chrome/browser/signin/mutable_profile_oauth2_token_service_delegate.h [modify] https://crrev.com/d373e8d468f12b8ca7bbf6b979961e3a3f29cc73/chrome/browser/signin/mutable_profile_oauth2_token_service_delegate_unittest.cc
,
Jul 12
|
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by dpa...@chromium.org
, Jul 3