[lacros shelf] relax crosapi/window event ordering

There may be a race where crosapi and Aura window events arrive out of
order, resulting in the removal of app instances that are still valid
(tabs in transit between browser windows). These instances need to be
removed while there is no valid window matching this instance and
recreated when there is one, to ensure that an instance always has a
valid window. A side effect of this is the reuse of app instance ID for
two different instances, but their lifetimes don't overlap, so it should
not cause problems.

Bug: 1327389, 1335569
Change-Id: I73fc5e483f21b0d8270865e8c096c798af8bf992
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3813089
Commit-Queue: Alexander Bolodurin <alexbn@chromium.org>
Reviewed-by: Eric Willigers <ericwilligers@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1032418}

chrome/browser/apps/app_service/browser_app_instance_registry.cc

@@ -247,17 +247,19 @@ void BrowserAppInstanceRegistry::OnBrowserAppAdded(
   auto window_id = update.window_id;
   RunOrEnqueueEventForWindow(
       window_id,
-      base::BindOnce(&BrowserAppInstanceRegistry::LacrosAppInstanceAdded,
-                     weak_ptr_factory_.GetWeakPtr(), std::move(update)));
+      base::BindOnce(
+          &BrowserAppInstanceRegistry::LacrosAppInstanceAddedOrUpdated,
+          weak_ptr_factory_.GetWeakPtr(), std::move(update)));
 }
 
 void BrowserAppInstanceRegistry::OnBrowserAppUpdated(
     apps::BrowserAppInstanceUpdate update) {
   auto window_id = update.window_id;
   RunOrEnqueueEventForWindow(
       window_id,
-      base::BindOnce(&BrowserAppInstanceRegistry::LacrosAppInstanceUpdated,
-                     weak_ptr_factory_.GetWeakPtr(), std::move(update)));
+      base::BindOnce(
+          &BrowserAppInstanceRegistry::LacrosAppInstanceAddedOrUpdated,
+          weak_ptr_factory_.GetWeakPtr(), std::move(update)));
 }
 
 void BrowserAppInstanceRegistry::OnBrowserAppRemoved(
@@ -368,30 +370,42 @@ void BrowserAppInstanceRegistry::LacrosWindowInstanceRemoved(
   }
 }
 
-void BrowserAppInstanceRegistry::LacrosAppInstanceAdded(
-    apps::BrowserAppInstanceUpdate update,
-    aura::Window* window) {
-  DCHECK(window);
-  auto id = update.id;
-  auto& instance = AddInstance(
-      lacros_app_instances_, id,
-      std::make_unique<BrowserAppInstance>(std::move(update), window));
-  for (auto& observer : observers_) {
-    observer.OnBrowserAppAdded(instance);
-  }
-}
-
-void BrowserAppInstanceRegistry::LacrosAppInstanceUpdated(
+void BrowserAppInstanceRegistry::LacrosAppInstanceAddedOrUpdated(
     apps::BrowserAppInstanceUpdate update,
     aura::Window* window) {
   DCHECK(window);
+  // Create instance if it does not already eixsts, update it if exists.
+  //
+  // In some cases this may result in the removal of an instance and then
+  // immediate recreation of it with the same ID, but it's necessary to maintain
+  // app instances with a valid window.
+  //
+  // For example, if the last tab is dragged from browser A into browser B, the
+  // tab will get reparented into a different window and browser A's window is
+  // destroyed. However app instance messages and window destruction events may
+  // arrive out of order because they originate from different sources now
+  // (crosapi and wayland). If a window is destroyed first, it leaves the app
+  // instance with an invalid window for a fraction of time. Rather than making
+  // the window pointer nullable, we remove the instance and then re-add it when
+  // an instance update message reparenting the instance into a new window
+  // arrives.
   BrowserAppInstance* instance = GetInstance(lacros_app_instances_, update.id);
-  if (instance && instance->MaybeUpdate(
-                      window, update.title, update.is_browser_active,
-                      update.is_web_contents_active, update.browser_session_id,
-                      update.restored_browser_session_id)) {
+  if (instance) {
+    if (instance->MaybeUpdate(window, update.title, update.is_browser_active,
+                              update.is_web_contents_active,
+                              update.browser_session_id,
+                              update.restored_browser_session_id)) {
+      for (auto& observer : observers_) {
+        observer.OnBrowserAppUpdated(*instance);
+      }
+    }
+  } else {
+    auto id = update.id;
+    auto& instance = AddInstance(
+        lacros_app_instances_, id,
+        std::make_unique<BrowserAppInstance>(std::move(update), window));
     for (auto& observer : observers_) {
-      observer.OnBrowserAppUpdated(*instance);
+      observer.OnBrowserAppAdded(instance);
     }
   }
 }

chrome/browser/apps/app_service/browser_app_instance_registry.h

@@ -193,10 +193,8 @@ class BrowserAppInstanceRegistry
                                    aura::Window* window);
   void LacrosWindowInstanceRemoved(apps::BrowserWindowInstanceUpdate update,
                                    aura::Window* window);
-  void LacrosAppInstanceAdded(apps::BrowserAppInstanceUpdate update,
-                              aura::Window* window);
-  void LacrosAppInstanceUpdated(apps::BrowserAppInstanceUpdate update,
-                                aura::Window* window);
+  void LacrosAppInstanceAddedOrUpdated(apps::BrowserAppInstanceUpdate update,
+                                       aura::Window* window);
   void LacrosAppInstanceRemoved(apps::BrowserAppInstanceUpdate update,
                                 aura::Window* window);