Commit 4f25e958 authored by Vladislav Kaznacheev's avatar Vladislav Kaznacheev Committed by Commit Bot

Cancel shelf context menu when ShelfItemDelegate is replaced.

ShelfContextMenuModel holds a raw pointer to ShelfItemDelegate.
If a delegate is deleted while a menu is being shown, selecting
a menu item will cause use-after-free.

A delegate might be deleted from 3 places:
- when a shelf item is removed (this is already handled correctly)
- when ShelfModel is destroyed on shutdown (does not seem to cause
  problems)
- when a delegate is replaced on an existing shelf item (handled
  in this CL).

Bug: 937507
Test: ShelfViewTest.ReplacingDelegateCancelsContextMenu
Change-Id: Iac551bbab7b97a863941c89ea5a056ddf330a910
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1585016
Commit-Queue: Vladislav Kaznacheev <kaznacheev@chromium.org>
Auto-Submit: Vladislav Kaznacheev <kaznacheev@chromium.org>
Reviewed-by: default avatarMichael Wasserman <msw@chromium.org>
Reviewed-by: default avatarYury Khmel <khmel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#654544}
parent 55699630
......@@ -2287,7 +2287,10 @@ void ShelfView::ShelfItemMoved(int start_index, int target_index) {
void ShelfView::ShelfItemDelegateChanged(const ShelfID& id,
ShelfItemDelegate* old_delegate,
ShelfItemDelegate* delegate) {}
ShelfItemDelegate* delegate) {
if (id == context_menu_id_ && shelf_menu_model_adapter_)
shelf_menu_model_adapter_->Cancel();
}
void ShelfView::ShelfItemStatusChanged(const ShelfID& id) {
int index = model_->ItemIndexByID(id);
......
......@@ -194,6 +194,17 @@ class ShelfItemSelectionTracker : public ShelfItemDelegate {
}
void ExecuteCommand(bool, int64_t, int32_t, int64_t) override {}
void Close() override {}
void GetContextMenuItems(int64_t display_id,
GetContextMenuItemsCallback callback) override {
ash::MenuItemList items;
ash::mojom::MenuItemPtr item(ash::mojom::MenuItem::New());
item->type = ui::MenuModel::TYPE_COMMAND;
item->command_id = 0;
item->label = base::UTF8ToUTF16("Item");
item->enabled = true;
items.push_back(std::move(item));
std::move(callback).Run(std::move(items));
}
private:
size_t item_selected_count_ = 0;
......@@ -2417,6 +2428,23 @@ TEST_F(ShelfViewTest, FirstAndLastVisibleIndex) {
}
}
TEST_F(ShelfViewTest, ReplacingDelegateCancelsContextMenu) {
ui::test::EventGenerator* generator = GetEventGenerator();
ShelfID app_button_id = AddAppShortcut();
generator->MoveMouseTo(GetButtonCenter(GetButtonByID(app_button_id)));
// Right click should open the context menu.
generator->PressRightButton();
generator->ReleaseRightButton();
EXPECT_TRUE(shelf_view_->IsShowingMenu());
// Replacing the item delegate should close the context menu.
model_->SetShelfItemDelegate(app_button_id,
std::make_unique<ShelfItemSelectionTracker>());
EXPECT_FALSE(shelf_view_->IsShowingMenu());
}
// Test class that tests both context and application menus.
class ShelfViewMenuTest : public ShelfViewTest,
public testing::WithParamInterface<bool> {
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment