DCHECK in chrome_elf!SetUploadConsentImpl when using any --force-fieldtrials |
||||||||||
Issue descriptionTried the scheduler on trunk this morning and found this crash (and on Canary it doesn't DCHECK but it results in weird behavior -- i.e. shutdown phase runs as part of a startup trace?!), will not have time to look before this afternoon. out\Release\chrome.exe --force-fieldtrials=BrowserScheduler/Enabled/ --force-fieldtrial-params=BrowserScheduler.Enabled:Background/1;3;1;0;10000/BackgroundFileIO/1;3;1;0;10000/Foreground/3;8;1;0;10000/ForegroundFileIO/3;8;1;0;10000/RedirectSequencedWorkerPools/false base!base::debug::BreakDebugger+0x11 [d:\src\chrome\src\base\debug\debugger_win.cc @ 21] base!logging::LogMessage::~LogMessage+0x208 [d:\src\chrome\src\base\logging.cc @ 751] chrome_elf!SetUploadConsentImpl+0x5b [d:\src\chrome\src\components\crash\content\app\crashpad.cc @ 314] chrome_60430000!ChromeMetricsServicesManagerClient::UpdateRunningServices+0x63 [d:\src\chrome\src\chrome\browser\metrics\chrome_metrics_services_manager_client.cc @ 272] chrome_60430000!metrics_services_manager::MetricsServicesManager::UpdateRunningServices+0x9c [d:\src\chrome\src\components\metrics_services_manager\metrics_services_manager.cc @ 95] chrome_60430000!metrics_services_manager::MetricsServicesManager::UpdatePermissions+0x69 [d:\src\chrome\src\components\metrics_services_manager\metrics_services_manager.cc @ 79] chrome_60430000!metrics_services_manager::MetricsServicesManager::UpdateUploadPermissions+0x1c [d:\src\chrome\src\components\metrics_services_manager\metrics_services_manager.cc @ 126] chrome_60430000!ChromeBrowserMainParts::StartMetricsRecording+0x8d [d:\src\chrome\src\chrome\browser\chrome_browser_main.cc @ 945] chrome_60430000!ChromeBrowserMainParts::PreMainMessageLoopRunImpl+0x179 [d:\src\chrome\src\chrome\browser\chrome_browser_main.cc @ 1572] chrome_60430000!ChromeBrowserMainParts::PreMainMessageLoopRun+0xa9 [d:\src\chrome\src\chrome\browser\chrome_browser_main.cc @ 1376] content!content::BrowserMainLoop::PreMainMessageLoopRun+0x71 [d:\src\chrome\src\content\browser\browser_main_loop.cc @ 938] content!base::internal::RunMixin<base::Callback<int __cdecl(void),1,1> >::Run+0x18 [d:\src\chrome\src\base\callback.h @ 64] content!content::StartupTaskRunner::RunAllTasksNow+0x17 [d:\src\chrome\src\content\browser\startup_task_runner.cc @ 45] content!content::BrowserMainLoop::CreateStartupTasks+0x183 [d:\src\chrome\src\content\browser\browser_main_loop.cc @ 829] content!content::BrowserMainRunnerImpl::Initialize+0x28f [d:\src\chrome\src\content\browser\browser_main_runner.cc @ 141] content!content::BrowserMain+0x77 [d:\src\chrome\src\content\browser\browser_main.cc @ 42] content!content::RunNamedProcessTypeMain+0xd0 [d:\src\chrome\src\content\app\content_main_runner.cc @ 418] content!content::ContentMainRunnerImpl::Run+0x11a [d:\src\chrome\src\content\app\content_main_runner.cc @ 786] content!content::ContentMain+0x23 [d:\src\chrome\src\content\app\content_main.cc @ 20] chrome_60430000!ChromeMain+0x9d [d:\src\chrome\src\chrome\app\chrome_main.cc @ 88] chrome!MainDllLoader::Launch+0x1fe [d:\src\chrome\src\chrome\app\main_dll_loader_win.cc @ 169] chrome!wWinMain+0x15c [d:\src\chrome\src\chrome\app\chrome_exe_main_win.cc @ 247] chrome!__scrt_common_main_seh+0xf6 [f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl @ 253] KERNEL32!BaseThreadInitThunk+0x24 ntdll_76ea0000!__RtlUserThreadStart+0x2f ntdll_76ea0000!_RtlUserThreadStart+0x1b
,
Sep 20 2016
Currently not repro for me on Debug abe918907ad9cb56a09000919fc1ed5fda90fb8d (Tue Sep 20 05:53:51 2016 -0700).
,
Sep 20 2016
I'm on Release @ r419734 (3 revisions from you :-O!). Repros even with a new --user-data-dir.
,
Sep 20 2016
Re-assigning to asvitkine for high priority fix or triage. Thanks!
,
Sep 20 2016
(can't seem to edit description, but any --force-fieldtrials=Foo/Bar/ will result in this being hit, even from clean --user-data-dir)
,
Sep 20 2016
New data point. This only repros on official builds.
,
Sep 20 2016
,
Sep 20 2016
R-V-G for chrome_elf
The DCHECK is...
void __declspec(dllexport) __cdecl SetUploadConsentImpl(bool consent) {
DCHECK_EQ(consent,
crash_reporter::GetCrashReporterClient()>GetCollectStatsConsent());
,
Sep 20 2016
[-RVG, chrome-elf is public] Hmmm, I think that by "official" build you mean "is_chrome_branded = true" builds, right? At least that's what I'm running but still not an official build. I think http://crrev.com/c882e48d is the culprit. +jwd
,
Sep 20 2016
I'm surprised that orce-fieldtrials would be related. I'm in a meeting right now, will look more when out, but it looks like this is the googler scenario. It's a confluence of running chrome with uma on by policy, and building a chrome branded build. It can also do with when the last time you built and ran a branded build. This workflow violates the assumptions of the dcheck, but not the correctness of the code. My current belief is that the dcheck should be removed.
,
Sep 20 2016
It repros with a brand new --user-data-dir (on first run and after first run) so it's at least not just my local setup. But it could definitely be a confluence of running a branded build w/ dcheck_always_on and being on a corp managed machine. A DCHECK that can be hit under some circumstances should definitely be removed though (or adapted to account for that edge case). DCHECKs should only be used to document invariants. Thanks!
,
Sep 20 2016
Reverting the metrics reordering changelists causes this dcheck to get hit.
Yes, we are getting enabled by policy
>reg query HKLM\SOFTWARE\Policies\Google\Chrome /v MetricsReportingEnabled
HKEY_LOCAL_MACHINE\SOFTWARE\Policies\Google\Chrome
MetricsReportingEnabled REG_DWORD 0x1
And... from the point of Chrome
may_record = false
may_upload = true
Which will hit the DCHECK.
,
Sep 20 2016
*Clarification, reverting the metrics reordering changelists STILL causes this DCHECK to get hit.
,
Sep 20 2016
,
Sep 20 2016
CL to remove DCHECK is out for review https://codereview.chromium.org/2353133003/
,
Sep 20 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6bbba65a7074b6cdbe1dada8d51d0ba90ff944b6 commit 6bbba65a7074b6cdbe1dada8d51d0ba90ff944b6 Author: jwd <jwd@chromium.org> Date: Tue Sep 20 22:57:13 2016 Removing dcheck in SetUploadConsentImpl. This DCHECK is very easy to trigger by developers, googlers specifically, without actually violating the intent of the DCHECK. BUG= 648594 Review-Url: https://codereview.chromium.org/2353133003 Cr-Commit-Position: refs/heads/master@{#419882} [modify] https://crrev.com/6bbba65a7074b6cdbe1dada8d51d0ba90ff944b6/components/crash/content/app/crashpad.cc
,
Sep 23 2016
Any updates here? If this indeed breaks --force-fieldtrials= on Windows, I think it should be RBS since that functionality is critical for QA to test things, I believe. Putting the label.
,
Sep 23 2016
Oops, not sure why I didn't see the CL landing in comment 16. I guess it's fixed then?
,
Sep 23 2016
Yup. Fixed by removing the DCHECK. Marking Fixed. |
||||||||||
►
Sign in to add a comment |
||||||||||
Comment 1 by robliao@chromium.org
, Sep 20 2016