chromeos: Fix DeviceLocalAccountTest under SingleProcessMash

Window close events are sent over mojo between browser and ash. The
test needs to wait for in-flight messages to complete before making
assertions about the state of ash.

Also fix a problem where the window delete message wasn't being sent
by WindowTreeClient.

Bug: 877118
Test: DeviceLocalAccountTest.LastWindowClosedLogoutReminder
Change-Id: I43145c8140462428a7db050489593038745c8f94
Reviewed-on: https://chromium-review.googlesource.com/c/1284059
Commit-Queue: James Cook <jamescook@chromium.org>
Reviewed-by: Pavol Marko <pmarko@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#600886}

chrome/browser/chromeos/policy/device_local_account_browsertest.cc

@@ -139,6 +139,7 @@
 #include "net/url_request/url_request_status.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "third_party/icu/source/common/unicode/locid.h"
+#include "ui/aura/test/mus/change_completion_waiter.h"
 #include "ui/base/ime/chromeos/extension_ime_util.h"
 #include "ui/base/ime/chromeos/input_method_descriptor.h"
 #include "ui/base/ime/chromeos/input_method_manager.h"
@@ -213,6 +214,25 @@ const char* const kInvalidRecommendedLocale[] = {
 const char kPublicSessionLocale[] = "de";
 const char kPublicSessionInputMethodIDTemplate[] = "_comp_ime_%sxkb:de:neo:ger";
 
+bool IsLogoutConfirmationDialogShowing() {
+  // Wait for any browser window close mojo messages to propagate to ash.
+  aura::test::WaitForAllChangesToComplete();
+
+  // TODO(mash): Add mojo test API for this.
+  return !!ash::Shell::Get()
+               ->logout_confirmation_controller()
+               ->dialog_for_testing();
+}
+
+void CloseLogoutConfirmationDialog() {
+  // TODO(mash): Add mojo test API for this.
+  ash::LogoutConfirmationDialog* dialog =
+      ash::Shell::Get()->logout_confirmation_controller()->dialog_for_testing();
+  ASSERT_TRUE(dialog);
+  dialog->GetWidget()->Close();
+  base::RunLoop().RunUntilIdle();
+}
+
 // Helper that serves extension update manifests to Chrome.
 class TestingUpdateManifestProvider
     : public base::RefCountedThreadSafe<TestingUpdateManifestProvider> {
@@ -1498,10 +1518,7 @@ IN_PROC_BROWSER_TEST_F(DeviceLocalAccountTest, LastWindowClosedLogoutReminder) {
   app_window_registry->AddObserver(this);
 
   // Verify that the logout confirmation dialog is not showing.
-  ash::LogoutConfirmationController* logout_confirmation_controller =
-      ash::Shell::Get()->logout_confirmation_controller();
-  ASSERT_TRUE(logout_confirmation_controller);
-  EXPECT_FALSE(logout_confirmation_controller->dialog_for_testing());
+  EXPECT_FALSE(IsLogoutConfirmationDialogShowing());
 
   // Remove policy that allows only explicitly whitelisted apps to be installed
   // in a public session.
@@ -1552,7 +1569,7 @@ IN_PROC_BROWSER_TEST_F(DeviceLocalAccountTest, LastWindowClosedLogoutReminder) {
 
   // Verify that the logout confirmation dialog is not showing because an app
   // window is still open.
-  EXPECT_FALSE(logout_confirmation_controller->dialog_for_testing());
+  EXPECT_FALSE(IsLogoutConfirmationDialogShowing());
 
   // Open a browser window.
   Browser* first_browser = CreateBrowser(profile);
@@ -1567,7 +1584,7 @@ IN_PROC_BROWSER_TEST_F(DeviceLocalAccountTest, LastWindowClosedLogoutReminder) {
 
   // Verify that the logout confirmation dialog is not showing because a browser
   // window is still open.
-  EXPECT_FALSE(logout_confirmation_controller->dialog_for_testing());
+  EXPECT_FALSE(IsLogoutConfirmationDialogShowing());
 
   // Open a second browser window.
   Browser* second_browser = CreateBrowser(profile);
@@ -1585,7 +1602,7 @@ IN_PROC_BROWSER_TEST_F(DeviceLocalAccountTest, LastWindowClosedLogoutReminder) {
 
   // Verify that the logout confirmation dialog is not showing because a browser
   // window is still open.
-  EXPECT_FALSE(logout_confirmation_controller->dialog_for_testing());
+  EXPECT_FALSE(IsLogoutConfirmationDialogShowing());
 
   // Close the second browser window.
   browser_window = second_browser->window();
@@ -1598,17 +1615,13 @@ IN_PROC_BROWSER_TEST_F(DeviceLocalAccountTest, LastWindowClosedLogoutReminder) {
   EXPECT_TRUE(browser_list->empty());
 
   // Verify that the logout confirmation dialog is showing.
-  ash::LogoutConfirmationDialog* dialog =
-      logout_confirmation_controller->dialog_for_testing();
-  ASSERT_TRUE(dialog);
+  EXPECT_TRUE(IsLogoutConfirmationDialogShowing());
 
   // Deny the logout.
-  dialog->GetWidget()->Close();
-  dialog = NULL;
-  base::RunLoop().RunUntilIdle();
+  ASSERT_NO_FATAL_FAILURE(CloseLogoutConfirmationDialog());
 
   // Verify that the logout confirmation dialog is no longer showing.
-  EXPECT_FALSE(logout_confirmation_controller->dialog_for_testing());
+  EXPECT_FALSE(IsLogoutConfirmationDialogShowing());
 
   // Open a browser window.
   browser = CreateBrowser(profile);
@@ -1625,13 +1638,10 @@ IN_PROC_BROWSER_TEST_F(DeviceLocalAccountTest, LastWindowClosedLogoutReminder) {
   EXPECT_TRUE(browser_list->empty());
 
   // Verify that the logout confirmation dialog is showing again.
-  dialog = logout_confirmation_controller->dialog_for_testing();
-  ASSERT_TRUE(dialog);
+  EXPECT_TRUE(IsLogoutConfirmationDialogShowing());
 
   // Deny the logout.
-  dialog->GetWidget()->Close();
-  dialog = NULL;
-  base::RunLoop().RunUntilIdle();
+  ASSERT_NO_FATAL_FAILURE(CloseLogoutConfirmationDialog());
 
   app_window_registry->RemoveObserver(this);
 };

testing/buildbot/filters/chromeos.single_process_mash.browser_tests.filter

@@ -51,9 +51,6 @@
 -PDFExtensionTest.*
 -PDFExtensionHitTestTest.*
 
-# ash::Shell access for LogoutConfirmationController
--DeviceLocalAccountTest.*
-
 # Fails: immersive_controller->IsRevealed() returns false.
 # http://crbug.com/791148
 -vZoomBubbleBrowserTest.*

ui/aura/mus/window_tree_client.cc

@@ -714,21 +714,23 @@ void WindowTreeClient::OnWindowMusDestroyed(WindowMus* window, Origin origin) {
   // deletion. The connection to the server is about to be dropped and the
   // server will take appropriate action.
   // TODO: decide how to deal with windows not owned by this client.
+  base::Optional<uint32_t> delete_change_id;
   if (!in_shutdown_ && origin == Origin::CLIENT &&
       (WasCreatedByThisClient(window) || IsRoot(window))) {
-    const uint32_t change_id =
+    delete_change_id =
         ScheduleInFlightChange(std::make_unique<CrashInFlightChange>(
             window, ChangeType::DELETE_WINDOW));
-    tree_->DeleteWindow(change_id, window->server_id());
+    tree_->DeleteWindow(delete_change_id.value(), window->server_id());
   }
 
   windows_.erase(window->server_id());
 
-  // Remove any InFlightChanges associated with the window.
+  // Remove any InFlightChanges associated with the window, except the delete.
   std::set<uint32_t> in_flight_change_ids_to_remove;
   for (const auto& pair : in_flight_map_) {
-    if (pair.second->window() == window)
-      in_flight_change_ids_to_remove.insert(pair.first);
+    const uint32_t change_id = pair.first;
+    if (pair.second->window() == window && change_id != delete_change_id)
+      in_flight_change_ids_to_remove.insert(change_id);
   }
   for (auto change_id : in_flight_change_ids_to_remove)
     in_flight_map_.erase(change_id);