Tests should initialize ScopedFeatureList before threads are created. |
||||||
Issue descriptionTests should initialize ScopedFeatureList before threads are created and ScopedFeatureList should be destroyed when all the threads are finished running. When initializing or destorying ScopedFetaurelist, the FeatureList global instance gets reassigned. During those changes, concurrent threads can potentially access the FeatureList global instance, which can result in data race issues.
,
May 24 2018
re. "most cases it is possible to move ScopedFeatureList initialization before threads are created" I don't think so. Most tests have a base::test::ScopedTaskEnvironment member (and it is recommended to put it first). What you want is to recommend putting ScopedFeatureList first (i.e. override the "ScopedTaskEnvironment first" rule). As such I think initializing in the constructor is the desired recommended default (i.e. that all Init*() methods should be renamed to "UnsageInit*()").
,
Oct 25
,
Nov 12
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1b876dd0841d6a1a44a5fbdadff2191012a1e0f1 commit 1b876dd0841d6a1a44a5fbdadff2191012a1e0f1 Author: Francois Doray <fdoray@chromium.org> Date: Mon Nov 12 17:09:37 2018 Base: Add feature to disable thread priorities. This will allow us to assess the impact of using thread priorities. The impact can be both positive (unimportant work gets out of the way) and negative (priority inversions when sharing locks across threads with different priorities). Note: This CL has extra complexity because we don't have guarantees that the FeatureList is initialized before threads are started in unit tests https://crbug.com/846380. Bug: 872820, 890978 , 902441, 846380 Change-Id: I8887c7b9e0eb77f7c11aa6e2be8af1ea7150b491 Reviewed-on: https://chromium-review.googlesource.com/c/1318686 Commit-Queue: François Doray <fdoray@chromium.org> Reviewed-by: Alexei Svitkine <asvitkine@chromium.org> Reviewed-by: Gabriel Charette <gab@chromium.org> Cr-Commit-Position: refs/heads/master@{#607276} [modify] https://crrev.com/1b876dd0841d6a1a44a5fbdadff2191012a1e0f1/base/BUILD.gn [modify] https://crrev.com/1b876dd0841d6a1a44a5fbdadff2191012a1e0f1/base/task/task_scheduler/task_scheduler_impl.cc [add] https://crrev.com/1b876dd0841d6a1a44a5fbdadff2191012a1e0f1/base/threading/platform_thread.cc [modify] https://crrev.com/1b876dd0841d6a1a44a5fbdadff2191012a1e0f1/base/threading/platform_thread.h [modify] https://crrev.com/1b876dd0841d6a1a44a5fbdadff2191012a1e0f1/base/threading/platform_thread_fuchsia.cc [modify] https://crrev.com/1b876dd0841d6a1a44a5fbdadff2191012a1e0f1/base/threading/platform_thread_mac.mm [modify] https://crrev.com/1b876dd0841d6a1a44a5fbdadff2191012a1e0f1/base/threading/platform_thread_posix.cc [modify] https://crrev.com/1b876dd0841d6a1a44a5fbdadff2191012a1e0f1/base/threading/platform_thread_win.cc
,
Nov 22
FYI, this was problematic just now @ https://chromium-review.googlesource.com/c/chromium/src/+/1347585/3 Failing tests: content_unittests : SessionStorageContextMojoTest.MigrationV0ToV1 SessionStorageContextMojoTest.Cloning SessionStorageContextMojoTest.GetUsage SessionStorageContextMojoTest.CorruptionOnDisk SessionStorageContextMojoTest.PurgeMemoryDoesNotCrashOrHang unit_tests: MediaEngagementScoreTest.OverrideFieldTrial DiceTurnSyncOnHelperTest.StartSync DiceTurnSyncOnHelperTest.ShowSyncDialogBlockedUntilSyncStartupFailedForEnterpriseAccount DiceTurnSyncOnHelperTest.ConfigureSync DiceTurnSyncOnHelperTest.UndoSync DiceTurnSyncOnHelperTest.ShowSyncDialogBlockedUntilSyncStartupCompletedForEnterpriseAccount DiceTurnSyncOnHelperTest.EnterpriseConfirmationContinue DiceTurnSyncOnHelperTestWithUnifiedConsent.ShowSyncDialogForEndConsumerAccount_UnifiedConsentEnabled DiceTurnSyncOnHelperTest.ShowSyncDialogForEndConsumerAccount DiceTurnSyncOnHelperTest.CrossAccountContinue Because ~ScopedFeatureList() is performed while there are still worker threads around (i.e. ~ScopedTaskEnvironment(), or ~TestBrowserThreadBundle() in this case, should precede ~ScopedFeatureList())
,
Nov 28
For reference, this bug is probably the cause of the test problem reported in bug 891959 (now disabled under ASAN).
,
Dec 12
This was problematic again @ https://chromium-review.googlesource.com/c/chromium/src/+/1359127 Can we prioritize fixing this for Q1? It's a source of complexity to use the feature API in numerous use cases. A simple way to verify proper ordering would be to DCHECK(!base::TaskScheduler::GetInstance()) in ScopedFeatureList's constructor and destructor. If failures identified in #5 are representative of this, it shouldn't be too many tests that need to be fixed in order to fix this for good (and prevent regressions).
,
Dec 12
+etiennep who was affected by this (re. #7)
,
Dec 12
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a6fb79e9459a484d08b6d1837128b161686ac538 commit a6fb79e9459a484d08b6d1837128b161686ac538 Author: Etienne Pierre-Doray <etiennep@chromium.org> Date: Wed Dec 12 22:48:04 2018 Use singleton feature list in content::UnitTestTestSuite. Commit 3f40244cd1ae911072fdccc827fdf446e1a66005 had to be reverted due to a data race between ScopedFeatureList's destructor and a feature in the scheduler. Sample failure: https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Linux%20TSan%20Tests/30398 There is no need for the feature list to be restored with ScopedFeatureList in the test launcher. Since the scheduler used for test launcher is leaked, it makes sense to have a singleton feature list instead of a ScopedFeatureList. Bug: 847501, 846380 Change-Id: I738ebebe5aa04871427399bcf012c072d7f0212c Reviewed-on: https://chromium-review.googlesource.com/c/1361441 Commit-Queue: Etienne Pierre-Doray <etiennep@chromium.org> Reviewed-by: Scott Violet <sky@chromium.org> Reviewed-by: François Doray <fdoray@chromium.org> Cr-Commit-Position: refs/heads/master@{#616090} [modify] https://crrev.com/a6fb79e9459a484d08b6d1837128b161686ac538/content/public/test/unittest_test_suite.cc [modify] https://crrev.com/a6fb79e9459a484d08b6d1837128b161686ac538/content/public/test/unittest_test_suite.h
,
Dec 18
|
||||||
►
Sign in to add a comment |
||||||
Comment 1 by gayane@chromium.org
, May 24 2018