Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2879293002:
Move setup of base::test::ScopedFeatureList earlier, to avoid data race. (Closed)
|
Created:
3 years, 7 months ago by Łukasz Anforowicz Modified:
3 years, 7 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, jam, darin-cc_chromium.org, extensions-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove setup of base::test::ScopedFeatureList earlier, to avoid data race.
base::test::ScopedFeatureList has to be setup before
content::BrowserTestBase::SetUp calls content::ContentMain
(because the latter spawns other threads that might start reading
the global state of features).
BUG=722409
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2879293002
Cr-Commit-Position: refs/heads/master@{#471911}
Committed: https://chromium.googlesource.com/chromium/src/+/0b772bacda31df9e862388f4859f1d56d3d217ae
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : . #Patch Set 4 : . #
Dependent Patchsets: Messages
Total messages: 34 (24 generated)
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Move setup of base::test::ScopedFeatureList earlier, to avoid data race. base::test::ScopedFeatureList has to be setup before content::BrowserTestBase::SetUp calls content::ContentMain (because the latter spawns other threads that might start reading the global state of features). BUG=722409 ========== to ========== Move setup of base::test::ScopedFeatureList earlier, to avoid data race. base::test::ScopedFeatureList has to be setup before content::BrowserTestBase::SetUp calls content::ContentMain (because the latter spawns other threads that might start reading the global state of features). BUG=722409 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lukasza@chromium.org changed reviewers: + asvitkine@chromium.org
asvitkine@, could you PTAL? This is a fairly minimal CL to make TSAN bot green again. I also have a follow-up WIP CL (@ https://codereview.chromium.org/2887523002) where I try to add a DCHECK that disallows changing features while a browser test has already launched the browser. It turns out that adding the DCHECK is difficult - there are quite a few browser tests that (incorrectly?) end up calling base::FeatureList::ClearInstanceForTesting *after* the browser is already running.
lgtm
The CQ bit was unchecked by lukasza@chromium.org
The CQ bit was checked by lukasza@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
lukasza@chromium.org changed reviewers: + creis@chromium.org
creis@, could you PTAL?
content/ LGTM. Thanks!
lukasza@chromium.org changed reviewers: + danakj@chromium.org
danakj@, could you PTAL @ the changes in base/test/scoped_feature_list.h? FWIW, the main author of this file (asvitkine@) has already taken a look, so I think this just needs a stamp of approval from a //base OWNER.
LGTM
The CQ bit was checked by lukasza@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1494882644906130,
"parent_rev": "d7c4b64db12ae0c5e19593df15cc64fd0817bc64", "commit_rev":
"0b772bacda31df9e862388f4859f1d56d3d217ae"}
Message was sent while issue was closed.
Description was changed from ========== Move setup of base::test::ScopedFeatureList earlier, to avoid data race. base::test::ScopedFeatureList has to be setup before content::BrowserTestBase::SetUp calls content::ContentMain (because the latter spawns other threads that might start reading the global state of features). BUG=722409 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Move setup of base::test::ScopedFeatureList earlier, to avoid data race. base::test::ScopedFeatureList has to be setup before content::BrowserTestBase::SetUp calls content::ContentMain (because the latter spawns other threads that might start reading the global state of features). BUG=722409 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2879293002 Cr-Commit-Position: refs/heads/master@{#471911} Committed: https://chromium.googlesource.com/chromium/src/+/0b772bacda31df9e862388f4859f... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/0b772bacda31df9e862388f4859f... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
