[webkit-reviews] review granted: [Bug 27275] [Chromium] popup menus can crash when the selected index is -1 : [Attachment 33102] Fix for crashing with invalid index.

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has granted Paul Godavari <paul at chromium.org>'s
request for review:
Bug 27275: [Chromium] popup menus can crash when the selected index is -1
https://bugs.webkit.org/show_bug.cgi?id=27275

Attachment 33102: Fix for crashing with invalid index.
https://bugs.webkit.org/attachment.cgi?id=33102&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
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.