Bug 92490 – [Qt]REGRESSION(r123786): It made 3 fast/animation tests fail

• Component: Tools / Tests
• Status: RESOLVED FIXED

[János Badics]

fast/animation/request-animation-frame-cancel2.html :

--- /ramdisk/qt-linux-64-release/build/layout-test-results/fast/animation/request-animation-frame-cancel2-expected.txt 
+++ /ramdisk/qt-linux-64-release/build/layout-test-results/fast/animation/request-animation-frame-cancel2-actual.txt 
@@ -3,7 +3,7 @@
 On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
 
 
-PASS callbackFired is false
+FAIL callbackFired should be false. Was true.
 PASS cancelFired is true
 PASS successfullyParsed is true


fast/animation/request-animation-frame-detach-element.html :

--- /ramdisk/qt-linux-64-release/build/layout-test-results/fast/animation/request-animation-frame-detach-element-expected.txt 
+++ /ramdisk/qt-linux-64-release/build/layout-test-results/fast/animation/request-animation-frame-detach-element-actual.txt 
@@ -1,1 +1,3 @@
+CONSOLE MESSAGE: line 12: TypeError: 'undefined' is not an object (evaluating 'window.frames[1].webkitRequestAnimationFrame')
+FAIL: Timed out waiting for notifyDone to be called
 Test passes is there is no crash.



fast/animation/request-animation-frame-during-modal.html :

--- /ramdisk/qt-linux-64-release/build/layout-test-results/fast/animation/request-animation-frame-during-modal-expected.txt 
+++ /ramdisk/qt-linux-64-release/build/layout-test-results/fast/animation/request-animation-frame-during-modal-actual.txt 
@@ -4,9 +4,9 @@
 
 
 Setting callback
+Callback fired
 Showing modal dialog
 Returned from modal dialog
-Callback fired
 PASS successfullyParsed is true
 
 TEST COMPLETE

[János Badics]

I skipped these tests until the proper fix in
http://trac.webkit.org/changeset/123868

[Balazs Ankes]

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

[Zeno Albisser]

Created attachment 155167 [details]
patch for review.

[Noam Rosenthal]

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 }.

[Jocelyn Turcotte]

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.

[Zeno Albisser]

Created attachment 155255 [details]
patch for review.

[Noam Rosenthal]

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?

[Zeno Albisser]

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.

[Noam Rosenthal]

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.

[Noam Rosenthal]

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.

[Zeno Albisser]

Created attachment 155284 [details]
patch

[Zeno Albisser]

Created attachment 155289 [details]
patch

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Zeno Albisser]

Created attachment 155478 [details]
patch for testing with cr-linux.

[WebKit Review Bot]

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.

[Csaba Osztrogonác]

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

[Csaba Osztrogonác]

*** Bug 92759 has been marked as a duplicate of this bug. ***

[Csaba Osztrogonác]

Comment on attachment 155478 [details]
patch for testing with cr-linux.

Please remove the tests from the skipped list before landing.

[Csaba Osztrogonác]

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

[Zeno Albisser]

Created attachment 155806 [details]
patch for landing. - Unskip affected tests as well.

[Noam Rosenthal]

(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...

[Zeno Albisser]

(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.

[Zeno Albisser]

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>

[Zeno Albisser]

All reviewed patches have been landed.  Closing bug.