Bug 187264 – REGRESSION(r233325): [GTK] Broke 40 animations tests

• Component: Animations
• Status: RESOLVED FIXED

[Michael Catanzaro]

r233325 "[Web Animations] Make imported/mozilla/css-animations/test_animation-starttime.html pass reliably" introduced about 40 animations test failures on the GTK bots. Almost all of these tests are now timing out. I'll follow up with a list of affected tests momentarily.

Interestingly, the aforementioned test was passing consistently for GTK until it started timing out after this commit.

[Michael Catanzaro]

Affected tests:

webkit.org/b/187264 animations/transition-and-animation-2.html [ Failure ]
webkit.org/b/187264 imported/w3c/web-platform-tests/web-animations/animation-model/keyframe-effects/effect-value-context.html [ Failure ]
webkit.org/b/187264 imported/w3c/web-platform-tests/web-animations/interfaces/Animation/cancel.html [ Failure ]
webkit.org/b/187264 imported/w3c/web-platform-tests/web-animations/interfaces/Animation/pause.html [ Failure ]
webkit.org/b/187264 imported/w3c/web-platform-tests/web-animations/interfaces/Animation/pending.html [ Failure ]
webkit.org/b/187264 imported/w3c/web-platform-tests/web-animations/interfaces/Animation/play.html [ Failure ]
webkit.org/b/187264 imported/w3c/web-platform-tests/web-animations/interfaces/Animation/playbackRate.html [ Failure ]
webkit.org/b/187264 imported/w3c/web-platform-tests/web-animations/interfaces/Animation/ready.html [ Failure ]
webkit.org/b/187264 imported/w3c/web-platform-tests/web-animations/interfaces/Animation/startTime.html [ Failure ]
webkit.org/b/187264 imported/w3c/web-platform-tests/web-animations/interfaces/AnimationEffectTiming/duration.html [ Failure ]
webkit.org/b/187264 imported/w3c/web-platform-tests/web-animations/timing-model/animations/canceling-an-animation.html [ Failure ]
webkit.org/b/187264 imported/w3c/web-platform-tests/web-animations/timing-model/animations/pausing-an-animation.html [ Failure ]
webkit.org/b/187264 imported/w3c/web-platform-tests/web-animations/timing-model/animations/playing-an-animation.html [ Failure ]
webkit.org/b/187264 imported/w3c/web-platform-tests/web-animations/timing-model/animations/reversing-an-animation.html [ Failure ]
webkit.org/b/187264 imported/w3c/web-platform-tests/web-animations/timing-model/animations/set-the-target-effect-of-an-animation.html [ Failure ]
webkit.org/b/187264 imported/w3c/web-platform-tests/web-animations/timing-model/animations/set-the-timeline-of-an-animation.html [ Failure ]
webkit.org/b/187264 imported/w3c/web-platform-tests/web-animations/timing-model/timelines/document-timelines.html [ Failure ]
webkit.org/b/187264 compositing/animation/layer-for-filling-animation.html [ Timeout ]
webkit.org/b/187264 imported/mozilla/css-animations/test_animation-cancel.html [ Timeout ]
webkit.org/b/187264 imported/mozilla/css-animations/test_animation-computed-timing.html [ Timeout ]
webkit.org/b/187264 imported/mozilla/css-animations/test_animation-currenttime.html [ Timeout ]
webkit.org/b/187264 imported/mozilla/css-animations/test_animation-finish.html [ Timeout ]
webkit.org/b/187264 imported/mozilla/css-animations/test_animation-pausing.html [ Timeout ]
webkit.org/b/187264 imported/mozilla/css-animations/test_animation-ready.html [ Timeout ]
webkit.org/b/187264 imported/mozilla/css-animations/test_animation-starttime.html [ Timeout ]
webkit.org/b/187264 imported/mozilla/css-animations/test_setting-effect.html [ Timeout ]
webkit.org/b/187264 imported/mozilla/css-transitions/test_animation-cancel.html [ Timeout ]
webkit.org/b/187264 imported/mozilla/css-transitions/test_animation-computed-timing.html [ Timeout ]
webkit.org/b/187264 imported/mozilla/css-transitions/test_animation-currenttime.html [ Timeout ]
webkit.org/b/187264 imported/mozilla/css-transitions/test_animation-pausing.html [ Timeout ]
webkit.org/b/187264 imported/mozilla/css-transitions/test_animation-ready.html [ Timeout ]
webkit.org/b/187264 imported/mozilla/css-transitions/test_animation-starttime.html [ Timeout ]
webkit.org/b/187264 imported/mozilla/css-transitions/test_event-dispatch.html [ Timeout ]
webkit.org/b/187264 imported/mozilla/css-transitions/test_setting-effect.html [ Timeout ]
webkit.org/b/187264 transitions/move-after-transition.html [ Timeout ]
webkit.org/b/187264 webanimations/opacity-animation-yields-compositing-span.html [ Timeout ]
webkit.org/b/187264 webanimations/opacity-animation-yields-compositing.html [ Timeout ]

Of these, all but two are timeouts (the other failures are timeouts not registered with the test runner).

animations/transition-and-animation-2.html has a weird text diff:

--- /home/slave/webkitgtk/gtk-linux-64-release-tests/build/layout-test-results/animations/transition-and-animation-2-expected.txt
+++ /home/slave/webkitgtk/gtk-linux-64-release-tests/build/layout-test-results/animations/transition-and-animation-2-actual.txt
@@ -1,3 +1,3 @@
 This test has a transition and animation on the same property (-webkit-transform). The animation starts and then the transition is triggered. The transition should start at the position before the animation started (the unanimated position), which is (0,0). If it starts from the start point of the animation (0,100) then there is an error
-PASS - "webkitTransform.5" property for "box" element at 0.4s saw something close to: 0
+FAIL - "webkitTransform.5" property for "box" element at 0.4s expected: 0 but saw: matrix(1, 0, 0, 1, 0, 100)

It almost looks unrelated, but the only animations-related commit within the range where it started failing is r233325, so I'll mark it against this bug.

Then imported/w3c/web-platform-tests/web-animations/interfaces/Animatable/animate-no-browsing-context.html started to fail with this diff:

--- /home/slave/webkitgtk/gtk-linux-64-release-tests/build/layout-test-results/imported/w3c/web-platform-tests/web-animations/interfaces/Animatable/animate-no-browsing-context-expected.txt
+++ /home/slave/webkitgtk/gtk-linux-64-release-tests/build/layout-test-results/imported/w3c/web-platform-tests/web-animations/interfaces/Animatable/animate-no-browsing-context-actual.txt
@@ -1,6 +1,6 @@
 
 PASS Element.animate() creates an animation with the correct timeline when called on an element in a document without a browsing context 
 PASS The timeline associated with an animation trigger on an element in a document without a browsing context is inactive 
-FAIL Replacing the timeline of an animation targetting an element in a document without a browsing context leaves it in the pending state assert_true: The animation should still be pending after replacing the document timeline expected true got false
+PASS Replacing the timeline of an animation targetting an element in a document without a browsing context leaves it in the pending state 
 PASS Replacing the timeline of an animation targetting an element in a document without a browsing context and then adopting that element causes it to start updating style 

I'll update our expectation for that test, since the PASS looks nicer than a FAIL to me.

[Carlos Garcia Campos]

This is because DocumentAnimationScheduler uses DisplayRefreshMonitor, that is only used by GTK port when accelerated compositing is enabled. Tests work if AC is forced, and fail if AC is disabled or even on demand. In the case of being on demand it fails because it expects a monitor refresh before entering AC mode. I'm not sure how to solve this, though in the case of on demand AC. I've added a DisplayRefreshMonitorGtk to be used when not in AC mode and it works, but I think it conflicts with the threaded refresh display monitor created by the threaded compositor when entering AC. It seems to work, but it's probably inefficient. So, maybe we could try to enter AC whenever DocumentAnimationScheduler tries to create a display refresh monitor. Another option could be to destroy the GTK refresh monitor when entering AC.

[Carlos Garcia Campos]

Created attachment 344275 [details]
Patch

[EWS Watchlist]

Attachment 344275 [details] did not pass style-queue:


ERROR: Source/WebKit/Shared/CoordinatedGraphics/threadedcompositor/ThreadedCompositor.cpp:59:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Catanzaro]

Comment on attachment 344275 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=344275&action=review

LGTM but I think it needs to be run past Miguel or Zan.

Do we now have two display refresh monitors running while in AC mode?

> Source/WebCore/platform/graphics/gtk/DisplayRefreshMonitorGtk.cpp:62
> +        m_window = gtk_offscreen_window_new();

I assume you've checked to ensure that you don't need to sink the ref.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #5)
> Comment on attachment 344275 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=344275&action=review
> 
> LGTM but I think it needs to be run past Miguel or Zan.
> 
> Do we now have two display refresh monitors running while in AC mode?

No, the GTK one is destroyed when entering AC.

> > Source/WebCore/platform/graphics/gtk/DisplayRefreshMonitorGtk.cpp:62
> > +        m_window = gtk_offscreen_window_new();
> 
> I assume you've checked to ensure that you don't need to sink the ref.

It's a toplevel.

[EWS Watchlist]

Comment on attachment 344275 [details]
Patch

Attachment 344275 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/8435876

New failing tests:
animations/needs-layout.html

[EWS Watchlist]

Created attachment 344282 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4

[Carlos Garcia Campos]

Committed r233512: <https://trac.webkit.org/changeset/233512>

[Radar WebKit Bug Importer]

<rdar://problem/41837559>