[ Mojave+ ] Layout Test compositing/fixed-with-main-thread-scrolling.html is a flaky timeout
https://bugs.webkit.org/show_bug.cgi?id=198757
Reviewed by Tim Horton.
Source/WebCore:
WheelEventTestMonitor depends on "deferral reasons" getting added and removed, such that there is always
at least one reason active until scrolling quiesces.
WheelEventTestMonitor made the incorrect assumption that every call into ScrollingTreeScrollingNodeDelegateMac::handleWheelEvent()
would result in a scroll change making it to the main thread, so it would defer "ScrollingThreadSyncNeeded" there,
and rely on AsyncScrollingCoordinator::updateScrollPositionAfterAsyncScroll() to remove that deferral reason.
That assumption is wrong, because wheel events may coalesce, or have no impact on scroll position if already scrolled
to the max/min extent (e.g. when rubber banding).
Fix by adding a new "HandlingWheelEvent" deferral reason for the duration that the scrolling thread is processing an wheel event,
and then having ScrollingThreadSyncNeeded just represent the phase where any resulting scroll is being sent to the UI process.
These phases should always overlap.
This required moving isMonitoringWheelEvents() from the root scrolling node to the ScrollingTree.
* page/WheelEventTestMonitor.cpp:
(WebCore::operator<<):
* page/WheelEventTestMonitor.h:
* page/scrolling/AsyncScrollingCoordinator.cpp:
(WebCore::AsyncScrollingCoordinator::updateScrollPositionAfterAsyncScroll):
* page/scrolling/ScrollingTree.cpp:
(WebCore::ScrollingTree::commitTreeState):
* page/scrolling/ScrollingTree.h:
(WebCore::ScrollingTree::isMonitoringWheelEvents const):
* page/scrolling/ScrollingTreeScrollingNode.cpp:
(WebCore::ScrollingTreeScrollingNode::commitStateBeforeChildren):
* page/scrolling/ScrollingTreeScrollingNode.h:
* page/scrolling/ThreadedScrollingTree.cpp:
(WebCore::ThreadedScrollingTree::scrollingTreeNodeDidScroll):
* page/scrolling/mac/ScrollingTreeScrollingNodeDelegateMac.mm:
(WebCore::ScrollingTreeScrollingNodeDelegateMac::handleWheelEvent):
(WebCore::ScrollingTreeScrollingNodeDelegateMac::deferWheelEventTestCompletionForReason const):
(WebCore::ScrollingTreeScrollingNodeDelegateMac::removeWheelEventTestCompletionDeferralForReason const):
LayoutTests:
Remove expectation for compositing/fixed-with-main-thread-scrolling.html.
* platform/mac-wk2/TestExpectations:
git-svn-id: http://svn.webkit.org/repository/webkit/trunk@251262 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
index 911a3a8..f3fd01ee 100644
--- a/LayoutTests/ChangeLog
+++ b/LayoutTests/ChangeLog
@@ -1,3 +1,14 @@
+2019-10-17 Simon Fraser <simon.fraser@apple.com>
+
+ [ Mojave+ ] Layout Test compositing/fixed-with-main-thread-scrolling.html is a flaky timeout
+ https://bugs.webkit.org/show_bug.cgi?id=198757
+
+ Reviewed by Tim Horton.
+
+ Remove expectation for compositing/fixed-with-main-thread-scrolling.html.
+
+ * platform/mac-wk2/TestExpectations:
+
2019-10-17 Sihui Liu <sihui_liu@apple.com>
Using version 1 CFRunloopSource for faster task dispatch
diff --git a/LayoutTests/platform/mac-wk2/TestExpectations b/LayoutTests/platform/mac-wk2/TestExpectations
index fddec70..fbe745b 100644
--- a/LayoutTests/platform/mac-wk2/TestExpectations
+++ b/LayoutTests/platform/mac-wk2/TestExpectations
@@ -916,8 +916,6 @@
webkit.org/b/198663 http/tests/storageAccess/request-and-grant-access-then-navigate-same-site-should-have-access.html [ Pass Timeout ]
-webkit.org/b/198757 [ Mojave+ ] compositing/fixed-with-main-thread-scrolling.html [ Pass Timeout ]
-
webkit.org/b/198774 imported/w3c/web-platform-tests/websockets/bufferedAmount-unchanged-by-sync-xhr.any.worker.html [ Failure ]
webkit.org/b/176030 [ Debug ] http/tests/websocket/tests/hybi/send-object-tostring-check.html [ Pass Failure ]
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
index d74dfd8..e1bd4af 100644
--- a/Source/WebCore/ChangeLog
+++ b/Source/WebCore/ChangeLog
@@ -1,3 +1,44 @@
+2019-10-17 Simon Fraser <simon.fraser@apple.com>
+
+ [ Mojave+ ] Layout Test compositing/fixed-with-main-thread-scrolling.html is a flaky timeout
+ https://bugs.webkit.org/show_bug.cgi?id=198757
+
+ Reviewed by Tim Horton.
+
+ WheelEventTestMonitor depends on "deferral reasons" getting added and removed, such that there is always
+ at least one reason active until scrolling quiesces.
+
+ WheelEventTestMonitor made the incorrect assumption that every call into ScrollingTreeScrollingNodeDelegateMac::handleWheelEvent()
+ would result in a scroll change making it to the main thread, so it would defer "ScrollingThreadSyncNeeded" there,
+ and rely on AsyncScrollingCoordinator::updateScrollPositionAfterAsyncScroll() to remove that deferral reason.
+ That assumption is wrong, because wheel events may coalesce, or have no impact on scroll position if already scrolled
+ to the max/min extent (e.g. when rubber banding).
+
+ Fix by adding a new "HandlingWheelEvent" deferral reason for the duration that the scrolling thread is processing an wheel event,
+ and then having ScrollingThreadSyncNeeded just represent the phase where any resulting scroll is being sent to the UI process.
+ These phases should always overlap.
+
+ This required moving isMonitoringWheelEvents() from the root scrolling node to the ScrollingTree.
+
+ * page/WheelEventTestMonitor.cpp:
+ (WebCore::operator<<):
+ * page/WheelEventTestMonitor.h:
+ * page/scrolling/AsyncScrollingCoordinator.cpp:
+ (WebCore::AsyncScrollingCoordinator::updateScrollPositionAfterAsyncScroll):
+ * page/scrolling/ScrollingTree.cpp:
+ (WebCore::ScrollingTree::commitTreeState):
+ * page/scrolling/ScrollingTree.h:
+ (WebCore::ScrollingTree::isMonitoringWheelEvents const):
+ * page/scrolling/ScrollingTreeScrollingNode.cpp:
+ (WebCore::ScrollingTreeScrollingNode::commitStateBeforeChildren):
+ * page/scrolling/ScrollingTreeScrollingNode.h:
+ * page/scrolling/ThreadedScrollingTree.cpp:
+ (WebCore::ThreadedScrollingTree::scrollingTreeNodeDidScroll):
+ * page/scrolling/mac/ScrollingTreeScrollingNodeDelegateMac.mm:
+ (WebCore::ScrollingTreeScrollingNodeDelegateMac::handleWheelEvent):
+ (WebCore::ScrollingTreeScrollingNodeDelegateMac::deferWheelEventTestCompletionForReason const):
+ (WebCore::ScrollingTreeScrollingNodeDelegateMac::removeWheelEventTestCompletionDeferralForReason const):
+
2019-10-17 Ryosuke Niwa <rniwa@webkit.org>
Make requestIdleCallback suspendable
diff --git a/Source/WebCore/page/WheelEventTestMonitor.cpp b/Source/WebCore/page/WheelEventTestMonitor.cpp
index 1b68303..103ef9f 100644
--- a/Source/WebCore/page/WheelEventTestMonitor.cpp
+++ b/Source/WebCore/page/WheelEventTestMonitor.cpp
@@ -105,6 +105,7 @@
TextStream& operator<<(TextStream& ts, WheelEventTestMonitor::DeferReason reason)
{
switch (reason) {
+ case WheelEventTestMonitor::HandlingWheelEvent: ts << "handling wheel event"; break;
case WheelEventTestMonitor::RubberbandInProgress: ts << "rubberbanding"; break;
case WheelEventTestMonitor::ScrollSnapInProgress: ts << "scroll-snapping"; break;
case WheelEventTestMonitor::ScrollingThreadSyncNeeded: ts << "scrolling thread sync needed"; break;
diff --git a/Source/WebCore/page/WheelEventTestMonitor.h b/Source/WebCore/page/WheelEventTestMonitor.h
index b0793a3..f9d861a 100644
--- a/Source/WebCore/page/WheelEventTestMonitor.h
+++ b/Source/WebCore/page/WheelEventTestMonitor.h
@@ -47,10 +47,11 @@
WEBCORE_EXPORT void clearAllTestDeferrals();
enum DeferReason {
- RubberbandInProgress = 1 << 0,
- ScrollSnapInProgress = 1 << 1,
- ScrollingThreadSyncNeeded = 1 << 2,
- ContentScrollInProgress = 1 << 3
+ HandlingWheelEvent = 1 << 0,
+ RubberbandInProgress = 1 << 1,
+ ScrollSnapInProgress = 1 << 2,
+ ScrollingThreadSyncNeeded = 1 << 3,
+ ContentScrollInProgress = 1 << 4,
};
typedef const void* ScrollableAreaIdentifier;
diff --git a/Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp b/Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp
index 54a90e0..429eee9 100644
--- a/Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp
+++ b/Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp
@@ -350,15 +350,6 @@
if (scrollingNodeID == frameView.scrollingNodeID()) {
reconcileScrollingState(frameView, scrollPosition, layoutViewportOrigin, scrollType, ViewportRectStability::Stable, scrollingLayerPositionAction);
-
-#if PLATFORM(COCOA)
- if (m_page->isMonitoringWheelEvents()) {
- frameView.scrollAnimator().setWheelEventTestMonitor(m_page->wheelEventTestMonitor());
- if (const auto& monitor = m_page->wheelEventTestMonitor())
- monitor->removeDeferralForReason(reinterpret_cast<WheelEventTestMonitor::ScrollableAreaIdentifier>(scrollingNodeID), WheelEventTestMonitor::ScrollingThreadSyncNeeded);
- }
-#endif
-
return;
}
@@ -371,14 +362,6 @@
if (scrollingLayerPositionAction == ScrollingLayerPositionAction::Set)
m_page->editorClient().overflowScrollPositionChanged();
-
-#if PLATFORM(COCOA)
- if (m_page->isMonitoringWheelEvents()) {
- frameView.scrollAnimator().setWheelEventTestMonitor(m_page->wheelEventTestMonitor());
- if (const auto& monitor = m_page->wheelEventTestMonitor())
- monitor->removeDeferralForReason(reinterpret_cast<WheelEventTestMonitor::ScrollableAreaIdentifier>(scrollingNodeID), WheelEventTestMonitor::ScrollingThreadSyncNeeded);
- }
-#endif
}
}
diff --git a/Source/WebCore/page/scrolling/ScrollingTree.cpp b/Source/WebCore/page/scrolling/ScrollingTree.cpp
index 37cc72a..0223b50 100644
--- a/Source/WebCore/page/scrolling/ScrollingTree.cpp
+++ b/Source/WebCore/page/scrolling/ScrollingTree.cpp
@@ -151,7 +151,8 @@
&& (rootStateNodeChanged
|| rootNode->hasChangedProperty(ScrollingStateFrameScrollingNode::EventTrackingRegion)
|| rootNode->hasChangedProperty(ScrollingStateScrollingNode::ScrolledContentsLayer)
- || rootNode->hasChangedProperty(ScrollingStateFrameScrollingNode::AsyncFrameOrOverflowScrollingEnabled))) {
+ || rootNode->hasChangedProperty(ScrollingStateFrameScrollingNode::AsyncFrameOrOverflowScrollingEnabled)
+ || rootNode->hasChangedProperty(ScrollingStateFrameScrollingNode::IsMonitoringWheelEvents))) {
LockHolder lock(m_treeStateMutex);
if (rootStateNodeChanged || rootNode->hasChangedProperty(ScrollingStateScrollingNode::ScrolledContentsLayer))
@@ -162,6 +163,9 @@
if (rootStateNodeChanged || rootNode->hasChangedProperty(ScrollingStateFrameScrollingNode::AsyncFrameOrOverflowScrollingEnabled))
m_asyncFrameOrOverflowScrollingEnabled = scrollingStateTree->rootStateNode()->asyncFrameOrOverflowScrollingEnabled();
+
+ if (rootStateNodeChanged || rootNode->hasChangedProperty(ScrollingStateFrameScrollingNode::IsMonitoringWheelEvents))
+ m_isMonitoringWheelEvents = scrollingStateTree->rootStateNode()->isMonitoringWheelEvents();
}
// unvisitedNodes starts with all nodes in the map; we remove nodes as we visit them. At the end, it's the unvisited nodes.
diff --git a/Source/WebCore/page/scrolling/ScrollingTree.h b/Source/WebCore/page/scrolling/ScrollingTree.h
index a192a2a..9221c36 100644
--- a/Source/WebCore/page/scrolling/ScrollingTree.h
+++ b/Source/WebCore/page/scrolling/ScrollingTree.h
@@ -158,6 +158,8 @@
WEBCORE_EXPORT String scrollingTreeAsText(ScrollingStateTreeAsTextBehavior = ScrollingStateTreeAsTextBehaviorNormal);
+ bool isMonitoringWheelEvents() const { return m_isMonitoringWheelEvents; }
+
protected:
void setMainFrameScrollPosition(FloatPoint);
@@ -211,6 +213,7 @@
unsigned m_fixedOrStickyNodeCount { 0 };
bool m_isHandlingProgrammaticScroll { false };
+ bool m_isMonitoringWheelEvents { false };
bool m_scrollingPerformanceLoggingEnabled { false };
bool m_asyncFrameOrOverflowScrollingEnabled { false };
bool m_wasScrolledByDelegatedScrollingSincePreviousCommit { false };
diff --git a/Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp b/Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp
index d647b92..73dca28 100644
--- a/Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp
+++ b/Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp
@@ -97,9 +97,6 @@
if (state.hasChangedProperty(ScrollingStateScrollingNode::ScrollableAreaParams))
m_scrollableAreaParameters = state.scrollableAreaParameters();
- if (state.hasChangedProperty(ScrollingStateScrollingNode::IsMonitoringWheelEvents))
- m_isMonitoringWheelEvents = state.isMonitoringWheelEvents();
-
if (state.hasChangedProperty(ScrollingStateScrollingNode::ScrollContainerLayer))
m_scrollContainerLayer = state.scrollContainerLayer();
diff --git a/Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.h b/Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.h
index ae86f33..4cbf4d1 100644
--- a/Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.h
+++ b/Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.h
@@ -126,8 +126,6 @@
bool hasEnabledHorizontalScrollbar() const { return m_scrollableAreaParameters.hasEnabledHorizontalScrollbar; }
bool hasEnabledVerticalScrollbar() const { return m_scrollableAreaParameters.hasEnabledVerticalScrollbar; }
- bool isMonitoringWheelEvents() const { return m_isMonitoringWheelEvents; }
-
LayoutPoint parentToLocalPoint(LayoutPoint) const override;
LayoutPoint localToContentsPoint(LayoutPoint) const override;
@@ -148,7 +146,6 @@
unsigned m_currentVerticalSnapPointIndex { 0 };
#endif
ScrollableAreaParameters m_scrollableAreaParameters;
- bool m_isMonitoringWheelEvents { false };
bool m_isFirstCommit { true };
LayerRepresentation m_scrollContainerLayer;
diff --git a/Source/WebCore/page/scrolling/ThreadedScrollingTree.cpp b/Source/WebCore/page/scrolling/ThreadedScrollingTree.cpp
index 19a681f..7d89a15 100644
--- a/Source/WebCore/page/scrolling/ThreadedScrollingTree.cpp
+++ b/Source/WebCore/page/scrolling/ThreadedScrollingTree.cpp
@@ -110,8 +110,20 @@
if (is<ScrollingTreeFrameScrollingNode>(node))
layoutViewportOrigin = downcast<ScrollingTreeFrameScrollingNode>(node).layoutViewport().location();
- RunLoop::main().dispatch([scrollingCoordinator = m_scrollingCoordinator, nodeID = node.scrollingNodeID(), scrollPosition, layoutViewportOrigin, scrollingLayerPositionAction] {
+ bool monitoringWheelEvents = false;
+#if PLATFORM(MAC)
+ monitoringWheelEvents = isMonitoringWheelEvents();
+ if (monitoringWheelEvents)
+ deferWheelEventTestCompletionForReason(reinterpret_cast<WheelEventTestMonitor::ScrollableAreaIdentifier>(node.scrollingNodeID()), WheelEventTestMonitor::ScrollingThreadSyncNeeded);
+#endif
+ RunLoop::main().dispatch([scrollingCoordinator = m_scrollingCoordinator, nodeID = node.scrollingNodeID(), scrollPosition, layoutViewportOrigin, scrollingLayerPositionAction, monitoringWheelEvents] {
scrollingCoordinator->scheduleUpdateScrollPositionAfterAsyncScroll(nodeID, scrollPosition, layoutViewportOrigin, scrollingLayerPositionAction);
+#if PLATFORM(MAC)
+ if (monitoringWheelEvents)
+ scrollingCoordinator->removeWheelEventTestCompletionDeferralForReason(reinterpret_cast<WheelEventTestMonitor::ScrollableAreaIdentifier>(nodeID), WheelEventTestMonitor::ScrollingThreadSyncNeeded);
+#else
+ UNUSED_PARAM(monitoringWheelEvents);
+#endif
});
}
diff --git a/Source/WebCore/page/scrolling/mac/ScrollingTreeScrollingNodeDelegateMac.mm b/Source/WebCore/page/scrolling/mac/ScrollingTreeScrollingNodeDelegateMac.mm
index da6d13b..f7ac792 100644
--- a/Source/WebCore/page/scrolling/mac/ScrollingTreeScrollingNodeDelegateMac.mm
+++ b/Source/WebCore/page/scrolling/mac/ScrollingTreeScrollingNodeDelegateMac.mm
@@ -90,15 +90,18 @@
}
#if ENABLE(CSS_SCROLL_SNAP) || ENABLE(RUBBER_BANDING)
- if (scrollingNode().isMonitoringWheelEvents()) {
- if (scrollingTree().shouldHandleWheelEventSynchronously(wheelEvent))
- removeWheelEventTestCompletionDeferralForReason(reinterpret_cast<WheelEventTestMonitor::ScrollableAreaIdentifier>(scrollingNode().scrollingNodeID()), WheelEventTestMonitor::ScrollingThreadSyncNeeded);
- else
- deferWheelEventTestCompletionForReason(reinterpret_cast<WheelEventTestMonitor::ScrollableAreaIdentifier>(scrollingNode().scrollingNodeID()), WheelEventTestMonitor::ScrollingThreadSyncNeeded);
- }
+ if (scrollingTree().isMonitoringWheelEvents())
+ deferWheelEventTestCompletionForReason(reinterpret_cast<WheelEventTestMonitor::ScrollableAreaIdentifier>(scrollingNode().scrollingNodeID()), WheelEventTestMonitor::HandlingWheelEvent);
#endif
- return m_scrollController.handleWheelEvent(wheelEvent);
+ auto handled = m_scrollController.handleWheelEvent(wheelEvent);
+
+#if ENABLE(CSS_SCROLL_SNAP) || ENABLE(RUBBER_BANDING)
+ if (scrollingTree().isMonitoringWheelEvents())
+ removeWheelEventTestCompletionDeferralForReason(reinterpret_cast<WheelEventTestMonitor::ScrollableAreaIdentifier>(scrollingNode().scrollingNodeID()), WheelEventTestMonitor::HandlingWheelEvent);
+#endif
+
+ return handled;
}
bool ScrollingTreeScrollingNodeDelegateMac::isScrollSnapInProgress() const
@@ -301,7 +304,7 @@
void ScrollingTreeScrollingNodeDelegateMac::deferWheelEventTestCompletionForReason(WheelEventTestMonitor::ScrollableAreaIdentifier identifier, WheelEventTestMonitor::DeferReason reason) const
{
- if (!scrollingNode().isMonitoringWheelEvents())
+ if (!scrollingTree().isMonitoringWheelEvents())
return;
LOG_WITH_STREAM(WheelEventTestMonitor, stream << isMainThread() << " ScrollingTreeScrollingNodeDelegateMac::deferForReason: STARTING deferral for " << identifier << " because of " << reason);
@@ -310,7 +313,7 @@
void ScrollingTreeScrollingNodeDelegateMac::removeWheelEventTestCompletionDeferralForReason(WheelEventTestMonitor::ScrollableAreaIdentifier identifier, WheelEventTestMonitor::DeferReason reason) const
{
- if (!scrollingNode().isMonitoringWheelEvents())
+ if (!scrollingTree().isMonitoringWheelEvents())
return;
LOG_WITH_STREAM(WheelEventTestMonitor, stream << isMainThread() << " ScrollingTreeScrollingNodeDelegateMac::deferForReason: ENDING deferral for " << identifier << " because of " << reason);
