New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 846380 link

Starred by 3 users

Issue metadata

Status: Untriaged
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug

Blocked on:
issue 891959



Sign in to add a comment

Tests should initialize ScopedFeatureList before threads are created.

Project Member Reported by gayane@chromium.org, May 24 2018

Issue description

Tests 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.

 

Comment 1 by gayane@chromium.org, May 24 2018

Proposed solution is to have safe version of ScopedFeatureList which will DCHECK that threads are not created before reassigning FeatureList global instance. 

In most cases it is possible to move ScopedFeatureList initialization before threads are created. However, in some cases the member constructor of the test fixture creates threads. For that, with new ScopedFeatureList it would be possible to init in ScopedFeatureList constructor which then should be called before the problematic member constructor.

The old ScopedFeatureList can be then renamed to UnsafeScopedFeatureList.

Comment 2 by gab@chromium.org, 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*()").
Cc: gayane@chromium.org
Owner: ----
Project Member

Comment 4 by bugdroid1@chromium.org, 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

Status: Available (was: Assigned)
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())
For reference, this bug is probably the cause of the test problem reported in bug 891959 (now disabled under ASAN).
Components: Test
Labels: -Pri-2 Target-74 Pri-1
Status: Untriaged (was: Available)
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).
Cc: etiennep@chromium.org
+etiennep who was affected by this (re. #7)
Project Member

Comment 9 by bugdroid1@chromium.org, 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

Blockedon: 891959

Sign in to add a comment