app_list: Fix OpenSearchResult UAF
Move search_controller_->OpenResult() call to the last because
opening a result could be self destructing via ViewClosing if
the result opens a window. This was fine when ViewClosing is an
asynchronous mojo call. It is a problem now because the mojo call
is replaced with a synchronous C++ call.
Bug: 964762
Change-Id: Ia18a4ad0b08791321d1058b73ff4b4cf36027be8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1618569
Reviewed-by: Alex Newcomer <newcomer@chromium.org>
Commit-Queue: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#661409}
diff --git a/chrome/browser/ui/app_list/app_list_client_impl.cc b/chrome/browser/ui/app_list/app_list_client_impl.cc
index 472d97f..ebd5a317 100644
--- a/chrome/browser/ui/app_list/app_list_client_impl.cc
+++ b/chrome/browser/ui/app_list/app_list_client_impl.cc
@@ -107,8 +107,6 @@
if (!result)
return;
- search_controller_->OpenResult(result, event_flags);
-
// Send training signal to search controller.
search_controller_->Train(result_id,
app_list::RankingItemTypeFromSearchResult(*result));
@@ -122,6 +120,9 @@
RecordSearchResultOpenTypeHistogram(
launched_from, result->GetSearchResultType(), IsTabletMode());
+
+ // OpenResult may cause |result| to be deleted.
+ search_controller_->OpenResult(result, event_flags);
}
void AppListClientImpl::InvokeSearchResultAction(const std::string& result_id,
diff --git a/chrome/browser/ui/app_list/app_list_client_impl_browsertest.cc b/chrome/browser/ui/app_list/app_list_client_impl_browsertest.cc
index 83e2365..f22a963 100644
--- a/chrome/browser/ui/app_list/app_list_client_impl_browsertest.cc
+++ b/chrome/browser/ui/app_list/app_list_client_impl_browsertest.cc
@@ -151,6 +151,50 @@
}
}
+// Test that OpenSearchResult that dismisses app list runs fine without
+// user-after-free.
+IN_PROC_BROWSER_TEST_F(AppListClientImplBrowserTest, OpenSearchResult) {
+ AppListClientImpl* client = AppListClientImpl::GetInstance();
+ ASSERT_TRUE(client);
+
+ // Associate |client| with the current profile.
+ client->UpdateProfile();
+
+ // Show the launcher.
+ client->ShowAppList();
+
+ AppListModelUpdater* model_updater = test::GetModelUpdater(client);
+ ASSERT_TRUE(model_updater);
+ app_list::SearchController* search_controller = client->search_controller();
+ ASSERT_TRUE(search_controller);
+
+ // Any app that opens a window to dismiss app list is good enough for this
+ // test.
+ const std::string app_title = "chromium";
+ const std::string app_result_id =
+ "chrome-extension://mgndgikekgjfcpckkfioiadnlibdjbkf/";
+
+ // Search by title and the app must present in the results.
+ model_updater->UpdateSearchBox(base::ASCIIToUTF16(app_title),
+ true /* initiated_by_user */);
+ ASSERT_TRUE(search_controller->FindSearchResult(app_result_id));
+
+ // Open the app result.
+ client->OpenSearchResult(
+ app_result_id, ui::EF_NONE,
+ ash::mojom::AppListLaunchedFrom::kLaunchedFromSearchBox,
+ ash::mojom::AppListLaunchType::kAppSearchResult, 0);
+
+ // App list should be dismissed.
+ EXPECT_FALSE(client->app_list_target_visibility());
+
+ // Needed to let AppLaunchEventLogger finish its work on worker thread.
+ // Otherwise, its |weak_factory_| is released on UI thread and causing
+ // the bound WeakPtr to fail sequence check on a worker thread.
+ // TODO(crbug.com/965065): Remove after fixing AppLaunchEventLogger.
+ content::RunAllTasksUntilIdle();
+}
+
// Test that browser launch time is recorded is recorded in preferences.
// This is important for suggested apps sorting.
IN_PROC_BROWSER_TEST_F(AppListClientImplBrowserTest,
