Change AppsGridView::IsValidIndex to cater for the last page of apps to avoid out-of-bounds access.

R=xiyuan@chromium.org
BUG=159630
TEST=In pop-up App Launcher, highlight something in the last row of apps
  on the last page and press down arrow. Browser should not crash.


Review URL: https://chromiumcodereview.appspot.com/11358263

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@168178 0039d316-1c4b-4281-b951-d872f2087c98

2 files changed, 41 insertions, 11 deletions

diff --git a/ui/app_list/apps_grid_view.cc b/ui/app_list/apps_grid_view.cc
index 63c3f6e..f4e5eb3 100644
--- a/ui/app_list/apps_grid_view.cc
+++ b/ui/app_list/apps_grid_view.cc
@@ -152,8 +152,10 @@ void AppsGridView::SetSelectedView(views::View* view) {
 }
 
 void AppsGridView::ClearSelectedView(views::View* view) {
-  if (IsSelectedView(view))
-    SetSelectedItemByIndex(Index());
+  if (view && IsSelectedView(view)) {
+    selected_view_->SchedulePaint();
+    selected_view_ = NULL;
+  }
 }
 
 bool AppsGridView::IsSelectedView(const views::View* view) const {
@@ -376,23 +378,26 @@ void AppsGridView::SetSelectedItemByIndex(const Index& index) {
   if (GetIndexOfView(selected_view_) == index)
     return;
 
+  views::View* new_selection = GetViewAtIndex(index);
+  if (!new_selection)
+    return; // Keep current selection.
+
   if (selected_view_)
     selected_view_->SchedulePaint();
 
-  selected_view_ = GetViewAtIndex(index);
-  if (selected_view_) {
-    EnsureViewVisible(selected_view_);
-    selected_view_->SchedulePaint();
-    if (GetWidget()) {
-      GetWidget()->NotifyAccessibilityEvent(
-          selected_view_, ui::AccessibilityTypes::EVENT_FOCUS, true);
-    }
+  selected_view_ = new_selection;
+  EnsureViewVisible(selected_view_);
+  selected_view_->SchedulePaint();
+  if (GetWidget()) {
+    GetWidget()->NotifyAccessibilityEvent(
+        selected_view_, ui::AccessibilityTypes::EVENT_FOCUS, true);
   }
 }
 
 bool AppsGridView::IsValidIndex(const Index& index) const {
   return index.page >= 0 && index.page < pagination_model_->total_pages() &&
-      index.slot >= 0 && index.slot < tiles_per_page();
+      index.slot >= 0 && index.slot < tiles_per_page() &&
+      index.page * tiles_per_page() + index.slot < view_model_.view_size();
 }
 
 AppsGridView::Index AppsGridView::GetIndexOfView(
diff --git a/ui/app_list/apps_grid_view_unittest.cc b/ui/app_list/apps_grid_view_unittest.cc
index dffafc8..ba5969e 100644
--- a/ui/app_list/apps_grid_view_unittest.cc
+++ b/ui/app_list/apps_grid_view_unittest.cc
@@ -184,6 +184,11 @@ class AppsGridViewTest : public testing::Test {
     apps_grid_view_->UpdateDrag(view, pointer, drag_event);
   }
 
+  void SimulateKeyPress(ui::KeyboardCode key_code) {
+    ui::KeyEvent key_event(ui::ET_KEY_PRESSED, key_code, 0, false);
+    apps_grid_view_->OnKeyPressed(key_event);
+  }
+
   scoped_ptr<AppListModel> model_;
   scoped_ptr<PaginationModel> pagination_model_;
   scoped_ptr<AppsGridView> apps_grid_view_;
@@ -366,5 +371,25 @@ TEST_F(AppsGridViewTest, SimultaneousDrag) {
   test_api_->LayoutToIdealBounds();
 }
 
+TEST_F(AppsGridViewTest, HighlightWithKeyboard) {
+  const int kTotalitems = 2;
+  PopulateApps(kTotalitems);
+  apps_grid_view_->SetSelectedView(GetItemViewAt(0));
+
+  // Try moving off the item to an invalid location.
+  SimulateKeyPress(ui::VKEY_UP);
+  EXPECT_TRUE(apps_grid_view_->IsSelectedView(GetItemViewAt(0)));
+  SimulateKeyPress(ui::VKEY_DOWN);
+  EXPECT_TRUE(apps_grid_view_->IsSelectedView(GetItemViewAt(0)));
+  SimulateKeyPress(ui::VKEY_LEFT);
+  EXPECT_TRUE(apps_grid_view_->IsSelectedView(GetItemViewAt(0)));
+
+  // Move to the second item and try to go past it.
+  SimulateKeyPress(ui::VKEY_RIGHT);
+  EXPECT_TRUE(apps_grid_view_->IsSelectedView(GetItemViewAt(1)));
+  SimulateKeyPress(ui::VKEY_RIGHT);
+  EXPECT_TRUE(apps_grid_view_->IsSelectedView(GetItemViewAt(1)));
+}
+
 }  // namespace test
 }  // namespace app_list