Bug 103468 - [EFL][WK2] Regression(135935) layout tests snapshots are flaky
Summary: [EFL][WK2] Regression(135935) layout tests snapshots are flaky
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords:
Depends on: 105687
Blocks:
  Show dependency treegraph
 
Reported: 2012-11-27 16:46 PST by Yael
Modified: 2013-01-10 05:41 PST (History)
12 users (show)

See Also:


Attachments
Patch (46.00 KB, patch)
2012-12-26 06:00 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (47.12 KB, patch)
2012-12-27 01:07 PST, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yael 2012-11-27 16:46:59 PST
After http://trac.webkit.org/changeset/135935, some test results need to be updated.
Comment 1 Jussi Kukkonen (jku) 2012-11-28 04:00:10 PST
This is not just about updating results. The tests seem to all be flaky now. Original bug is bug 102833
Comment 2 Alexandru Chiculita 2012-11-29 15:07:02 PST
This might be related to https://bugs.webkit.org/show_bug.cgi?id=103609.
Comment 3 Jussi Kukkonen (jku) 2012-12-14 07:26:52 PST
(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.
Comment 4 Chris Dumez 2012-12-23 06:29:04 PST
I may have a fix for this one but I will land a small snapshot code refactoring patch first.
Comment 5 Chris Dumez 2012-12-25 06:06:35 PST
Never mind, my fix did not help after all. I'll keep investigating.
Comment 6 Chris Dumez 2012-12-26 06:00:05 PST
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.
Comment 7 Yael 2012-12-26 06:29:46 PST
(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?
Comment 8 Chris Dumez 2012-12-26 07:08:57 PST
(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.
Comment 9 Chris Dumez 2012-12-27 00:23:18 PST
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.
Comment 10 Chris Dumez 2012-12-27 01:07:57 PST
Created attachment 180778 [details]
Patch

Suspend animations before taking the snapshot and resume them afterward to address the issue raised by ostap (Thanks!).
Comment 11 WebKit Review Bot 2012-12-27 08:17:37 PST
Comment on attachment 180778 [details]
Patch

Clearing flags on attachment: 180778

Committed r138504: <http://trac.webkit.org/changeset/138504>
Comment 12 WebKit Review Bot 2012-12-27 08:17:43 PST
All reviewed patches have been landed.  Closing bug.
Comment 13 Balazs Kelemen 2012-12-31 07:17:06 PST
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?