- Revision
- 131804
- Author
- bda...@apple.com
- Date
- 2012-10-18 14:25:35 -0700 (Thu, 18 Oct 2012)
Log Message
https://bugs.webkit.org/show_bug.cgi?id=99668
REGRESSION: Crash in
WebCore::ScrollingStateScrollingNode::setNonFastScrollableRegion
-and corresponding-
<rdar://problem/12491901>
Reviewed by Simon Fraser.
http://trac.webkit.org/changeset/130783 changed the lifetime of the
ScrollingStateTree's rootStateNode. Before that patch, the root state
node was never destroyed. It was just constantly re-used for
different RenderLayerBackings. This crash is just one of a few bugs
that has occurred because of that change. I have fixed the other bugs
individually, but I think that long-term, it is the safest solution
to go back to the original ownership model.
So this patch ensures that the state tree will always have a root
state node. Instead of destroying and re-creating the root node when
it's scroll ID changes, we just update the ID.
attachToStateTree() now takes an additional ID representing the ID of
the parent node.
* page/scrolling/ScrollingCoordinator.h:
(WebCore::ScrollingCoordinator::attachToStateTree):
Add a way to set the scrolling node ID.
* page/scrolling/ScrollingStateNode.h:
(WebCore::ScrollingStateNode::setScrollingNodeID):
This code that provided a way to mark all properties as having
changed was added in http://trac.webkit.org/changeset/130989 as a way
to ensure we would re-set ScrollingThread's nodes when we destroyed
and re-created the rootStateNode. Now that we are no longer
destroying and re-creating the rootStateNode, this code is no longer
necessary.
* page/scrolling/ScrollingStateScrollingNode.cpp:
* page/scrolling/ScrollingStateScrollingNode.h:
create m_rootStateNode right in the ScrollingStateTree's constructor.
* page/scrolling/ScrollingStateTree.cpp:
(WebCore::ScrollingStateTree::ScrollingStateTree):
Don't let removeNode() destroy m_rootStateNode.
(WebCore::ScrollingStateTree::removeNode):
Also a part of r130989 that is no longer needed.
(WebCore::ScrollingStateTree::rootLayerDidChange():
* page/scrolling/ScrollingStateTree.h:
(WebCore::ScrollingStateTree::rootStateNode):
(ScrollingStateTree):
(WebCore::ScrollingStateTree::setRootStateNode):
attachToStateTree() now takes an additional ID representing the ID of
the parent node.
* page/scrolling/mac/ScrollingCoordinatorMac.h:
(ScrollingCoordinatorMac):
We no longer need ScrollingStateTree::rootLayerDidChange()
* page/scrolling/mac/ScrollingCoordinatorMac.mm:
(WebCore::ScrollingCoordinatorMac::frameViewRootLayerDidChange):
Do not destroy and re-create the state node. Just update its ID. When
we support child nodes soon, we will create them in this function.
(WebCore::ScrollingCoordinatorMac::attachToStateTree):
No need to null-check the rootStateNode.
(WebCore::ScrollingCoordinatorMac::clearStateTree):
Send 0 as the parent node ID to attachToStateTree() to represent the
root node.
(WebCore::ScrollingCoordinatorMac::ensureRootStateNodeForFrameView):
* rendering/RenderLayerBacking.cpp:
RenderLayerBacking::attachToScrollingCoordinator() now takes a parent
layer.
(WebCore::RenderLayerBacking::attachToScrollingCoordinator):
* rendering/RenderLayerBacking.h:
(RenderLayerBacking):
Since this is the root, send 0 to represent the parent layer.
* rendering/RenderLayerCompositor.cpp:
(WebCore::RenderLayerCompositor::updateBacking):
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (131803 => 131804)
--- trunk/Source/WebCore/ChangeLog 2012-10-18 21:21:41 UTC (rev 131803)
+++ trunk/Source/WebCore/ChangeLog 2012-10-18 21:25:35 UTC (rev 131804)
@@ -1,3 +1,88 @@
+2012-10-18 Beth Dakin <bda...@apple.com>
+
+ https://bugs.webkit.org/show_bug.cgi?id=99668
+ REGRESSION: Crash in
+ WebCore::ScrollingStateScrollingNode::setNonFastScrollableRegion
+ -and corresponding-
+ <rdar://problem/12491901>
+
+ Reviewed by Simon Fraser.
+
+ http://trac.webkit.org/changeset/130783 changed the lifetime of the
+ ScrollingStateTree's rootStateNode. Before that patch, the root state
+ node was never destroyed. It was just constantly re-used for
+ different RenderLayerBackings. This crash is just one of a few bugs
+ that has occurred because of that change. I have fixed the other bugs
+ individually, but I think that long-term, it is the safest solution
+ to go back to the original ownership model.
+
+ So this patch ensures that the state tree will always have a root
+ state node. Instead of destroying and re-creating the root node when
+ it's scroll ID changes, we just update the ID.
+
+ attachToStateTree() now takes an additional ID representing the ID of
+ the parent node.
+ * page/scrolling/ScrollingCoordinator.h:
+ (WebCore::ScrollingCoordinator::attachToStateTree):
+
+ Add a way to set the scrolling node ID.
+ * page/scrolling/ScrollingStateNode.h:
+ (WebCore::ScrollingStateNode::setScrollingNodeID):
+
+ This code that provided a way to mark all properties as having
+ changed was added in http://trac.webkit.org/changeset/130989 as a way
+ to ensure we would re-set ScrollingThread's nodes when we destroyed
+ and re-created the rootStateNode. Now that we are no longer
+ destroying and re-creating the rootStateNode, this code is no longer
+ necessary.
+ * page/scrolling/ScrollingStateScrollingNode.cpp:
+ * page/scrolling/ScrollingStateScrollingNode.h:
+
+ create m_rootStateNode right in the ScrollingStateTree's constructor.
+ * page/scrolling/ScrollingStateTree.cpp:
+ (WebCore::ScrollingStateTree::ScrollingStateTree):
+
+ Don't let removeNode() destroy m_rootStateNode.
+ (WebCore::ScrollingStateTree::removeNode):
+
+ Also a part of r130989 that is no longer needed.
+ (WebCore::ScrollingStateTree::rootLayerDidChange():
+ * page/scrolling/ScrollingStateTree.h:
+ (WebCore::ScrollingStateTree::rootStateNode):
+ (ScrollingStateTree):
+ (WebCore::ScrollingStateTree::setRootStateNode):
+
+ attachToStateTree() now takes an additional ID representing the ID of
+ the parent node.
+ * page/scrolling/mac/ScrollingCoordinatorMac.h:
+ (ScrollingCoordinatorMac):
+
+ We no longer need ScrollingStateTree::rootLayerDidChange()
+ * page/scrolling/mac/ScrollingCoordinatorMac.mm:
+ (WebCore::ScrollingCoordinatorMac::frameViewRootLayerDidChange):
+
+ Do not destroy and re-create the state node. Just update its ID. When
+ we support child nodes soon, we will create them in this function.
+ (WebCore::ScrollingCoordinatorMac::attachToStateTree):
+
+ No need to null-check the rootStateNode.
+ (WebCore::ScrollingCoordinatorMac::clearStateTree):
+
+ Send 0 as the parent node ID to attachToStateTree() to represent the
+ root node.
+ (WebCore::ScrollingCoordinatorMac::ensureRootStateNodeForFrameView):
+ * rendering/RenderLayerBacking.cpp:
+
+ RenderLayerBacking::attachToScrollingCoordinator() now takes a parent
+ layer.
+ (WebCore::RenderLayerBacking::attachToScrollingCoordinator):
+ * rendering/RenderLayerBacking.h:
+ (RenderLayerBacking):
+
+ Since this is the root, send 0 to represent the parent layer.
+ * rendering/RenderLayerCompositor.cpp:
+ (WebCore::RenderLayerCompositor::updateBacking):
+
2012-10-18 Yael Aharon <yael.aha...@intel.com>
[EFL] GraphicsContext3D::m_renderStyle is not initialized
Modified: trunk/Source/WebCore/page/scrolling/ScrollingCoordinator.h (131803 => 131804)
--- trunk/Source/WebCore/page/scrolling/ScrollingCoordinator.h 2012-10-18 21:21:41 UTC (rev 131803)
+++ trunk/Source/WebCore/page/scrolling/ScrollingCoordinator.h 2012-10-18 21:25:35 UTC (rev 131804)
@@ -109,7 +109,7 @@
virtual bool requestScrollPositionUpdate(FrameView*, const IntPoint&) { return false; }
virtual bool handleWheelEvent(FrameView*, const PlatformWheelEvent&) { return true; }
virtual void updateMainFrameScrollPositionAndScrollLayerPosition() { }
- virtual ScrollingNodeID attachToStateTree(ScrollingNodeID nodeID) { return nodeID; }
+ virtual ScrollingNodeID attachToStateTree(ScrollingNodeID newNodeID, ScrollingNodeID /*parentID*/) { return newNodeID; }
virtual void detachFromStateTree(ScrollingNodeID) { }
virtual void clearStateTree() { }
Modified: trunk/Source/WebCore/page/scrolling/ScrollingStateNode.h (131803 => 131804)
--- trunk/Source/WebCore/page/scrolling/ScrollingStateNode.h 2012-10-18 21:21:41 UTC (rev 131803)
+++ trunk/Source/WebCore/page/scrolling/ScrollingStateNode.h 2012-10-18 21:25:35 UTC (rev 131804)
@@ -71,6 +71,7 @@
void setScrollingStateTree(ScrollingStateTree* tree) { m_scrollingStateTree = tree; }
ScrollingNodeID scrollingNodeID() const { return m_nodeID; }
+ void setScrollingNodeID(ScrollingNodeID nodeID) { m_nodeID = nodeID; }
ScrollingStateNode* parent() const { return m_parent; }
void setParent(ScrollingStateNode* parent) { m_parent = parent; }
Modified: trunk/Source/WebCore/page/scrolling/ScrollingStateScrollingNode.cpp (131803 => 131804)
--- trunk/Source/WebCore/page/scrolling/ScrollingStateScrollingNode.cpp 2012-10-18 21:21:41 UTC (rev 131803)
+++ trunk/Source/WebCore/page/scrolling/ScrollingStateScrollingNode.cpp 2012-10-18 21:25:35 UTC (rev 131804)
@@ -77,12 +77,6 @@
{
}
-void ScrollingStateScrollingNode::setHasChangedProperties()
-{
- m_changedProperties = All;
- ScrollingStateNode::setHasChangedProperties();
-}
-
PassOwnPtr<ScrollingStateNode> ScrollingStateScrollingNode::cloneAndResetNode()
{
OwnPtr<ScrollingStateScrollingNode> clone = adoptPtr(new ScrollingStateScrollingNode(this));
Modified: trunk/Source/WebCore/page/scrolling/ScrollingStateScrollingNode.h (131803 => 131804)
--- trunk/Source/WebCore/page/scrolling/ScrollingStateScrollingNode.h 2012-10-18 21:21:41 UTC (rev 131803)
+++ trunk/Source/WebCore/page/scrolling/ScrollingStateScrollingNode.h 2012-10-18 21:25:35 UTC (rev 131804)
@@ -56,7 +56,6 @@
VerticalScrollbarMode = 1 << 10,
ScrollOrigin = 1 << 11,
RequestedScrollPosition = 1 << 12,
- All = (1 << 13) - 1 // This will need to be updated if we add or remove anything the ChangedProperties.
};
virtual bool isScrollingStateScrollingNode() OVERRIDE { return true; }
@@ -66,7 +65,6 @@
virtual bool hasChangedProperties() const OVERRIDE { return m_changedProperties; }
virtual unsigned changedProperties() const OVERRIDE { return m_changedProperties; }
virtual void resetChangedProperties() OVERRIDE { m_changedProperties = 0; }
- virtual void setHasChangedProperties();
const IntRect& viewportRect() const { return m_viewportRect; }
void setViewportRect(const IntRect&);
Modified: trunk/Source/WebCore/page/scrolling/ScrollingStateTree.cpp (131803 => 131804)
--- trunk/Source/WebCore/page/scrolling/ScrollingStateTree.cpp 2012-10-18 21:21:41 UTC (rev 131803)
+++ trunk/Source/WebCore/page/scrolling/ScrollingStateTree.cpp 2012-10-18 21:25:35 UTC (rev 131804)
@@ -36,7 +36,8 @@
}
ScrollingStateTree::ScrollingStateTree()
- : m_hasChangedProperties(false)
+ : m_rootStateNode(ScrollingStateScrollingNode::create(this, 0))
+ , m_hasChangedProperties(false)
{
}
@@ -63,13 +64,6 @@
void ScrollingStateTree::removeNode(ScrollingStateNode* node)
{
ASSERT(m_rootStateNode);
-
- if (node == m_rootStateNode) {
- didRemoveNode(m_rootStateNode->scrollingNodeID());
- m_rootStateNode = 0;
- return;
- }
-
m_rootStateNode->removeChild(node);
}
@@ -78,16 +72,6 @@
m_nodesRemovedSinceLastCommit.append(nodeID);
}
-void ScrollingStateTree::rootLayerDidChange()
-{
- // If the root layer has changed, then destroyed and re-created the root state node. That means that the
- // cached properties in ScrollingStateScrollingNode are no longer reflective of the properties we have
- // cached over in the ScrollingTree. To resolve this, we will mark all of the properties as having changed
- // so that the ScrollingTree will be in synch with the state tree.
- setHasChangedProperties(true);
- rootStateNode()->setHasChangedProperties();
-}
-
} // namespace WebCore
#endif // ENABLE(THREADED_SCROLLING)
Modified: trunk/Source/WebCore/page/scrolling/ScrollingStateTree.h (131803 => 131804)
--- trunk/Source/WebCore/page/scrolling/ScrollingStateTree.h 2012-10-18 21:21:41 UTC (rev 131803)
+++ trunk/Source/WebCore/page/scrolling/ScrollingStateTree.h 2012-10-18 21:25:35 UTC (rev 131804)
@@ -55,7 +55,6 @@
~ScrollingStateTree();
ScrollingStateScrollingNode* rootStateNode() const { return m_rootStateNode.get(); }
- void setRootStateNode(PassOwnPtr<ScrollingStateScrollingNode> rootStateNode) { m_rootStateNode = rootStateNode; }
// Copies the current tree state and clears the changed properties mask in the original.
PassOwnPtr<ScrollingStateTree> commit();
@@ -64,14 +63,14 @@
void didRemoveNode(ScrollingNodeID);
const Vector<ScrollingNodeID>& removedNodes() const { return m_nodesRemovedSinceLastCommit; }
- void rootLayerDidChange();
-
void setHasChangedProperties(bool changedProperties) { m_hasChangedProperties = changedProperties; }
bool hasChangedProperties() const { return m_hasChangedProperties; }
private:
ScrollingStateTree();
+ void setRootStateNode(PassOwnPtr<ScrollingStateScrollingNode> rootStateNode) { m_rootStateNode = rootStateNode; }
+
PassOwnPtr<ScrollingStateTree> clone();
OwnPtr<ScrollingStateScrollingNode> m_rootStateNode;
Modified: trunk/Source/WebCore/page/scrolling/mac/ScrollingCoordinatorMac.h (131803 => 131804)
--- trunk/Source/WebCore/page/scrolling/mac/ScrollingCoordinatorMac.h 2012-10-18 21:21:41 UTC (rev 131803)
+++ trunk/Source/WebCore/page/scrolling/mac/ScrollingCoordinatorMac.h 2012-10-18 21:25:35 UTC (rev 131804)
@@ -66,7 +66,7 @@
// These functions are used to indicate that a layer should be (or should not longer be) represented by a node
// in the scrolling tree.
- virtual ScrollingNodeID attachToStateTree(ScrollingNodeID);
+ virtual ScrollingNodeID attachToStateTree(ScrollingNodeID newNodeID, ScrollingNodeID parentID);
virtual void detachFromStateTree(ScrollingNodeID);
// This function wipes out the current tree.
Modified: trunk/Source/WebCore/page/scrolling/mac/ScrollingCoordinatorMac.mm (131803 => 131804)
--- trunk/Source/WebCore/page/scrolling/mac/ScrollingCoordinatorMac.mm 2012-10-18 21:21:41 UTC (rev 131803)
+++ trunk/Source/WebCore/page/scrolling/mac/ScrollingCoordinatorMac.mm 2012-10-18 21:25:35 UTC (rev 131804)
@@ -147,7 +147,6 @@
// If the root layer does not have a ScrollingStateNode, then we should create one.
ensureRootStateNodeForFrameView(frameView);
ASSERT(m_scrollingStateTree->rootStateNode());
- m_scrollingStateTree->rootLayerDidChange();
ScrollingCoordinator::frameViewRootLayerDidChange(frameView);
@@ -234,22 +233,26 @@
scrollLayer->setPosition(-frameView->scrollPosition());
}
-ScrollingNodeID ScrollingCoordinatorMac::attachToStateTree(ScrollingNodeID scrollLayerID)
+ScrollingNodeID ScrollingCoordinatorMac::attachToStateTree(ScrollingNodeID newNodeID, ScrollingNodeID parentID)
{
- ASSERT(scrollLayerID);
+ ASSERT(newNodeID);
- ScrollingStateScrollingNode* existingNode = stateNodeForID(scrollLayerID);
+ ScrollingStateScrollingNode* existingNode = stateNodeForID(newNodeID);
if (existingNode && existingNode == m_scrollingStateTree->rootStateNode())
- return scrollLayerID;
+ return newNodeID;
- clearStateTree();
+ // If there is no parent, this is the root node. Right now, we only support the root node.
+ // FIXME: In the future, we should append child nodes in the appropriate spot in the state
+ // tree.
+ if (!parentID) {
+ // If we're resetting the root node, we should clear the HashMap and destroy the current children.
+ clearStateTree();
- // FIXME: In the future, this function will have to take a parent ID so that it can
- // append the node in the appropriate spot in the state tree. For now we always assume
- // this is the root node.
- m_scrollingStateTree->setRootStateNode(ScrollingStateScrollingNode::create(m_scrollingStateTree.get(), scrollLayerID));
- m_stateNodeMap.set(scrollLayerID, m_scrollingStateTree->rootStateNode());
- return scrollLayerID;
+ m_scrollingStateTree->rootStateNode()->setScrollingNodeID(newNodeID);
+ m_stateNodeMap.set(newNodeID, m_scrollingStateTree->rootStateNode());
+ }
+
+ return newNodeID;
}
void ScrollingCoordinatorMac::detachFromStateTree(ScrollingNodeID scrollLayerID)
@@ -275,8 +278,7 @@
void ScrollingCoordinatorMac::clearStateTree()
{
m_stateNodeMap.clear();
- if (ScrollingStateScrollingNode* node = m_scrollingStateTree->rootStateNode())
- m_scrollingStateTree->removeNode(node);
+ m_scrollingStateTree->removeNode(m_scrollingStateTree->rootStateNode());
}
ScrollingStateScrollingNode* ScrollingCoordinatorMac::stateNodeForID(ScrollingNodeID scrollLayerID)
@@ -294,7 +296,7 @@
void ScrollingCoordinatorMac::ensureRootStateNodeForFrameView(FrameView* frameView)
{
- attachToStateTree(frameView->scrollLayerID());
+ attachToStateTree(frameView->scrollLayerID(), 0);
}
void ScrollingCoordinatorMac::setScrollLayerForNode(GraphicsLayer* scrollLayer, ScrollingStateNode* node)
Modified: trunk/Source/WebCore/rendering/RenderLayerBacking.cpp (131803 => 131804)
--- trunk/Source/WebCore/rendering/RenderLayerBacking.cpp 2012-10-18 21:21:41 UTC (rev 131803)
+++ trunk/Source/WebCore/rendering/RenderLayerBacking.cpp 2012-10-18 21:25:35 UTC (rev 131804)
@@ -965,7 +965,7 @@
return layerChanged;
}
-void RenderLayerBacking::attachToScrollingCoordinator()
+void RenderLayerBacking::attachToScrollingCoordinator(RenderLayerBacking* parent)
{
// If m_scrollLayerID non-zero, then this backing is already attached to the ScrollingCoordinator.
if (m_scrollLayerID)
@@ -978,8 +978,9 @@
ScrollingCoordinator* scrollingCoordinator = page->scrollingCoordinator();
if (!scrollingCoordinator)
return;
-
- m_scrollLayerID = scrollingCoordinator->attachToStateTree(scrollingCoordinator->uniqueScrollLayerID());
+
+ ScrollingNodeID parentID = parent ? parent->scrollLayerID() : 0;
+ m_scrollLayerID = scrollingCoordinator->attachToStateTree(scrollingCoordinator->uniqueScrollLayerID(), parentID);
}
void RenderLayerBacking::detachFromScrollingCoordinator()
Modified: trunk/Source/WebCore/rendering/RenderLayerBacking.h (131803 => 131804)
--- trunk/Source/WebCore/rendering/RenderLayerBacking.h 2012-10-18 21:21:41 UTC (rev 131803)
+++ trunk/Source/WebCore/rendering/RenderLayerBacking.h 2012-10-18 21:25:35 UTC (rev 131804)
@@ -88,7 +88,7 @@
GraphicsLayer* scrollingLayer() const { return m_scrollingLayer.get(); }
GraphicsLayer* scrollingContentsLayer() const { return m_scrollingContentsLayer.get(); }
- void attachToScrollingCoordinator();
+ void attachToScrollingCoordinator(RenderLayerBacking* parent);
uint64_t scrollLayerID() const { return m_scrollLayerID; }
bool hasMaskLayer() const { return m_maskLayer != 0; }
Modified: trunk/Source/WebCore/rendering/RenderLayerCompositor.cpp (131803 => 131804)
--- trunk/Source/WebCore/rendering/RenderLayerCompositor.cpp 2012-10-18 21:21:41 UTC (rev 131803)
+++ trunk/Source/WebCore/rendering/RenderLayerCompositor.cpp 2012-10-18 21:25:35 UTC (rev 131804)
@@ -510,7 +510,7 @@
// At this time, the ScrollingCooridnator only supports the top-level frame.
if (layer->isRootLayer() && !m_renderView->document()->ownerElement()) {
- layer->backing()->attachToScrollingCoordinator();
+ layer->backing()->attachToScrollingCoordinator(0);
if (ScrollingCoordinator* scrollingCoordinator = this->scrollingCoordinator())
scrollingCoordinator->frameViewRootLayerDidChange(m_renderView->frameView());
}
