Bug 206884

Summary: Asynchronous scrolling of overflow element can enter a recursive loop
Product: WebKit Reporter: cathiechen <cathiechen>
Component: ScrollingAssignee: cathiechen <cathiechen>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarcelo, commit-queue, esprehn+autocc, ews-watchlist, fred.wang, glenn, jamesr, kondapallykalyan, luiz, pdr, simon.fraser, tonikitoo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 204882    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

cathiechen
Reported 2020-01-28 09:05:53 PST
Implement RenderLayer::requestScrollPositionUpdate so that we can call requestScrollPositionUpdate more convinient. There's a recursive code path, if we implement it directly. AsyncScrollingCoordinator::requestScrollPositionUpdate AsyncScrollingCoordinator::updateScrollPositionAfterAsyncScroll ScrollableArea::scrollToOffsetWithoutAnimation ScrollAnimator::scrollToOffsetWithoutAnimation ScrollAnimator::notifyPositionChanged ScrollableArea::setScrollOffsetFromAnimation RenderLayer::requestScrollPositionUpdate AsyncScrollingCoordinator::requestScrollPositionUpdate In order to break the circle, I'll try to call notifyScrollPositionChanged in AsyncScrollingCoordinator::updateScrollPositionAfterAsyncScroll instead.
Attachments
Patch (10.62 KB, patch)
2020-01-28 09:10 PST, cathiechen 		no flags
DetailsFormatted DiffDiff
Patch (11.66 KB, patch)
2020-01-29 07:59 PST, cathiechen 		no flags
DetailsFormatted DiffDiff
Patch (7.01 KB, patch)
2020-01-31 05:17 PST, cathiechen 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
cathiechen
Comment 1 2020-01-28 09:10:08 PST
Created attachment 389014 [details] Patch
Frédéric Wang (:fredw)
Comment 2 2020-01-28 09:20:47 PST
Comment on attachment 389014 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=389014&action=review > Source/WebCore/ChangeLog:8 > + Test: fast/scrolling/ios/programmativ-scroll-element.html programmatic*
Simon Fraser (smfr)
Comment 3 2020-01-28 13:10:26 PST
The title of this bug doesn't parse.
cathiechen
Comment 4 2020-01-29 07:59:10 PST
Created attachment 389138 [details] Patch
cathiechen
Comment 5 2020-01-29 08:01:18 PST
(In reply to Simon Fraser (smfr) from comment #3) > The title of this bug doesn't parse. Done, sorry for the confusion.
Frédéric Wang (:fredw)
Comment 6 2020-01-29 08:28:41 PST
Comment on attachment 389138 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=389138&action=review > Source/WebCore/ChangeLog:9 > + while perform asynchronous programmatic scrolling for overflow elements. In order to break performing* > LayoutTests/fast/scrolling/ios/programmatic-scroll-element-crash.html:35 > + }, `Element async scroll by ${scrollFunction}() shouldn't crash`); What is the "crash" exactly? I'm not sure we have a notion of "crashtests" in WPT (yet). Is the goal of this test to only reproduce the issue rather than to do conformance testing? If so, maybe we should just have a traditional webkit test without assert/promise_test, with the minimal repro code and with the text "This test passes if it does not crash". > LayoutTests/fast/scrolling/ios/resources/scroll-behavior.js:1 > +function observeScrolling(elements, callback) { I guess it's ok, but only resetNode/scrollNode seem to be used? I wonder if we really want all of these or just inline the needed code in the test? Especially if you put only the minimal code in the html file...
cathiechen
Comment 7 2020-01-31 05:17:05 PST
Comment on attachment 389138 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=389138&action=review Hi Fred, Thanks for the review:) >> Source/WebCore/ChangeLog:9 >> + while perform asynchronous programmatic scrolling for overflow elements. In order to break > > performing* Done >> LayoutTests/fast/scrolling/ios/programmatic-scroll-element-crash.html:35 >> + }, `Element async scroll by ${scrollFunction}() shouldn't crash`); > > What is the "crash" exactly? > > I'm not sure we have a notion of "crashtests" in WPT (yet). Is the goal of this test to only reproduce the issue rather than to do conformance testing? If so, maybe we should just have a traditional webkit test without assert/promise_test, with the minimal repro code and with the text "This test passes if it does not crash". Sorry for the confusion. Any programmatic scroll of overflow element will do. This test was from scroll-behavior and reused. I'll write a new for this. >> LayoutTests/fast/scrolling/ios/resources/scroll-behavior.js:1 >> +function observeScrolling(elements, callback) { > > I guess it's ok, but only resetNode/scrollNode seem to be used? I wonder if we really want all of these or just inline the needed code in the test? Especially if you put only the minimal code in the html file... Got it. This file is also from scroll-behavior. We don't need it in the new test.
cathiechen
Comment 8 2020-01-31 05:17:38 PST
Created attachment 389342 [details] Patch
WebKit Commit Bot
Comment 9 2020-01-31 09:24:04 PST
The commit-queue encountered the following flaky tests while processing attachment 389342 [details]: imported/w3c/web-platform-tests/IndexedDB/interleaved-cursors-small.html bug 203433 The commit-queue is continuing to process your patch.
WebKit Commit Bot
Comment 10 2020-01-31 09:24:42 PST
Comment on attachment 389342 [details] Patch Clearing flags on attachment: 389342 Committed r255501: <https://trac.webkit.org/changeset/255501>
WebKit Commit Bot
Comment 11 2020-01-31 09:24:44 PST
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 12 2020-01-31 09:25:14 PST
<rdar://problem/59063923>
Note You need to log in before you can comment on or make changes to this bug.