New issue
Advanced search Search tips

Issue 811320 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Feb 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Mutating CSSAdditiveAnimations/WebAnimationsAPI runtime flags in LayoutTests is dangerous

Project Member Reported by smcgruer@chromium.org, Feb 12 2018

Issue description

In http://crrev.com/823e14d71a44eeeda943e6a13d0b2a97739cbee2 I added the ability to change the value of RuntimeEnabledFlags::CSSAdditiveAnimations/WebAnimationsAPI inside LayoutTests. This was an attempt to test different configurations of flags and their effects.

It turns out that two nasty things can coalesce to make such an ability
dangerous:

1. The bindings code caches which RuntimeEnabledFeatures are enabled the
   very first time a V8 object is instantiated. This cache is kept for
   the lifetime of the renderer.

2. A single content_shell instance may be reused for multiple
   LayoutTests.

For example:

$ ./out/Release/content_shell --run-layout-test --enable-experimental-web-platform-features animations/keyframe-effect-options-composite-parsing.html animations/multiple-same-name-css-animations.html

Content-Type: text/plain
This is a testharness.js-based test.
PASS In stable Chrome, valid composite values should not throw
PASS When just CSSAdditiveAnimations is enabled, valid composite values should not throw
PASS When just WebAnimationsAPI is enabled, valid composite values should not throw
Harness: the test ran to completion.

#EOF
#EOF
#EOF
Content-Type: text/plain
This is a testharness.js-based test.
FAIL Multiple same animation names should start multiple animations. Cannot read property 'timing' of undefined
FAIL Multiple same animation names should persist with animation timing updates. Cannot read property 'timing' of undefined
FAIL Mixed multiple same animation names should persist based on their same name relative position Cannot read property 'timing' of undefined
FAIL Removing same animation names should cancel animations from the end of the name list. Cannot read property 'timing' of undefined
FAIL Adding same animation names should start additional animations from the end of the name list. Cannot read property 'timing' of undefined
Harness: the test ran to completion.

#EOF
#EOF
#EOF

The first test eliminated 'anim.effect', so the second test starts to fail.

Since mutating these runtime flags causes unpredictable failures, we shouldn't be doing that.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Feb 12 2018

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

commit 5819a2a74d936f87e858e374cf5b8a77598d3668
Author: Stephen McGruer <smcgruer@chromium.org>
Date: Mon Feb 12 18:12:42 2018

Remove ability to set CSSAdditiveAnimations and WebAnimationsAPI in LayoutTests

It turns out that two nasty things can coalesce to make such an ability
dangerous:

1. The bindings code caches which RuntimeEnabledFeatures are enabled the
   very first time a V8 object is instantiated. This cache is kept for
   the lifetime of the renderer.

2. A single content_shell instance may be reused for multiple
   LayoutTests.

To avoid accidentally cross-polluting other tests, I am removing the
ability to override these RuntimeEnabledFeatures in tests. Instead, one
of the three cases covered by the test is added in a virtual/stable test
- which is really the important case.

Bug:  811320 
Change-Id: Idcb6656ec03a2a21bbe0af42cd33e6ca14933087
Reviewed-on: https://chromium-review.googlesource.com/911948
Commit-Queue: Stephen McGruer <smcgruer@chromium.org>
Reviewed-by: Robert Flack <flackr@chromium.org>
Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#536128}
[delete] https://crrev.com/7c7587e45a1eaa68b74361191a8355059a611804/third_party/WebKit/LayoutTests/animations/keyframe-effect-options-composite-parsing.html
[modify] https://crrev.com/5819a2a74d936f87e858e374cf5b8a77598d3668/third_party/WebKit/LayoutTests/virtual/stable/web-animations-api/additive-animations-unsupported.html
[modify] https://crrev.com/5819a2a74d936f87e858e374cf5b8a77598d3668/third_party/WebKit/Source/platform/runtime_enabled_features.json5

Status: Fixed (was: Started)

Comment 3 by flackr@chromium.org, Feb 12 2018

Labels: Test-Layout
Status: Started (was: Fixed)

Comment 4 by flackr@chromium.org, Feb 12 2018

Status: Fixed (was: Started)
Apologies, somehow I changed it at the same time.

Sign in to add a comment