Bug 34182

Summary:

Crash in WebKit!WebCore::RenderMenuList::itemStyle

Product:

WebKit

Reporter:

Brian Weinstein <bweinstein>

Component:

New Bugs

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Other

OS:

OS X 10.5

Attachments:

Description	Flags
Patch	jhoneycutt: review+, jhoneycutt: commit-queue-

Brian Weinstein

Reported 2010-01-26 13:41:55 PST

Crash in WebKit!WebCore::RenderMenuList::itemStyle

Attachments
Patch (7.33 KB, patch) 2010-01-26 13:55 PST, Brian Weinstein	jhoneycutt: review+ jhoneycutt: commit-queue-	Details Formatted Diff Diff
View All Add attachment proposed patch, testcase, etc.

Brian Weinstein

Comment 1 2010-01-26 13:55:21 PST

Created attachment 47445 [details] Patch

WebKit Review Bot

Comment 2 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.

Jon Honeycutt

Comment 3 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

Brian Weinstein

Comment 4 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!

Brian Weinstein

Comment 5 2010-01-26 14:46:16 PST

Landed in r53687.

Eric Seidel (no email)

Comment 6 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.

Note You need to log in before you can comment on or make changes to this bug.