Bug 34182

Summary: Crash in WebKit!WebCore::RenderMenuList::itemStyle
Product: WebKit Reporter: Brian Weinstein <bweinstein>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Attachments:
Description Flags
Patch jhoneycutt: review+, jhoneycutt: commit-queue-

Description Brian Weinstein 2010-01-26 13:41:55 PST
Crash in WebKit!WebCore::RenderMenuList::itemStyle
Comment 1 Brian Weinstein 2010-01-26 13:55:21 PST
Created attachment 47445 [details]
Patch
Comment 2 WebKit Review Bot 2010-01-26 14:00:47 PST
Attachment 47445 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
Traceback (most recent call last):
  File "WebKitTools/Scripts/check-webkit-style", line 94, in <module>
    main()
  File "WebKitTools/Scripts/check-webkit-style", line 87, in main
    style_checker.check_patch(patch)
  File "/mnt/git/webkit-style-queue/WebKitTools/Scripts/webkitpy/style/checker.py", line 932, in check_patch
    self.check_file(file_path, handle_patch_style_error)
  File "/mnt/git/webkit-style-queue/WebKitTools/Scripts/webkitpy/style/checker.py", line 897, in check_file
    process_file(processor, file_path, handle_style_error)
  File "/mnt/git/webkit-style-queue/WebKitTools/Scripts/webkitpy/style/checker.py", line 858, in _process_file
    'One or more unexpected \\r (^M) found;'
TypeError: handle_patch_style_error() takes exactly 4 arguments (5 given)


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Jon Honeycutt 2010-01-26 14:19:57 PST
Comment on attachment 47445 [details]
Patch

> 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
Comment 4 Brian Weinstein 2010-01-26 14:39:45 PST
(In reply to comment #3)
> You should be able to add an automated test using the AccessibilityController
> to show the menu:
> 
> document.getElementById("dropDown").focus();
> accessibilityController.focusedUIElement.showMenu();

I wasn't able to get the automated test to hit my breakpoints, which means it wasn't doing anything, so I'll do a manual test for now.

> You should set listIndex to 0 as we discussed.

Done.

> r=me

Thanks!
Comment 5 Brian Weinstein 2010-01-26 14:46:16 PST
Landed in r53687.
Comment 6 Eric Seidel (no email) 2010-01-26 14:55:41 PST
Comment on attachment 47445 [details]
Patch

I think I would have used some listItem(index) function which did the bounds check and return 0 instead of usign a local Vector& in every function.