[Webkit-unassigned] [Bug 43166] InsertOrderedList does not switch the list type properly when it has an inner list.

[bugzilla-daemon at webkit.org]

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


Darin Adler <darin at apple.com> changed:

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




--- Comment #6 from Darin Adler <darin at apple.com>  2010-07-29 16:30:13 PST ---
(From update of attachment 63000)
> +PassRefPtr<HTMLElement> InsertListCommand::mergeWithNeighboringLists(HTMLElement* listElement)
> +{
> +    Element* previousList = listElement->previousElementSibling();
> +    Element* nextList = listElement->nextElementSibling();
> +    if (canMergeLists(previousList, listElement))
> +        mergeIdenticalElements(previousList, listElement);
> +    if (canMergeLists(listElement, nextList)) {
> +        mergeIdenticalElements(listElement, nextList);
> +        return static_cast<HTMLElement*>(nextList);
> +    }
> +    return listElement;
> +}

Thanks for changing the function result into a PassRefPtr.

If DOM mutation events can run while we make changes, then this code is unsafe. After either of the two calls to mergeIdenticalElements returns, listElement or nextList might be pointing to a deleted object. Thus, they should both be RefPtr or PassRefPtr.

Calling nextElementSibling after handling previousList would be another way of avoiding using a possibly-bad nextList pointer, but it wouldn't help you safely return nextList after calling mergeIdenticalElements, so I think you need a RefPtr.

The patch otherwise looks great.

review- because I'd like you to use RefPtr more to avoid potential use-after-free as explained above

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