[Webkit-unassigned] [Bug 27275] [Chromium] popup menus can crash when the selected index is -1

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=27275


Eric Seidel <eric at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #33102|review?                     |review+
               Flag|                            |




--- Comment #11 from Eric Seidel <eric at webkit.org>  2009-07-20 13:51:38 PDT ---
(From update of attachment 33102)
I spoke w/ Paul at his desk.

The real bug here seems to be the API design for pointToRowIndex. 
pointToRowIndex returns the special value "-1" which all other places which use
"int index" are forced to then handle.

+    if (index >= numItems())
+        return;

is not necessary.  I can be an ASSERT, but doesn't seem otherwise needed.

+    if (index < 0) {

could be index == -1, since -1 is the only special value.  It's OK as is
though.

-    ASSERT(index >= 0 && index < numItems());
+    if (index < 0 || index >= numItems())
+        return;

This looks like it will crash in the future?  Seems we need to handle at least
the -1 case.
bool PopupListBox::isSelectableItem(int index)
 {
+    ASSERT(index >= 0 && index < numItems());

Since we know the source of the only invalid indices (pointToRowIndex), seems
we should fix the source of these invalid indices.

bool hitTest(IntPoint, int& hitRow);
seems like a better API than pointToRowIndex as it would force all callers to
handle the out-of bounds case.

r+ because this fixes the crash.  Please consider filing a bug about the
re-archtecture, or post-ing a fix using the hitTest approach instead.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.