Attachment 360472 Details for Bug 193907 – Patch

[patch] Patch

bug-193907-20190129103946.patch (text/plain), 36.26 KB, created by Simon Fraser (smfr) on 2019-01-29 10:39:47 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Simon Fraser (smfr)

Created: 2019-01-29 10:39:47 PST

Size: 36.26 KB

patch

obsolete

>Subversion Revision: 240610
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index b9260e6555305a7d596f479efd283471bac95674..5645d9db57fb99fb693aef41bd485f1e36986ee2 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,73 @@
>+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 FrÃ©dÃ©ric Wang.
>+
>+        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.
>+
>+        Test: compositing/geometry/composited-frame-contents.html
>+
>+        * 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::updateTreeFromStateNode):
>+        (WebCore::ScrollingTree::latchedNode):
>+        (WebCore::ScrollingTree::setLatchedNode):
>+        (WebCore::ScrollingTree::clearLatchedNode):
>+        (WebCore::ScrollingTree::scrollingTreeAsText):
>+        (WebCore::ScrollingTree::removeDestroyedNodes): Deleted.
>+        * page/scrolling/ScrollingTree.h:
>+        (WebCore::ScrollingTree::hasLatchedNode const):
>+        * rendering/RenderLayerCompositor.cpp:
>+        (WebCore::RenderLayerCompositor::ensureRootLayer): The scroll layer needs a 0,0,0 anchor point so that
>+        setting its position doesn't offset it relative to the center.
>+
> 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..8a83adb3c232941eb1f1200feb35343245044503 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 FrÃ©dÃ©ric Wang.
>+
>+        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/WebCore/rendering/RenderLayerCompositor.cpp b/Source/WebCore/rendering/RenderLayerCompositor.cpp
>index a16cdba69b41c28164814f6516ab2e0357a6f66a..2b77d1e7cf5bf9d82466c6754400d221bfbc25d1 100644
>--- a/Source/WebCore/rendering/RenderLayerCompositor.cpp
>+++ b/Source/WebCore/rendering/RenderLayerCompositor.cpp
>@@ -3444,6 +3444,7 @@ void RenderLayerCompositor::ensureRootLayer()
> #endif
>             m_scrollLayer = GraphicsLayer::create(graphicsLayerFactory(), *this, scrollLayerType);
>             m_scrollLayer->setName("frame scrolling");
>+            m_scrollLayer->setAnchorPoint({ });
> 
>             if (scrollLayerType == GraphicsLayer::Type::Scrolling) {
>                 // Scroll layer clips so there is no need for a separate clipping layer.
>@@ -3452,7 +3453,7 @@ void RenderLayerCompositor::ensureRootLayer()
>                 m_clipLayer = GraphicsLayer::create(graphicsLayerFactory(), *this);
>                 m_clipLayer->setName("frame clipping");
>                 m_clipLayer->setMasksToBounds(true);
>-                m_clipLayer->setAnchorPoint(FloatPoint3D());
>+                m_clipLayer->setAnchorPoint({ });
> 
>                 m_clipLayer->addChild(*m_scrollLayer);
>                 m_overflowControlsHostLayer->addChild(*m_clipLayer);
>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
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index b580a7d443ae6c4d8daf7ade4101c2f952b8e67c..b775dc3be82ada11cf7bf9073db923c89537df24 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,20 @@
>+2019-01-29  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 FrÃ©dÃ©ric Wang.
>+        
>+        New ref test for layer positions in composited frames.
>+        
>+        New baselines with anchor point on the scroll layer.
>+
>+        * compositing/geometry/composited-frame-contents-expected.html: Added.
>+        * compositing/geometry/composited-frame-contents.html: Added.
>+        * platform/ios-wk2/scrollingcoordinator/scrolling-tree/fixed-inside-frame-expected.txt:
>+        * scrollingcoordinator/scrolling-tree/fixed-inside-frame-expected.txt:
>+
> 2019-01-28  Daniel Bates  <dabates@apple.com>
> 
>         [iOS] Make Window virtual key code computation match Mac
>diff --git a/LayoutTests/compositing/geometry/composited-frame-contents-expected.html b/LayoutTests/compositing/geometry/composited-frame-contents-expected.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..5a796de547a1addcfdf8014fb9a468853cb5d8a1
>--- /dev/null
>+++ b/LayoutTests/compositing/geometry/composited-frame-contents-expected.html
>@@ -0,0 +1,49 @@
>+<!DOCTYPE html>
>+<html>
>+<head>
>+    <style>
>+        body {
>+            margin: 0;
>+        }
>+        .container {
>+            border: 20px solid blue;
>+            padding: 12px;
>+            height: 400px;
>+            width: 500px;
>+            margin: 20px;
>+            background-color: orange;
>+            outline: 12px solid rgba(0, 0, 0, 0.25);
>+        }
>+        
>+        .frame {
>+            height: 100%;
>+            width: 100%;
>+            background-color: silver;
>+            padding: 10px;
>+            box-sizing: border-box;
>+        }
>+        
>+        .box {
>+            width: 200px;
>+            height: 200px;
>+            background-color: green;
>+        }
>+        .scrollbar-obscurer {
>+            position: absolute;
>+            height: 400px;
>+            width: 18px;
>+            top: 52px;
>+            left: 534px;
>+            background-color: gray;
>+        }
>+    </style>
>+</head>
>+<body>
>+    <div class="container">
>+        <div class="frame">
>+            <div class="box"></div>
>+        </div>
>+    </div>
>+    <div class="scrollbar-obscurer"></div>
>+</body>
>+</html>
>diff --git a/LayoutTests/compositing/geometry/composited-frame-contents.html b/LayoutTests/compositing/geometry/composited-frame-contents.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..2042fc0a9ed3a1925e5a647f2beb9f41471002dc
>--- /dev/null
>+++ b/LayoutTests/compositing/geometry/composited-frame-contents.html
>@@ -0,0 +1,52 @@
>+<!DOCTYPE html>
>+<html>
>+<head>
>+    <style>
>+        body {
>+            margin: 0;
>+        }
>+        iframe {
>+            border: 20px solid blue;
>+            padding: 12px;
>+            height: 400px;
>+            width: 500px;
>+            margin: 20px;
>+            background-color: orange;
>+            outline: 12px solid rgba(0, 0, 0, 0.25);
>+        }
>+        .scrollbar-obscurer {
>+            position: absolute;
>+            height: 400px;
>+            width: 18px;
>+            top: 52px;
>+            left: 534px;
>+            background-color: gray;
>+        }
>+    </style>
>+    <script>
>+        if (window.internals)
>+            internals.settings.setAsyncFrameScrollingEnabled(true);
>+    </script>
>+</head>
>+<body>
>+    <iframe srcdoc="
>+        <style>
>+        body {
>+            height: 1000px;
>+            background-color: silver;
>+            margin: 10px;
>+        }
>+        .box {
>+            width: 200px;
>+            height: 200px;
>+            background-color: green;
>+        }
>+        
>+        </style>
>+        <body>
>+            <div class=box></div>
>+        </body>
>+    "></iframe>
>+    <div class="scrollbar-obscurer"></div>
>+</body>
>+</html>
>diff --git a/LayoutTests/platform/ios-wk2/scrollingcoordinator/scrolling-tree/fixed-inside-frame-expected.txt b/LayoutTests/platform/ios-wk2/scrollingcoordinator/scrolling-tree/fixed-inside-frame-expected.txt
>index df5af7a160cb64747ba1d17182610e31fd837f59..033e738014a26254b193457b9d10c285d58249de 100644
>--- a/LayoutTests/platform/ios-wk2/scrollingcoordinator/scrolling-tree/fixed-inside-frame-expected.txt
>+++ b/LayoutTests/platform/ios-wk2/scrollingcoordinator/scrolling-tree/fixed-inside-frame-expected.txt
>@@ -66,6 +66,7 @@
>                   (children 1
>                     (GraphicsLayer
>                       (position 0.00 -120.00)
>+                      (anchor 0.00 0.00)
>                       (children 1
>                         (GraphicsLayer
>                           (anchor 0.00 0.00)
>diff --git a/LayoutTests/scrollingcoordinator/scrolling-tree/fixed-inside-frame-expected.txt b/LayoutTests/scrollingcoordinator/scrolling-tree/fixed-inside-frame-expected.txt
>index 312272dbfdea90a25c9544e80aca09d0ffb08451..b8adf93adabff5af9f4cbebb939315d89ed899ce 100644
>--- a/LayoutTests/scrollingcoordinator/scrolling-tree/fixed-inside-frame-expected.txt
>+++ b/LayoutTests/scrollingcoordinator/scrolling-tree/fixed-inside-frame-expected.txt
>@@ -68,6 +68,7 @@
>                   (children 1
>                     (GraphicsLayer
>                       (position 0.00 -120.00)
>+                      (anchor 0.00 0.00)
>                       (children 1
>                         (GraphicsLayer
>                           (anchor 0.00 0.00)

Flags:

ews-watchlist: commit-queue-

Actions: View | Formatted Diff | Diff

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