Bug 27275 - [Chromium] popup menus can crash when the selected index is -1
Summary: [Chromium] popup menus can crash when the selected index is -1
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: David Levin
Depends on:
Reported: 2009-07-14 15:07 PDT by Paul Godavari
Modified: 2009-07-21 12:19 PDT (History)
2 users (show)

See Also:

Fix for crashing with invalid index. (1.44 KB, patch)
2009-07-14 15:14 PDT, Paul Godavari
no flags Details | Formatted Diff | Diff
Updated fix for an invalid vector index crash (3.63 KB, patch)
2009-07-16 17:55 PDT, Paul Godavari
eric: review-
Details | Formatted Diff | Diff
Fix for crashing with invalid index. (3.71 KB, patch)
2009-07-20 13:26 PDT, Paul Godavari
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Paul Godavari 2009-07-14 15:07:43 PDT
We've received crash dumps from users with call stacks indicating that PopupListBox::isSelectableItem has been passed an index of -1. This method should prevent such an invalid index from crashing.
Comment 1 Paul Godavari 2009-07-14 15:14:15 PDT
Created attachment 32738 [details]
Fix for crashing with invalid index.
Comment 2 Eric Seidel 2009-07-15 11:41:13 PDT
Can we create a manual test?
Comment 3 Paul Godavari 2009-07-15 11:45:34 PDT
I wasn't able to reproduce this manually so I'm not sure how to create a test for it, but we are getting a number of crash dumps from users for this specific problem.
Comment 4 Eric Seidel 2009-07-15 16:06:18 PDT
Can you make a guess from code inspection as to how this could be happening?  it's possible this is the wrong fix if we don't understand why it occurs...
Comment 5 Paul Godavari 2009-07-15 16:19:30 PDT
Unfortunately, the crash dumps are mini-dumps from users so they don't contain enough of the stack to make complete sense.

What I do see is an attempt to check for menu item selectability after a mouse move event. My guess is that the child window tracking the mouse has detected the user has moved out of the popup window since PopupListBox::pointToRowIndex is returning -1 to PopupListBox::selectIndex.

selectIndex has an ASSERT to catch the case of invalid index values, but that's only in debug builds and won't protect the user in any case.
Comment 6 Eric Seidel 2009-07-15 17:02:25 PDT
Should we be returning early out of selectIndex() instead of here?  I'm not against making this code more robust here, but I worry we're masking another bug instead of fixing the root cause.
Comment 7 Paul Godavari 2009-07-15 17:09:41 PDT
I don't believe we are masking a bug here, since it's perfectly valid for the popup to try and hit test outside of its region (thereby generating an index of -1) during mouse operations. In this case, the ASSERT is incorrect since there is at least one place in the code where we return a value that violates the assert.

Perhaps a better fix is to go through all the PopupListBox vector indexing to make sure that we can never index outside its valid range [0, numItems()).
Comment 8 Paul Godavari 2009-07-16 17:55:02 PDT
Created attachment 32908 [details]
Updated fix for an invalid vector index crash

Update the patch to more thoroughly enforce correct vector index values through out PopupListBox.
Comment 9 Eric Seidel 2009-07-19 23:02:25 PDT
Comment on attachment 32908 [details]
Updated fix for an invalid vector index crash

This seems wrong:
     if (index < 0 || index >= numItems())
 890         return;
888892     if (index == -1 && m_popupClient) {

You've made some of the code unreachable.
Comment 10 Paul Godavari 2009-07-20 13:26:27 PDT
Created attachment 33102 [details]
Fix for crashing with invalid index.

Updated the code per previous review comments. Changed the check from "index == -1" to "index < 0" to be consistent with other PopupListBox methods.
Comment 11 Eric Seidel 2009-07-20 13:51:38 PDT
Comment on attachment 33102 [details]
Fix for crashing with invalid index.

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.
Comment 12 David Levin 2009-07-21 11:14:13 PDT
Assign to levin for landing.
Comment 13 David Levin 2009-07-21 11:28:26 PDT
Please make sure to include the bug title and bug link in the changelog in the future

  prepare-ChangeLog --bug YourBugNumber

makes this trivial.
Comment 14 David Levin 2009-07-21 12:19:27 PDT
Committed as http://trac.webkit.org/changeset/46184