Bug 47696 - SelectElement should check if its renderer exists after calling Element::focus()
Summary: SelectElement should check if its renderer exists after calling Element::focus()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-10-14 16:00 PDT by James Robinson
Modified: 2010-10-14 23:27 PDT (History)
6 users (show)

See Also:


Attachments
repro (1.49 KB, text/html)
2010-10-14 16:01 PDT, James Robinson
no flags Details
Patch (3.66 KB, patch)
2010-10-14 16:11 PDT, James Robinson
no flags Details | Formatted Diff | Diff
Patch (6.18 KB, patch)
2010-10-14 17:37 PDT, James Robinson
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.