Bug 231600 - REGRESSION: (r283871) [ macOS wk2 Release ] 2 css/cssom-view/scroll-behavior-main-frame tests are failing
Summary: REGRESSION: (r283871) [ macOS wk2 Release ] 2 css/cssom-view/scroll-behavior-...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nikos Mouchtaris
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-10-12 11:40 PDT by Eric Hutchison
Modified: 2021-11-11 14:41 PST (History)
20 users (show)

See Also:


Attachments
Patch (5.18 KB, patch)
2021-11-10 12:49 PST, Nikos Mouchtaris
no flags Details | Formatted Diff | Diff
Patch (45.45 KB, patch)
2021-11-10 17:35 PST, Nikos Mouchtaris
no flags Details | Formatted Diff | Diff
Patch (5.03 KB, patch)
2021-11-10 17:46 PST, Nikos Mouchtaris
no flags Details | Formatted Diff | Diff
Patch (6.35 KB, patch)
2021-11-10 22:44 PST, Nikos Mouchtaris
simon.fraser: review+
simon.fraser: commit-queue-
Details | Formatted Diff | Diff
Patch (6.94 KB, patch)
2021-11-11 12:49 PST, Nikos Mouchtaris
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Hutchison 2021-10-12 11:40:24 PDT
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
Comment 1 Radar WebKit Bug Importer 2021-10-12 11:41:40 PDT
<rdar://problem/84158655>
Comment 2 Eric Hutchison 2021-10-12 12:05:40 PDT
Updated test expectations at https://trac.webkit.org/changeset/284000/webkit
Comment 3 Nikos Mouchtaris 2021-11-10 12:49:46 PST
Created attachment 443854 [details]
Patch
Comment 4 Nikos Mouchtaris 2021-11-10 17:35:47 PST
Created attachment 443883 [details]
Patch
Comment 5 Nikos Mouchtaris 2021-11-10 17:46:37 PST
Created attachment 443886 [details]
Patch
Comment 6 Simon Fraser (smfr) 2021-11-10 20:23:12 PST
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?
Comment 7 Simon Fraser (smfr) 2021-11-10 20:24:00 PST
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.
Comment 8 Nikos Mouchtaris 2021-11-10 22:44:04 PST
Created attachment 443911 [details]
Patch
Comment 9 Simon Fraser (smfr) 2021-11-11 11:58:05 PST
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'.
Comment 10 Nikos Mouchtaris 2021-11-11 12:49:00 PST
Created attachment 443990 [details]
Patch
Comment 11 EWS 2021-11-11 14:41:47 PST
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].