Bug 103468 – [EFL][WK2] Regression(135935) layout tests snapshots are flaky

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Yael]

After http://trac.webkit.org/changeset/135935, some test results need to be updated.

[Jussi Kukkonen (jku)]

This is not just about updating results. The tests seem to all be flaky now. Original bug is bug 102833

[Alexandru Chiculita]

This might be related to https://bugs.webkit.org/show_bug.cgi?id=103609.

[Jussi Kukkonen (jku)]

(In reply to comment #1)
> This is not just about updating results. The tests seem to all be flaky now. Original bug is bug 102833

I'll just update the current status here:

We have more than 500 tests that are flaky -- I'm guessing all ref tests are, some are just more flaky than others. The build seems to affect this as well as media tests are flaky on Release but not on Debug.

I've seen a couple of different failures: one had correct looking rendering but it seemed as if it had scrolled 100px to the left or up. Other times (quite often) the rendering was empty.

One example of the empty rendering is fast/forms/input-first-letter-edit.html: I can fairly reliably get that to fail, but only with a slightly modified test runner script.

There was one bug found already (bug 103641) where a window resize broke all the tests in the same worker. The remaining tests do not fail with a similar pattern though: I does not seem like any single test makes other tests fail.

I've looked at the patch and the snapshotting code, and I just cannot understand what goes wrong.

[Chris Dumez]

I may have a fix for this one but I will land a small snapshot code refactoring patch first.

[Chris Dumez]

Never mind, my fix did not help after all. I'll keep investigating.

[Chris Dumez]

Created attachment 180744 [details]
Patch

Yoohoo :) Merry Christmas everyone!

It took a while but I have finally found the cause of the flakiness in our ref tests:
- We process repaint requests in the event loop in EwkViewImpl. Therefore, the view may have pending repaint requests when we are taking the snapshot for ref / pixel tests.
My patch makes sure pending repaint requests are processed because taking the snapshot. I can no longer reproduce the flakiness after this.

Note that 2 of the tests previously marked as flaky are now consistently failing due to the text being blurry when scaled. I filed Bug 105766 for that. I remember we had this bug in the past but I thought this was fixed already. Someone may want to investigate.

[Yael]

(In reply to comment #6)
> Created an attachment (id=180744) [details]
> Patch
> 
> Yoohoo :) Merry Christmas everyone!
> 
> It took a while but I have finally found the cause of the flakiness in our ref tests:
> - We process repaint requests in the event loop in EwkViewImpl. Therefore, the view may have pending repaint requests when we are taking the snapshot for ref / pixel tests.
> My patch makes sure pending repaint requests are processed because taking the snapshot. I can no longer reproduce the flakiness after this.
> 
> Note that 2 of the tests previously marked as flaky are now consistently failing due to the text being blurry when scaled. I filed Bug 105766 for that. I remember we had this bug in the past but I thought this was fixed already. Someone may want to investigate.
This is great finding :)
We force repaint after the test is done and before taking the snapshot, and we have a callback after the forced repaint. Only after the callback, we take the snapshot. So what is causing the additional paint events?

[Chris Dumez]

(In reply to comment #7)
> (In reply to comment #6)
> > Created an attachment (id=180744) [details] [details]
> > Patch
> > 
> > Yoohoo :) Merry Christmas everyone!
> > 
> > It took a while but I have finally found the cause of the flakiness in our ref tests:
> > - We process repaint requests in the event loop in EwkViewImpl. Therefore, the view may have pending repaint requests when we are taking the snapshot for ref / pixel tests.
> > My patch makes sure pending repaint requests are processed because taking the snapshot. I can no longer reproduce the flakiness after this.
> > 
> > Note that 2 of the tests previously marked as flaky are now consistently failing due to the text being blurry when scaled. I filed Bug 105766 for that. I remember we had this bug in the past but I thought this was fixed already. Someone may want to investigate.
> This is great finding :)
> We force repaint after the test is done and before taking the snapshot, and we have a callback after the forced repaint. Only after the callback, we take the snapshot. So what is causing the additional paint events?

Actually, I believe those pending painting events are from your forceRepaint. From what I could see, the forceRepaint callback gets called when all the layer changes are flushed on the WebProcess side but you are taking the snapshot on UIProcess side.

[Chris Dumez]

Comment on attachment 180744 [details]
Patch

I had a discussion with Slava (ostap) on IRC. There is a risk that the repaint timer is always active in case of animations. I don't think this is an issue for WKTR but it may be one if we decide to expose this method as public API. I think we better stop all animations before taking the snapshot and resume them afterwards.

[Chris Dumez]

Created attachment 180778 [details]
Patch

Suspend animations before taking the snapshot and resume them afterward to address the issue raised by ostap (Thanks!).

[WebKit Review Bot]

Comment on attachment 180778 [details]
Patch

Clearing flags on attachment: 180778

Committed r138504: <http://trac.webkit.org/changeset/138504>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Balazs Kelemen]

Do you think this is needed for Qt as well? I'm not sure I understand why suspend/resume animations is necessary and how it is related to your display timer. The snapshot should be taken when the forced repaint has flushed in the web process. Could you clue me up a bit?