Changeset 243607 in webkit


Ignore:
Timestamp:
Mar 28, 2019, 9:44:27 AM (7 years ago)
Author:
Simon Fraser
Message:

[macOS WK2] Overlays on instagram.com are shifted if you click on a photo after scrolling
​https://bugs.webkit.org/show_bug.cgi?id=196330
rdar://problem/49100304

Source/WebCore:

Reviewed by Antti Koivisto.

When we call ScrollingTree::applyLayerPositions() on the main thread after a flush,
we need to ensure that the most recent version of the scrolling tree has been committed,
because it has to have state (like requested scroll position and layout viewport rect)
that match the layer flush.

To fix this we have to have the main thread wait for the commit to complete, so
ThreadedScrollingTree keeps track of a pending commit count, and uses a condition
variable to allow the main thread to safely wait for it to reach zero.

Tracing shows that this works as expected, and the main thread is never blocked for
more than a few tens of microseconds.

Also lock the tree mutex in ScrollingTree::handleWheelEvent(), since we enter the
scrolling tree here and we don't want that racing with applyLayerPositions() on the
main thread.

Test: scrollingcoordinator/mac/fixed-scrolled-body.html

  • page/scrolling/ScrollingTree.cpp:

(WebCore::ScrollingTree::handleWheelEvent):
(WebCore::ScrollingTree::applyLayerPositions):

  • page/scrolling/ScrollingTree.h:
  • page/scrolling/ThreadedScrollingTree.cpp:

(WebCore::ThreadedScrollingTree::commitTreeState):
(WebCore::ThreadedScrollingTree::incrementPendingCommitCount):
(WebCore::ThreadedScrollingTree::decrementPendingCommitCount):
(WebCore::ThreadedScrollingTree::waitForPendingCommits):
(WebCore::ThreadedScrollingTree::applyLayerPositions):

  • page/scrolling/ThreadedScrollingTree.h:
  • page/scrolling/mac/ScrollingCoordinatorMac.mm:

(WebCore::ScrollingCoordinatorMac::commitTreeState):

LayoutTests:

Reviewed by NOBODY (OOPS!).

  • scrollingcoordinator/mac/fixed-scrolled-body-expected.html: Added.
  • scrollingcoordinator/mac/fixed-scrolled-body.html: Added.
Location:
trunk
Files:
2 added
7 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r243605 r243607  
     12019-03-28  Simon Fraser  <simon.fraser@apple.com>
     2
     3        [macOS WK2] Overlays on instagram.com are shifted if you click on a photo after scrolling
     4        https://bugs.webkit.org/show_bug.cgi?id=196330
     5        rdar://problem/49100304
     6
     7        Reviewed by Antti Koivisto.
     8
     9        * scrollingcoordinator/mac/fixed-scrolled-body-expected.html: Added.
     10        * scrollingcoordinator/mac/fixed-scrolled-body.html: Added.
     11
    1122019-03-28  Zalan Bujtas  <zalan@apple.com>
    213

  • trunk/Source/WebCore/ChangeLog

    r243605 r243607  
     12019-03-28  Simon Fraser  <simon.fraser@apple.com>
     2
     3        [macOS WK2] Overlays on instagram.com are shifted if you click on a photo after scrolling
     4        https://bugs.webkit.org/show_bug.cgi?id=196330
     5        rdar://problem/49100304
     6
     7        Reviewed by Antti Koivisto.
     8
     9        When we call ScrollingTree::applyLayerPositions() on the main thread after a flush,
     10        we need to ensure that the most recent version of the scrolling tree has been committed,
     11        because it has to have state (like requested scroll position and layout viewport rect)
     12        that match the layer flush.
     13
     14        To fix this we have to have the main thread wait for the commit to complete, so
     15        ThreadedScrollingTree keeps track of a pending commit count, and uses a condition
     16        variable to allow the main thread to safely wait for it to reach zero.
     17
     18        Tracing shows that this works as expected, and the main thread is never blocked for
     19        more than a few tens of microseconds.
     20
     21        Also lock the tree mutex in ScrollingTree::handleWheelEvent(), since we enter the
     22        scrolling tree here and we don't want that racing with applyLayerPositions() on the
     23        main thread.
     24
     25        Test: scrollingcoordinator/mac/fixed-scrolled-body.html
     26
     27        * page/scrolling/ScrollingTree.cpp:
     28        (WebCore::ScrollingTree::handleWheelEvent):
     29        (WebCore::ScrollingTree::applyLayerPositions):
     30        * page/scrolling/ScrollingTree.h:
     31        * page/scrolling/ThreadedScrollingTree.cpp:
     32        (WebCore::ThreadedScrollingTree::commitTreeState):
     33        (WebCore::ThreadedScrollingTree::incrementPendingCommitCount):
     34        (WebCore::ThreadedScrollingTree::decrementPendingCommitCount):
     35        (WebCore::ThreadedScrollingTree::waitForPendingCommits):
     36        (WebCore::ThreadedScrollingTree::applyLayerPositions):
     37        * page/scrolling/ThreadedScrollingTree.h:
     38        * page/scrolling/mac/ScrollingCoordinatorMac.mm:
     39        (WebCore::ScrollingCoordinatorMac::commitTreeState):
     40
    1412019-03-28  Zalan Bujtas  <zalan@apple.com>
    242

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

    r243380 r243607  
    9494    LOG_WITH_STREAM(Scrolling, stream << "ScrollingTree " << this << " handleWheelEvent (async scrolling enabled: " << asyncFrameOrOverflowScrollingEnabled() << ")");
    9595
     96    LockHolder locker(m_treeMutex);
     97
    9698    if (!asyncFrameOrOverflowScrollingEnabled()) {
    9799        if (m_rootNode)
    … …  
    107109    }
    108110
    109     LockHolder locker(m_treeMutex);
    110111    if (m_rootNode) {
    111112        auto& frameScrollingNode = downcast<ScrollingTreeFrameScrollingNode>(*m_rootNode);
    … …  
    261262}
    262263
    263 // Called from the main thread.
    264264void ScrollingTree::applyLayerPositions()
    265265{
     266    ASSERT(isMainThread());
    266267    LockHolder locker(m_treeMutex);
    267268

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

    r243380 r243607  
    7070    WEBCORE_EXPORT virtual void commitTreeState(std::unique_ptr<ScrollingStateTree>);
    7171   
    72     WEBCORE_EXPORT void applyLayerPositions();
     72    WEBCORE_EXPORT virtual void applyLayerPositions();
    7373
    7474    virtual Ref<ScrollingTreeNode> createScrollingTreeNode(ScrollingNodeType, ScrollingNodeID) = 0;
    … …  
    154154
    155155    WEBCORE_EXPORT String scrollingTreeAsText(ScrollingStateTreeAsTextBehavior = ScrollingStateTreeAsTextBehaviorNormal);
    156    
     156
    157157protected:
    158158    void setMainFrameScrollPosition(FloatPoint);

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

    r242132 r243607  
    9090    ASSERT(ScrollingThread::isCurrentThread());
    9191    ScrollingTree::commitTreeState(WTFMove(scrollingStateTree));
     92   
     93    decrementPendingCommitCount();
    9294}
    9395
    … …  
    125127}
    126128
     129void ThreadedScrollingTree::incrementPendingCommitCount()
     130{
     131    LockHolder commitLocker(m_pendingCommitCountMutex);
     132    ++m_pendingCommitCount;
     133}
     134
     135void ThreadedScrollingTree::decrementPendingCommitCount()
     136{
     137    LockHolder commitLocker(m_pendingCommitCountMutex);
     138    ASSERT(m_pendingCommitCount > 0);
     139    if (!--m_pendingCommitCount)
     140        m_commitCondition.notifyOne();
     141}
     142
     143void ThreadedScrollingTree::waitForPendingCommits()
     144{
     145    ASSERT(isMainThread());
     146
     147    LockHolder commitLocker(m_pendingCommitCountMutex);
     148    while (m_pendingCommitCount)
     149        m_commitCondition.wait(m_pendingCommitCountMutex);
     150}
     151
     152void ThreadedScrollingTree::applyLayerPositions()
     153{
     154    waitForPendingCommits();
     155    ScrollingTree::applyLayerPositions();
     156}
     157
    127158#if PLATFORM(COCOA)
    128159void ThreadedScrollingTree::currentSnapPointIndicesDidChange(ScrollingNodeID nodeID, unsigned horizontal, unsigned vertical)

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

    r242132 r243607  
    3030#include "ScrollingStateTree.h"
    3131#include "ScrollingTree.h"
     32#include <wtf/Condition.h>
    3233#include <wtf/RefPtr.h>
    3334
    … …  
    5556    void invalidate() override;
    5657
     58    void incrementPendingCommitCount();
     59    void decrementPendingCommitCount();
     60
    5761protected:
    5862    explicit ThreadedScrollingTree(AsyncScrollingCoordinator&);
    … …  
    7579private:
    7680    bool isThreadedScrollingTree() const override { return true; }
     81    void applyLayerPositions() override;
    7782
    7883    RefPtr<AsyncScrollingCoordinator> m_scrollingCoordinator;
     84
     85    void waitForPendingCommits();
     86
     87    Lock m_pendingCommitCountMutex;
     88    unsigned m_pendingCommitCount { 0 };
     89    Condition m_commitCondition;
    7990};
    8091

  • trunk/Source/WebCore/page/scrolling/mac/ScrollingCoordinatorMac.mm

    r240787 r243607  
    123123    ScrollingStateTree* unprotectedTreeState = scrollingStateTree()->commit(LayerRepresentation::PlatformLayerRepresentation).release();
    124124
     125    threadedScrollingTree->incrementPendingCommitCount();
     126
    125127    ScrollingThread::dispatch([threadedScrollingTree, unprotectedTreeState] {
    126128        std::unique_ptr<ScrollingStateTree> treeState(unprotectedTreeState);
Note: See TracChangeset for help on using the changeset viewer.