DCHECK fires in FieldTrialList constructor |
||||||||
Issue descriptionDCHECK(!global_) fires in FieldTrialList::FieldTrialList() if a browser test is run in debug (e.g. with --gtest_filter=*UserAffiliation*). Looks like it's initialized twice: First initialization: libbase.so!base::FieldTrialList::FieldTrialList(base::FieldTrialList * this, std::__1::unique_ptr<base::FieldTrial::EntropyProvider const, std::__1::default_delete<base::FieldTrial::EntropyProvider const> > entropy_provider) (base/metrics/field_trial.cc:500) ChromeBrowserMainParts::SetupFieldTrials(chromeos::ChromeBrowserMainPartsChromeos * this) (chrome/browser/chrome_browser_main.cc:836) ChromeBrowserMainParts::LoadLocalState(chromeos::ChromeBrowserMainPartsChromeos * this, base::internal::SchedulerSequencedTaskRunner * local_state_task_runner, bool * failed_to_load_resource_bundle) (chrome/browser/chrome_browser_main.cc:1133) ChromeBrowserMainParts::PreEarlyInitialization(chromeos::ChromeBrowserMainPartsChromeos * this) (chrome/browser/chrome_browser_main.cc:1016) ChromeBrowserMainPartsPosix::PreEarlyInitialization(chromeos::ChromeBrowserMainPartsChromeos * this) (chrome/browser/chrome_browser_main_posix.cc:116) chromeos::ChromeBrowserMainPartsChromeos::PreEarlyInitialization(chromeos::ChromeBrowserMainPartsChromeos * this) (chrome/browser/chromeos/chrome_browser_main_chromeos.cc:612) libcontent.so!content::BrowserMainLoop::EarlyInitialization(content::BrowserMainLoop * this) (content/browser/browser_main_loop.cc:626) libcontent.so!content::BrowserMainRunnerImpl::Initialize(content::BrowserMainRunnerImpl * this, const content::MainFunctionParams & parameters) (content/browser/browser_main_runner_impl.cc:120) libcontent.so!content::BrowserMain(const content::MainFunctionParams & parameters) (content/browser/browser_main.cc:43) libcontent.so!content::RunBrowserProcessMain(const content::MainFunctionParams & main_function_params, ChromeMainDelegate * delegate) (content/app/content_main_runner_impl.cc:596) libcontent.so!content::ContentMainRunnerImpl::Run(content::ContentMainRunnerImpl * this, bool start_service_manager_only) (content/app/content_main_runner_impl.cc:947) libcontent.so!content::ContentServiceManagerMainDelegate::RunEmbedderProcess(content::ContentServiceManagerMainDelegate * this) (content/app/content_service_manager_main_delegate.cc:53) libembedder.so!service_manager::Main(const service_manager::MainParams & params) (services/service_manager/embedder/main.cc:472) libcontent.so!content::ContentMain(const content::ContentMainParams & params) (content/app/content_main.cc:19) content::BrowserTestBase::SetUp(policy::UserAffiliationBrowserTest_PRE_PRE_TestAffiliation_Test * this) (content/public/test/browser_test_base.cc:322) InProcessBrowserTest::SetUp(policy::UserAffiliationBrowserTest_PRE_PRE_TestAffiliation_Test * this) (chrome/test/base/in_process_browser_test.cc:251) testing::internal::InvokeHelper<std::__1::vector<user_manager::User*, std::__1::allocator<user_manager::User*> > const&, std::__1::tuple<> >::InvokeMethod<chromeos::MockUserManager, std::__1::vector<user_manager::User*, std::__1::allocator<user_manager::User*> > const& (chromeos::MockUserManager::*)() const>(chromeos::MockUserManager*, std::__1::vector<user_manager::User*, std::__1::allocator<user_manager::User*> > const& (chromeos::MockUserManager::*)() const, std::__1::tuple<> const&)(policy::UserAffiliationBrowserTest_PRE_PRE_TestAffiliation_Test * obj_ptr, const std::__1::vector<user_manager::User*, std::__1::allocator<user_manager::User*> > &(chromeos::MockUserManager::*)(const chromeos::MockUserManager * const) method_ptr) (third_party/googletest/src/googlemock/include/gmock/gmock-generated-actions.h:67) testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(policy::UserAffiliationBrowserTest_PRE_PRE_TestAffiliation_Test * object, void (testing::Test::*)(testing::Test * const) method, const char * location) (third_party/googletest/src/googletest/src/gtest.cc:2472) testing::Test::Run(policy::UserAffiliationBrowserTest_PRE_PRE_TestAffiliation_Test * this) (third_party/googletest/src/googletest/src/gtest.cc:2487) testing::TestInfo::Run(testing::TestInfo * this) (third_party/googletest/src/googletest/src/gtest.cc:2667) Second initialization: libbase.so!base::FieldTrialList::FieldTrialList(base::FieldTrialList * this, std::__1::unique_ptr<base::FieldTrial::EntropyProvider const, std::__1::default_delete<base::FieldTrial::EntropyProvider const> > entropy_provider) (base/metrics/field_trial.cc:500) make_unique<base::FieldTrialList, nullptr_t>(<unknown type in out/Debug/browser_tests, CU 0x0, DIE 0x42a42> __args) (buildtools/third_party/libc++/trunk/include/memory:3114) content::BrowserTestBase::BrowserTestBase(content::BrowserTestBase * this) (content/public/test/browser_test_base.cc:125) InProcessBrowserTest::InProcessBrowserTest(content::BrowserTestBase * this) (chrome/test/base/in_process_browser_test.cc:117) policy::UserAffiliationBrowserTest::UserAffiliationBrowserTest(content::BrowserTestBase * this) (chrome/browser/chromeos/policy/user_affiliation_browsertest.cc:120) policy::UserAffiliationBrowserTest_PRE_PRE_TestAffiliation_Test::UserAffiliationBrowserTest_PRE_PRE_TestAffiliation_Test(content::BrowserTestBase * this) (chrome/browser/chromeos/policy/user_affiliation_browsertest.cc:254) testing::internal::ParameterizedTestFactory<policy::UserAffiliationBrowserTest_PRE_PRE_TestAffiliation_Test>::CreateTest(testing::internal::ParameterizedTestFactory<policy::UserAffiliationBrowserTest_PRE_PRE_TestAffiliation_Test> * this) (third_party/googletest/src/googletest/include/gtest/internal/gtest-param-util.h:410) testing::internal::InvokeHelper<std::__1::vector<user_manager::User*, std::__1::allocator<user_manager::User*> > const&, std::__1::tuple<> >::InvokeMethod<chromeos::MockUserManager, std::__1::vector<user_manager::User*, std::__1::allocator<user_manager::User*> > const& (chromeos::MockUserManager::*)() const>(chromeos::MockUserManager*, std::__1::vector<user_manager::User*, std::__1::allocator<user_manager::User*> > const& (chromeos::MockUserManager::*)() const, std::__1::tuple<> const&)(testing::internal::ParameterizedTestFactory<policy::UserAffiliationBrowserTest_PRE_PRE_TestAffiliation_Test> * obj_ptr, const std::__1::vector<user_manager::User*, std::__1::allocator<user_manager::User*> > &(chromeos::MockUserManager::*)(const chromeos::MockUserManager * const) method_ptr) (third_party/googletest/src/googlemock/include/gmock/gmock-generated-actions.h:67) testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::TestFactoryBase, testing::Test*>(testing::internal::ParameterizedTestFactory<policy::UserAffiliationBrowserTest_PRE_PRE_TestAffiliation_Test> * object, testing::Test *(testing::internal::TestFactoryBase::*)(testing::internal::TestFactoryBase * const) method, const char * location) (third_party/googletest/src/googletest/src/gtest.cc:2472) testing::TestInfo::Run(testing::TestInfo * this) (third_party/googletest/src/googletest/src/gtest.cc:2658) testing::TestCase::Run(testing::TestCase * this) (third_party/googletest/src/googletest/src/gtest.cc:2785) testing::internal::UnitTestImpl::RunAllTests(testing::internal::UnitTestImpl * this) (third_party/googletest/src/googletest/src/gtest.cc:5047) testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl * object, bool (testing::internal::UnitTestImpl::*)(testing::internal::UnitTestImpl * const) method, const char * location) (third_party/googletest/src/googletest/src/gtest.cc:2417) testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl * object, bool (testing::internal::UnitTestImpl::*)(testing::internal::UnitTestImpl * const) method, const char * location) (third_party/googletest/src/googletest/src/gtest.cc:2472) testing::UnitTest::Run(testing::UnitTest * this) (third_party/googletest/src/googletest/src/gtest.cc:4663) RUN_ALL_TESTS() (third_party/googletest/src/googletest/include/gtest/gtest.h:2329) base::TestSuite::Run(ChromeTestSuite * this) (base/test/test_suite.cc:277) ChromeTestSuiteRunner::RunTestSuite(ChromeTestSuiteRunner * this, int argc, char ** argv) (chrome/test/base/chrome_test_launcher.cc:65) ChromeTestLauncherDelegate::RunTestSuite(ChromeTestLauncherDelegate * this, int argc, char ** argv) (chrome/test/base/chrome_test_launcher.cc:74) content::LaunchTests(ChromeTestLauncherDelegate * launcher_delegate, size_t parallel_jobs, int argc, char ** argv) (content/public/test/test_launcher.cc:645)
,
Aug 8
So the UserAffiliation tests appear to be Chrome-OS specific. Does this problem happen on other platforms or just CrOS?
,
Aug 8
Also, I thought bots run with DCHECKs enabled. Is there a reason we're not seeing this on the bots?
,
Aug 8
Hmm, I'm not able to repro the DCHECK but I see both codepaths in question being hit when running an arbitrary browser test on Mac. Will investigate further.
,
Aug 8
On Mac, first content::BrowserTestBase::BrowserTestBase code path runs. But then, that instance is deleted via: 6 browser_tests 0x00000001167bedbf std::__1::unique_ptr<base::FieldTrialList, std::__1::default_delete<base::FieldTrialList> >::reset(base::FieldTrialList*) + 95 7 browser_tests 0x00000001167be99f content::BrowserTestBase::SetUp() + 1919 8 browser_tests 0x00000001148aacfa InProcessBrowserTest::SetUp() + 1674 9 browser_tests 0x00000001116e4ebe void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) + 126 10 browser_tests 0x00000001116c5932 void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) + 114 11 browser_tests 0x00000001116c5814 testing::Test::Run() + 116 12 browser_tests 0x00000001116c62f0 testing::TestInfo::Run() + 224 13 browser_tests 0x00000001116c6f3f testing::TestCase::Run() + 239 14 browser_tests 0x00000001116cf754 testing::internal::UnitTestImpl::RunAllTests() + 692 15 browser_tests 0x00000001116e8d4e bool testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) + 126 16 browser_tests 0x00000001116cf422 bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) + 114 17 browser_tests 0x00000001116cf305 testing::UnitTest::Run() + 197 And then the ChromeBrowserMainParts::SetupFieldTrials() instance is created. It sounds like maybe on ChromeOS, the order of those two (BrowserTestBase::BrowserTestBase() ctor & ChromeBrowserMainParts::SetupFieldTrials()) is reversed? Or maybe the SetUp() code doesn't run? I think we need someone on CrOS to debug further. ljusten: since you reported the issue, would you mind checking which is it?
,
Aug 8
I'll take a look.
,
Aug 28
On Mac, this DCHECK also appears to fire if you run more than one browser_tests in --single_process mode (no test runner).
,
Sep 28
The --single_process failure is a different I see, I think.
I believe it regressed when we made FieldTrialList leaky:
// Initialize FieldTrialList to support FieldTrials. This is intentionally
// leaked since it needs to live for the duration of the browser process and
// there's no benefit in cleaning it up at exit.
base::FieldTrialList* leaked_field_trial_list = new base::FieldTrialList(
browser_process_->GetMetricsServicesManager()->CreateEntropyProvider());
ANNOTATE_LEAKING_OBJECT_PTR(leaked_field_trial_list);
ignore_result(leaked_field_trial_list);
This was to fix some shutdown crashes where some threads outlived the ChromeBrowserMainParts object.
We could make a more complicated set up where that instance is instead held in a static and not allocated again if that codepath is hit again - or deleting it and re-creating. It's not obvious to me whether that complexity is worth it - so I want to ask some more questions.
What is the benefit of --single_process mode for browser tests? Why would one need to run it?
And isn't it inherently both prone to flakes and breakage - breakage because we clearly don't run on bots - and flakes because since we don't run tests on the bots in this mode that there's no reason why browser tests would be coded to clean up after themselves if they change global state - since that's not how they are run - and so we probably have a ton of tests that have various side effects that would cause other tests to fail if run after. It's not obvious to me that we should spend any effort on this specific small problem if there's more fundamental problems with that mode.
Feedback/discussion welcome - the above are just my thoughts.
(I believe the above is all a bit orthogonal to the CrOS failures which from my understanding are not related to --single_process mode.)
,
Oct 8
> What is the benefit of --single_process mode for browser tests? Why would one need to run it? Running under a debugger is the primary use case.
,
Jan 9
I agree that running more than one test in --single_process mode does not make sense. In fact, the debugging paragraph in https://www.chromium.org/developers/testing/browser-tests discourages that. Per above, each test runs in a new browser process. To aid in debugging, you can start the browser test binary with --single_process flag (along with -- gtest_filter=Foo.Bar). That way you can attach to the process that just got created, which will contain the test code. >>> In this mode, make sure the filter only executes one test. Running more than one test will likely result in undefined behavior because the test fixture does not attempt to reinitialize any global state. <<< I'll pick up on this and see whether I can reproduce it for one test. If I ran multiple with --single_process, then it was clearly user error and I'll close this bug.
,
Jan 9
There's actually a use case where you need to run multiple tests even with --single_process, namely if you have a PRE_ test, e.g. WebViewTest.StoragePersistence. Repro: out/Debug/browser_tests --gtest_filter=WebViewTest.*StoragePersistence --single_process (Interestingly, the * is needed with --single_process only: out/Debug/browser_tests --gtest_filter=WebViewTest.StoragePersistence executes both PRE_ test and test. However, out/Debug/browser_tests --gtest_filter=WebViewTest.StoragePersistence --single_process only executes the actual test, but it fails since the PRE_ test didn't run.) Any suggestions how tests with PRE_ could be debugged? Maybe the PRE_ tests could run in multi-process mode (to prevent global leaks) and only the test you want to debug would run in single-process mode?
,
Jan 9
I guess as a workaround one could run the PRE_tests one by one once manually, then the actual test with debugger attached.
,
Jan 10
In any case, supporting PRE tests should be done in a way that doesn't require them all to run in a single process since that would be an uphill battle we couldn't win long-term. Marking as WontFix since the issue was user error (ran multiple tests with --single_process). Maybe we should add a large warning.
,
Jan 10
This is a dissatisfying resolution. I think there's a difference between the couched language of "likely result in undefined behavior" and the FieldTrial code preventing this from working at all.
,
Jan 10
Reopening for further discussion. I agree there's a difference in what the docs say and what actually happens (must be the first time *ever* at Google). However, I'm not sure whether trying to fix this DCHECK would be the right approach. Multiple tests + single process is an almost untested code path and hence it would be very hard to maintain, plus tests are known to have undefined behavior. I think I'd rather reword the docs. |
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by asvitk...@chromium.org
, Aug 7