Bug 47696

Summary: SelectElement should check if its renderer exists after calling Element::focus()
Product: WebKit Reporter: James Robinson <jamesr>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, ap, dglazkov, eric, tonikitoo, yael
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
repro
none
Patch
none
Patch simon.fraser: review+

Description James Robinson 2010-10-14 16:00:21 PDT
from SelectElement.cpp:

void SelectElement::listBoxDefaultEventHandler(SelectElementData& data, Element* element, Event* event, HTMLFormElement* htmlForm)
{
    const Vector<Element*>& listItems = data.listItems(element);

    if (event->type() == eventNames().mousedownEvent && event->isMouseEvent() && static_cast<MouseEvent*>(event)->button() == LeftButton) {
        element->focus();

        // Convert to coords relative to the list box if needed.
        MouseEvent* mouseEvent = static_cast<MouseEvent*>(event);
        IntPoint localOffset = roundedIntPoint(element->renderer()->absoluteToLocal(mouseEvent->absoluteLocation(), false, true));
 

this is called by SelectElement::defaultEventHandler(), which checks if element->renderer() is NULL before doing anything else. However calling element->focus() might cause the element's renderer to go away (since it can invoke arbitrary javascript event handlers) so it's possible to crash out here.

Originally reported in chromium bug tracker as http://code.google.com/p/chromium/issues/detail?id=58879.
Comment 1 James Robinson 2010-10-14 16:01:12 PDT
Created attachment 70792 [details]
repro
Comment 2 James Robinson 2010-10-14 16:01:42 PDT
Repro instructions from the original bug:
Steps:
1. There is one dropdown. Select 'show' - and the second one appears.
2. Focus remains on the first dropdown. Press 'b' on your keyboard - that will select 'b' in the first dropdon instead of 'show'.
3. Drag your mouse over scrollbar of the second dropdown - scroll to the bottom, for example.
4. Browser crashes
Comment 3 James Robinson 2010-10-14 16:11:23 PDT
Created attachment 70793 [details]
Patch
Comment 4 James Robinson 2010-10-14 16:13:10 PDT
Patch for discussion - I haven't written proper regression tests yet or a ChangeLog body, so this isn't quite ready to land.  This patch assumes that if the element has no renderer then the event is not marked as handled which is consistent with what happens if the renderer is NULL at the initial call to SelectElement::defaultEventHandler().  I dunno if this is really the proper behavior, however.
Comment 5 James Robinson 2010-10-14 17:37:03 PDT
Created attachment 70810 [details]
Patch
Comment 6 James Robinson 2010-10-14 17:47:36 PDT
Committed r69827: <http://trac.webkit.org/changeset/69827>
Comment 7 Alexey Proskuryakov 2010-10-14 23:27:49 PDT
+        * fast/forms/select-listbox-focus-displaynone.html: Added.

There is no -expected.txt in ChangeLog. Not that I care - I can't think of any practical difference for anyone.