Bug 126142 – REGRESSION (Lazy tree creation): css3/calc/transitions-dependent.html is frequently failing

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Alexey Proskuryakov]

css3/calc/transitions-dependent.html started to fail at around the time lazy render tree patch was landed:

http://webkit-test-results.appspot.com/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=css3%2Fcalc%2Ftransitions-dependent.html

[Alexey Proskuryakov]

@@ -1,12 +1,12 @@
 This tests that calc() expressions depending on transitioning elements behave correctly.
-PASS - "width" property for "inner" element at 0s was: 60
-PASS - "width" property for "inner" element at 0.25s was: 110
-PASS - "width" property for "inner" element at 0.5s was: 160
-PASS - "width" property for "inner" element at 0.75s was: 210
+FAIL - "width" property for "inner" element at 0s expected: 60 but saw: 260
+FAIL - "width" property for "inner" element at 0.25s expected: 110 but saw: 260
+FAIL - "width" property for "inner" element at 0.5s expected: 160 but saw: 260
+FAIL - "width" property for "inner" element at 0.75s expected: 210 but saw: 260
 PASS - "width" property for "inner" element at 1s was: 260
-PASS - "width" property for "innerTransition" element at 0s was: 20
-PASS - "width" property for "innerTransition" element at 0.25s was: 70
-PASS - "width" property for "innerTransition" element at 0.5s was: 165
-PASS - "width" property for "innerTransition" element at 0.75s was: 305
+FAIL - "width" property for "innerTransition" element at 0s expected: 20 but saw: 490
+FAIL - "width" property for "innerTransition" element at 0.25s expected: 70 but saw: 490
+FAIL - "width" property for "innerTransition" element at 0.5s expected: 165 but saw: 490
+FAIL - "width" property for "innerTransition" element at 0.75s expected: 305 but saw: 490
 PASS - "width" property for "innerTransition" element at 1s was: 490

[Alexey Proskuryakov]

<rdar://problem/15725243>

[Alexey Proskuryakov]

Marked the test as flaky in <http://trac.webkit.org/r161073>.

[Alexey Proskuryakov]

Antti, did you have a chance to look into this?

[Diego Pino]

*** Bug 132182 has been marked as a duplicate of this bug. ***

[Diego Pino]

Created attachment 435670 [details]
Patch

[Alexey Proskuryakov]

Do the new results match other browsers? Not sure how to tell if the test is failing or passing now.

[Diego Pino]

Created attachment 444485 [details]
Patch

[Diego Pino]

I modernized the code of the test to be able to run it in other browsers. I run it in Chrome and Firefox and I got the same expected results.

[Alexey Proskuryakov]

Comment on attachment 444485 [details]
Patch

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

> LayoutTests/TestExpectations:1209
> +css3/calc/transitions-dependent.html [ DumpJSConsoleLogInStdErr ]

What does this test dump to console? DumpJSConsoleLogInStdErr is supposed to only be used for imported tests, which we cannot modify.

> LayoutTests/css3/calc/transitions-dependent.html:77
> +  let element = document.getElementById(elementId);

Looks like existing code in this test uses four-space indentation.

> LayoutTests/css3/calc/transitions-dependent.html:111
> +  window.testRunner.waitUntilDone();

Ditto. Also, no need to prefix with window. on this line.

[Diego Pino]

Created attachment 448823 [details]
Patch

[Diego Pino]

(In reply to Alexey Proskuryakov from comment #10)
> Comment on attachment 444485 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=444485&action=review
> 
> > LayoutTests/TestExpectations:1209
> > +css3/calc/transitions-dependent.html [ DumpJSConsoleLogInStdErr ]
> 
> What does this test dump to console? DumpJSConsoleLogInStdErr is supposed to
> only be used for imported tests, which we cannot modify.
> 

The `pauseTransitionAtTimeOnElement` call was printing out several messages on console:

https://webkit-search.igalia.com/webkit/source/LayoutTests/transitions/resources/transition-test-helpers.js#374

It seems the first 5 expected values should only check the `dependsOn` property while the last 5 expected values should check the `elementId` property.

I changed the test accordingly.

[EWS]

Committed r287874 (245918@main): <https://commits.webkit.org/245918@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 448823 [details].

[Dawn Morningstar]

*** Bug 239254 has been marked as a duplicate of this bug. ***

[Dawn Morningstar]

(In reply to Diego Pino from comment #12)
> (In reply to Alexey Proskuryakov from comment #10)
> > Comment on attachment 444485 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=444485&action=review
> > 
> > > LayoutTests/TestExpectations:1209
> > > +css3/calc/transitions-dependent.html [ DumpJSConsoleLogInStdErr ]
> > 
> > What does this test dump to console? DumpJSConsoleLogInStdErr is supposed to
> > only be used for imported tests, which we cannot modify.
> > 
> 
> The `pauseTransitionAtTimeOnElement` call was printing out several messages
> on console:
> 
> https://webkit-search.igalia.com/webkit/source/LayoutTests/transitions/
> resources/transition-test-helpers.js#374
> 
> It seems the first 5 expected values should only check the `dependsOn`
> property while the last 5 expected values should check the `elementId`
> property.
> 
> I changed the test accordingly.

I am still seeing a failure and the following console messages:
+CONSOLE MESSAGE: A transition for property width could not be found
+CONSOLE MESSAGE: A transition for property width could not be found
+CONSOLE MESSAGE: A transition for property width could not be found
+CONSOLE MESSAGE: A transition for property width could not be found
+CONSOLE MESSAGE: A transition for property width could not be found
+CONSOLE MESSAGE: A transition for property width could not be found
+CONSOLE MESSAGE: A transition for property width could not be found
+CONSOLE MESSAGE: A transition for property width could not be found
+CONSOLE MESSAGE: A transition for property width could not be found
+CONSOLE MESSAGE: A transition for property width could not be found

I had created a new bug here https://bugs.webkit.org/show_bug.cgi?id=239254
But rather I shall mark that as a dupe and re-open this one.

[Alexey Proskuryakov]

That appears to be a new different issue, so it should be tracked in a new bug.

[Alexey Proskuryakov]

Please disregard my comment, I didn't analyze the history enough to tell if it's a different issue, or same one continued.

[Diego Pino]

Pull request: https://github.com/webkit/webkit/pull/3346

[Diego Pino]

The regression is 100% reproducible when running the test at least 2 times:

```
$ Tools/Scripts/run-webkit-tests --release --gtk --iterations=2 css3/calc/transitions-dependent.html
```

In the cases where the actual results didn't match the expected results, I noticed it seemed that the issue was that the animation had already finished. For instance:


https://build.webkit.org/results/GTK-Linux-64-bit-Debug-Tests/253440@main%20(6596)/css3/calc/transitions-dependent-pretty-diff.html

In this test result, the outer animation as well as the innerTransition animation have for each of their intervals a width that corresponds to the value of the width when the animation ends.

To prevent this error what I did in the patch was to set the animation on pause as soon as it's initialized and also create and initialize the animations separately (first the outer element and then innerTransition element).

Once that is done I can no longer reproduce the error even by repeating the test several times:

```
$ Tools/Scripts/run-webkit-tests --release --gtk --iterations=2  --repeat=50 css3/calc/transitions-dependent.html
```

I think this will solve the flakiness of the test.

[EWS]

Committed 255618@main (eae54080e781): <https://commits.webkit.org/255618@main>

Reviewed commits have been landed. Closing PR #3346 and removing active labels.