| Summary: | [Qt]REGRESSION(r123786): It made 3 fast/animation tests fail | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | János Badics <jbadics> | ||||||||||||||||
| Component: | Tools / Tests | Assignee: | Zeno Albisser <zeno> | ||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||
| Severity: | Normal | CC: | bank, dglazkov, jbadics, jturcotte, kadam, noam, ossy, szledan, webkit.review.bot, zeno | ||||||||||||||||
| Priority: | P2 | ||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||
| Bug Depends on: | |||||||||||||||||||
| Bug Blocks: | 79666 | ||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
|
Description
János Badics
2012-07-27 04:50:58 PDT
I skipped these tests until the proper fix in http://trac.webkit.org/changeset/123868 There are some fails on Qt Wk2 bot also. fast/animation/request-animation-frame-iframe.html: --- /ramdisk/qt-linux-32-release-webkit2/build/layout-test-results/fast/animation/request-animation-frame-iframe-expected.txt +++ /ramdisk/qt-linux-32-release-webkit2/build/layout-test-results/fast/animation/request-animation-frame-iframe-actual.txt @@ -3,7 +3,7 @@ On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE". -PASS callbackInvoked is true +FAIL callbackInvoked should be true. Was false. PASS successfullyParsed is true TEST COMPLETE fast/animation/request-animation-frame-timestamps-advance.html: --- /ramdisk/qt-linux-32-release-webkit2/build/layout-test-results/fast/animation/request-animation-frame-timestamps-advance-expected.txt +++ /ramdisk/qt-linux-32-release-webkit2/build/layout-test-results/fast/animation/request-animation-frame-timestamps-advance-actual.txt @@ -1,12 +1,2 @@ -Tests the timestamps provided to requestAnimationFrame callbacks advance +FAIL: Timed out waiting for notifyDone to be called -On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE". - - -PASS firstTimestamp is defined. -PASS secondTimestamp is defined. -PASS secondTimestamp > firstTimestamp is true -PASS successfullyParsed is true - -TEST COMPLETE - fast/animation/request-animation-frame-timestamps.html: --- /ramdisk/qt-linux-32-release-webkit2/build/layout-test-results/fast/animation/request-animation-frame-timestamps-expected.txt +++ /ramdisk/qt-linux-32-release-webkit2/build/layout-test-results/fast/animation/request-animation-frame-timestamps-actual.txt @@ -3,10 +3,7 @@ On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE". -PASS firstTimestamp is defined. -PASS secondTimestamp is defined. -PASS firstTimestamp is secondTimestamp -PASS firstTimestamp is defined. +FAIL firstTimestamp should be defined. Was undefined PASS successfullyParsed is true TEST COMPLETE fast/animation/request-animation-frame-within-callback.html: --- /ramdisk/qt-linux-32-release-webkit2/build/layout-test-results/fast/animation/request-animation-frame-within-callback-expected.txt +++ /ramdisk/qt-linux-32-release-webkit2/build/layout-test-results/fast/animation/request-animation-frame-within-callback-actual.txt @@ -3,7 +3,6 @@ On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE". -PASS sameFrame is false PASS successfullyParsed is true TEST COMPLETE fast/animation/request-animation-frame.html: --- /ramdisk/qt-linux-32-release-webkit2/build/layout-test-results/fast/animation/request-animation-frame-expected.txt +++ /ramdisk/qt-linux-32-release-webkit2/build/layout-test-results/fast/animation/request-animation-frame-actual.txt @@ -3,7 +3,7 @@ On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE". -PASS callbackInvoked is true +FAIL callbackInvoked should be true. Was false. PASS successfullyParsed is true TEST COMPLETE Created attachment 155167 [details]
patch for review.
Comment on attachment 155167 [details] patch for review. View in context: https://bugs.webkit.org/attachment.cgi?id=155167&action=review > Source/WebKit2/ChangeLog:8 > + Make sure that an actual repaint happens, when flushPendingLayerChanges > + is exeuted from within forceRepaint. In this case we do not want to wait > + for the UIProcess. This looks like a good fix, but is it related to the animation bug? > Source/WebKit2/WebProcess/WebPage/LayerTreeCoordinator/LayerTreeCoordinator.h:107 > + bool flushPendingLayerChanges(bool forceRepaint = false); Boolean trap. Please use an enum { ForceRepaint, DontForceRepaint }. Comment on attachment 155167 [details] patch for review. View in context: https://bugs.webkit.org/attachment.cgi?id=155167&action=review > Source/WebKit2/WebProcess/WebPage/LayerTreeCoordinator/LayerTreeCoordinator.cpp:254 > - m_waitingForUIProcess = true; > + m_waitingForUIProcess = !forceRepaint; Humm I know I hinted you this change, but I was wrong. I thought that the problem was that there was a m_waitingForUIProcess check when calling forceRepaint and it's not the case. The problem seems to be that since forceRepaint calls flushPendingLayerChanges, it doesn't end up calling serviceScriptedAnimations. So I think a more deterministic fix would be to move serviceScriptedAnimations deeper inside flushPendingLayerChange instead of performScheduledLayerFlush to make sure that it gets called when forceRepaint is called as well. Created attachment 155255 [details]
patch for review.
Comment on attachment 155255 [details] patch for review. View in context: https://bugs.webkit.org/attachment.cgi?id=155255&action=review > Source/WebKit2/WebProcess/WebPage/LayerTreeCoordinator/LayerTreeCoordinator.cpp:181 > + m_webPage->layoutIfNeeded(); I thought flushPendingLayerChanges already calls layoutIfNeeded. isn't this superfluous? Comment on attachment 155255 [details] patch for review. View in context: https://bugs.webkit.org/attachment.cgi?id=155255&action=review >> Source/WebKit2/WebProcess/WebPage/LayerTreeCoordinator/LayerTreeCoordinator.cpp:181 >> + m_webPage->layoutIfNeeded(); > > I thought flushPendingLayerChanges already calls layoutIfNeeded. isn't this superfluous? No, flushPendingLayerChanges does not. performScheduledLayerFlush does. But this one returns early due to m_waitingForUIProcess in case of running layout tests. Comment on attachment 155255 [details] patch for review. View in context: https://bugs.webkit.org/attachment.cgi?id=155255&action=review > Source/WebKit2/WebProcess/WebPage/LayerTreeCoordinator/LayerTreeCoordinator.cpp:180 > + m_webPage->corePage()->mainFrame()->view()->serviceScriptedAnimations(convertSecondsToDOMTimeStamp(currentTime())); I prefer it if this moved to a different function, e.g. LayerTreeCoordinator::serviceScriptedAnimations() >>> Source/WebKit2/WebProcess/WebPage/LayerTreeCoordinator/LayerTreeCoordinator.cpp:181 >>> + m_webPage->layoutIfNeeded(); >> >> I thought flushPendingLayerChanges already calls layoutIfNeeded. isn't this superfluous? > > No, flushPendingLayerChanges does not. performScheduledLayerFlush does. But this one returns early due to m_waitingForUIProcess in case of running layout tests. OK, then shouldn't we move layoutIfNeeded into flushPendingLayerChanges? I don't see how this behavior should be specific to rAF, it might break other tests as well. Comment on attachment 155255 [details] patch for review. View in context: https://bugs.webkit.org/attachment.cgi?id=155255&action=review >>>> Source/WebKit2/WebProcess/WebPage/LayerTreeCoordinator/LayerTreeCoordinator.cpp:181 >>>> + m_webPage->layoutIfNeeded(); >>> >>> I thought flushPendingLayerChanges already calls layoutIfNeeded. isn't this superfluous? >> >> No, flushPendingLayerChanges does not. performScheduledLayerFlush does. But this one returns early due to m_waitingForUIProcess in case of running layout tests. > > OK, then shouldn't we move layoutIfNeeded into flushPendingLayerChanges? > I don't see how this behavior should be specific to rAF, it might break other tests as well. Or, even better, have something like a syncDisplayState() function that calls serviceAnimations, layout and flushLayerChanges, and call that function both from forceRepaint and from performFlush. Created attachment 155284 [details]
patch
Created attachment 155289 [details]
patch
Comment on attachment 155289 [details] patch Attachment 155289 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13381836 New failing tests: platform/chromium/compositing/accelerated-drawing/svg-filters.html platform/chromium/compositing/accelerated-drawing/alpha.html Created attachment 155310 [details]
Archive of layout-test-results from gce-cr-linux-06
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-06 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Created attachment 155478 [details]
patch for testing with cr-linux.
Comment on attachment 155289 [details] patch Cleared Noam Rosenthal's review+ from obsolete attachment 155289 [details] so that this bug does not appear in http://webkit.org/pending-commit. And it made one more test fail in debug mode: expected.txt +++ /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/layout-test-results/fast/animation/request-animation-frame-timestamps-actual.txt @@ -5,7 +5,7 @@ PASS firstTimestamp is defined. PASS secondTimestamp is defined. -PASS firstTimestamp is secondTimestamp +FAIL firstTimestamp should be 1343744605297. Was 1343744605207. PASS firstTimestamp is defined. PASS successfullyParsed is true *** Bug 92759 has been marked as a duplicate of this bug. *** Comment on attachment 155478 [details]
patch for testing with cr-linux.
Please remove the tests from the skipped list before landing.
And one more test on WK2: --- /home/webkitbuildbot/slaves/release64bitWebKit2_EC2/buildslave/qt-linux-64-release-webkit2/build/layout-test-results/fast/dom/Window/post-message-crash-expected.txt +++ /home/webkitbuildbot/slaves/release64bitWebKit2_EC2/buildslave/qt-linux-64-release-webkit2/build/layout-test-results/fast/dom/Window/post-message-crash-actual.txt @@ -1,3 +1,3 @@ This ensures that postMessage called in a callback does not crash. -PASS +FAIL Created attachment 155806 [details]
patch for landing. - Unskip affected tests as well.
(In reply to comment #21) > Created an attachment (id=155806) [details] > patch for landing. - Unskip affected tests as well. If this is for landing - why the r? I thought we reviewed this patch already... (In reply to comment #22) > If this is for landing - why the r? I thought we reviewed this patch already... I had to modify it slightly. The cr-linux bot was not happy with the reviewed one. Also i unskipped the tests. Comment on attachment 155806 [details] patch for landing. - Unskip affected tests as well. Clearing flags on attachment: 155806 Committed r124336: <http://trac.webkit.org/changeset/124336> All reviewed patches have been landed. Closing bug. |