Bug 126142

Summary: REGRESSION (Lazy tree creation): css3/calc/transitions-dependent.html is frequently failing
Product: WebKit Reporter: Alexey Proskuryakov <ap>
Component: CSSAssignee: Diego Pino <dpino>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, dino, dpino, eocanha, graouts, kling, koivisto, Morningstar, simon.fraser, webkit-bug-importer
Priority: P1 Keywords: InRadar, Regression
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=239990
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Alexey Proskuryakov 2013-12-22 17:47:07 PST
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
Comment 1 Alexey Proskuryakov 2013-12-23 09:37:42 PST
@@ -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
Comment 2 Alexey Proskuryakov 2013-12-25 15:45:47 PST
<rdar://problem/15725243>
Comment 3 Alexey Proskuryakov 2013-12-25 15:51:37 PST
Marked the test as flaky in <http://trac.webkit.org/r161073>.
Comment 4 Alexey Proskuryakov 2014-02-06 14:59:03 PST
Antti, did you have a chance to look into this?
Comment 5 Diego Pino 2021-08-17 01:00:24 PDT
*** Bug 132182 has been marked as a duplicate of this bug. ***
Comment 6 Diego Pino 2021-08-17 01:07:03 PDT
Created attachment 435670 [details]
Patch
Comment 7 Alexey Proskuryakov 2021-09-01 09:17:28 PDT
Do the new results match other browsers? Not sure how to tell if the test is failing or passing now.
Comment 8 Diego Pino 2021-11-16 23:39:55 PST
Created attachment 444485 [details]
Patch
Comment 9 Diego Pino 2021-11-16 23:42:00 PST
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.
Comment 10 Alexey Proskuryakov 2022-01-10 08:53:46 PST
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.
Comment 11 Diego Pino 2022-01-10 18:20:15 PST
Created attachment 448823 [details]
Patch
Comment 12 Diego Pino 2022-01-10 18:27:23 PST
(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.
Comment 13 EWS 2022-01-11 05:23:21 PST
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].
Comment 14 Dawn Morningstar 2022-04-20 19:14:52 PDT
*** Bug 239254 has been marked as a duplicate of this bug. ***
Comment 15 Dawn Morningstar 2022-04-20 19:16:29 PDT
(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.
Comment 16 Alexey Proskuryakov 2022-04-23 19:32:47 PDT
That appears to be a new different issue, so it should be tracked in a new bug.
Comment 17 Alexey Proskuryakov 2022-04-24 12:52:51 PDT
Please disregard my comment, I didn't analyze the history enough to tell if it's a different issue, or same one continued.
Comment 18 Diego Pino 2022-08-15 23:06:26 PDT
Pull request: https://github.com/webkit/webkit/pull/3346
Comment 19 Diego Pino 2022-08-15 23:14:24 PDT
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.
Comment 20 EWS 2022-10-17 05:08:17 PDT
Committed 255618@main (eae54080e781): <https://commits.webkit.org/255618@main>

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