Attachment 373066 Details for Bug 199297 – Patch

[patch] Patch

bug-199297-20190627161424.patch (text/plain), 6.75 KB, created by Simon Fraser (smfr) on 2019-06-27 16:14:25 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Simon Fraser (smfr)

Created: 2019-06-27 16:14:25 PDT

Size: 6.75 KB

patch

obsolete

>Subversion Revision: 246899
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 596824f4ada88cfc9c95aff5b4ee73979fb1481e..10f9cc66227392f7f477ab9fa69b4221e03f0f80 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,27 @@
>+2019-06-27  Simon Fraser  <simon.fraser@apple.com>
>+
>+        Fix crash in ScrollingStateNode::insertChild()
>+        https://bugs.webkit.org/show_bug.cgi?id=199297
>+        rdar://problem/49415136
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Crash data suggest that 'parent' can be deleted in ScrollingStateTree::insertNode(). To avoid this,
>+        have ScrollingStateTree::m_stateNodeMap store RefPts, and do the same for ScrollingTree::m_nodeMap.
>+
>+        * page/scrolling/ScrollingStateNode.cpp:
>+        (WebCore::ScrollingStateNode::ScrollingStateNode): The relaxAdoptionRequirement() is required
>+        to avoid ASSERT(!m_adoptionIsRequired) when the node is added to the tree in its constructor.
>+        * page/scrolling/ScrollingStateTree.cpp:
>+        (WebCore::ScrollingStateTree::unparentNode):
>+        (WebCore::ScrollingStateTree::unparentChildrenAndDestroyNode):
>+        (WebCore::ScrollingStateTree::detachAndDestroySubtree):
>+        (WebCore::ScrollingStateTree::stateNodeForID const):
>+        * page/scrolling/ScrollingStateTree.h:
>+        * page/scrolling/ScrollingTree.cpp:
>+        (WebCore::ScrollingTree::updateTreeFromStateNode):
>+        * page/scrolling/ScrollingTree.h:
>+
> 2019-06-27  Simon Fraser  <simon.fraser@apple.com>
> 
>         REGRESSION (r246869): ASSERTION FAILED: !renderer().hasRepaintLayoutRects() || renderer().repaintLayoutRects().m_repaintRect == renderer().clippedOverflowRectForRepaint(renderer().containerForRepaint())
>diff --git a/Source/WebCore/page/scrolling/ScrollingStateNode.cpp b/Source/WebCore/page/scrolling/ScrollingStateNode.cpp
>index 882d733f06d51cbe91654d37da768c777fce5d71..2e6d4d3f82b9f247e3453384501a8abe65b91c06 100644
>--- a/Source/WebCore/page/scrolling/ScrollingStateNode.cpp
>+++ b/Source/WebCore/page/scrolling/ScrollingStateNode.cpp
>@@ -53,6 +53,8 @@ ScrollingStateNode::ScrollingStateNode(const ScrollingStateNode& stateNode, Scro
> {
>     if (hasChangedProperty(Layer))
>         setLayer(stateNode.layer().toRepresentation(adoptiveTree.preferredLayerRepresentation()));
>+
>+    relaxAdoptionRequirement();
>     scrollingStateTree().addNode(*this);
> }
> 
>diff --git a/Source/WebCore/page/scrolling/ScrollingStateTree.cpp b/Source/WebCore/page/scrolling/ScrollingStateTree.cpp
>index 8a96bcbaf1ef868f1697f24a64fdfcf099309c20..f61e28e0ec503012b372fb25569a8cb7dd53aa70 100644
>--- a/Source/WebCore/page/scrolling/ScrollingStateTree.cpp
>+++ b/Source/WebCore/page/scrolling/ScrollingStateTree.cpp
>@@ -209,7 +209,7 @@ void ScrollingStateTree::unparentNode(ScrollingNodeID nodeID)
>     LOG_WITH_STREAM(Scrolling, stream << "ScrollingStateTree " << this << " unparentNode " << nodeID);
> 
>     // The node may not be found if clear() was recently called.
>-    RefPtr<ScrollingStateNode> protectedNode = m_stateNodeMap.get(nodeID);
>+    auto protectedNode = m_stateNodeMap.get(nodeID);
>     if (!protectedNode)
>         return;
> 
>@@ -228,7 +228,7 @@ void ScrollingStateTree::unparentChildrenAndDestroyNode(ScrollingNodeID nodeID)
>     LOG_WITH_STREAM(Scrolling, stream << "ScrollingStateTree " << this << " unparentChildrenAndDestroyNode " << nodeID);
> 
>     // The node may not be found if clear() was recently called.
>-    RefPtr<ScrollingStateNode> protectedNode = m_stateNodeMap.take(nodeID);
>+    auto protectedNode = m_stateNodeMap.take(nodeID);
>     if (!protectedNode)
>         return;
> 
>@@ -256,13 +256,13 @@ void ScrollingStateTree::detachAndDestroySubtree(ScrollingNodeID nodeID)
>     LOG_WITH_STREAM(Scrolling, stream << "ScrollingStateTree " << this << " detachAndDestroySubtree " << nodeID);
> 
>     // The node may not be found if clear() was recently called.
>-    auto* node = m_stateNodeMap.take(nodeID);
>+    auto node = m_stateNodeMap.take(nodeID);
>     if (!node)
>         return;
> 
>     // If the node was unparented, remove it from m_unparentedNodes (keeping it alive until this function returns).
>     auto unparentedNode = m_unparentedNodes.take(nodeID);
>-    removeNodeAndAllDescendants(node);
>+    removeNodeAndAllDescendants(node.get());
> }
> 
> void ScrollingStateTree::clear()
>@@ -365,7 +365,7 @@ ScrollingStateNode* ScrollingStateTree::stateNodeForID(ScrollingNodeID scrollLay
>         return nullptr;
> 
>     ASSERT(it->value->scrollingNodeID() == scrollLayerID);
>-    return it->value;
>+    return it->value.get();
> }
> 
> void ScrollingStateTree::reconcileLayerPositionsRecursive(ScrollingStateNode& currNode, const LayoutRect& layoutViewport, ScrollingLayerPositionAction action)
>diff --git a/Source/WebCore/page/scrolling/ScrollingStateTree.h b/Source/WebCore/page/scrolling/ScrollingStateTree.h
>index 24d8180f31250471371abee2ee1a1c4ec58264d1..626c90fdd68cf60b45d2f6f57e7aa8325c5745b5 100644
>--- a/Source/WebCore/page/scrolling/ScrollingStateTree.h
>+++ b/Source/WebCore/page/scrolling/ScrollingStateTree.h
>@@ -69,7 +69,7 @@ public:
> 
>     unsigned nodeCount() const { return m_stateNodeMap.size(); }
> 
>-    typedef HashMap<ScrollingNodeID, ScrollingStateNode*> StateNodeMap;
>+    typedef HashMap<ScrollingNodeID, RefPtr<ScrollingStateNode>> StateNodeMap;
>     const StateNodeMap& nodeMap() const { return m_stateNodeMap; }
> 
>     LayerRepresentation::Type preferredLayerRepresentation() const { return m_preferredLayerRepresentation; }
>diff --git a/Source/WebCore/page/scrolling/ScrollingTree.cpp b/Source/WebCore/page/scrolling/ScrollingTree.cpp
>index 10e380d30668745f56d742c6ad4b62456e887009..5136462b9334f59e3d7963bd0d13f87d99d81536 100644
>--- a/Source/WebCore/page/scrolling/ScrollingTree.cpp
>+++ b/Source/WebCore/page/scrolling/ScrollingTree.cpp
>@@ -218,7 +218,7 @@ void ScrollingTree::updateTreeFromStateNode(const ScrollingStateNode* stateNode,
>         auto parentIt = m_nodeMap.find(parentNodeID);
>         ASSERT_WITH_SECURITY_IMPLICATION(parentIt != m_nodeMap.end());
>         if (parentIt != m_nodeMap.end()) {
>-            auto* parent = parentIt->value;
>+            auto* parent = parentIt->value.get();
> 
>             auto* oldParent = node->parent();
>             if (oldParent)
>diff --git a/Source/WebCore/page/scrolling/ScrollingTree.h b/Source/WebCore/page/scrolling/ScrollingTree.h
>index b65b660eb8d41e1d3f7362cf897962c2fcf5d823..30117fb14bd993e86573e448a487be5dc2b72f9a 100644
>--- a/Source/WebCore/page/scrolling/ScrollingTree.h
>+++ b/Source/WebCore/page/scrolling/ScrollingTree.h
>@@ -175,7 +175,7 @@ private:
> 
>     RefPtr<ScrollingTreeFrameScrollingNode> m_rootNode;
> 
>-    using ScrollingTreeNodeMap = HashMap<ScrollingNodeID, ScrollingTreeNode*>;
>+    using ScrollingTreeNodeMap = HashMap<ScrollingNodeID, RefPtr<ScrollingTreeNode>>;
>     ScrollingTreeNodeMap m_nodeMap;
> 
>     RelatedNodesMap m_overflowRelatedNodesMap;

Actions: View | Formatted Diff | Diff

Attachments on bug 199297: 373066