[Webkit-unassigned] [Bug 28697] WebKit crash on WebCore::Node::nodeIndex()

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=28697


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #44006|review?                     |review-
               Flag|                            |




--- Comment #24 from Darin Adler <darin at apple.com>  2009-11-30 16:06:35 PST ---
(From update of attachment 44006)
> -static ExceptionCode willRemoveChild(Node *child)
> +static void willRemoveChild(const RefPtr<Node>& child)

There's no reason to make this a const RefPtr<Node>& instead of a Node*. I
suggest you change it back.

> +static void willRemoveChildrenFromNode(const RefPtr<ContainerNode> node)

I think this should be called willRemoveChildren. I think the argument should
be a ContainerNode* and be named container.

> +    // FIXME: Adding new children from event handlers can cause an infinite loop here.
> +    for (RefPtr<Node> child = node->firstChild(); child; child = child->nextSibling()) {
> +        // fire removed from document mutation events.
> +        dispatchChildRemovalEvents(child.get());
> +
> +        if (child->attached())
> +            child->willRemove();
> +    }

Instead of keeping this possible bug alive, maybe we should just gather all the
children into a local stack buffer. You could do this:

    Vector<RefPtr<Node>, 32> children;
    for (Node* child = node->firstChild(); child; child = child->nextSibling())
        children.append(child);

And then use the children array for the events and such. But you could do that
in another patch I guess.

> +    ASSERT(!eventDispatchForbidden());
> +
>      RefPtr<Node> c = child;
>      RefPtr<Document> document = child->document();
>  
> -    // update auxiliary doc info (e.g. iterators) to note that node is being removed
> -    document->nodeWillBeRemoved(child);
> -
> -    document->incDOMTreeVersion();
> +    // document->incDOMTreeVersion() and document->nodeWillBeRemoved() should be
> +    // handled by the caller as this function may be called in a loop.

This comment would make sense in a change log entry, but does not make sense in
the code that remains. If you want to add a comment about calling
nodeWillBeRemoved and incDOMTreeVersion, that should probably go before the
function rather than in the middle of the implementation.

> +    for (HashSet<NodeIterator*>::const_iterator it = m_nodeIterators.begin(); it != nodeIteratorsEnd; ++it)
> +        for (Node* n = container->firstChild(); n; n = n->nextSibling())
> +            (*it)->nodeWillBeRemoved(n);

This needs braces.

review- because I think you should do at least one tweak

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.