Bug 47696 – SelectElement should check if its renderer exists after calling Element::focus()

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[James Robinson]

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.

[James Robinson]

Created attachment 70792 [details]
repro

[James Robinson]

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

[James Robinson]

Created attachment 70793 [details]
Patch

[James Robinson]

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.

[James Robinson]

Created attachment 70810 [details]
Patch

[James Robinson]

Committed r69827: <http://trac.webkit.org/changeset/69827>

[Alexey Proskuryakov]

+        * 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.