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

Issue 822764 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2018
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug

Blocking:
issue 822396



Sign in to add a comment

base::test::ScopedFeatureList Needs To Be Initialized In Test Init

Project Member Reported by bcwh...@chromium.org, Mar 16 2018

Issue description

To use base::test::ScopedFeatureList, it has to be initialized in either the test ctor or in the Setup() method.

Problem introduced here:
https://chromium-review.googlesource.com/c/chromium/src/+/895842

If it's done later, there can be memory problems (caught by TSAN) if other running threads try to access the feature list after it has been changed.  Overriding the features inside test init ensures that its done before other threads start.

Offending lines:

https://cs.chromium.org/chromium/src/content/browser/site_per_process_hit_test_browsertest.cc?rcl=353fc9f1a01762c738cfe15ede8fb32ca5ff96dc&l=1890

https://cs.chromium.org/chromium/src/content/browser/site_per_process_hit_test_browsertest.cc?rcl=353fc9f1a01762c738cfe15ede8fb32ca5ff96dc&l=1924

 
Project Member

Comment 1 by bugdroid1@chromium.org, Mar 16 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/431c6dbf0a42d0c31c7dccd6553c6c496f1042a0

commit 431c6dbf0a42d0c31c7dccd6553c6c496f1042a0
Author: Sahel Sharifymoghaddam <sahel@chromium.org>
Date: Fri Mar 16 18:30:51 2018

Revert "Disable wheel scroll latching in failing content_browsertests."

This reverts commit f2bf009a8ad55dd79d87c14a7cad6473ece890bc.

Reason for revert: There has been a few bug fixes related to oopif and
latching, I will enable the latching flag to see if the failing tests
are fixed or not. If not I will disable the tests again addressing  crbug.com/822764  bug as well.

Original change's description:
> Disable wheel scroll latching in failing content_browsertests.
> 
> I cannot reproduce the test failures on my local official windows build,
> in this cl I force wheel scroll latching to be disabled in the tests to
> see if it changes the test results on Win official or not.
> 
> Bug: 800822
> Change-Id: Ife07a29617ca5e0142e75f926967685cb61e2a08
> Reviewed-on: https://chromium-review.googlesource.com/895842
> Reviewed-by: James MacLean <wjmaclean@chromium.org>
> Reviewed-by: Nasko Oskov <nasko@chromium.org>
> Commit-Queue: Sahel Sharifymoghaddam <sahel@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#533347}

TBR=nasko@chromium.org,wjmaclean@chromium.org,sahel@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 800822,  822764 
Change-Id: I563672dfbcb01298324a2b884cbed0e69c220339
Reviewed-on: https://chromium-review.googlesource.com/966687
Reviewed-by: Sahel Sharifymoghaddam <sahel@chromium.org>
Commit-Queue: Sahel Sharifymoghaddam <sahel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#543765}
[modify] https://crrev.com/431c6dbf0a42d0c31c7dccd6553c6c496f1042a0/content/browser/site_per_process_hit_test_browsertest.cc

Project Member

Comment 2 by bugdroid1@chromium.org, Mar 17 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/aba7e429ab0894d55f03467b8ec57bf5b9e7682a

commit aba7e429ab0894d55f03467b8ec57bf5b9e7682a
Author: Charlie Reis <creis@chromium.org>
Date: Sat Mar 17 00:27:10 2018

Reland "Disable wheel scroll latching in failing content_browsertests."

This reverts commit 431c6dbf0a42d0c31c7dccd6553c6c496f1042a0.

Reason for revert: r543765 caused more test failures, tracked in https://crbug.com/822980.

Original change's description:
> Revert "Disable wheel scroll latching in failing content_browsertests."
> 
> This reverts commit f2bf009a8ad55dd79d87c14a7cad6473ece890bc.
> 
> Reason for revert: There has been a few bug fixes related to oopif and
> latching, I will enable the latching flag to see if the failing tests
> are fixed or not. If not I will disable the tests again addressing  crbug.com/822764  bug as well.
> 
> Original change's description:
> > Disable wheel scroll latching in failing content_browsertests.
> > 
> > I cannot reproduce the test failures on my local official windows build,
> > in this cl I force wheel scroll latching to be disabled in the tests to
> > see if it changes the test results on Win official or not.
> > 
> > Bug: 800822
> > Change-Id: Ife07a29617ca5e0142e75f926967685cb61e2a08
> > Reviewed-on: https://chromium-review.googlesource.com/895842
> > Reviewed-by: James MacLean <wjmaclean@chromium.org>
> > Reviewed-by: Nasko Oskov <nasko@chromium.org>
> > Commit-Queue: Sahel Sharifymoghaddam <sahel@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#533347}
> 
> TBR=nasko@chromium.org,wjmaclean@chromium.org,sahel@chromium.org
> 
> # Not skipping CQ checks because original CL landed > 1 day ago.
> 
> Bug: 800822,  822764 
> Change-Id: I563672dfbcb01298324a2b884cbed0e69c220339
> Reviewed-on: https://chromium-review.googlesource.com/966687
> Reviewed-by: Sahel Sharifymoghaddam <sahel@chromium.org>
> Commit-Queue: Sahel Sharifymoghaddam <sahel@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#543765}

TBR=nasko@chromium.org,wjmaclean@chromium.org,sahel@chromium.org

Change-Id: Ie0212e01b308826c96eac54b86168ce9d1d04231
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 800822,  822764 
Reviewed-on: https://chromium-review.googlesource.com/967522
Reviewed-by: Charlie Reis <creis@chromium.org>
Commit-Queue: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#543883}
[modify] https://crrev.com/aba7e429ab0894d55f03467b8ec57bf5b9e7682a/content/browser/site_per_process_hit_test_browsertest.cc

Comment 3 by sahel@chromium.org, Mar 21 2018

Cc: creis@chromium.org sahel@chromium.org
 Issue 823713  has been merged into this issue.
Project Member

Comment 4 by bugdroid1@chromium.org, Mar 22 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b23ffdff9c21cfe220d739bd3ada55d3fd6a6924

commit b23ffdff9c21cfe220d739bd3ada55d3fd6a6924
Author: Sahel Sharify <sahel@chromium.org>
Date: Thu Mar 22 20:22:26 2018

Moved feature_list initialization to test SetUp.

Since I didn't want to disable wheel scroll latching in
InputEventRouterWheelTargetTest I moved MainframeWheelEventsOnMainThread
and SubframeWheelEventsOnMainThread tests to a new subclass that
disables WheelScrollLatching in the SetUp function.
The change is for avoiding data race due to initialization of the
feature_list in the tests.

Bug:  822764 
Change-Id: I76d9fee1474e8e7a50eca8d311941bcd969dafe3
Reviewed-on: https://chromium-review.googlesource.com/974366
Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Commit-Queue: Sahel Sharifymoghaddam <sahel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#545216}
[modify] https://crrev.com/b23ffdff9c21cfe220d739bd3ada55d3fd6a6924/content/browser/site_per_process_hit_test_browsertest.cc
[modify] https://crrev.com/b23ffdff9c21cfe220d739bd3ada55d3fd6a6924/testing/buildbot/filters/viz.content_browsertests.filter

Project Member

Comment 5 by bugdroid1@chromium.org, Mar 22 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a33992f07c49cf533d3f787a262d2d58d30cd73f

commit a33992f07c49cf533d3f787a262d2d58d30cd73f
Author: Matthew Wolenetz <wolenetz@chromium.org>
Date: Thu Mar 22 21:00:44 2018

Revert "Moved feature_list initialization to test SetUp."

This reverts commit b23ffdff9c21cfe220d739bd3ada55d3fd6a6924.

Reason for revert: Strong suspect of breaking the tree:
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Linux%20Builder/100140

Original change's description:
> Moved feature_list initialization to test SetUp.
> 
> Since I didn't want to disable wheel scroll latching in
> InputEventRouterWheelTargetTest I moved MainframeWheelEventsOnMainThread
> and SubframeWheelEventsOnMainThread tests to a new subclass that
> disables WheelScrollLatching in the SetUp function.
> The change is for avoiding data race due to initialization of the
> feature_list in the tests.
> 
> Bug:  822764 
> Change-Id: I76d9fee1474e8e7a50eca8d311941bcd969dafe3
> Reviewed-on: https://chromium-review.googlesource.com/974366
> Reviewed-by: Ken Buchanan <kenrb@chromium.org>
> Commit-Queue: Sahel Sharifymoghaddam <sahel@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#545216}

TBR=kenrb@chromium.org,sahel@chromium.org

Change-Id: If121cc768138e89668a72b7b6e22576fb3e23875
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  822764 
Reviewed-on: https://chromium-review.googlesource.com/976443
Reviewed-by: Matthew Wolenetz <wolenetz@chromium.org>
Commit-Queue: Matthew Wolenetz <wolenetz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#545246}
[modify] https://crrev.com/a33992f07c49cf533d3f787a262d2d58d30cd73f/content/browser/site_per_process_hit_test_browsertest.cc
[modify] https://crrev.com/a33992f07c49cf533d3f787a262d2d58d30cd73f/testing/buildbot/filters/viz.content_browsertests.filter

Project Member

Comment 6 by bugdroid1@chromium.org, Mar 26 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/df22a1bc4fea8295fedd71fc1c758d02aa87284b

commit df22a1bc4fea8295fedd71fc1c758d02aa87284b
Author: Sahel Sharify <sahel@chromium.org>
Date: Mon Mar 26 15:39:16 2018

Reland "Moved feature_list initialization to test SetUp."

This is a reland of b23ffdff9c21cfe220d739bd3ada55d3fd6a6924

Original change's description:
> Moved feature_list initialization to test SetUp.
>
> Since I didn't want to disable wheel scroll latching in
> InputEventRouterWheelTargetTest I moved MainframeWheelEventsOnMainThread
> and SubframeWheelEventsOnMainThread tests to a new subclass that
> disables WheelScrollLatching in the SetUp function.
> The change is for avoiding data race due to initialization of the
> feature_list in the tests.
>
> Bug:  822764 
> Change-Id: I76d9fee1474e8e7a50eca8d311941bcd969dafe3
> Reviewed-on: https://chromium-review.googlesource.com/974366
> Reviewed-by: Ken Buchanan <kenrb@chromium.org>
> Commit-Queue: Sahel Sharifymoghaddam <sahel@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#545216}

Bug:  822764 
Change-Id: I385b522c2c56bede81f432060519b4055407e859
TBR: kenrb@chromium.org
Reviewed-on: https://chromium-review.googlesource.com/978421
Reviewed-by: Sahel Sharifymoghaddam <sahel@chromium.org>
Commit-Queue: Sahel Sharifymoghaddam <sahel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#545800}
[modify] https://crrev.com/df22a1bc4fea8295fedd71fc1c758d02aa87284b/content/browser/site_per_process_hit_test_browsertest.cc
[modify] https://crrev.com/df22a1bc4fea8295fedd71fc1c758d02aa87284b/testing/buildbot/filters/viz.content_browsertests.filter

Comment 7 by sahel@chromium.org, Mar 26 2018

Status: Fixed (was: Assigned)

Sign in to add a comment