[Dice] DCHECK on settings page |
|||||||
Issue descriptionRepro steps: - signin to Chrome - go to settings The stack trace is: [80144:80144:0308/163420.476518:FATAL:signin_ui_util.cc(156)] Check failed: !signin_manager->IsAuthenticated(). #0 0x7f1eba75749c base::debug::StackTrace::StackTrace() #1 0x7f1eba7816ec logging::LogMessage::~LogMessage() #2 0x5609ec5cc772 signin_ui_util::GetAccountsForDicePromos() #3 0x5609ed52d1e7 settings::PeopleHandler::GetStoredAccountsList() #4 0x5609ed52b653 settings::PeopleHandler::HandleGetStoredAccounts() #5 0x7f1eb81ab515 content::WebUIImpl::ProcessWebUIMessage() Potential fix: https://chromium-review.googlesource.com/c/chromium/src/+/952964/4/chrome/browser/signin/signin_ui_util.cc
,
Mar 8 2018
If this is RBS, should we up the priority to 1?
,
Mar 9 2018
,
Mar 12 2018
,
Mar 13 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8fc5347404d27ea7f74c418148e480dee4702141 commit 8fc5347404d27ea7f74c418148e480dee4702141 Author: David Roger <droger@chromium.org> Date: Tue Mar 13 10:40:01 2018 [signin] Fix DCHECK in GetAccountsForDicePromos() This function is called by the settings page even when the user is not signed-in. The DCHECK can be removed, because it was not actually necessary. Bug: 820083 Change-Id: I910e1b22acf99a2c3d755523569a67c2ef252dfa Reviewed-on: https://chromium-review.googlesource.com/957184 Reviewed-by: Mihai Sardarescu <msarda@chromium.org> Commit-Queue: David Roger <droger@chromium.org> Cr-Commit-Position: refs/heads/master@{#542762} [modify] https://crrev.com/8fc5347404d27ea7f74c418148e480dee4702141/chrome/browser/signin/signin_ui_util.cc
,
Mar 13 2018
,
Mar 14 2018
Your change meets the bar and is auto-approved for M66. Please go ahead and merge the CL to branch 3359 manually. Please contact milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), josafat@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Mar 14 2018
Tested the issue on Win-10 and mac 10.13.3 using chrome version without fix i.e #67.0.3365.0. Attached a screen cast and screen shots for reference. Following are the steps followed to reproduce the issue. ------------ 1. Launched chrome from terminal --enable-features=AccountConsistency to enable dice. 2. Signed into chrome 3. Navigated to chrome://settings. 4. Did not observe any crash in the terminal or in the page. droger@ - Could you please check the attached screen cast or screen shots and please let us know if anything missed from our end. Also please confirm if this is related to developer build ? Thanks...!!
,
Mar 14 2018
This is a DCHECK meaning that you need a Debug build to see it. I don't know if there are debug builds easily available for you to test. Just in case here is more context about what a DCHECK is: - In a debug build, DCHECK(condition) tests that |condition| is true, and if it is not, it crashes the program. - in release build, a DCHECK does nothing. There is an equivalent of DCHECK that also works in release builds: CHECK (without the D, which stands for "Debug") - CHECK and DCHECK are typically used when we want to make an assumption in the code. In this particular example, the DCHECK was used to verify that GetAccountsForDicePromos() was only used when the user is not signed in. Since the DCHECK was hit, it makes clear that the assumption was wrong. So I fixed GetAccountsForDicePromos() to actually handle the case where the user is signed in (which removes the need for the assumption) and removed the DCHECK.
,
Mar 14 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b77e1e4ba9196ec7527541dd84f7f18c330f2edd commit b77e1e4ba9196ec7527541dd84f7f18c330f2edd Author: David Roger <droger@chromium.org> Date: Wed Mar 14 15:42:55 2018 [signin] Fix DCHECK in GetAccountsForDicePromos() This function is called by the settings page even when the user is not signed-in. The DCHECK can be removed, because it was not actually necessary. TBR=droger@chromium.org (cherry picked from commit 8fc5347404d27ea7f74c418148e480dee4702141) Bug: 820083 Change-Id: I910e1b22acf99a2c3d755523569a67c2ef252dfa Reviewed-on: https://chromium-review.googlesource.com/957184 Reviewed-by: Mihai Sardarescu <msarda@chromium.org> Commit-Queue: David Roger <droger@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#542762} Reviewed-on: https://chromium-review.googlesource.com/962781 Reviewed-by: David Roger <droger@chromium.org> Cr-Commit-Position: refs/branch-heads/3359@{#232} Cr-Branched-From: 66afc5e5d10127546cc4b98b9117aff588b5e66b-refs/heads/master@{#540276} [modify] https://crrev.com/b77e1e4ba9196ec7527541dd84f7f18c330f2edd/chrome/browser/signin/signin_ui_util.cc |
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by droger@chromium.org
, Mar 8 2018Labels: OS-Linux OS-Mac OS-Windows