Changeset 269312 in webkit


Ignore:
Timestamp:
Nov 3, 2020, 9:59:11 AM (4 years ago)
Author:
Simon Fraser
Message:

Scroll position can get reset after programmatic scroll
​https://bugs.webkit.org/show_bug.cgi?id=218477

Reviewed by Antti Koivisto.

Source/WebCore:

Part of rdar://problem/69599531: Scrolling on netflix.com sometimes jumps to the top.

Scrolling thread scroll notifications are handled on the main thread asynchronously, and
there are two sources of delay: first, the RunLoop::main().dispatch() in ThreadedScrollingTree::scrollingTreeNodeDidScroll(),
and second the zero-delay m_updateNodeScrollPositionTimer in AsyncScrollingCoordinator.

This is a problem when doing programmatic scrolls, since the main thread can
programmatically scroll, then the above asynchrony means that
AsyncScrollingCoordinator::updateScrollPositionAfterAsyncScroll() gets called for an earlier
user scroll (or, as is often the case on netflix.com, a scroll triggered by the
rubberbanding timer on the scrolling thread).

This patches addresses these out-of-order scrolls by storing scrolling thread scroll updates
in a threadsafe data structure on the scrolling tree, and always applying those on the main
thread before handling other scroll changes.

This patch removes the zero-delay timer, which did serve a purpose of coalescing scroll
notifications from the scrolling thread and UI process (for iOS). With the patch, we still
get coalescing per RunLoop::main().dispatch(). We lose coalescing on iOS, but those
notifications should be just once per frame. If we find that it was necessary to coalesce,
we can put a timer back.

Not testable because it's very timing dependent.

  • page/scrolling/AsyncScrollingCoordinator.cpp:

(WebCore::AsyncScrollingCoordinator::AsyncScrollingCoordinator):
(WebCore::AsyncScrollingCoordinator::requestScrollPositionUpdate):
(WebCore::AsyncScrollingCoordinator::synchronizeStateFromScrollingTree):
(WebCore::AsyncScrollingCoordinator::noteScrollingThreadSyncCompleteForNode):
(WebCore::AsyncScrollingCoordinator::applyPendingScrollUpdates):
(WebCore::AsyncScrollingCoordinator::applyScrollUpdate):
(WebCore::AsyncScrollingCoordinator::updateScrollPositionAfterAsyncScroll):
(WebCore::AsyncScrollingCoordinator::scheduleUpdateScrollPositionAfterAsyncScroll): Deleted.
(WebCore::AsyncScrollingCoordinator::updateScrollPositionAfterAsyncScrollTimerFired): Deleted.

  • page/scrolling/AsyncScrollingCoordinator.h:

(WebCore::AsyncScrollingCoordinator::ScheduledScrollUpdate::ScheduledScrollUpdate): Deleted.
(WebCore::AsyncScrollingCoordinator::ScheduledScrollUpdate::matchesUpdateType const): Deleted.

  • page/scrolling/ScrollingTree.cpp:

(WebCore::ScrollingTree::applyLayerPositionsInternal):
(WebCore::ScrollingTree::addPendingScrollUpdate):
(WebCore::ScrollingTree::takePendingScrollUpdates):

  • page/scrolling/ScrollingTree.h:

(WebCore::ScrollingTree::ScrollUpdate::ScrollUpdate):
(WebCore::ScrollingTree::ScrollUpdate::canMerge const):
(WebCore::ScrollingTree::ScrollUpdate::merge):

  • page/scrolling/ThreadedScrollingTree.cpp:

(WebCore::ThreadedScrollingTree::scrollingTreeNodeDidScroll):

Source/WebKit:

  • WebProcess/WebPage/RemoteLayerTree/RemoteScrollingCoordinator.mm:

(WebKit::RemoteScrollingCoordinator::scrollPositionChangedForNode):

Location:
trunk/Source
Files:
8 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebCore/ChangeLog

    r269308 r269312  
     12020-11-02  Simon Fraser  <simon.fraser@apple.com>
     2
     3        Scroll position can get reset after programmatic scroll
     4        https://bugs.webkit.org/show_bug.cgi?id=218477
     5
     6        Reviewed by Antti Koivisto.
     7
     8        Part of rdar://problem/69599531: Scrolling on netflix.com sometimes jumps to the top.
     9
     10        Scrolling thread scroll notifications are handled on the main thread asynchronously, and
     11        there are two sources of delay: first, the RunLoop::main().dispatch() in ThreadedScrollingTree::scrollingTreeNodeDidScroll(),
     12        and second the zero-delay m_updateNodeScrollPositionTimer in AsyncScrollingCoordinator.
     13       
     14        This is a problem when doing programmatic scrolls, since the main thread can
     15        programmatically scroll, then the above asynchrony means that
     16        AsyncScrollingCoordinator::updateScrollPositionAfterAsyncScroll() gets called for an earlier
     17        user scroll (or, as is often the case on netflix.com, a scroll triggered by the
     18        rubberbanding timer on the scrolling thread).
     19
     20        This patches addresses these out-of-order scrolls by storing scrolling thread scroll updates
     21        in a threadsafe data structure on the scrolling tree, and always applying those on the main
     22        thread before handling other scroll changes.
     23       
     24        This patch removes the zero-delay timer, which did serve a purpose of coalescing scroll
     25        notifications from the scrolling thread and UI process (for iOS). With the patch, we still
     26        get coalescing per RunLoop::main().dispatch(). We lose coalescing on iOS, but those
     27        notifications should be just once per frame. If we find that it was necessary to coalesce,
     28        we can put a timer back.
     29
     30        Not testable because it's very timing dependent.
     31
     32        * page/scrolling/AsyncScrollingCoordinator.cpp:
     33        (WebCore::AsyncScrollingCoordinator::AsyncScrollingCoordinator):
     34        (WebCore::AsyncScrollingCoordinator::requestScrollPositionUpdate):
     35        (WebCore::AsyncScrollingCoordinator::synchronizeStateFromScrollingTree):
     36        (WebCore::AsyncScrollingCoordinator::noteScrollingThreadSyncCompleteForNode):
     37        (WebCore::AsyncScrollingCoordinator::applyPendingScrollUpdates):
     38        (WebCore::AsyncScrollingCoordinator::applyScrollUpdate):
     39        (WebCore::AsyncScrollingCoordinator::updateScrollPositionAfterAsyncScroll):
     40        (WebCore::AsyncScrollingCoordinator::scheduleUpdateScrollPositionAfterAsyncScroll): Deleted.
     41        (WebCore::AsyncScrollingCoordinator::updateScrollPositionAfterAsyncScrollTimerFired): Deleted.
     42        * page/scrolling/AsyncScrollingCoordinator.h:
     43        (WebCore::AsyncScrollingCoordinator::ScheduledScrollUpdate::ScheduledScrollUpdate): Deleted.
     44        (WebCore::AsyncScrollingCoordinator::ScheduledScrollUpdate::matchesUpdateType const): Deleted.
     45        * page/scrolling/ScrollingTree.cpp:
     46        (WebCore::ScrollingTree::applyLayerPositionsInternal):
     47        (WebCore::ScrollingTree::addPendingScrollUpdate):
     48        (WebCore::ScrollingTree::takePendingScrollUpdates):
     49        * page/scrolling/ScrollingTree.h:
     50        (WebCore::ScrollingTree::ScrollUpdate::ScrollUpdate):
     51        (WebCore::ScrollingTree::ScrollUpdate::canMerge const):
     52        (WebCore::ScrollingTree::ScrollUpdate::merge):
     53        * page/scrolling/ThreadedScrollingTree.cpp:
     54        (WebCore::ThreadedScrollingTree::scrollingTreeNodeDidScroll):
     55
    1562020-11-03  Sam Weinig  <weinig@apple.com>
    257

  • trunk/Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp

    r268522 r269312  
    5959AsyncScrollingCoordinator::AsyncScrollingCoordinator(Page* page)
    6060    : ScrollingCoordinator(page)
    61     , m_updateNodeScrollPositionTimer(*this, &AsyncScrollingCoordinator::updateScrollPositionAfterAsyncScrollTimerFired)
    6261    , m_scrollingStateTree(makeUnique<ScrollingStateTree>(this))
    6362{
    … …  
    256255    ASSERT(isMainThread());
    257256    ASSERT(m_page);
    258 
    259257    auto scrollingNodeID = scrollableArea.scrollingNodeID();
    260258    if (!scrollingNodeID)
    … …  
    271269    bool inProgrammaticScroll = scrollableArea.currentScrollType() == ScrollType::Programmatic;
    272270    if (inProgrammaticScroll || inBackForwardCache)
    273         updateScrollPositionAfterAsyncScroll(scrollingNodeID, scrollPosition, { }, ScrollType::Programmatic, ScrollingLayerPositionAction::Set, InformWheelEventMonitor::No);
     271        applyScrollUpdate(scrollingNodeID, scrollPosition, { }, ScrollType::Programmatic, ScrollingLayerPositionAction::Set, InformWheelEventMonitor::No);
    274272
    275273    ASSERT(inProgrammaticScroll == (scrollType == ScrollType::Programmatic));
    … …  
    302300{
    303301    ASSERT(isMainThread());
     302    applyPendingScrollUpdates();
    304303
    305304    m_scrollingTree->traverseScrollingTree([&](ScrollingNodeID nodeID, ScrollingNodeType, Optional<FloatPoint> scrollPosition, Optional<FloatPoint> layoutViewportOrigin, bool scrolledSinceLastCommit) {
    … …  
    315314#if PLATFORM(MAC)
    316315    if (m_page && m_page->isMonitoringWheelEvents()) {
    317         LOG_WITH_STREAM(WheelEventTestMonitor, stream << "    (!) AsyncScrollingCoordinator::scheduleUpdateScrollPositionAfterAsyncScroll: Removing deferral on " << nodeID << " for reason " << WheelEventTestMonitor::ScrollingThreadSyncNeeded);
     316        LOG_WITH_STREAM(WheelEventTestMonitor, stream << "    (!) AsyncScrollingCoordinator::noteScrollingThreadSyncCompleteForNode: Removing deferral on " << nodeID << " for reason " << WheelEventTestMonitor::ScrollingThreadSyncNeeded);
    318317        m_page->wheelEventTestMonitor()->removeDeferralForReason(reinterpret_cast<WheelEventTestMonitor::ScrollableAreaIdentifier>(nodeID), WheelEventTestMonitor::ScrollingThreadSyncNeeded);
    319318    }
    … …  
    323322}
    324323
    325 void AsyncScrollingCoordinator::scheduleUpdateScrollPositionAfterAsyncScroll(ScrollingNodeID nodeID, const FloatPoint& scrollPosition, const Optional<FloatPoint>& layoutViewportOrigin, ScrollingLayerPositionAction scrollingLayerPositionAction)
    326 {
    327     ScheduledScrollUpdate scrollUpdate(nodeID, scrollPosition, layoutViewportOrigin, scrollingLayerPositionAction);
    328    
    329     if (m_updateNodeScrollPositionTimer.isActive()) {
    330         if (m_scheduledScrollUpdate.matchesUpdateType(scrollUpdate)) {
    331             m_scheduledScrollUpdate.scrollPosition = scrollPosition;
    332             m_scheduledScrollUpdate.layoutViewportOrigin = layoutViewportOrigin;
    333             return;
    334         }
    335    
    336         // If the parameters don't match what was previously scheduled, dispatch immediately.
    337         m_updateNodeScrollPositionTimer.stop();
    338         updateScrollPositionAfterAsyncScroll(m_scheduledScrollUpdate.nodeID, m_scheduledScrollUpdate.scrollPosition, m_scheduledScrollUpdate.layoutViewportOrigin, ScrollType::User, m_scheduledScrollUpdate.updateLayerPositionAction);
    339         updateScrollPositionAfterAsyncScroll(nodeID, scrollPosition, layoutViewportOrigin, ScrollType::User, scrollingLayerPositionAction);
    340         return;
    341     }
    342 
    343     m_scheduledScrollUpdate = scrollUpdate;
    344     m_updateNodeScrollPositionTimer.startOneShot(0_s);
    345 }
    346 
    347 void AsyncScrollingCoordinator::updateScrollPositionAfterAsyncScrollTimerFired()
    348 {
    349     updateScrollPositionAfterAsyncScroll(m_scheduledScrollUpdate.nodeID, m_scheduledScrollUpdate.scrollPosition, m_scheduledScrollUpdate.layoutViewportOrigin, ScrollType::User, m_scheduledScrollUpdate.updateLayerPositionAction);
     324void AsyncScrollingCoordinator::applyPendingScrollUpdates()
     325{
     326    if (!m_scrollingTree)
     327        return;
     328
     329    auto scrollUpdates = m_scrollingTree->takePendingScrollUpdates();
     330    for (auto& update : scrollUpdates) {
     331        LOG_WITH_STREAM(Scrolling, stream << "AsyncScrollingCoordinator::applyPendingScrollUpdates - node " << update.nodeID << " scroll position " << update.scrollPosition);
     332        updateScrollPositionAfterAsyncScroll(update.nodeID, update.scrollPosition, update.layoutViewportOrigin, ScrollType::User, update.updateLayerPositionAction, InformWheelEventMonitor::Yes);
     333    }
    350334}
    351335
    … …  
    382366}
    383367
     368void AsyncScrollingCoordinator::applyScrollUpdate(ScrollingNodeID scrollingNodeID, const FloatPoint& scrollPosition, Optional<FloatPoint> layoutViewportOrigin, ScrollType scrollType, ScrollingLayerPositionAction scrollingLayerPositionAction, InformWheelEventMonitor informWheelEventMonitor)
     369{
     370    applyPendingScrollUpdates();
     371    updateScrollPositionAfterAsyncScroll(scrollingNodeID, scrollPosition, layoutViewportOrigin, scrollType, scrollingLayerPositionAction, informWheelEventMonitor);
     372}
     373
    384374void AsyncScrollingCoordinator::updateScrollPositionAfterAsyncScroll(ScrollingNodeID scrollingNodeID, const FloatPoint& scrollPosition, Optional<FloatPoint> layoutViewportOrigin, ScrollType scrollType, ScrollingLayerPositionAction scrollingLayerPositionAction, InformWheelEventMonitor informWheelEventMonitor)
    385375{
    … …  
    396386        return;
    397387
    398     LOG_WITH_STREAM(Scrolling, stream << "AsyncScrollingCoordinator::updateScrollPositionAfterAsyncScroll node " << scrollingNodeID << " scrollPosition " << scrollPosition << " action " << scrollingLayerPositionAction);
     388    LOG_WITH_STREAM(Scrolling, stream << "AsyncScrollingCoordinator::updateScrollPositionAfterAsyncScroll node " << scrollingNodeID << " " << scrollType << " scrollPosition " << scrollPosition << " action " << scrollingLayerPositionAction);
    399389
    400390    auto& frameView = *frameViewPtr;

  • trunk/Source/WebCore/page/scrolling/AsyncScrollingCoordinator.h

    r268522 r269312  
    5252    void scrollingStateTreePropertiesChanged();
    5353
    54     WEBCORE_EXPORT void scheduleUpdateScrollPositionAfterAsyncScroll(ScrollingNodeID, const FloatPoint&, const Optional<FloatPoint>& layoutViewportOrigin, ScrollingLayerPositionAction);
     54    void applyPendingScrollUpdates();
    5555
    5656    enum class InformWheelEventMonitor { Yes, No };
    57     void updateScrollPositionAfterAsyncScroll(ScrollingNodeID, const FloatPoint&, Optional<FloatPoint> layoutViewportOrigin, ScrollType, ScrollingLayerPositionAction, InformWheelEventMonitor = InformWheelEventMonitor::Yes);
     57    WEBCORE_EXPORT void applyScrollUpdate(ScrollingNodeID, const FloatPoint&, Optional<FloatPoint> layoutViewportOrigin, ScrollType, ScrollingLayerPositionAction, InformWheelEventMonitor = InformWheelEventMonitor::Yes);
    5858
    5959#if PLATFORM(COCOA)
    … …  
    156156    void ensureRootStateNodeForFrameView(FrameView&);
    157157
    158     void updateScrollPositionAfterAsyncScrollTimerFired();
    159158    void setEventTrackingRegionsDirty();
    160159    void updateEventTrackingRegions();
    161160   
    162161    void noteScrollingThreadSyncCompleteForNode(ScrollingNodeID);
     162
     163    void updateScrollPositionAfterAsyncScroll(ScrollingNodeID, const FloatPoint&, Optional<FloatPoint> layoutViewportOrigin, ScrollType, ScrollingLayerPositionAction, InformWheelEventMonitor);
    163164   
    164165    FrameView* frameViewForScrollingNode(ScrollingNodeID) const;
    165 
    166     Timer m_updateNodeScrollPositionTimer;
    167 
    168     struct ScheduledScrollUpdate {
    169         ScheduledScrollUpdate() = default;
    170         ScheduledScrollUpdate(ScrollingNodeID scrollingNodeID, FloatPoint point, Optional<FloatPoint> viewportOrigin, ScrollingLayerPositionAction udpateAction)
    171             : nodeID(scrollingNodeID)
    172             , scrollPosition(point)
    173             , layoutViewportOrigin(viewportOrigin)
    174             , updateLayerPositionAction(udpateAction)
    175         { }
    176 
    177         ScrollingNodeID nodeID { 0 };
    178         FloatPoint scrollPosition;
    179         Optional<FloatPoint> layoutViewportOrigin;
    180         ScrollingLayerPositionAction updateLayerPositionAction { ScrollingLayerPositionAction::Sync };
    181        
    182         bool matchesUpdateType(const ScheduledScrollUpdate& other) const
    183         {
    184             return nodeID == other.nodeID && updateLayerPositionAction == other.updateLayerPositionAction;
    185         }
    186     };
    187 
    188     ScheduledScrollUpdate m_scheduledScrollUpdate;
    189166
    190167    std::unique_ptr<ScrollingStateTree> m_scrollingStateTree;

  • trunk/Source/WebCore/page/scrolling/ScrollingTree.cpp

    r269184 r269312  
    403403        return;
    404404
    405     LOG(Scrolling, "\nScrollingTree %p applyLayerPositions (main thread %d)", this, isMainThread());
     405//    LOG(Scrolling, "\nScrollingTree %p applyLayerPositions (main thread %d)", this, isMainThread());
    406406    applyLayerPositionsRecursive(*m_rootNode);
    407407}
    … …  
    576576
    577577    return false;
     578}
     579
     580void ScrollingTree::addPendingScrollUpdate(ScrollUpdate&& update)
     581{
     582    LockHolder locker(m_pendingScrollUpdatesLock);
     583    for (auto& existingUpdate : m_pendingScrollUpdates) {
     584        if (existingUpdate.canMerge(update)) {
     585            existingUpdate.merge(WTFMove(update));
     586            return;
     587        }
     588    }
     589
     590    m_pendingScrollUpdates.append(WTFMove(update));
     591}
     592
     593Vector<ScrollingTree::ScrollUpdate> ScrollingTree::takePendingScrollUpdates()
     594{
     595    LockHolder locker(m_pendingScrollUpdatesLock);
     596    return std::exchange(m_pendingScrollUpdates, { });
    578597}
    579598

  • trunk/Source/WebCore/page/scrolling/ScrollingTree.h

    r268544 r269312  
    212212    WEBCORE_EXPORT void willProcessWheelEvent();
    213213
     214    struct ScrollUpdate {
     215        ScrollingNodeID nodeID { 0 };
     216        FloatPoint scrollPosition;
     217        Optional<FloatPoint> layoutViewportOrigin;
     218        ScrollingLayerPositionAction updateLayerPositionAction { ScrollingLayerPositionAction::Sync };
     219       
     220        bool canMerge(const ScrollUpdate& other) const
     221        {
     222            return nodeID == other.nodeID && updateLayerPositionAction == other.updateLayerPositionAction;
     223        }
     224       
     225        void merge(ScrollUpdate&& other)
     226        {
     227            scrollPosition = other.scrollPosition;
     228            layoutViewportOrigin = other.layoutViewportOrigin;
     229        }
     230    };
     231
     232    void addPendingScrollUpdate(ScrollUpdate&&);
     233    Vector<ScrollUpdate> takePendingScrollUpdates();
     234
    214235protected:
    215236    WheelEventHandlingResult handleWheelEventWithNode(const PlatformWheelEvent&, OptionSet<WheelEventProcessingSteps>, ScrollingTreeNode*);
    … …  
    280301    SwipeState m_swipeState;
    281302
     303    Lock m_pendingScrollUpdatesLock;
     304    Vector<ScrollUpdate> m_pendingScrollUpdates;
     305
    282306    Lock m_lastWheelEventTimeMutex;
    283307    MonotonicTime m_lastWheelEventTime;

  • trunk/Source/WebCore/page/scrolling/ThreadedScrollingTree.cpp

    r268544 r269312  
    147147#endif
    148148
     149    if (RunLoop::isMain()) {
     150        m_scrollingCoordinator->applyScrollUpdate(node.scrollingNodeID(), scrollPosition, layoutViewportOrigin, ScrollType::User, scrollingLayerPositionAction);
     151        return;
     152    }
     153
    149154    LOG_WITH_STREAM(Scrolling, stream << "ThreadedScrollingTree::scrollingTreeNodeDidScroll " << node.scrollingNodeID() << " to " << scrollPosition << " bouncing to main thread");
    150155
    151     if (RunLoop::isMain()) {
    152         m_scrollingCoordinator->updateScrollPositionAfterAsyncScroll(node.scrollingNodeID(), scrollPosition, layoutViewportOrigin, ScrollType::User, scrollingLayerPositionAction);
    153         return;
    154     }
    155 
    156     RunLoop::main().dispatch([strongThis = makeRef(*this), nodeID = node.scrollingNodeID(), scrollPosition, layoutViewportOrigin, scrollingLayerPositionAction] {
     156    auto scrollUpdate = ScrollUpdate { node.scrollingNodeID(), scrollPosition, layoutViewportOrigin, scrollingLayerPositionAction };
     157    addPendingScrollUpdate(WTFMove(scrollUpdate));
     158
     159    RunLoop::main().dispatch([strongThis = makeRef(*this)] {
    157160        if (auto* scrollingCoordinator = strongThis->m_scrollingCoordinator.get())
    158             scrollingCoordinator->scheduleUpdateScrollPositionAfterAsyncScroll(nodeID, scrollPosition, layoutViewportOrigin, scrollingLayerPositionAction);
     161            scrollingCoordinator->applyPendingScrollUpdates();
    159162    });
    160163}

  • trunk/Source/WebKit/ChangeLog

    r269307 r269312  
     12020-11-02  Simon Fraser  <simon.fraser@apple.com>
     2
     3        Scroll position can get reset after programmatic scroll
     4        https://bugs.webkit.org/show_bug.cgi?id=218477
     5
     6        Reviewed by Antti Koivisto.
     7
     8        * WebProcess/WebPage/RemoteLayerTree/RemoteScrollingCoordinator.mm:
     9        (WebKit::RemoteScrollingCoordinator::scrollPositionChangedForNode):
     10
    1112020-11-03  Brent Fulgham  <bfulgham@apple.com>
    212

  • trunk/Source/WebKit/WebProcess/WebPage/RemoteLayerTree/RemoteScrollingCoordinator.mm

    r268014 r269312  
    108108void RemoteScrollingCoordinator::scrollPositionChangedForNode(ScrollingNodeID nodeID, const FloatPoint& scrollPosition, bool syncLayerPosition)
    109109{
    110     scheduleUpdateScrollPositionAfterAsyncScroll(nodeID, scrollPosition, WTF::nullopt, syncLayerPosition ? ScrollingLayerPositionAction::Sync : ScrollingLayerPositionAction::Set);
     110    applyScrollUpdate(nodeID, scrollPosition, WTF::nullopt, ScrollType::User, syncLayerPosition ? ScrollingLayerPositionAction::Sync : ScrollingLayerPositionAction::Set);
    111111}
    112112
Note: See TracChangeset for help on using the changeset viewer.