Attachment 360431 Details for Bug 193907 – Patch

[patch] Patch

bug-193907-20190128205419.patch (text/plain), 29.09 KB, created by Simon Fraser (smfr) on 2019-01-28 20:54:20 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Simon Fraser (smfr)

Created: 2019-01-28 20:54:20 PST

Size: 29.09 KB

patch

obsolete

>Subversion Revision: 240610
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index b9260e6555305a7d596f479efd283471bac95674..e208e6095007575c4dba1cb93c44989574659d0a 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,69 @@
>+2019-01-28  Simon Fraser  <simon.fraser@apple.com>
>+
>+        REGRESSION(r240553): [iOS] Crash in ScrollingTree::updateTreeFromStateNode when attempting to log in to icloud.com
>+        https://bugs.webkit.org/show_bug.cgi?id=193907
>+        rdar://problem/47604080
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Recent scrolling tree changes can trigger unparenting and reparenting of subtrees in the
>+        state tree. If a subframe's state tree nodes are unparented, a scrolling tree commit would
>+        show these as nodes being destroyed, which destroyed the tree nodes. When re-parented, the
>+        commit would re-create the tree node, but the state node would only have a subset of the
>+        change flags set, so the new tree node would fail to get all of the state (for example, it
>+        would be missing layers and scrolling geometry).
>+
>+        Fix by ensuring that when we reparent state node subtrees, we set all the change flags
>+        so that the full set of data is sent to the scrolling tree (the UI process, in the case of iOS WK2).
>+        Annoyingly, virtual setAllPropertiesChanged() functions are needed so each state node subclass can
>+        set the right change flags.
>+
>+        This patch also gets rid of m_nodesRemovedSinceLastCommit in the state tree. We can gain the same
>+        information by using copying all of the nodeIDs in m_nodeMap into a HashSet, and removing nodes
>+        as we encounter them in the tree walk.
>+        
>+        Rename m_latchedNode to m_latchedNodeID in ScrollingTree, since it's a nodeID, not a node pointer.
>+
>+        Tested by existing tests.
>+
>+        * page/scrolling/ScrollingStateFixedNode.cpp:
>+        (WebCore::ScrollingStateFixedNode::setAllPropertiesChanged):
>+        * page/scrolling/ScrollingStateFixedNode.h:
>+        * page/scrolling/ScrollingStateFrameScrollingNode.cpp:
>+        (WebCore::ScrollingStateFrameScrollingNode::setAllPropertiesChanged):
>+        * page/scrolling/ScrollingStateFrameScrollingNode.h:
>+        * page/scrolling/ScrollingStateNode.cpp:
>+        (WebCore::ScrollingStateNode::setPropertyChanged):
>+        (WebCore::ScrollingStateNode::setAllPropertiesChanged):
>+        * page/scrolling/ScrollingStateNode.h:
>+        (WebCore::ScrollingStateNode::setPropertyChangedBit):
>+        * page/scrolling/ScrollingStateScrollingNode.cpp:
>+        (WebCore::ScrollingStateScrollingNode::setAllPropertiesChanged):
>+        * page/scrolling/ScrollingStateScrollingNode.h:
>+        * page/scrolling/ScrollingStateStickyNode.cpp:
>+        (WebCore::ScrollingStateStickyNode::setAllPropertiesChanged):
>+        * page/scrolling/ScrollingStateStickyNode.h:
>+        * page/scrolling/ScrollingStateTree.cpp:
>+        (WebCore::ScrollingStateTree::insertNode): Add a RELEASE_ASSERT on the type of the node created
>+        if parentID == 0, since mistakes here can associate a ScrollingNodeType::MainFrame node with some
>+        other nodeID which can result in type confusion later.
>+        (WebCore::ScrollingStateTree::nodeWasReattachedRecursive):
>+        (WebCore::ScrollingStateTree::commit):
>+        (WebCore::ScrollingStateTree::willRemoveNode):
>+        (WebCore::ScrollingStateTree::setRemovedNodes): Deleted.
>+        * page/scrolling/ScrollingStateTree.h:
>+        (WebCore::ScrollingStateTree::removedNodes const): Deleted.
>+        * page/scrolling/ScrollingTree.cpp:
>+        (WebCore::ScrollingTree::shouldHandleWheelEventSynchronously):
>+        (WebCore::ScrollingTree::commitTreeState):
>+        (WebCore::ScrollingTree::latchedNode):
>+        (WebCore::ScrollingTree::setLatchedNode):
>+        (WebCore::ScrollingTree::clearLatchedNode):
>+        (WebCore::ScrollingTree::scrollingTreeAsText):
>+        (WebCore::ScrollingTree::removeDestroyedNodes): Deleted.
>+        * page/scrolling/ScrollingTree.h:
>+        (WebCore::ScrollingTree::hasLatchedNode const):
>+
> 2019-01-28  Simon Fraser  <simon.fraser@apple.com>
> 
>         svg/text/select-text-inside-non-static-position.html crashes under ScrollingStateTree::unparentChildrenAndDestroyNode()
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index 779b645b41cbc39a30ada85409e10833e665fd7f..7cc712bc5b257af2e568ee4b7edeb24c3d27901c 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,17 @@
>+2019-01-28  Simon Fraser  <simon.fraser@apple.com>
>+
>+        REGRESSION(r240553): [iOS] Crash in ScrollingTree::updateTreeFromStateNode when attempting to log in to icloud.com
>+        https://bugs.webkit.org/show_bug.cgi?id=193907
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Remove encode/decode of removedNodes.
>+
>+        * Shared/RemoteLayerTree/RemoteScrollingCoordinatorTransaction.cpp:
>+        (WebKit::RemoteScrollingCoordinatorTransaction::encode const):
>+        (WebKit::RemoteScrollingCoordinatorTransaction::decode):
>+        (WebKit::dump):
>+
> 2019-01-28  Joseph Pecoraro  <pecoraro@apple.com>
> 
>         [iOS] Attempting to open a Keynote document to iCloud.com from iCloud Files causes crash
>diff --git a/Source/WebCore/page/scrolling/ScrollingStateFixedNode.cpp b/Source/WebCore/page/scrolling/ScrollingStateFixedNode.cpp
>index 7070b3c8daf31ccc399eef87e9e0e8ef6abb8de7..7c677ae6b5267a5203b7375295583a5e1418bacf 100644
>--- a/Source/WebCore/page/scrolling/ScrollingStateFixedNode.cpp
>+++ b/Source/WebCore/page/scrolling/ScrollingStateFixedNode.cpp
>@@ -58,6 +58,12 @@ Ref<ScrollingStateNode> ScrollingStateFixedNode::clone(ScrollingStateTree& adopt
>     return adoptRef(*new ScrollingStateFixedNode(*this, adoptiveTree));
> }
> 
>+void ScrollingStateFixedNode::setAllPropertiesChanged()
>+{
>+    setPropertyChangedBit(ViewportConstraints);
>+    ScrollingStateNode::setAllPropertiesChanged();
>+}
>+
> void ScrollingStateFixedNode::updateConstraints(const FixedPositionViewportConstraints& constraints)
> {
>     if (m_constraints == constraints)
>diff --git a/Source/WebCore/page/scrolling/ScrollingStateFixedNode.h b/Source/WebCore/page/scrolling/ScrollingStateFixedNode.h
>index 576712a427b8198ccb4cdc51d61416aef5b4092a..8e1f9f0cb3c96e3d3965d3faa015f8e1837a2800 100644
>--- a/Source/WebCore/page/scrolling/ScrollingStateFixedNode.h
>+++ b/Source/WebCore/page/scrolling/ScrollingStateFixedNode.h
>@@ -55,6 +55,8 @@ private:
>     ScrollingStateFixedNode(ScrollingStateTree&, ScrollingNodeID);
>     ScrollingStateFixedNode(const ScrollingStateFixedNode&, ScrollingStateTree&);
> 
>+    void setAllPropertiesChanged() override;
>+
>     void reconcileLayerPositionForViewportRect(const LayoutRect& viewportRect, ScrollingLayerPositionAction) override;
> 
>     void dumpProperties(WTF::TextStream&, ScrollingStateTreeAsTextBehavior) const override;
>diff --git a/Source/WebCore/page/scrolling/ScrollingStateFrameScrollingNode.cpp b/Source/WebCore/page/scrolling/ScrollingStateFrameScrollingNode.cpp
>index 23d14ca45bd1c57503804a17369a003691571e8a..76b1d0f56c48c9024f7992ef2f14f079ccddf36c 100644
>--- a/Source/WebCore/page/scrolling/ScrollingStateFrameScrollingNode.cpp
>+++ b/Source/WebCore/page/scrolling/ScrollingStateFrameScrollingNode.cpp
>@@ -94,6 +94,33 @@ Ref<ScrollingStateNode> ScrollingStateFrameScrollingNode::clone(ScrollingStateTr
>     return adoptRef(*new ScrollingStateFrameScrollingNode(*this, adoptiveTree));
> }
> 
>+void ScrollingStateFrameScrollingNode::setAllPropertiesChanged()
>+{
>+    setPropertyChangedBit(FrameScaleFactor);
>+    setPropertyChangedBit(EventTrackingRegion);
>+    setPropertyChangedBit(ReasonsForSynchronousScrolling);
>+    setPropertyChangedBit(ScrolledContentsLayer);
>+    setPropertyChangedBit(CounterScrollingLayer);
>+    setPropertyChangedBit(InsetClipLayer);
>+    setPropertyChangedBit(ContentShadowLayer);
>+    setPropertyChangedBit(HeaderHeight);
>+    setPropertyChangedBit(FooterHeight);
>+    setPropertyChangedBit(HeaderLayer);
>+    setPropertyChangedBit(FooterLayer);
>+    setPropertyChangedBit(VerticalScrollbarLayer);
>+    setPropertyChangedBit(HorizontalScrollbarLayer);
>+    setPropertyChangedBit(PainterForScrollbar);
>+    setPropertyChangedBit(BehaviorForFixedElements);
>+    setPropertyChangedBit(TopContentInset);
>+    setPropertyChangedBit(FixedElementsLayoutRelativeToFrame);
>+    setPropertyChangedBit(VisualViewportEnabled);
>+    setPropertyChangedBit(LayoutViewport);
>+    setPropertyChangedBit(MinLayoutViewportOrigin);
>+    setPropertyChangedBit(MaxLayoutViewportOrigin);
>+
>+    ScrollingStateScrollingNode::setAllPropertiesChanged();
>+}
>+
> void ScrollingStateFrameScrollingNode::setFrameScaleFactor(float scaleFactor)
> {
>     if (m_frameScaleFactor == scaleFactor)
>diff --git a/Source/WebCore/page/scrolling/ScrollingStateFrameScrollingNode.h b/Source/WebCore/page/scrolling/ScrollingStateFrameScrollingNode.h
>index 13b3415fe23e931fd99f96cb669c64aebdc63d9a..7b5970a5443efdfecb0ec35cad0e33d905868edb 100644
>--- a/Source/WebCore/page/scrolling/ScrollingStateFrameScrollingNode.h
>+++ b/Source/WebCore/page/scrolling/ScrollingStateFrameScrollingNode.h
>@@ -145,6 +145,8 @@ private:
>     ScrollingStateFrameScrollingNode(ScrollingStateTree&, ScrollingNodeType, ScrollingNodeID);
>     ScrollingStateFrameScrollingNode(const ScrollingStateFrameScrollingNode&, ScrollingStateTree&);
> 
>+    void setAllPropertiesChanged() override;
>+
>     LayerRepresentation m_counterScrollingLayer;
>     LayerRepresentation m_insetClipLayer;
>     LayerRepresentation m_contentShadowLayer;
>diff --git a/Source/WebCore/page/scrolling/ScrollingStateNode.cpp b/Source/WebCore/page/scrolling/ScrollingStateNode.cpp
>index 53b11a5ff48037a9cb3cabbc60338c9a73b92909..36390966191454b1b623a8e87d63d390ff8c711d 100644
>--- a/Source/WebCore/page/scrolling/ScrollingStateNode.cpp
>+++ b/Source/WebCore/page/scrolling/ScrollingStateNode.cpp
>@@ -63,7 +63,14 @@ void ScrollingStateNode::setPropertyChanged(unsigned propertyBit)
>     if (hasChangedProperty(propertyBit))
>         return;
> 
>-    m_changedProperties |= (static_cast<ChangedProperties>(1) << propertyBit);
>+    setPropertyChangedBit(propertyBit);
>+    m_scrollingStateTree.setHasChangedProperties();
>+}
>+
>+void ScrollingStateNode::setAllPropertiesChanged()
>+{
>+    setPropertyChangedBit(ScrollLayer);
>+    setPropertyChangedBit(ChildNodes);
>     m_scrollingStateTree.setHasChangedProperties();
> }
> 
>diff --git a/Source/WebCore/page/scrolling/ScrollingStateNode.h b/Source/WebCore/page/scrolling/ScrollingStateNode.h
>index 11530885d5bf242c78944e75342b3ed61115c996..bbefeb5124dbf3afc4b91b06517a14fc8a62ef54 100644
>--- a/Source/WebCore/page/scrolling/ScrollingStateNode.h
>+++ b/Source/WebCore/page/scrolling/ScrollingStateNode.h
>@@ -207,6 +207,7 @@ public:
>     Ref<ScrollingStateNode> cloneAndReset(ScrollingStateTree& adoptiveTree);
>     void cloneAndResetChildren(ScrollingStateNode&, ScrollingStateTree& adoptiveTree);
> 
>+    // FIXME: using an OptionSet<> for these and derived class bits would simplify code.
>     enum {
>         ScrollLayer = 0,
>         ChildNodes,
>@@ -218,6 +219,7 @@ public:
>     bool hasChangedProperty(unsigned propertyBit) const { return m_changedProperties & (static_cast<ChangedProperties>(1) << propertyBit); }
>     void resetChangedProperties() { m_changedProperties = 0; }
>     void setPropertyChanged(unsigned propertyBit);
>+    virtual void setAllPropertiesChanged();
> 
>     ChangedProperties changedProperties() const { return m_changedProperties; }
>     void setChangedProperties(ChangedProperties changedProperties) { m_changedProperties = changedProperties; }
>@@ -254,7 +256,9 @@ protected:
>     ScrollingStateNode(const ScrollingStateNode&, ScrollingStateTree&);
> 
>     virtual void dumpProperties(WTF::TextStream&, ScrollingStateTreeAsTextBehavior) const;
>-    
>+
>+    inline void setPropertyChangedBit(unsigned propertyBit);
>+
> private:
>     void dump(WTF::TextStream&, ScrollingStateTreeAsTextBehavior) const;
> 
>@@ -270,6 +274,11 @@ private:
>     LayerRepresentation m_layer;
> };
> 
>+void ScrollingStateNode::setPropertyChangedBit(unsigned propertyBit)
>+{
>+    m_changedProperties |= (static_cast<ChangedProperties>(1) << propertyBit);
>+}
>+
> } // namespace WebCore
> 
> #define SPECIALIZE_TYPE_TRAITS_SCROLLING_STATE_NODE(ToValueTypeName, predicate) \
>diff --git a/Source/WebCore/page/scrolling/ScrollingStateScrollingNode.cpp b/Source/WebCore/page/scrolling/ScrollingStateScrollingNode.cpp
>index 153362bb19823a3d7c41e7ce9fce354a44373dbf..bef0ca752b0900cbb266f9c013c48d453c95e176 100644
>--- a/Source/WebCore/page/scrolling/ScrollingStateScrollingNode.cpp
>+++ b/Source/WebCore/page/scrolling/ScrollingStateScrollingNode.cpp
>@@ -60,6 +60,30 @@ ScrollingStateScrollingNode::ScrollingStateScrollingNode(const ScrollingStateScr
> 
> ScrollingStateScrollingNode::~ScrollingStateScrollingNode() = default;
> 
>+void ScrollingStateScrollingNode::setAllPropertiesChanged()
>+{
>+    setPropertyChangedBit(ScrollableAreaSize);
>+    setPropertyChangedBit(TotalContentsSize);
>+    setPropertyChangedBit(ReachableContentsSize);
>+    setPropertyChangedBit(ParentRelativeScrollableRect);
>+    setPropertyChangedBit(ScrollPosition);
>+    setPropertyChangedBit(ScrollOrigin);
>+    setPropertyChangedBit(ScrollableAreaParams);
>+    setPropertyChangedBit(RequestedScrollPosition);
>+#if ENABLE(CSS_SCROLL_SNAP)
>+    setPropertyChangedBit(HorizontalSnapOffsets);
>+    setPropertyChangedBit(VerticalSnapOffsets);
>+    setPropertyChangedBit(HorizontalSnapOffsetRanges);
>+    setPropertyChangedBit(VerticalSnapOffsetRanges);
>+    setPropertyChangedBit(CurrentHorizontalSnapOffsetIndex);
>+    setPropertyChangedBit(CurrentVerticalSnapOffsetIndex);
>+#endif
>+    setPropertyChangedBit(ExpectsWheelEventTestTrigger);
>+    setPropertyChangedBit(ScrolledContentsLayer);
>+
>+    ScrollingStateNode::setAllPropertiesChanged();
>+}
>+
> void ScrollingStateScrollingNode::setScrollableAreaSize(const FloatSize& size)
> {
>     if (m_scrollableAreaSize == size)
>diff --git a/Source/WebCore/page/scrolling/ScrollingStateScrollingNode.h b/Source/WebCore/page/scrolling/ScrollingStateScrollingNode.h
>index 7de737b6487c0dda34a4c5db7ee2886293048dc8..300db985b1e73effe08c8bae99343827f7f3c7a4 100644
>--- a/Source/WebCore/page/scrolling/ScrollingStateScrollingNode.h
>+++ b/Source/WebCore/page/scrolling/ScrollingStateScrollingNode.h
>@@ -116,8 +116,10 @@ protected:
>     ScrollingStateScrollingNode(ScrollingStateTree&, ScrollingNodeType, ScrollingNodeID);
>     ScrollingStateScrollingNode(const ScrollingStateScrollingNode&, ScrollingStateTree&);
> 
>+    void setAllPropertiesChanged() override;
>+
>     void dumpProperties(WTF::TextStream&, ScrollingStateTreeAsTextBehavior) const override;
>-    
>+
> private:
>     FloatSize m_scrollableAreaSize;
>     FloatSize m_totalContentsSize;
>diff --git a/Source/WebCore/page/scrolling/ScrollingStateStickyNode.cpp b/Source/WebCore/page/scrolling/ScrollingStateStickyNode.cpp
>index 127919c020b6a67061751854a920f60231234996..f2ae5c643894c2ab9b50caba4b9749fcb71243c3 100644
>--- a/Source/WebCore/page/scrolling/ScrollingStateStickyNode.cpp
>+++ b/Source/WebCore/page/scrolling/ScrollingStateStickyNode.cpp
>@@ -58,6 +58,12 @@ Ref<ScrollingStateNode> ScrollingStateStickyNode::clone(ScrollingStateTree& adop
>     return adoptRef(*new ScrollingStateStickyNode(*this, adoptiveTree));
> }
> 
>+void ScrollingStateStickyNode::setAllPropertiesChanged()
>+{
>+    setPropertyChangedBit(ViewportConstraints);
>+    ScrollingStateNode::setAllPropertiesChanged();
>+}
>+
> void ScrollingStateStickyNode::updateConstraints(const StickyPositionViewportConstraints& constraints)
> {
>     if (m_constraints == constraints)
>diff --git a/Source/WebCore/page/scrolling/ScrollingStateStickyNode.h b/Source/WebCore/page/scrolling/ScrollingStateStickyNode.h
>index 6136a849943f4a7d9e329144c3b50994d0dc580b..a5f867c0fd92e11bc158cfcd5b499a39b5c76dd0 100644
>--- a/Source/WebCore/page/scrolling/ScrollingStateStickyNode.h
>+++ b/Source/WebCore/page/scrolling/ScrollingStateStickyNode.h
>@@ -55,6 +55,8 @@ private:
>     ScrollingStateStickyNode(ScrollingStateTree&, ScrollingNodeID);
>     ScrollingStateStickyNode(const ScrollingStateStickyNode&, ScrollingStateTree&);
> 
>+    void setAllPropertiesChanged() override;
>+
>     void reconcileLayerPositionForViewportRect(const LayoutRect& viewportRect, ScrollingLayerPositionAction) override;
> 
>     void dumpProperties(WTF::TextStream&, ScrollingStateTreeAsTextBehavior) const override;
>diff --git a/Source/WebCore/page/scrolling/ScrollingStateTree.cpp b/Source/WebCore/page/scrolling/ScrollingStateTree.cpp
>index 8382a6738e0be25031643d5e9d602f3e4c1af1fe..83da374a6f073b568a60bb93ac5a2ce6bbc52cdf 100644
>--- a/Source/WebCore/page/scrolling/ScrollingStateTree.cpp
>+++ b/Source/WebCore/page/scrolling/ScrollingStateTree.cpp
>@@ -153,6 +153,7 @@ ScrollingNodeID ScrollingStateTree::insertNode(ScrollingNodeType nodeType, Scrol
> 
>     ScrollingStateNode* newNode = nullptr;
>     if (!parentID) {
>+        RELEASE_ASSERT(nodeType == ScrollingNodeType::MainFrame);
>         ASSERT(!childIndex || childIndex == notFound);
>         // If we're resetting the root node, we should clear the HashMap and destroy the current children.
>         clear();
>@@ -169,7 +170,10 @@ ScrollingNodeID ScrollingStateTree::insertNode(ScrollingNodeType nodeType, Scrol
> 
>         if (parentID) {
>             if (auto unparentedNode = m_unparentedNodes.take(newNodeID)) {
>+                LOG_WITH_STREAM(Scrolling, stream << "ScrollingStateTree " << this << " insertNode " << newNodeID << " getting node from unparented nodes");
>                 newNode = unparentedNode.get();
>+                nodeWasReattachedRecursive(*unparentedNode);
>+
>                 if (childIndex == notFound)
>                     parent->appendChild(unparentedNode.releaseNonNull());
>                 else
>@@ -188,7 +192,6 @@ ScrollingNodeID ScrollingStateTree::insertNode(ScrollingNodeType nodeType, Scrol
>     }
> 
>     addNode(*newNode);
>-    m_nodesRemovedSinceLastCommit.remove(newNodeID);
>     return newNodeID;
> }
> 
>@@ -265,6 +268,17 @@ void ScrollingStateTree::clear()
>     m_unparentedNodes.clear();
> }
> 
>+void ScrollingStateTree::nodeWasReattachedRecursive(ScrollingStateNode& node)
>+{
>+    // When a node is re-attached, the ScrollingTree is recreating the ScrollingNode from scratch, so we need to set all the dirty bits.
>+    node.setAllPropertiesChanged();
>+
>+    if (auto* children = node.children()) {
>+        for (auto& child : *children)
>+            nodeWasReattachedRecursive(*child);
>+    }
>+}
>+
> std::unique_ptr<ScrollingStateTree> ScrollingStateTree::commit(LayerRepresentation::Type preferredLayerRepresentation)
> {
>     if (!m_unparentedNodes.isEmpty()) {
>@@ -279,9 +293,6 @@ std::unique_ptr<ScrollingStateTree> ScrollingStateTree::commit(LayerRepresentati
>     if (m_rootStateNode)
>         treeStateClone->setRootStateNode(static_reference_cast<ScrollingStateFrameScrollingNode>(m_rootStateNode->cloneAndReset(*treeStateClone)));
> 
>-    // Copy the IDs of the nodes that have been removed since the last commit into the clone.
>-    treeStateClone->m_nodesRemovedSinceLastCommit.swap(m_nodesRemovedSinceLastCommit);
>-
>     // Now the clone tree has changed properties, and the original tree does not.
>     treeStateClone->m_hasChangedProperties = m_hasChangedProperties;
>     m_hasChangedProperties = false;
>@@ -334,16 +345,10 @@ void ScrollingStateTree::recursiveNodeWillBeRemoved(ScrollingStateNode* currNode
> 
> void ScrollingStateTree::willRemoveNode(ScrollingStateNode* node)
> {
>-    m_nodesRemovedSinceLastCommit.add(node->scrollingNodeID());
>     m_stateNodeMap.remove(node->scrollingNodeID());
>     setHasChangedProperties();
> }
> 
>-void ScrollingStateTree::setRemovedNodes(HashSet<ScrollingNodeID> nodes)
>-{
>-    m_nodesRemovedSinceLastCommit = WTFMove(nodes);
>-}
>-
> ScrollingStateNode* ScrollingStateTree::stateNodeForID(ScrollingNodeID scrollLayerID) const
> {
>     if (!scrollLayerID)
>diff --git a/Source/WebCore/page/scrolling/ScrollingStateTree.h b/Source/WebCore/page/scrolling/ScrollingStateTree.h
>index a7e5f258ca2054a3a671a18b68293518670be9e8..e5c1620e6f4e65b9aa79c2731688b830cc5fd3b9 100644
>--- a/Source/WebCore/page/scrolling/ScrollingStateTree.h
>+++ b/Source/WebCore/page/scrolling/ScrollingStateTree.h
>@@ -57,9 +57,6 @@ public:
>     void unparentChildrenAndDestroyNode(ScrollingNodeID);
>     void detachAndDestroySubtree(ScrollingNodeID);
>     void clear();
>-    
>-    const HashSet<ScrollingNodeID>& removedNodes() const { return m_nodesRemovedSinceLastCommit; }
>-    WEBCORE_EXPORT void setRemovedNodes(HashSet<ScrollingNodeID>);
> 
>     // Copies the current tree state and clears the changed properties mask in the original.
>     WEBCORE_EXPORT std::unique_ptr<ScrollingStateTree> commit(LayerRepresentation::Type preferredLayerRepresentation);
>@@ -82,6 +79,8 @@ private:
>     void setRootStateNode(Ref<ScrollingStateFrameScrollingNode>&&);
>     void addNode(ScrollingStateNode&);
> 
>+    void nodeWasReattachedRecursive(ScrollingStateNode&);
>+
>     Ref<ScrollingStateNode> createNode(ScrollingNodeType, ScrollingNodeID);
> 
>     bool nodeTypeAndParentMatch(ScrollingStateNode&, ScrollingNodeType, ScrollingStateNode* parentNode) const;
>@@ -98,7 +97,6 @@ private:
>     HashMap<ScrollingNodeID, RefPtr<ScrollingStateNode>> m_unparentedNodes;
> 
>     RefPtr<ScrollingStateFrameScrollingNode> m_rootStateNode;
>-    HashSet<ScrollingNodeID> m_nodesRemovedSinceLastCommit;
>     bool m_hasChangedProperties { false };
>     bool m_hasNewRootStateNode { false };
>     LayerRepresentation::Type m_preferredLayerRepresentation { LayerRepresentation::GraphicsLayerRepresentation };
>diff --git a/Source/WebCore/page/scrolling/ScrollingTree.cpp b/Source/WebCore/page/scrolling/ScrollingTree.cpp
>index 434f584079b4454911cbb28a31e577e516c73a69..7fb27f0327f3c68c65dadd23ae21f831d9406b28 100644
>--- a/Source/WebCore/page/scrolling/ScrollingTree.cpp
>+++ b/Source/WebCore/page/scrolling/ScrollingTree.cpp
>@@ -56,7 +56,7 @@ bool ScrollingTree::shouldHandleWheelEventSynchronously(const PlatformWheelEvent
>         return false;
> 
>     if (shouldSetLatch)
>-        m_latchedNode = 0;
>+        m_latchedNodeID = 0;
>     
>     if (!m_eventTrackingRegions.isEmpty() && m_rootNode) {
>         auto& frameScrollingNode = downcast<ScrollingTreeFrameScrollingNode>(*m_rootNode);
>@@ -138,13 +138,26 @@ void ScrollingTree::commitTreeState(std::unique_ptr<ScrollingStateTree> scrollin
>     bool scrollRequestIsProgammatic = rootNode ? rootNode->requestedScrollPositionRepresentsProgrammaticScroll() : false;
>     SetForScope<bool> changeHandlingProgrammaticScroll(m_isHandlingProgrammaticScroll, scrollRequestIsProgammatic);
> 
>-    removeDestroyedNodes(*scrollingStateTree);
>+    // unvisitedNodes starts with all nodes in the map; we remove nodes as we visit them. At the end, it's the unvisited nodes.
>+    // We can't use orphanNodes for this, because orphanNodes won't contain descendants of removed nodes.
>+    HashSet<ScrollingNodeID> unvisitedNodes;
>+    for (auto nodeID : m_nodeMap.keys())
>+        unvisitedNodes.add(nodeID);
> 
>+    // orphanNodes keeps child nodes alive while we rebuild child lists.
>     OrphanScrollingNodeMap orphanNodes;
>-    updateTreeFromStateNode(rootNode, orphanNodes);
>+    updateTreeFromStateNode(rootNode, orphanNodes, unvisitedNodes);
>+    
>+    for (auto nodeID : unvisitedNodes) {
>+        if (nodeID == m_latchedNodeID)
>+            clearLatchedNode();
>+        
>+        LOG(Scrolling, "ScrollingTree::commitTreeState - removing unvisited node %llu", nodeID);
>+        m_nodeMap.remove(nodeID);
>+    }
> }
> 
>-void ScrollingTree::updateTreeFromStateNode(const ScrollingStateNode* stateNode, OrphanScrollingNodeMap& orphanNodes)
>+void ScrollingTree::updateTreeFromStateNode(const ScrollingStateNode* stateNode, OrphanScrollingNodeMap& orphanNodes, HashSet<ScrollingNodeID>& unvisitedNodes)
> {
>     if (!stateNode) {
>         m_nodeMap.clear();
>@@ -158,9 +171,10 @@ void ScrollingTree::updateTreeFromStateNode(const ScrollingStateNode* stateNode,
>     auto it = m_nodeMap.find(nodeID);
> 
>     RefPtr<ScrollingTreeNode> node;
>-    if (it != m_nodeMap.end())
>+    if (it != m_nodeMap.end()) {
>         node = it->value;
>-    else {
>+        unvisitedNodes.remove(nodeID);
>+    } else {
>         node = createScrollingTreeNode(stateNode->nodeType(), nodeID);
>         if (!parentNodeID) {
>             // This is the root node. Clear the node map.
>@@ -195,21 +209,12 @@ void ScrollingTree::updateTreeFromStateNode(const ScrollingStateNode* stateNode,
>     // Now update the children if we have any.
>     if (auto children = stateNode->children()) {
>         for (auto& child : *children)
>-            updateTreeFromStateNode(child.get(), orphanNodes);
>+            updateTreeFromStateNode(child.get(), orphanNodes, unvisitedNodes);
>     }
> 
>     node->commitStateAfterChildren(*stateNode);
> }
> 
>-void ScrollingTree::removeDestroyedNodes(const ScrollingStateTree& stateTree)
>-{
>-    for (const auto& removedNodeID : stateTree.removedNodes()) {
>-        m_nodeMap.remove(removedNodeID);
>-        if (removedNodeID == m_latchedNode)
>-            clearLatchedNode();
>-    }
>-}
>-
> ScrollingTreeNode* ScrollingTree::nodeForID(ScrollingNodeID nodeID) const
> {
>     if (!nodeID)
>@@ -364,19 +369,19 @@ bool ScrollingTree::scrollingPerformanceLoggingEnabled()
> ScrollingNodeID ScrollingTree::latchedNode()
> {
>     LockHolder locker(m_mutex);
>-    return m_latchedNode;
>+    return m_latchedNodeID;
> }
> 
> void ScrollingTree::setLatchedNode(ScrollingNodeID node)
> {
>     LockHolder locker(m_mutex);
>-    m_latchedNode = node;
>+    m_latchedNodeID = node;
> }
> 
> void ScrollingTree::clearLatchedNode()
> {
>     LockHolder locker(m_mutex);
>-    m_latchedNode = 0;
>+    m_latchedNodeID = 0;
> }
> 
> String ScrollingTree::scrollingTreeAsText()
>@@ -386,8 +391,8 @@ String ScrollingTree::scrollingTreeAsText()
>     TextStream::GroupScope scope(ts);
>     ts << "scrolling tree";
>     
>-    if (m_latchedNode)
>-        ts.dumpProperty("latched node", m_latchedNode);
>+    if (m_latchedNodeID)
>+        ts.dumpProperty("latched node", m_latchedNodeID);
> 
>     if (m_mainFrameScrollPosition != IntPoint())
>         ts.dumpProperty("main frame scroll position", m_mainFrameScrollPosition);
>diff --git a/Source/WebCore/page/scrolling/ScrollingTree.h b/Source/WebCore/page/scrolling/ScrollingTree.h
>index 6d2468307a8ed0243c028374d64f978c98a03ec9..ec3dd6e0766537b53ac3a475e968b11697eca77f 100644
>--- a/Source/WebCore/page/scrolling/ScrollingTree.h
>+++ b/Source/WebCore/page/scrolling/ScrollingTree.h
>@@ -143,7 +143,7 @@ public:
>     void setLatchedNode(ScrollingNodeID);
>     void clearLatchedNode();
> 
>-    bool hasLatchedNode() const { return m_latchedNode; }
>+    bool hasLatchedNode() const { return m_latchedNodeID; }
>     void setOrClearLatchedNode(const PlatformWheelEvent&, ScrollingNodeID);
> 
>     bool hasFixedOrSticky() const { return !!m_fixedOrStickyNodeCount; }
>@@ -163,16 +163,14 @@ protected:
>     WEBCORE_EXPORT virtual void handleWheelEvent(const PlatformWheelEvent&);
> 
> private:
>-    void removeDestroyedNodes(const ScrollingStateTree&);
>-    
>-    typedef HashMap<ScrollingNodeID, RefPtr<ScrollingTreeNode>> OrphanScrollingNodeMap;
>-    void updateTreeFromStateNode(const ScrollingStateNode*, OrphanScrollingNodeMap&);
>+    using OrphanScrollingNodeMap = HashMap<ScrollingNodeID, RefPtr<ScrollingTreeNode>>;
>+    void updateTreeFromStateNode(const ScrollingStateNode*, OrphanScrollingNodeMap&, HashSet<ScrollingNodeID>& unvisitedNodes);
> 
>     ScrollingTreeNode* nodeForID(ScrollingNodeID) const;
> 
>     RefPtr<ScrollingTreeNode> m_rootNode;
> 
>-    typedef HashMap<ScrollingNodeID, ScrollingTreeNode*> ScrollingTreeNodeMap;
>+    using ScrollingTreeNodeMap = HashMap<ScrollingNodeID, ScrollingTreeNode*>;
>     ScrollingTreeNodeMap m_nodeMap;
> 
>     Lock m_mutex;
>@@ -181,7 +179,7 @@ private:
> 
>     Lock m_swipeStateMutex;
>     ScrollPinningBehavior m_scrollPinningBehavior { DoNotPin };
>-    ScrollingNodeID m_latchedNode { 0 };
>+    ScrollingNodeID m_latchedNodeID { 0 };
> 
>     unsigned m_fixedOrStickyNodeCount { 0 };
> 
>diff --git a/Source/WebKit/Shared/RemoteLayerTree/RemoteScrollingCoordinatorTransaction.cpp b/Source/WebKit/Shared/RemoteLayerTree/RemoteScrollingCoordinatorTransaction.cpp
>index 03a9f8f2009339de081668d3b52aaf34e4a8004e..059ed28fc972a882131e33ddf8a877e6bb99004e 100644
>--- a/Source/WebKit/Shared/RemoteLayerTree/RemoteScrollingCoordinatorTransaction.cpp
>+++ b/Source/WebKit/Shared/RemoteLayerTree/RemoteScrollingCoordinatorTransaction.cpp
>@@ -413,7 +413,6 @@ void RemoteScrollingCoordinatorTransaction::encode(IPC::Encoder& encoder) const
>             encodeNodeAndDescendants(encoder, *rootNode, numNodesEncoded);
> 
>         ASSERT_UNUSED(numNodesEncoded, numNodesEncoded == numNodes);
>-        encoder << m_scrollingStateTree->removedNodes();
>     } else
>         encoder << Vector<ScrollingNodeID>();
> }
>@@ -486,14 +485,6 @@ bool RemoteScrollingCoordinatorTransaction::decode(IPC::Decoder& decoder)
> 
>     m_scrollingStateTree->setHasNewRootStateNode(hasNewRootNode);
> 
>-    // Removed nodes
>-    HashSet<ScrollingNodeID> removedNodes;
>-    if (!decoder.decode(removedNodes))
>-        return false;
>-    
>-    if (removedNodes.size())
>-        m_scrollingStateTree->setRemovedNodes(removedNodes);
>-
>     return true;
> }
> 
>@@ -653,9 +644,6 @@ static void dump(TextStream& ts, const ScrollingStateTree& stateTree, bool chang
> 
>     if (stateTree.rootStateNode())
>         recursiveDumpNodes(ts, *stateTree.rootStateNode(), changedPropertiesOnly);
>-
>-    if (!stateTree.removedNodes().isEmpty())
>-        ts.dumpProperty<Vector<ScrollingNodeID>>("removed-nodes", copyToVector(stateTree.removedNodes()));
> }
> 
> WTF::CString RemoteScrollingCoordinatorTransaction::description() const

Actions: View | Formatted Diff | Diff

Attachments on bug 193907: 360431 | 360456 | 360472 | 360481 | 360483 | 360486 | 360488