[macOS WK2] Overlays on instagram.com are shifted if you click on a photo after scrolling
authorsimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 28 Mar 2019 16:44:27 +0000 (16:44 +0000)
committersimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 28 Mar 2019 16:44:27 +0000 (16:44 +0000)
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.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@243607 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/scrollingcoordinator/mac/fixed-scrolled-body-expected.html [new file with mode: 0644]
LayoutTests/scrollingcoordinator/mac/fixed-scrolled-body.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/page/scrolling/ScrollingTree.cpp
Source/WebCore/page/scrolling/ScrollingTree.h
Source/WebCore/page/scrolling/ThreadedScrollingTree.cpp
Source/WebCore/page/scrolling/ThreadedScrollingTree.h
Source/WebCore/page/scrolling/mac/ScrollingCoordinatorMac.mm

index b7e9d0e..dd996a5 100644 (file)
@@ -1,3 +1,14 @@
+2019-03-28  Simon Fraser  <simon.fraser@apple.com>
+
+        [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
+
+        Reviewed by Antti Koivisto.
+
+        * scrollingcoordinator/mac/fixed-scrolled-body-expected.html: Added.
+        * scrollingcoordinator/mac/fixed-scrolled-body.html: Added.
+
 2019-03-28  Zalan Bujtas  <zalan@apple.com>
 
         [SimpleLineLayout] Disable SLL when text-underline-position is not auto.
diff --git a/LayoutTests/scrollingcoordinator/mac/fixed-scrolled-body-expected.html b/LayoutTests/scrollingcoordinator/mac/fixed-scrolled-body-expected.html
new file mode 100644 (file)
index 0000000..273bde7
--- /dev/null
@@ -0,0 +1,29 @@
+<!DOCTYPE html>
+<html>
+<head>
+    <style>
+        body, html {
+            height: 100%;
+            margin: 0;
+            position: fixed;
+            top: -200px;
+        }
+        .section {
+            height: 2000px;
+        }
+        .overlay {
+            position: fixed;
+            left: 0;
+            top: 0;
+            width: 400px;
+            height: 100px;
+            background-color: blue;
+        }
+    </style>
+</head>
+<body>
+    <div class="section"></div>
+    <div class="overlay"></div>
+</body>
+</html>
+
diff --git a/LayoutTests/scrollingcoordinator/mac/fixed-scrolled-body.html b/LayoutTests/scrollingcoordinator/mac/fixed-scrolled-body.html
new file mode 100644 (file)
index 0000000..9f63169
--- /dev/null
@@ -0,0 +1,46 @@
+<!DOCTYPE html>
+<html>
+<head>
+    <style>
+        body, html {
+            height: 100%;
+            width: 100%;
+            margin: 0;
+        }
+        .section {
+            height: 2000px;
+        }
+        .overlay {
+            position: fixed;
+            left: 0;
+            top: 0;
+            width: 400px;
+            height: 100px;
+            background-color: blue;
+        }
+    </style>
+    <script>
+        if (window.testRunner)
+            testRunner.waitUntilDone();
+
+        function doTest()
+        {
+            window.scrollTo(0, 200);
+            requestAnimationFrame(() => {
+                document.body.style.top = -window.scrollY + 'px';
+                document.body.style.position = 'fixed';
+
+                if (window.testRunner)
+                    testRunner.notifyDone();
+            });
+        }
+        
+        window.addEventListener('load', doTest, false);
+    </script>
+</head>
+<body>
+    <div class="section"></div>
+    <div class="overlay"></div>
+</body>
+</html>
+
index 8790618..15136a8 100644 (file)
@@ -1,3 +1,43 @@
+2019-03-28  Simon Fraser  <simon.fraser@apple.com>
+
+        [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
+
+        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):
+
 2019-03-28  Zalan Bujtas  <zalan@apple.com>
 
         [SimpleLineLayout] Disable SLL when text-underline-position is not auto.
index 294c8f2..16d7335 100644 (file)
@@ -93,6 +93,8 @@ ScrollingEventResult ScrollingTree::handleWheelEvent(const PlatformWheelEvent& w
 {
     LOG_WITH_STREAM(Scrolling, stream << "ScrollingTree " << this << " handleWheelEvent (async scrolling enabled: " << asyncFrameOrOverflowScrollingEnabled() << ")");
 
+    LockHolder locker(m_treeMutex);
+
     if (!asyncFrameOrOverflowScrollingEnabled()) {
         if (m_rootNode)
             downcast<ScrollingTreeScrollingNode>(*m_rootNode).handleWheelEvent(wheelEvent);
@@ -106,7 +108,6 @@ ScrollingEventResult ScrollingTree::handleWheelEvent(const PlatformWheelEvent& w
             return downcast<ScrollingTreeScrollingNode>(*node).handleWheelEvent(wheelEvent);
     }
 
-    LockHolder locker(m_treeMutex);
     if (m_rootNode) {
         auto& frameScrollingNode = downcast<ScrollingTreeFrameScrollingNode>(*m_rootNode);
 
@@ -260,9 +261,9 @@ void ScrollingTree::updateTreeFromStateNode(const ScrollingStateNode* stateNode,
     node->commitStateAfterChildren(*stateNode);
 }
 
-// Called from the main thread.
 void ScrollingTree::applyLayerPositions()
 {
+    ASSERT(isMainThread());
     LockHolder locker(m_treeMutex);
 
     if (!m_rootNode)
index c4e2037..b4fbdaf 100644 (file)
@@ -69,7 +69,7 @@ public:
     virtual void invalidate() { }
     WEBCORE_EXPORT virtual void commitTreeState(std::unique_ptr<ScrollingStateTree>);
     
-    WEBCORE_EXPORT void applyLayerPositions();
+    WEBCORE_EXPORT virtual void applyLayerPositions();
 
     virtual Ref<ScrollingTreeNode> createScrollingTreeNode(ScrollingNodeType, ScrollingNodeID) = 0;
     
@@ -153,7 +153,7 @@ public:
     HashSet<ScrollingNodeID>& positionedNodesWithRelatedOverflow() { return m_positionedNodesWithRelatedOverflow; }
 
     WEBCORE_EXPORT String scrollingTreeAsText(ScrollingStateTreeAsTextBehavior = ScrollingStateTreeAsTextBehaviorNormal);
-    
+
 protected:
     void setMainFrameScrollPosition(FloatPoint);
 
index 1d9512e..0e455a1 100644 (file)
@@ -89,6 +89,8 @@ void ThreadedScrollingTree::commitTreeState(std::unique_ptr<ScrollingStateTree>
 {
     ASSERT(ScrollingThread::isCurrentThread());
     ScrollingTree::commitTreeState(WTFMove(scrollingStateTree));
+    
+    decrementPendingCommitCount();
 }
 
 void ThreadedScrollingTree::scrollingTreeNodeDidScroll(ScrollingTreeScrollingNode& node, ScrollingLayerPositionAction scrollingLayerPositionAction)
@@ -124,6 +126,35 @@ void ThreadedScrollingTree::reportExposedUnfilledArea(MonotonicTime timestamp, u
     });
 }
 
+void ThreadedScrollingTree::incrementPendingCommitCount()
+{
+    LockHolder commitLocker(m_pendingCommitCountMutex);
+    ++m_pendingCommitCount;
+}
+
+void ThreadedScrollingTree::decrementPendingCommitCount()
+{
+    LockHolder commitLocker(m_pendingCommitCountMutex);
+    ASSERT(m_pendingCommitCount > 0);
+    if (!--m_pendingCommitCount)
+        m_commitCondition.notifyOne();
+}
+
+void ThreadedScrollingTree::waitForPendingCommits()
+{
+    ASSERT(isMainThread());
+
+    LockHolder commitLocker(m_pendingCommitCountMutex);
+    while (m_pendingCommitCount)
+        m_commitCondition.wait(m_pendingCommitCountMutex);
+}
+
+void ThreadedScrollingTree::applyLayerPositions()
+{
+    waitForPendingCommits();
+    ScrollingTree::applyLayerPositions();
+}
+
 #if PLATFORM(COCOA)
 void ThreadedScrollingTree::currentSnapPointIndicesDidChange(ScrollingNodeID nodeID, unsigned horizontal, unsigned vertical)
 {
index dccafc4..e32ac5a 100644 (file)
@@ -29,6 +29,7 @@
 
 #include "ScrollingStateTree.h"
 #include "ScrollingTree.h"
+#include <wtf/Condition.h>
 #include <wtf/RefPtr.h>
 
 namespace WebCore {
@@ -54,6 +55,9 @@ public:
 
     void invalidate() override;
 
+    void incrementPendingCommitCount();
+    void decrementPendingCommitCount();
+
 protected:
     explicit ThreadedScrollingTree(AsyncScrollingCoordinator&);
 
@@ -74,8 +78,15 @@ protected:
 
 private:
     bool isThreadedScrollingTree() const override { return true; }
+    void applyLayerPositions() override;
 
     RefPtr<AsyncScrollingCoordinator> m_scrollingCoordinator;
+
+    void waitForPendingCommits();
+
+    Lock m_pendingCommitCountMutex;
+    unsigned m_pendingCommitCount { 0 };
+    Condition m_commitCondition;
 };
 
 } // namespace WebCore
index d157ceb..3cc2dc0 100644 (file)
@@ -122,6 +122,8 @@ void ScrollingCoordinatorMac::commitTreeState()
     RefPtr<ThreadedScrollingTree> threadedScrollingTree = downcast<ThreadedScrollingTree>(scrollingTree());
     ScrollingStateTree* unprotectedTreeState = scrollingStateTree()->commit(LayerRepresentation::PlatformLayerRepresentation).release();
 
+    threadedScrollingTree->incrementPendingCommitCount();
+
     ScrollingThread::dispatch([threadedScrollingTree, unprotectedTreeState] {
         std::unique_ptr<ScrollingStateTree> treeState(unprotectedTreeState);
         threadedScrollingTree->commitTreeState(WTFMove(treeState));