Asynchronous scrolling of overflow element can enter a recursive loop

• Component: Scrolling
• Status: RESOLVED FIXED

[cathiechen]

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.

[cathiechen]

Created attachment 389014 [details]
Patch

[Frédéric Wang (:fredw)]

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)]

The title of this bug doesn't parse.

[cathiechen]

Created attachment 389138 [details]
Patch

[cathiechen]

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

Created attachment 389342 [details]
Patch

[WebKit Commit Bot]

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 on attachment 389342 [details]
Patch

Clearing flags on attachment: 389342

Committed r255501: <https://trac.webkit.org/changeset/255501>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/59063923>