- Revision
- 251262
- Author
- simon.fra...@apple.com
- Date
- 2019-10-17 15:15:09 -0700 (Thu, 17 Oct 2019)
Log Message
[ 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:
Modified Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (251261 => 251262)
--- trunk/LayoutTests/ChangeLog 2019-10-17 22:05:24 UTC (rev 251261)
+++ trunk/LayoutTests/ChangeLog 2019-10-17 22:15:09 UTC (rev 251262)
@@ -1,3 +1,14 @@
+2019-10-17 Simon Fraser <simon.fra...@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_...@apple.com>
Using version 1 CFRunloopSource for faster task dispatch
Modified: trunk/LayoutTests/platform/mac-wk2/TestExpectations (251261 => 251262)
--- trunk/LayoutTests/platform/mac-wk2/TestExpectations 2019-10-17 22:05:24 UTC (rev 251261)
+++ trunk/LayoutTests/platform/mac-wk2/TestExpectations 2019-10-17 22:15:09 UTC (rev 251262)
@@ -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 ]
Modified: trunk/Source/WebCore/ChangeLog (251261 => 251262)
--- trunk/Source/WebCore/ChangeLog 2019-10-17 22:05:24 UTC (rev 251261)
+++ trunk/Source/WebCore/ChangeLog 2019-10-17 22:15:09 UTC (rev 251262)
@@ -1,3 +1,44 @@
+2019-10-17 Simon Fraser <simon.fra...@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 <rn...@webkit.org>
Make requestIdleCallback suspendable
Modified: trunk/Source/WebCore/page/WheelEventTestMonitor.cpp (251261 => 251262)
--- trunk/Source/WebCore/page/WheelEventTestMonitor.cpp 2019-10-17 22:05:24 UTC (rev 251261)
+++ trunk/Source/WebCore/page/WheelEventTestMonitor.cpp 2019-10-17 22:15:09 UTC (rev 251262)
@@ -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;
Modified: trunk/Source/WebCore/page/WheelEventTestMonitor.h (251261 => 251262)
--- trunk/Source/WebCore/page/WheelEventTestMonitor.h 2019-10-17 22:05:24 UTC (rev 251261)
+++ trunk/Source/WebCore/page/WheelEventTestMonitor.h 2019-10-17 22:15:09 UTC (rev 251262)
@@ -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;
Modified: trunk/Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp (251261 => 251262)
--- trunk/Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp 2019-10-17 22:05:24 UTC (rev 251261)
+++ trunk/Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp 2019-10-17 22:15:09 UTC (rev 251262)
@@ -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
}
}
Modified: trunk/Source/WebCore/page/scrolling/ScrollingTree.cpp (251261 => 251262)
--- trunk/Source/WebCore/page/scrolling/ScrollingTree.cpp 2019-10-17 22:05:24 UTC (rev 251261)
+++ trunk/Source/WebCore/page/scrolling/ScrollingTree.cpp 2019-10-17 22:15:09 UTC (rev 251262)
@@ -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.
Modified: trunk/Source/WebCore/page/scrolling/ScrollingTree.h (251261 => 251262)
--- trunk/Source/WebCore/page/scrolling/ScrollingTree.h 2019-10-17 22:05:24 UTC (rev 251261)
+++ trunk/Source/WebCore/page/scrolling/ScrollingTree.h 2019-10-17 22:15:09 UTC (rev 251262)
@@ -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 };
Modified: trunk/Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp (251261 => 251262)
--- trunk/Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp 2019-10-17 22:05:24 UTC (rev 251261)
+++ trunk/Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp 2019-10-17 22:15:09 UTC (rev 251262)
@@ -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();
Modified: trunk/Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.h (251261 => 251262)
--- trunk/Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.h 2019-10-17 22:05:24 UTC (rev 251261)
+++ trunk/Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.h 2019-10-17 22:15:09 UTC (rev 251262)
@@ -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;
Modified: trunk/Source/WebCore/page/scrolling/ThreadedScrollingTree.cpp (251261 => 251262)
--- trunk/Source/WebCore/page/scrolling/ThreadedScrollingTree.cpp 2019-10-17 22:05:24 UTC (rev 251261)
+++ trunk/Source/WebCore/page/scrolling/ThreadedScrollingTree.cpp 2019-10-17 22:15:09 UTC (rev 251262)
@@ -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
});
}
Modified: trunk/Source/WebCore/page/scrolling/mac/ScrollingTreeScrollingNodeDelegateMac.mm (251261 => 251262)
--- trunk/Source/WebCore/page/scrolling/mac/ScrollingTreeScrollingNodeDelegateMac.mm 2019-10-17 22:05:24 UTC (rev 251261)
+++ trunk/Source/WebCore/page/scrolling/mac/ScrollingTreeScrollingNodeDelegateMac.mm 2019-10-17 22:15:09 UTC (rev 251262)
@@ -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);
