REGRESSION: (r283871) [ macOS wk2 Release ] 2 css/cssom-view/scroll-behavior-main-frame tests are failing

• Component: CSS
• Status: RESOLVED FIXED

[Eric Hutchison]

imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-main-frame-window.html
imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-main-frame-root.html

are constantly failing on Catalina, BigSur wk2 Release since r283871 (https://trac.webkit.org/changeset/283871/webkit).

History: https://results.webkit.org/?platform=ios&platform=mac&suite=layout-tests&suite=layout-tests&test=imported%2Fw3c%2Fweb-platform-tests%2Fcss%2Fcssom-view%2Fscroll-behavior-main-frame-root.html&test=imported%2Fw3c%2Fweb-platform-tests%2Fcss%2Fcssom-view%2Fscroll-behavior-main-frame-window.html

Build: https://build.webkit.org/#/builders/70/builds/5814

Results: https://build.webkit.org/results/Apple-BigSur-Release-WK2-Tests/r283982%20(5814)/results.html

Diff: 

--- /Volumes/Data/worker/bigsur-release-tests-wk2/build/layout-test-results/imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-main-frame-root-expected.txt
+++ /Volumes/Data/worker/bigsur-release-tests-wk2/build/layout-test-results/imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-main-frame-root-actual.txt
@@ -36,6 +36,6 @@
 PASS Set scrollLeft to frame with smooth scroll-behavior
 PASS Set scrollTop to frame with auto scroll-behavior
 PASS Set scrollTop to frame with smooth scroll-behavior
-PASS Aborting an ongoing smooth scrolling on the main frame with another smooth scrolling
-PASS Aborting an ongoing smooth scrolling on the main frame with an instant scrolling
+FAIL Aborting an ongoing smooth scrolling on the main frame with another smooth scrolling assert_equals: Final value of scrollLeft expected 1215 but got 2430
+FAIL Aborting an ongoing smooth scrolling on the main frame with an instant scrolling assert_equals: Final value of scrollLeft expected 0 but got 2430
 
--- /Volumes/Data/worker/bigsur-release-tests-wk2/build/layout-test-results/imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-main-frame-window-expected.txt
+++ /Volumes/Data/worker/bigsur-release-tests-wk2/build/layout-test-results/imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-main-frame-window-actual.txt
@@ -25,5 +25,5 @@
 PASS Main frame with smooth scroll-behavior ; scrollBy() with instant behavior
 PASS Main frame with smooth scroll-behavior ; scrollBy() with smooth behavior
 PASS Aborting an ongoing smooth scrolling on the main frame with another smooth scrolling
-PASS Aborting an ongoing smooth scrolling on the main frame with an instant scrolling
+FAIL Aborting an ongoing smooth scrolling on the main frame with an instant scrolling assert_equals: Final value of scrollLeft expected 0 but got 2430
 
The failure is flaky, but frequent on scroll-behavior-main-frame-window.html and constant on scroll-behavior-main-frame-root.html. 


Reproduced test failure locally using run-webkit-tests --iterations 1000 --exit-after-n-failures 1 -f --force --clobber-old-results --release imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-main-frame-window.html and run-webkit-tests --iterations 1000 --exit-after-n-failures 1 -f --force --clobber-old-results --release imported/w3c/web-platform-tests/css/cssom-view/scroll-behavior-main-frame-root.html

[Radar WebKit Bug Importer]

<rdar://problem/84158655>

[Eric Hutchison]

Updated test expectations at https://trac.webkit.org/changeset/284000/webkit

[Nikos Mouchtaris]

Created attachment 443854 [details]
Patch

[Nikos Mouchtaris]

Created attachment 443883 [details]
Patch

[Nikos Mouchtaris]

Created attachment 443886 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 443886 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=443886&action=review

I think the real bug might be that ThreadedScrollingTree::scrollingTreeNodeRequestsScroll() should use m_nodesWithPendingScrollAnimations.set() instead of m_nodesWithPendingScrollAnimations.add() (see HashMap.h)

> Source/WebCore/ChangeLog:9
> +        Temporary fix for issue where second scroll doesn't cancel a current
> +        scroll animation.

Is it temporary?

[Simon Fraser (smfr)]

Comment on attachment 443886 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=443886&action=review

> Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp:256
> +    scrollingTree().clearAnimationsForScrollingNodeID(scrollingNodeID());
>  
>      if (requestedScrollData.requestType == ScrollRequestType::CancelAnimatedScroll)
>          return;

But maybe we do need to clear the pending animation if we hit the CancelAnimatedScroll clause.

[Nikos Mouchtaris]

Created attachment 443911 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 443911 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=443911&action=review

> Source/WebCore/ChangeLog:10
> +        request has come in (first animation still in m_nodesWithPendingScrollAnimations). For the

"come in" could be more specific. Maybe "committed to the scrolling tree" or "received by the scrolling thread", whichever it is.

> Source/WebCore/page/scrolling/ThreadedScrollingTree.h:67
> +    void removePendingScrollAnimationForNode(ScrollingNodeID nodeID) WTF_REQUIRES_LOCK(m_treeLock) override { m_nodesWithPendingScrollAnimations.remove(nodeID); }

Since this is virtual, I would move the implementation to the .cpp file.

I think it can be 'final' instead of 'override'.

[Nikos Mouchtaris]

Created attachment 443990 [details]
Patch

[EWS]

Committed r285669 (244156@main): <https://commits.webkit.org/244156@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 443990 [details].