[M116 Merge][wayland] Keep Output state consistent when propagating n…

…otifications

This CL updates OnOutputRemoved() such that both display_id_map_ and
display_list_ are updated at the same time after a new primary
display has been set.

Currently we are hitting an error condition where the display is
removed from the display_id_map_ first, then the primary display
is updated, then it is removed from display_list_.

As clients respond to the change of primary display, they may call
back into WaylandScreen and end up hitting DCHECKs/CHECKs due to
inconsistency between the two structures.

Specifically this was happening when updating the primary
display in OnOutputRemoved(). Clients could end up calling into
WaylandWindow::GetPreferredEnteredOutputId(), which had the
potential to check the state of display_list_.

(cherry picked from commit 367e530)

Bug: 1408304
Change-Id: I517ccb394c565dd04c8d1ab644eb5fd672e8d19f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4633241
Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
Commit-Queue: Thomas Lukaszewicz <tluk@chromium.org>
Code-Coverage: Findit <findit-for-me@appspot.gserviceaccount.com>
Cr-Original-Commit-Position: refs/heads/main@{#1161606}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4643752
Auto-Submit: Thomas Lukaszewicz <tluk@chromium.org>
Cr-Commit-Position: refs/branch-heads/5845@{#94}
Cr-Branched-From: 5a5dff6-refs/heads/main@{#1160321}

ui/ozone/platform/wayland/host/wayland_screen.cc

@@ -144,9 +144,8 @@ void WaylandScreen::OnOutputRemoved(WaylandOutput::Id output_id) {
   if (iter == display_id_map_.end()) {
     return;
   }
-  int64_t display_id = iter->second;
-  display_id_map_.erase(iter);
 
+  int64_t display_id = iter->second;
   if (display_id == GetPrimaryDisplay().id()) {
     // First, set a new primary display as required by the |display_list_|. It's
     // safe to set any of the displays to be a primary one. Once the output is
@@ -162,6 +161,13 @@ void WaylandScreen::OnOutputRemoved(WaylandOutput::Id output_id) {
       }
     }
   }
+
+  // The `display_id_map_` and the `display_list_` must be updated at the same
+  // time to ensure internal display state is consistent. Code may otherwise
+  // draw different conclusions on the availability of a display depending on
+  // which of these structures are queried (see crbug.com/1408304).
+  display_id_map_.erase(iter);
+
   auto it = display_list_.FindDisplayById(display_id);
   if (it != display_list_.displays().end())
     display_list_.RemoveDisplay(display_id);
@@ -528,6 +534,25 @@ display::TabletState WaylandScreen::GetTabletState() const {
 }
 #endif
 
+bool WaylandScreen::VerifyOutputStateConsistentForTesting() const {
+  // The number of displays tracked by the display_list_ and the display_id_map_
+  // should match.
+  const auto& displays = display_list_.displays();
+  if (display_id_map_.size() != displays.size()) {
+    return false;
+  }
+
+  // Both the display_list_ and the display_id_map_ should be tracking the same
+  // displays.
+  for (const auto& pair : display_id_map_) {
+    if (base::ranges::find(displays, pair.second, &display::Display::id) ==
+        displays.end()) {
+      return false;
+    }
+  }
+  return true;
+}
+
 void WaylandScreen::DumpState(std::ostream& out) const {
   out << "WaylandScreen:" << std::endl;
   for (const auto& display : display_list_.displays()) {

ui/ozone/platform/wayland/host/wayland_screen.h

@@ -84,6 +84,10 @@ class WaylandScreen : public PlatformScreen {
 
   void DumpState(std::ostream& out) const;
 
+  // True if the internal representations for output objects is consistent for
+  // the screen.
+  bool VerifyOutputStateConsistentForTesting() const;
+
  protected:
   // Suspends or un-suspends the platform-specific screensaver, and returns
   // whether the operation was successful. Can be called more than once with the

ui/ozone/platform/wayland/host/wayland_screen_unittest.cc

@@ -83,17 +83,25 @@ class TestDisplayObserver : public display::DisplayObserver {
                                uint32_t changed_metrics) override {
     changed_metrics_ = changed_metrics;
     display_ = display;
+    if (display_metrics_changed_closure_) {
+      display_metrics_changed_closure_.Run();
+    }
   }
 
   void set_did_remove_display_closure(base::RepeatingClosure closure) {
     did_remove_display_closure_ = std::move(closure);
   }
 
+  void set_display_metrics_changed_closure(base::RepeatingClosure closure) {
+    display_metrics_changed_closure_ = std::move(closure);
+  }
+
  private:
   uint32_t changed_metrics_ = 0;
   display::Display display_;
   display::Display removed_display_;
   base::RepeatingClosure did_remove_display_closure_{};
+  base::RepeatingClosure display_metrics_changed_closure_{};
 };
 
 }  // namespace
@@ -1117,6 +1125,46 @@ TEST_P(WaylandScreenTest, DualOutput) {
   EXPECT_EQ(3u, platform_screen_->GetAllDisplays().size());
 }
 
+// Regression test for crbug.com/1408304. Ensures that the WaylandScreen's
+// internal output state is consistent when propagating change notifications to
+// clients.
+TEST_P(WaylandScreenTest, OutputStateIsConsistentWhenNotifyingObservers) {
+  // This has to be stored on the client thread, but must be used only on the
+  // server thread.
+  wl::TestOutput* output1 = nullptr;
+  wl::TestOutput* output2 = nullptr;
+
+  // Test to ensure that WaylandScreen output state remains consistent as
+  // metrics changed notifications are propagated.
+  TestDisplayObserver observer;
+  observer.set_display_metrics_changed_closure(
+      base::BindLambdaForTesting([&]() {
+        EXPECT_TRUE(platform_screen_->VerifyOutputStateConsistentForTesting());
+      }));
+  platform_screen_->AddObserver(&observer);
+
+  PostToServerAndWait([&output1](wl::TestWaylandServerThread* server) {
+    output1 = server->output();
+  });
+  ASSERT_FALSE(output1->GetPhysicalSize().IsEmpty());
+
+  // Add a second display. The second display is located to the right of first
+  // display like | || |.
+  PostToServerAndWait(
+      [&output2, &output1](wl::TestWaylandServerThread* server) {
+        output2 = server->CreateAndInitializeOutput(
+            wl::TestOutputMetrics({GetRightX(output1), 0, 800, 600}));
+        ASSERT_TRUE(output2);
+      });
+  WaitForAllDisplaysReady();
+
+  // Destroy primary display.
+  PostToServerAndWait([&output1](wl::TestWaylandServerThread* server) {
+    output1->DestroyGlobal();
+    output1 = nullptr;
+  });
+}
+
 #if BUILDFLAG(IS_CHROMEOS_LACROS)
 
 class WaylandAuraShellScreenTest : public WaylandScreenTest {