From 9134bb3a836d2dba78931e35d80ff4a551036638 Mon Sep 17 00:00:00 2001
From: Daniel Andersson <dandersson@chromium.org>
Date: Tue, 20 Jun 2023 23:35:33 +0000
Subject: [PATCH] saved_desks: Fix DCHECK when closing ARC window in overview
We have code in OverviewGrid to determine if there are windows that the
saved desk features do not support. This code is run when a window is
added to overview mode, and when it is removed. The issue here is a race
between ARC task and aura window destruction.
We call out to full_restore::GetAppId to get the application ID of a
window. In some cases when an ARC window is being destroyed, this
function will return an empty application ID. The fix here, which is
somewhat temporary, is to also check the window's kAppIDKey property.
BUG=b:270429246
Change-Id: I915e0b0917b0f551dab53e81f8d25425bdbfedd6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4628703
Reviewed-by: Sammie Quon <sammiequon@chromium.org>
Commit-Queue: Daniel Andersson <dandersson@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1160353}
---
.../desks/templates/restore_data_collector.cc | 3 +--
ash/wm/desks/templates/saved_desk_unittest.cc | 3 +--
ash/wm/desks/templates/saved_desk_util.cc | 19 +++++++++++++++++++
ash/wm/desks/templates/saved_desk_util.h | 6 ++++++
ash/wm/overview/overview_grid.cc | 3 +--
5 files changed, 28 insertions(+), 6 deletions(-)
diff --git a/ash/wm/desks/templates/restore_data_collector.cc b/ash/wm/desks/templates/restore_data_collector.cc
index 3d890f5e93638..aee67eac89758 100644
--- a/ash/wm/desks/templates/restore_data_collector.cc
+++ b/ash/wm/desks/templates/restore_data_collector.cc
@@ -13,7 +13,6 @@
#include "ash/wm/window_restore/window_restore_util.h"
#include "base/uuid.h"
#include "components/app_restore/app_launch_info.h"
-#include "components/app_restore/full_restore_utils.h"
#include "components/app_restore/restore_data.h"
#include "components/app_restore/window_info.h"
#include "ui/wm/core/window_util.h"
@@ -65,7 +64,7 @@ void RestoreDataCollector::CaptureActiveDeskAsSavedDesk(
}
// Skip windows that do not associate with a full restore app id.
- const std::string app_id = full_restore::GetAppId(window);
+ const std::string app_id = saved_desk_util::GetAppId(window);
if (!Shell::Get()
->overview_controller()
->disable_app_id_check_for_saved_desks() &&
diff --git a/ash/wm/desks/templates/saved_desk_unittest.cc b/ash/wm/desks/templates/saved_desk_unittest.cc
index 740307bdcbc87..a1ac30ea32e72 100644
--- a/ash/wm/desks/templates/saved_desk_unittest.cc
+++ b/ash/wm/desks/templates/saved_desk_unittest.cc
@@ -68,7 +68,6 @@
#include "cc/test/pixel_comparator.h"
#include "components/app_constants/constants.h"
#include "components/app_restore/app_launch_info.h"
-#include "components/app_restore/full_restore_utils.h"
#include "components/app_restore/window_info.h"
#include "components/app_restore/window_properties.h"
#include "components/desks_storage/core/local_desk_data_manager.h"
@@ -2127,7 +2126,7 @@ TEST_F(SavedDeskTest, AllUnsupportedAppsDisablesSaveDeskButtons) {
auto no_app_id_window = CreateAppWindow();
auto* delegate = Shell::Get()->saved_desk_delegate();
ASSERT_TRUE(delegate->IsWindowSupportedForSavedDesk(no_app_id_window.get()));
- ASSERT_TRUE(full_restore::GetAppId(no_app_id_window.get()).empty());
+ ASSERT_TRUE(saved_desk_util::GetAppId(no_app_id_window.get()).empty());
// Open overview.
ToggleOverview();
diff --git a/ash/wm/desks/templates/saved_desk_util.cc b/ash/wm/desks/templates/saved_desk_util.cc
index f1d64d1e3fb79..eafbbe8375d19 100644
--- a/ash/wm/desks/templates/saved_desk_util.cc
+++ b/ash/wm/desks/templates/saved_desk_util.cc
@@ -7,11 +7,13 @@
#include "ash/constants/ash_features.h"
#include "ash/constants/ash_pref_names.h"
#include "ash/public/cpp/session/session_types.h"
+#include "ash/public/cpp/window_properties.h"
#include "ash/session/session_controller_impl.h"
#include "ash/shell.h"
#include "ash/wm/desks/templates/saved_desk_constants.h"
#include "ash/wm/overview/overview_controller.h"
#include "ash/wm/overview/overview_session.h"
+#include "components/app_restore/full_restore_utils.h"
#include "components/app_restore/window_properties.h"
#include "components/prefs/pref_registry_simple.h"
#include "components/prefs/pref_service.h"
@@ -84,6 +86,23 @@ SavedDeskPresenter* GetSavedDeskPresenter() {
return presenter;
}
+std::string GetAppId(aura::Window* window) {
+ // Check with full restore first.
+ if (std::string app_id = full_restore::GetAppId(window); !app_id.empty()) {
+ return app_id;
+ }
+
+ // Otherwise, check if there's a window property.
+ // TODO(b/288153585): Figure out if the kAppIDKey property is always going to
+ // be set for window types that we care about. If that is the case, we should
+ // be able to drop the call to full restore.
+ if (const std::string* app_id_property = window->GetProperty(kAppIDKey)) {
+ return *app_id_property;
+ }
+
+ return "";
+}
+
bool IsAdminTemplateWindow(aura::Window* window) {
const int32_t* activation_index =
window->GetProperty(app_restore::kActivationIndexKey);
diff --git a/ash/wm/desks/templates/saved_desk_util.h b/ash/wm/desks/templates/saved_desk_util.h
index 7e026d05cd9ec..f79fb552c76eb 100644
--- a/ash/wm/desks/templates/saved_desk_util.h
+++ b/ash/wm/desks/templates/saved_desk_util.h
@@ -5,6 +5,8 @@
#ifndef ASH_WM_DESKS_TEMPLATES_SAVED_DESK_UTIL_H_
#define ASH_WM_DESKS_TEMPLATES_SAVED_DESK_UTIL_H_
+#include <string>
+
#include "ash/ash_export.h"
class PrefRegistrySimple;
@@ -34,6 +36,10 @@ ASH_EXPORT SavedDeskDialogController* GetSavedDeskDialogController();
// Will return null if overview mode is not active.
ASH_EXPORT SavedDeskPresenter* GetSavedDeskPresenter();
+// Returns the app ID of the window, if present. Returns an empty string
+// otherwise.
+ASH_EXPORT std::string GetAppId(aura::Window* window);
+
// Returns true if `window` was launched from an admin template.
bool IsAdminTemplateWindow(aura::Window* window);
diff --git a/ash/wm/overview/overview_grid.cc b/ash/wm/overview/overview_grid.cc
index 86003fcc8ff9a..1d03150dabfa9 100644
--- a/ash/wm/overview/overview_grid.cc
+++ b/ash/wm/overview/overview_grid.cc
@@ -66,7 +66,6 @@
#include "base/ranges/algorithm.h"
#include "chromeos/constants/chromeos_features.h"
#include "chromeos/ui/base/window_properties.h"
-#include "components/app_restore/full_restore_utils.h"
#include "ui/aura/client/aura_constants.h"
#include "ui/base/l10n/l10n_util.h"
#include "ui/chromeos/styles/cros_tokens_color_mappings.h"
@@ -2729,7 +2728,7 @@ void OverviewGrid::UpdateNumSavedDeskUnsupportedWindows(aura::Window* window,
(Shell::Get()
->overview_controller()
->disable_app_id_check_for_saved_desks() ||
- !full_restore::GetAppId(window).empty());
+ !saved_desk_util::GetAppId(window).empty());
int addend = increment ? 1 : -1;
if (!DeskTemplate::IsAppTypeSupported(window) || !has_restore_id) {
num_unsupported_windows_ += addend;