WebKit Bugzilla
Attachment 360472 Details for
Bug 193907
: REGRESSION(r240553): [iOS] Crash in ScrollingTree::updateTreeFromStateNode when attempting to log in to icloud.com
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
Remember
[x]
|
Forgot Password
Login:
[x]
[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:
Patch
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)
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Flags:
ews-watchlist
:
commit-queue-
Actions:
View
|
Formatted Diff
|
Diff
Attachments on
bug 193907
:
360431
|
360456
| 360472 |
360481
|
360483
|
360486
|
360488