Commit ba13136b authored by Xiyuan Xia's avatar Xiyuan Xia Committed by Commit Bot

app_list: Fix UAF after menu close.

App list now uses ui::SimpleMenuModel created in chrome directly.
The SimpleMenuModel uses AppContextMenu as its delegate and its
life time is bound to ChromeAppListItem and ChromeSearchResult,
which is different with the UI associated with them. When menu
is dismissed, it schedules SimpleMenuModel::OnMenuClosed() call
and could be reached after chrome side AppContextMenu is released.

The CL wire up OnMenuClosed in the UI elements to releases the
menu model. This happens before chrome releasing AppContextMenu
on dismissing app list.

Bug: 965848
Test: Select "App Info" on context menu of SearchResultTileItemView.
Change-Id: I56dfbb0551c336b31326297ca01eab6e166529bc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1625649Reviewed-by: default avatarXiyuan Xia <xiyuan@chromium.org>
Reviewed-by: default avatarAlex Newcomer <newcomer@chromium.org>
Commit-Queue: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#662277}
parent 817019ee
...@@ -784,6 +784,10 @@ void AppListItemView::AnimationProgressed(const gfx::Animation* animation) { ...@@ -784,6 +784,10 @@ void AppListItemView::AnimationProgressed(const gfx::Animation* animation) {
} }
void AppListItemView::OnMenuClosed() { void AppListItemView::OnMenuClosed() {
// Release menu since its menu model delegate (AppContextMenu) could be
// released as a result of menu command execution.
context_menu_.reset();
if (!menu_close_initiated_from_drag_) { if (!menu_close_initiated_from_drag_) {
// If the menu was not closed due to a drag sequence(e.g. multi touch) reset // If the menu was not closed due to a drag sequence(e.g. multi touch) reset
// the drag state. // the drag state.
......
...@@ -382,6 +382,9 @@ void SearchResultTileItemView::OnGetContextMenuModel( ...@@ -382,6 +382,9 @@ void SearchResultTileItemView::OnGetContextMenuModel(
} }
void SearchResultTileItemView::OnMenuClosed() { void SearchResultTileItemView::OnMenuClosed() {
// Release menu since its menu model delegate (AppContextMenu) could be
// released as a result of menu command execution.
context_menu_.reset();
OnBlur(); OnBlur();
} }
......
...@@ -436,6 +436,12 @@ bool SearchResultView::IsSearchResultHoveredOrSelected() { ...@@ -436,6 +436,12 @@ bool SearchResultView::IsSearchResultHoveredOrSelected() {
return IsMouseHovered() || selected(); return IsMouseHovered() || selected();
} }
void SearchResultView::OnMenuClosed() {
// Release menu since its menu model delegate (AppContextMenu) could be
// released as a result of menu command execution.
context_menu_.reset();
}
void SearchResultView::ShowContextMenuForViewImpl( void SearchResultView::ShowContextMenuForViewImpl(
views::View* source, views::View* source,
const gfx::Point& point, const gfx::Point& point,
...@@ -455,7 +461,7 @@ void SearchResultView::OnGetContextMenu( ...@@ -455,7 +461,7 @@ void SearchResultView::OnGetContextMenu(
const gfx::Point& point, const gfx::Point& point,
ui::MenuSourceType source_type, ui::MenuSourceType source_type,
std::unique_ptr<ui::SimpleMenuModel> menu_model) { std::unique_ptr<ui::SimpleMenuModel> menu_model) {
if (!menu_model || context_menu_->IsShowingMenu()) if (!menu_model || (context_menu_ && context_menu_->IsShowingMenu()))
return; return;
AppLaunchedMetricParams metric_params = { AppLaunchedMetricParams metric_params = {
...@@ -466,7 +472,9 @@ void SearchResultView::OnGetContextMenu( ...@@ -466,7 +472,9 @@ void SearchResultView::OnGetContextMenu(
context_menu_ = std::make_unique<AppListMenuModelAdapter>( context_menu_ = std::make_unique<AppListMenuModelAdapter>(
std::string(), std::move(menu_model), GetWidget(), source_type, std::string(), std::move(menu_model), GetWidget(), source_type,
metric_params, AppListMenuModelAdapter::SEARCH_RESULT, metric_params, AppListMenuModelAdapter::SEARCH_RESULT,
base::OnceClosure(), view_delegate_->GetSearchModel()->tablet_mode()); base::BindOnce(&SearchResultView::OnMenuClosed,
weak_ptr_factory_.GetWeakPtr()),
view_delegate_->GetSearchModel()->tablet_mode());
context_menu_->Run(gfx::Rect(point, gfx::Size()), context_menu_->Run(gfx::Rect(point, gfx::Size()),
views::MenuAnchorPosition::kTopLeft, views::MenuAnchorPosition::kTopLeft,
views::MenuRunner::HAS_MNEMONICS); views::MenuRunner::HAS_MNEMONICS);
......
...@@ -113,6 +113,9 @@ class APP_LIST_EXPORT SearchResultView ...@@ -113,6 +113,9 @@ class APP_LIST_EXPORT SearchResultView
void OnSearchResultActionActivated(size_t index, int event_flags) override; void OnSearchResultActionActivated(size_t index, int event_flags) override;
bool IsSearchResultHoveredOrSelected() override; bool IsSearchResultHoveredOrSelected() override;
// Invoked when the context menu closes.
void OnMenuClosed();
// Parent list view. Owned by views hierarchy. // Parent list view. Owned by views hierarchy.
SearchResultListView* list_view_; SearchResultListView* list_view_;
......
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