[Chromium] popup menus can crash when the selected index is -1

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Paul Godavari]

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.

[Paul Godavari]

Created attachment 32738 [details]
Fix for crashing with invalid index.

[Eric Seidel (no email)]

Can we create a manual test?

[Paul Godavari]

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.

[Eric Seidel (no email)]

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

[Paul Godavari]

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.

[Eric Seidel (no email)]

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.

[Paul Godavari]

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()).

[Paul Godavari]

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.

[Eric Seidel (no email)]

Comment on attachment 32908 [details]
Updated fix for an invalid vector index crash

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

You've made some of the code unreachable.

[Paul Godavari]

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.

[Eric Seidel (no email)]

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.

[David Levin]

Assign to levin for landing.

[David Levin]

Please make sure to include the bug title and bug link in the changelog in the future

  prepare-ChangeLog --bug YourBugNumber

makes this trivial.

[David Levin]

Committed as http://trac.webkit.org/changeset/46184