[webkit-reviews] review granted: [Bug 34182] Crash in WebKit!WebCore::RenderMenuList::itemStyle : [Attachment 47445] Patch

Tue Jan 26 14:19:57 PST 2010

Jon Honeycutt <jhoneycutt at apple.com> has granted Brian Weinstein
<bweinstein at apple.com>'s request for review:
Bug 34182: Crash in WebKit!WebCore::RenderMenuList::itemStyle
https://bugs.webkit.org/show_bug.cgi?id=34182

Attachment 47445: Patch
https://bugs.webkit.org/attachment.cgi?id=47445&action=review

------- Additional Comments from Jon Honeycutt <jhoneycutt at apple.com>
> Index: WebCore/manual-tests/select-delete-item.html
> ===================================================================
> --- WebCore/manual-tests/select-delete-item.html	(revision 0)
> +++ WebCore/manual-tests/select-delete-item.html	(revision 0)
> @@ -0,0 +1,21 @@
> +<html>
> +<head>
> +    <title>RenderMenuList::itemStyle Select Element Crash</title>
> +    <script>
> +	   function removeItem() {
> +	       var select = document.getElementById("dropDown");
> +	       select.removeChild(document.getElementsByTagName("option")[2]);
> +	   }
> +    </script>
> +</head>
> +<body>
> +    <select id="dropDown" onfocus="setTimeout('removeItem();', 2000);">
> +	   <option>Option 1</option>
> +	   <option>Option 2</option>
> +	   <option>Option 3</option>
> +    </select>
> +    <p>This is a test for bug <a href="http://webkit.org/b/34182">34182</a>
Crash in WebKit!WebCore::RenderMenuList::itemStyle.
> +    Once the select gets focus, in 2 seconds it will delete an item. This
test passes
> +    if you have the select open when it deletes an item, and doesn't
crash.</p>
> +</body>
> +</html>

You should be able to add an automated test using the AccessibilityController
to show the menu:

document.getElementById("dropDown").focus();
accessibilityController.focusedUIElement.showMenu();

> Index: WebCore/rendering/RenderMenuList.cpp
> ===================================================================
> --- WebCore/rendering/RenderMenuList.cpp	(revision 53736)
> +++ WebCore/rendering/RenderMenuList.cpp	(working copy)
> @@ -359,7 +368,18 @@ bool RenderMenuList::itemIsEnabled(unsig
>  PopupMenuStyle RenderMenuList::itemStyle(unsigned listIndex) const
>  {
>      SelectElement* select = toSelectElement(static_cast<Element*>(node()));
> -    Element* element = select->listItems()[listIndex];
> +    const Vector<Element*>& listItems = select->listItems();
> +    if (listIndex >= listItems.size()) {
> +	   // If we are making an out of bounds access, then we want to use the
style
> +	   // of the option element before us. However, if there isn't an
option element
> +	   // before us, we fall back to the default menu style.
> +	   if (!listIndex)
> +	       return menuStyle();
> +
> +	   // Try to retrieve the style of the previous option element.
> +	   listIndex--;
> +    }

You should set listIndex to 0 as we discussed.

r=me