Bug 99211

Summary: When ScrollingStateNodes are destroyed, they should be removed ScrollingCoordinator's HashMap
Product: WebKit Reporter: Beth Dakin <bdakin>
Component: Layout and RenderingAssignee: Beth Dakin <bdakin>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, bdakin, jamesr, tonikitoo, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch sam: review+

Description Beth Dakin 2012-10-12 14:44:53 PDT
There is a FIXME in ScrollingCoordinatorMac::detachFromStateTree() that says:

    // FIXME: removeNode() will destroy children, and those children might still be in the HashMap.
    // This will be a big problem once there are actually children in the tree.

This bugs is to track resolving that FIXME.
Comment 1 Beth Dakin 2012-10-12 14:51:52 PDT
Created attachment 168489 [details]
Patch
Comment 2 Brady Eidson 2012-10-12 16:25:00 PDT
Comment on attachment 168489 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=168489&action=review

> Source/WebCore/page/scrolling/ScrollingStateTree.cpp:58
> +    // Copy and clear the array of node IDs representing nodes that have been removed since the last
> +    // time we committed the tree.
> +    size_t size = m_nodesRemovedSinceLastCommit->size();
> +    for (size_t i = 0; i < size; ++i)
> +        treeState->didRemoveNode(m_nodesRemovedSinceLastCommit->at(i));
> +    m_nodesRemovedSinceLastCommit->clear();

I don't see where the copy is that the comment alludes to.

I might be missing something here:  doesn't didRemoveNode() just call back in to the same m_nodesRemovedSinceLastCommit?
Comment 3 Beth Dakin 2012-10-12 16:49:28 PDT
Created attachment 168513 [details]
Patch
Comment 4 Beth Dakin 2012-10-12 16:52:01 PDT
Thanks Sam and Brady! http://trac.webkit.org/changeset/131238