Changeset 131804 in webkit


Ignore:
Timestamp:
Oct 18, 2012, 2:25:35 PM (13 years ago)
Author:
Beth Dakin
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):

Location:
trunk/Source/WebCore
Files:
12 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebCore/ChangeLog

    r131796 r131804  
     12012-10-18  Beth Dakin  <bdakin@apple.com>
     2
     3        https://bugs.webkit.org/show_bug.cgi?id=99668
     4        REGRESSION: Crash in
     5        WebCore::ScrollingStateScrollingNode::setNonFastScrollableRegion
     6        -and corresponding-
     7        <rdar://problem/12491901>
     8
     9        Reviewed by Simon Fraser.
     10
     11        http://trac.webkit.org/changeset/130783 changed the lifetime of the
     12        ScrollingStateTree's rootStateNode. Before that patch, the root state
     13        node was never destroyed. It was just constantly re-used for
     14        different RenderLayerBackings. This crash is just one of a few bugs
     15        that has occurred because of that change. I have fixed the other bugs
     16        individually, but I think that long-term, it is the safest solution
     17        to go back to the original ownership model.
     18
     19        So this patch ensures that the state tree will always have a root
     20        state node. Instead of destroying and re-creating the root node when
     21        it's scroll ID changes, we just update the ID.
     22
     23        attachToStateTree() now takes an additional ID representing the ID of
     24        the parent node.
     25        * page/scrolling/ScrollingCoordinator.h:
     26        (WebCore::ScrollingCoordinator::attachToStateTree):
     27
     28        Add a way to set the scrolling node ID.
     29        * page/scrolling/ScrollingStateNode.h:
     30        (WebCore::ScrollingStateNode::setScrollingNodeID):
     31
     32        This code that provided a way to mark all properties as having
     33        changed was added in http://trac.webkit.org/changeset/130989 as a way
     34        to ensure we would re-set ScrollingThread's nodes when we destroyed
     35        and re-created the rootStateNode. Now that we are no longer
     36        destroying and re-creating the rootStateNode, this code is no longer
     37        necessary.
     38        * page/scrolling/ScrollingStateScrollingNode.cpp:
     39        * page/scrolling/ScrollingStateScrollingNode.h:
     40
     41        create m_rootStateNode right in the ScrollingStateTree's constructor.
     42        * page/scrolling/ScrollingStateTree.cpp:
     43        (WebCore::ScrollingStateTree::ScrollingStateTree):
     44
     45        Don't let removeNode() destroy m_rootStateNode.
     46        (WebCore::ScrollingStateTree::removeNode):
     47
     48        Also a part of r130989 that is no longer needed.
     49        (WebCore::ScrollingStateTree::rootLayerDidChange():
     50        * page/scrolling/ScrollingStateTree.h:
     51        (WebCore::ScrollingStateTree::rootStateNode):
     52        (ScrollingStateTree):
     53        (WebCore::ScrollingStateTree::setRootStateNode):
     54
     55        attachToStateTree() now takes an additional ID representing the ID of
     56        the parent node.
     57        * page/scrolling/mac/ScrollingCoordinatorMac.h:
     58        (ScrollingCoordinatorMac):
     59
     60        We no longer need ScrollingStateTree::rootLayerDidChange()
     61        * page/scrolling/mac/ScrollingCoordinatorMac.mm:
     62        (WebCore::ScrollingCoordinatorMac::frameViewRootLayerDidChange):
     63
     64        Do not destroy and re-create the state node. Just update its ID. When
     65        we support child nodes soon, we will create them in this function.
     66        (WebCore::ScrollingCoordinatorMac::attachToStateTree):
     67
     68        No need to null-check the rootStateNode.
     69        (WebCore::ScrollingCoordinatorMac::clearStateTree):
     70
     71        Send 0 as the parent node ID to attachToStateTree() to represent the
     72        root node.
     73        (WebCore::ScrollingCoordinatorMac::ensureRootStateNodeForFrameView):
     74        * rendering/RenderLayerBacking.cpp:
     75
     76        RenderLayerBacking::attachToScrollingCoordinator() now takes a parent
     77        layer.
     78        (WebCore::RenderLayerBacking::attachToScrollingCoordinator):
     79        * rendering/RenderLayerBacking.h:
     80        (RenderLayerBacking):
     81
     82        Since this is the root, send 0 to represent the parent layer.
     83        * rendering/RenderLayerCompositor.cpp:
     84        (WebCore::RenderLayerCompositor::updateBacking):
     85
    1862012-10-18  Yael Aharon  <yael.aharon@intel.com>
    287

  • trunk/Source/WebCore/page/scrolling/ScrollingCoordinator.h

    r131502 r131804  
    110110    virtual bool handleWheelEvent(FrameView*, const PlatformWheelEvent&) { return true; }
    111111    virtual void updateMainFrameScrollPositionAndScrollLayerPosition() { }
    112     virtual ScrollingNodeID attachToStateTree(ScrollingNodeID nodeID) { return nodeID; }
     112    virtual ScrollingNodeID attachToStateTree(ScrollingNodeID newNodeID, ScrollingNodeID /*parentID*/) { return newNodeID; }
    113113    virtual void detachFromStateTree(ScrollingNodeID) { }
    114114    virtual void clearStateTree() { }

  • trunk/Source/WebCore/page/scrolling/ScrollingStateNode.h

    r131502 r131804  
    7272
    7373    ScrollingNodeID scrollingNodeID() const { return m_nodeID; }
     74    void setScrollingNodeID(ScrollingNodeID nodeID) { m_nodeID = nodeID; }
    7475
    7576    ScrollingStateNode* parent() const { return m_parent; }

  • trunk/Source/WebCore/page/scrolling/ScrollingStateScrollingNode.cpp

    r131221 r131804  
    7878}
    7979
    80 void ScrollingStateScrollingNode::setHasChangedProperties()
    81 {
    82     m_changedProperties = All;
    83     ScrollingStateNode::setHasChangedProperties();
    84 }
    85 
    8680PassOwnPtr<ScrollingStateNode> ScrollingStateScrollingNode::cloneAndResetNode()
    8781{

  • trunk/Source/WebCore/page/scrolling/ScrollingStateScrollingNode.h

    r131221 r131804  
    5757        ScrollOrigin = 1 << 11,
    5858        RequestedScrollPosition = 1 << 12,
    59         All = (1 << 13) - 1 // This will need to be updated if we add or remove anything the ChangedProperties.
    6059    };
    6160
    … …  
    6766    virtual unsigned changedProperties() const OVERRIDE { return m_changedProperties; }
    6867    virtual void resetChangedProperties() OVERRIDE { m_changedProperties = 0; }
    69     virtual void setHasChangedProperties();
    7068
    7169    const IntRect& viewportRect() const { return m_viewportRect; }

  • trunk/Source/WebCore/page/scrolling/ScrollingStateTree.cpp

    r131238 r131804  
    3737
    3838ScrollingStateTree::ScrollingStateTree()
    39     : m_hasChangedProperties(false)
     39    : m_rootStateNode(ScrollingStateScrollingNode::create(this, 0))
     40    , m_hasChangedProperties(false)
    4041{
    4142}
    … …  
    6465{
    6566    ASSERT(m_rootStateNode);
    66 
    67     if (node == m_rootStateNode) {
    68         didRemoveNode(m_rootStateNode->scrollingNodeID());
    69         m_rootStateNode = 0;
    70         return;
    71     }
    72 
    7367    m_rootStateNode->removeChild(node);
    7468}
    … …  
    7973}
    8074
    81 void ScrollingStateTree::rootLayerDidChange()
    82 {
    83     // If the root layer has changed, then destroyed and re-created the root state node. That means that the
    84     // cached properties in ScrollingStateScrollingNode are no longer reflective of the properties we have
    85     // cached over in the ScrollingTree. To resolve this, we will mark all of the properties as having changed
    86     // so that the ScrollingTree will be in synch with the state tree.
    87     setHasChangedProperties(true);
    88     rootStateNode()->setHasChangedProperties();
    89 }
    90 
    9175} // namespace WebCore
    9276

  • trunk/Source/WebCore/page/scrolling/ScrollingStateTree.h

    r131238 r131804  
    5656
    5757    ScrollingStateScrollingNode* rootStateNode() const { return m_rootStateNode.get(); }
    58     void setRootStateNode(PassOwnPtr<ScrollingStateScrollingNode> rootStateNode) { m_rootStateNode = rootStateNode; }
    5958
    6059    // Copies the current tree state and clears the changed properties mask in the original.
    … …  
    6564    const Vector<ScrollingNodeID>& removedNodes() const { return m_nodesRemovedSinceLastCommit; }
    6665
    67     void rootLayerDidChange();
    68 
    6966    void setHasChangedProperties(bool changedProperties) { m_hasChangedProperties = changedProperties; }
    7067    bool hasChangedProperties() const { return m_hasChangedProperties; }
    … …  
    7269private:
    7370    ScrollingStateTree();
     71
     72    void setRootStateNode(PassOwnPtr<ScrollingStateScrollingNode> rootStateNode) { m_rootStateNode = rootStateNode; }
    7473
    7574    PassOwnPtr<ScrollingStateTree> clone();

  • trunk/Source/WebCore/page/scrolling/mac/ScrollingCoordinatorMac.h

    r131137 r131804  
    6767    // These functions are used to indicate that a layer should be (or should not longer be) represented by a node
    6868    // in the scrolling tree.
    69     virtual ScrollingNodeID attachToStateTree(ScrollingNodeID);
     69    virtual ScrollingNodeID attachToStateTree(ScrollingNodeID newNodeID, ScrollingNodeID parentID);
    7070    virtual void detachFromStateTree(ScrollingNodeID);
    7171

  • trunk/Source/WebCore/page/scrolling/mac/ScrollingCoordinatorMac.mm

    r131336 r131804  
    148148    ensureRootStateNodeForFrameView(frameView);
    149149    ASSERT(m_scrollingStateTree->rootStateNode());
    150     m_scrollingStateTree->rootLayerDidChange();
    151150
    152151    ScrollingCoordinator::frameViewRootLayerDidChange(frameView);
    … …  
    235234}
    236235
    237 ScrollingNodeID ScrollingCoordinatorMac::attachToStateTree(ScrollingNodeID scrollLayerID)
    238 {
    239     ASSERT(scrollLayerID);
    240 
    241     ScrollingStateScrollingNode* existingNode = stateNodeForID(scrollLayerID);
     236ScrollingNodeID ScrollingCoordinatorMac::attachToStateTree(ScrollingNodeID newNodeID, ScrollingNodeID parentID)
     237{
     238    ASSERT(newNodeID);
     239
     240    ScrollingStateScrollingNode* existingNode = stateNodeForID(newNodeID);
    242241    if (existingNode && existingNode == m_scrollingStateTree->rootStateNode())
    243         return scrollLayerID;
    244 
    245     clearStateTree();
    246 
    247     // FIXME: In the future, this function will have to take a parent ID so that it can
    248     // append the node in the appropriate spot in the state tree. For now we always assume
    249     // this is the root node.
    250     m_scrollingStateTree->setRootStateNode(ScrollingStateScrollingNode::create(m_scrollingStateTree.get(), scrollLayerID));
    251     m_stateNodeMap.set(scrollLayerID, m_scrollingStateTree->rootStateNode());
    252     return scrollLayerID;
     242        return newNodeID;
     243
     244    // If there is no parent, this is the root node. Right now, we only support the root node.
     245    // FIXME: In the future, we should append child nodes in the appropriate spot in the state
     246    // tree.
     247    if (!parentID) {
     248        // If we're resetting the root node, we should clear the HashMap and destroy the current children.
     249        clearStateTree();
     250
     251        m_scrollingStateTree->rootStateNode()->setScrollingNodeID(newNodeID);
     252        m_stateNodeMap.set(newNodeID, m_scrollingStateTree->rootStateNode());
     253    }
     254
     255    return newNodeID;
    253256}
    254257
    … …  
    276279{
    277280    m_stateNodeMap.clear();
    278     if (ScrollingStateScrollingNode* node = m_scrollingStateTree->rootStateNode())
    279         m_scrollingStateTree->removeNode(node);
     281    m_scrollingStateTree->removeNode(m_scrollingStateTree->rootStateNode());
    280282}
    281283
    … …  
    295297void ScrollingCoordinatorMac::ensureRootStateNodeForFrameView(FrameView* frameView)
    296298{
    297     attachToStateTree(frameView->scrollLayerID());
     299    attachToStateTree(frameView->scrollLayerID(), 0);
    298300}
    299301

  • trunk/Source/WebCore/rendering/RenderLayerBacking.cpp

    r131680 r131804  
    966966}
    967967
    968 void RenderLayerBacking::attachToScrollingCoordinator()
     968void RenderLayerBacking::attachToScrollingCoordinator(RenderLayerBacking* parent)
    969969{
    970970    // If m_scrollLayerID non-zero, then this backing is already attached to the ScrollingCoordinator.
    … …  
    979979    if (!scrollingCoordinator)
    980980        return;
    981    
    982     m_scrollLayerID = scrollingCoordinator->attachToStateTree(scrollingCoordinator->uniqueScrollLayerID());
     981
     982    ScrollingNodeID parentID = parent ? parent->scrollLayerID() : 0;
     983    m_scrollLayerID = scrollingCoordinator->attachToStateTree(scrollingCoordinator->uniqueScrollLayerID(), parentID);
    983984}
    984985

  • trunk/Source/WebCore/rendering/RenderLayerBacking.h

    r131680 r131804  
    8989    GraphicsLayer* scrollingContentsLayer() const { return m_scrollingContentsLayer.get(); }
    9090
    91     void attachToScrollingCoordinator();
     91    void attachToScrollingCoordinator(RenderLayerBacking* parent);
    9292    uint64_t scrollLayerID() const { return m_scrollLayerID; }
    9393   

  • trunk/Source/WebCore/rendering/RenderLayerCompositor.cpp

    r131680 r131804  
    511511            // At this time, the ScrollingCooridnator only supports the top-level frame.
    512512            if (layer->isRootLayer() && !m_renderView->document()->ownerElement()) {
    513                 layer->backing()->attachToScrollingCoordinator();
     513                layer->backing()->attachToScrollingCoordinator(0);
    514514                if (ScrollingCoordinator* scrollingCoordinator = this->scrollingCoordinator())
    515515                    scrollingCoordinator->frameViewRootLayerDidChange(m_renderView->frameView());
Note: See TracChangeset for help on using the changeset viewer.