Summary: | [EFL][WK2] Regression(135935) layout tests snapshots are flaky | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yael <yael> | ||||||
Component: | WebKit EFL | Assignee: | Chris Dumez <cdumez> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | achicu, cdumez, gyuyoung.kim, jussi.kukkonen, kbalazs, kenneth, laszlo.gombos, lucas.de.marchi, rakuco, tonikitoo, webkit.review.bot, yael | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Bug Depends on: | 105687 | ||||||||
Bug Blocks: | |||||||||
Attachments: |
|
Description
Yael
2012-11-27 16:46:59 PST
This is not just about updating results. The tests seem to all be flaky now. Original bug is bug 102833 This might be related to https://bugs.webkit.org/show_bug.cgi?id=103609. (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. I may have a fix for this one but I will land a small snapshot code refactoring patch first. Never mind, my fix did not help after all. I'll keep investigating. 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. (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? (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 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.
Created attachment 180778 [details]
Patch
Suspend animations before taking the snapshot and resume them afterward to address the issue raised by ostap (Thanks!).
Comment on attachment 180778 [details] Patch Clearing flags on attachment: 180778 Committed r138504: <http://trac.webkit.org/changeset/138504> All reviewed patches have been landed. Closing bug. 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? |