Bug 27275 - [Chromium] popup menus can crash when the selected index is -1
: [Chromium] popup menus can crash when the selected index is -1
Status: RESOLVED FIXED
: WebKit
WebCore Misc.
: 528+ (Nightly build)
: PC All
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2009-07-14 15:07 PST by
Modified: 2009-07-21 12:19 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-07-14 15:07:43 PST
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 From 2009-07-14 15:14:15 PST -------
Created an attachment (id=32738) [details]
Fix for crashing with invalid index.
------- Comment #2 From 2009-07-15 11:41:13 PST -------
Can we create a manual test?
------- Comment #3 From 2009-07-15 11:45:34 PST -------
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 From 2009-07-15 16:06:18 PST -------
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 From 2009-07-15 16:19:30 PST -------
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 From 2009-07-15 17:02:25 PST -------
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 From 2009-07-15 17:09:41 PST -------
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 From 2009-07-16 17:55:02 PST -------
Created an attachment (id=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 From 2009-07-19 23:02:25 PST -------
(From update of attachment 32908 [details])
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.
------- Comment #10 From 2009-07-20 13:26:27 PST -------
Created an attachment (id=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 From 2009-07-20 13:51:38 PST -------
(From update of attachment 33102 [details])
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 From 2009-07-21 11:14:13 PST -------
Assign to levin for landing.
------- Comment #13 From 2009-07-21 11:28:26 PST -------
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 From 2009-07-21 12:19:27 PST -------
Committed as http://trac.webkit.org/changeset/46184