ozone/wayland: Fix a startup crash in multi-monitor scenario

WaylandConnection initialization is considered done once at least one of
the wl_outputs advertised gets its "done" event (ie: all its metadata
was already received, ie: size, origin, mode, etc), which has been
working well so far, at least with the Wayland compositors tested until
now.

It turns out that such initialization is broken for multi-screen
scenarios, where the compositor sends events in the following order:

...
wl_registry::global for wl_output object 1
wl_registry::global for wl_output object 2
wl_output::geometry for wl_output 1
wl_output::mode for wl_output 1        <-- output 1 ready
wl_output::geometry for wl_output 2
wl_output::mode for wl_output 2        <-- output 2 ready
...

This is how Exo compositor sends the events, which leads Ozone/Wayland
to crash at startup. To fix it, this patch modifies code that deals with
wl_output and WaylandScreen so that it only creates display::Display
objects for wl_outputs once they are "ready" (ie: all required metadata
already received from the server). In addition, some cleanup and minor
fixes are made.

R=tonikitoo@igalia.com

Bug: 1111905
Test: Covered by unit tests.
Change-Id: I25d23c5a76d0b2ab900bab4460344cf908db07e9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2332844
Commit-Queue: Nick Yamane <nickdiego@igalia.com>
Reviewed-by: Maksim Sisov (GMT+3) <msisov@igalia.com>
Reviewed-by: Antonio Gomes (GMT-4) <tonikitoo@igalia.com>
Cr-Commit-Position: refs/heads/master@{#794255}

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

@@ -11,7 +11,7 @@
 
 namespace ui {
 
-WaylandOutput::WaylandOutput(const uint32_t output_id, wl_output* output)
+WaylandOutput::WaylandOutput(uint32_t output_id, wl_output* output)
     : output_id_(output_id),
       output_(output),
       scale_factor_(kDefaultScaleFactor),
@@ -31,7 +31,7 @@ void WaylandOutput::Initialize(Delegate* delegate) {
   wl_output_add_listener(output_.get(), &output_listener, this);
 }
 
-void WaylandOutput::TriggerDelegateNotification() const {
+void WaylandOutput::TriggerDelegateNotifications() const {
   DCHECK(!rect_in_physical_pixels_.IsEmpty());
   delegate_->OnOutputHandleMetrics(output_id_, rect_in_physical_pixels_,
                                    scale_factor_);
@@ -67,9 +67,8 @@ void WaylandOutput::OutputHandleMode(void* data,
 
 // static
 void WaylandOutput::OutputHandleDone(void* data, struct wl_output* wl_output) {
-  WaylandOutput* wayland_output = static_cast<WaylandOutput*>(data);
-  if (wayland_output)
-    wayland_output->TriggerDelegateNotification();
+  if (auto* output = static_cast<WaylandOutput*>(data))
+    output->TriggerDelegateNotifications();
 }
 
 // static

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

@@ -20,23 +20,23 @@ class WaylandOutput {
  public:
   class Delegate {
    public:
-    virtual ~Delegate() {}
-
     virtual void OnOutputHandleMetrics(uint32_t output_id,
                                        const gfx::Rect& new_bounds,
                                        int32_t scale_factor) = 0;
+
+   protected:
+    virtual ~Delegate() = default;
   };
 
-  WaylandOutput(const uint32_t output_id, wl_output* output);
+  WaylandOutput(uint32_t output_id, wl_output* output);
   ~WaylandOutput();
 
   void Initialize(Delegate* delegate);
 
-  void TriggerDelegateNotification() const;
-
   uint32_t output_id() const { return output_id_; }
   bool has_output(wl_output* output) const { return output_.get() == output; }
   int32_t scale_factor() const { return scale_factor_; }
+  gfx::Rect bounds() const { return rect_in_physical_pixels_; }
 
   // Tells if the output has already received physical screen dimensions in the
   // global compositor space.
@@ -45,6 +45,8 @@ class WaylandOutput {
  private:
   static constexpr int32_t kDefaultScaleFactor = 1;
 
+  void TriggerDelegateNotifications() const;
+
   // Callback functions used for setting geometric properties of the output
   // and available modes.
   static void OutputHandleGeometry(void* data,

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

@@ -4,6 +4,8 @@
 
 #include "ui/ozone/platform/wayland/host/wayland_output_manager.h"
 
+#include <algorithm>
+#include <cstdint>
 #include <memory>
 
 #include "ui/ozone/platform/wayland/host/wayland_connection.h"
@@ -15,13 +17,16 @@ WaylandOutputManager::WaylandOutputManager() = default;
 
 WaylandOutputManager::~WaylandOutputManager() = default;
 
+// Output is considered ready when at least one wl_output is fully configured
+// (i.e: wl_output::done received), so that WaylandOutputManager is able to
+// instantiate a valid WaylandScreen when requested by the upper layer.
 bool WaylandOutputManager::IsOutputReady() const {
-  if (output_list_.empty())
-    return false;
-  return output_list_.front()->is_ready();
+  return std::find_if(output_list_.begin(), output_list_.end(),
+                      [](const auto& output) { return output->is_ready(); }) !=
+         output_list_.end();
 }
 
-void WaylandOutputManager::AddWaylandOutput(const uint32_t output_id,
+void WaylandOutputManager::AddWaylandOutput(uint32_t output_id,
                                             wl_output* output) {
   // Make sure an output with |output_id| has not been added yet. It's very
   // unlikely to happen, unless a compositor has a bug in the numeric names
@@ -29,26 +34,26 @@ void WaylandOutputManager::AddWaylandOutput(const uint32_t output_id,
   auto output_it = GetOutputItById(output_id);
   DCHECK(output_it == output_list_.end());
   auto wayland_output = std::make_unique<WaylandOutput>(output_id, output);
-  WaylandOutput* wayland_output_ptr = wayland_output.get();
-  output_list_.push_back(std::move(wayland_output));
-
-  OnWaylandOutputAdded(output_id);
 
   // Even if WaylandScreen has not been created, the output still must be
   // initialized, which results in setting up a wl_listener and getting the
   // geometry and the scaling factor from the Wayland Compositor.
-  wayland_output_ptr->Initialize(this);
+  wayland_output->Initialize(this);
+  DCHECK(!wayland_output->is_ready());
+
+  output_list_.push_back(std::move(wayland_output));
 }
 
-void WaylandOutputManager::RemoveWaylandOutput(const uint32_t output_id) {
+void WaylandOutputManager::RemoveWaylandOutput(uint32_t output_id) {
   auto output_it = GetOutputItById(output_id);
 
   // Check the comment in the WaylandConnetion::GlobalRemove.
   if (output_it == output_list_.end())
     return;
 
+  if (wayland_screen_)
+    wayland_screen_->OnOutputRemoved(output_id);
   output_list_.erase(output_it);
-  OnWaylandOutputRemoved(output_id);
 }
 
 std::unique_ptr<WaylandScreen> WaylandOutputManager::CreateWaylandScreen(
@@ -64,10 +69,10 @@ std::unique_ptr<WaylandScreen> WaylandOutputManager::CreateWaylandScreen(
   // OutOutputHandleScale. All the other hot geometry and scale changes are done
   // automatically, and the |wayland_screen_| is notified immediately about the
   // changes.
-  if (!output_list_.empty()) {
-    for (auto& output : output_list_) {
-      OnWaylandOutputAdded(output->output_id());
-      output->TriggerDelegateNotification();
+  for (const auto& output : output_list_) {
+    if (output->is_ready()) {
+      wayland_screen->OnOutputAddedOrUpdated(
+          output->output_id(), output->bounds(), output->scale_factor());
     }
   }
 
@@ -90,22 +95,13 @@ WaylandOutput* WaylandOutputManager::GetOutput(uint32_t id) const {
   return output_it->get();
 }
 
-void WaylandOutputManager::OnWaylandOutputAdded(uint32_t output_id) {
-  if (wayland_screen_)
-    wayland_screen_->OnOutputAdded(output_id);
-}
-
-void WaylandOutputManager::OnWaylandOutputRemoved(uint32_t output_id) {
-  if (wayland_screen_)
-    wayland_screen_->OnOutputRemoved(output_id);
-}
-
 void WaylandOutputManager::OnOutputHandleMetrics(uint32_t output_id,
                                                  const gfx::Rect& new_bounds,
                                                  int32_t scale_factor) {
-  if (wayland_screen_)
-    wayland_screen_->OnOutputMetricsChanged(output_id, new_bounds,
+  if (wayland_screen_) {
+    wayland_screen_->OnOutputAddedOrUpdated(output_id, new_bounds,
                                             scale_factor);
+  }
 }
 
 WaylandOutputManager::OutputList::const_iterator

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

@@ -44,9 +44,6 @@ class WaylandOutputManager : public WaylandOutput::Delegate {
   WaylandScreen* wayland_screen() const { return wayland_screen_.get(); }
 
  private:
-  void OnWaylandOutputAdded(uint32_t output_id);
-  void OnWaylandOutputRemoved(uint32_t output_id);
-
   // WaylandOutput::Delegate:
   void OnOutputHandleMetrics(uint32_t output_id,
                              const gfx::Rect& new_bounds,

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

@@ -10,8 +10,10 @@
 #include "base/stl_util.h"
 #include "ui/display/display.h"
 #include "ui/display/display_finder.h"
+#include "ui/display/display_list.h"
 #include "ui/display/display_observer.h"
 #include "ui/gfx/geometry/point.h"
+#include "ui/gfx/geometry/rect.h"
 #include "ui/gfx/native_widget_types.h"
 #include "ui/ozone/platform/wayland/host/wayland_connection.h"
 #include "ui/ozone/platform/wayland/host/wayland_cursor_position.h"
@@ -26,9 +28,10 @@ WaylandScreen::WaylandScreen(WaylandConnection* connection)
 
 WaylandScreen::~WaylandScreen() = default;
 
-void WaylandScreen::OnOutputAdded(uint32_t output_id) {
-  display_list_.AddDisplay(display::Display(output_id),
-                           display::DisplayList::Type::NOT_PRIMARY);
+void WaylandScreen::OnOutputAddedOrUpdated(uint32_t output_id,
+                                           const gfx::Rect& bounds,
+                                           int32_t scale) {
+  AddOrUpdateDisplay(output_id, bounds, scale);
 }
 
 void WaylandScreen::OnOutputRemoved(uint32_t output_id) {
@@ -49,41 +52,32 @@ void WaylandScreen::OnOutputRemoved(uint32_t output_id) {
   display_list_.RemoveDisplay(output_id);
 }
 
-void WaylandScreen::OnOutputMetricsChanged(uint32_t output_id,
-                                           const gfx::Rect& new_bounds,
-                                           int32_t device_pixel_ratio) {
+void WaylandScreen::AddOrUpdateDisplay(uint32_t output_id,
+                                       const gfx::Rect& new_bounds,
+                                       int32_t scale_factor) {
   display::Display changed_display(output_id);
   if (!display::Display::HasForceDeviceScaleFactor())
-    changed_display.set_device_scale_factor(device_pixel_ratio);
+    changed_display.set_device_scale_factor(scale_factor);
   changed_display.set_bounds(new_bounds);
   changed_display.set_work_area(new_bounds);
 
-  bool is_primary = false;
-  display::Display display_nearest_origin =
-      GetDisplayNearestPoint(gfx::Point(0, 0));
-  // If bounds of the nearest to origin display are empty, it must have been the
-  // very first and the same display added before.
-  if (display_nearest_origin.bounds().IsEmpty()) {
-    DCHECK_EQ(display_nearest_origin.id(), changed_display.id());
-    is_primary = true;
-  } else if (changed_display.bounds().origin() <
-             display_nearest_origin.bounds().origin()) {
-    // If changed display is nearer to the origin than the previous display,
-    // that one must become a primary display.
-    is_primary = true;
-  } else if (changed_display.bounds().OffsetFromOrigin() ==
-             display_nearest_origin.bounds().OffsetFromOrigin()) {
-    // If changed display has the same origin as the nearest to origin display,
-    // |changed_display| must become a primary one or it has already been the
-    // primary one. If a user changed positions of two displays (the second at
-    // x,x was set to 0,0), the second change will modify geometry of the
-    // display, which used to be the one nearest to the origin.
-    is_primary = true;
+  // There are 2 cases where |changed_display| must be set as primary:
+  // 1. When it is the first one being added to the |display_list_|. Or
+  // 2. If it is nearest the origin than the previous primary or has the same
+  // origin as it. When an user, for example, swaps two side-by-side displays,
+  // at some point, as the notification come in, both will have the same
+  // origin.
+  auto type = display::DisplayList::Type::NOT_PRIMARY;
+  if (display_list_.displays().empty()) {
+    type = display::DisplayList::Type::PRIMARY;
+  } else {
+    auto nearest_origin = GetDisplayNearestPoint({0, 0}).bounds().origin();
+    auto changed_origin = changed_display.bounds().origin();
+    if (changed_origin < nearest_origin || changed_origin == nearest_origin)
+      type = display::DisplayList::Type::PRIMARY;
   }
 
-  display_list_.UpdateDisplay(
-      changed_display, is_primary ? display::DisplayList::Type::PRIMARY
-                                  : display::DisplayList::Type::NOT_PRIMARY);
+  display_list_.AddOrUpdateDisplay(changed_display, type);
 
   auto* wayland_window_manager = connection_->wayland_window_manager();
   for (auto* window : wayland_window_manager->GetWindowsOnOutput(output_id))

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

@@ -14,6 +14,10 @@
 #include "ui/gfx/geometry/point.h"
 #include "ui/ozone/public/platform_screen.h"
 
+namespace gfx {
+class Rect;
+}
+
 namespace ui {
 
 class WaylandConnection;
@@ -26,11 +30,10 @@ class WaylandScreen : public PlatformScreen {
   WaylandScreen& operator=(const WaylandScreen&) = delete;
   ~WaylandScreen() override;
 
-  void OnOutputAdded(uint32_t output_id);
-  void OnOutputRemoved(uint32_t output_id);
-  void OnOutputMetricsChanged(uint32_t output_id,
+  void OnOutputAddedOrUpdated(uint32_t output_id,
                               const gfx::Rect& bounds,
                               int32_t output_scale);
+  void OnOutputRemoved(uint32_t output_id);
 
   base::WeakPtr<WaylandScreen> GetWeakPtr();
 
@@ -53,6 +56,10 @@ class WaylandScreen : public PlatformScreen {
   void RemoveObserver(display::DisplayObserver* observer) override;
 
  private:
+  void AddOrUpdateDisplay(uint32_t output_id,
+                          const gfx::Rect& bounds,
+                          int32_t scale);
+
   WaylandConnection* connection_ = nullptr;
 
   display::DisplayList display_list_;

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

@@ -159,7 +159,15 @@ TEST_P(WaylandWindowManagerTest, GetCurrentKeyboardFocusedWindow) {
 TEST_P(WaylandWindowManagerTest, GetWindowsOnOutput) {
   MockPlatformWindowDelegate delegate;
 
+  // Create a second wl_output.
   wl::TestOutput* output = server_.CreateAndInitializeOutput();
+  Sync();
+
+  // Place it at the right of the first output.
+  int x = server_.output()->GetRect().x();
+  output->SetRect({x, 0, 800, 600});
+  output->Flush();
+  Sync();
 
   auto window1 = CreateWaylandWindowWithParams(PlatformWindowType::kWindow,
                                                kDefaultBounds, &delegate);
@@ -195,6 +203,9 @@ TEST_P(WaylandWindowManagerTest, GetWindowsOnOutput) {
 
   windows_on_output = manager_->GetWindowsOnOutput(output_id);
   EXPECT_EQ(2u, windows_on_output.size());
+
+  output->DestroyGlobal();
+  Sync();
 }
 
 TEST_P(WaylandWindowManagerTest, GetAllWindows) {

ui/ozone/platform/wayland/test/test_output.cc

@@ -6,6 +6,8 @@
 
 #include <wayland-server-protocol.h>
 
+#include "base/optional.h"
+
 namespace wl {
 
 namespace {
@@ -17,23 +19,48 @@ TestOutput::TestOutput()
 
 TestOutput::~TestOutput() = default;
 
-// Notify clients of the change for output position.
-void TestOutput::OnBind() {
-  if (rect_.IsEmpty())
+void TestOutput::SetRect(const gfx::Rect& rect) {
+  pending_rect_ = rect;
+}
+
+void TestOutput::SetScale(int32_t factor) {
+  pending_scale_ = factor;
+}
+
+void TestOutput::Flush() {
+  constexpr char kUnknownMake[] = "unknown_make";
+  constexpr char kUnknownModel[] = "unknown_model";
+
+  if (!pending_rect_ && !pending_scale_)
     return;
 
-  const char* kUnknownMake = "unknown";
-  const char* kUnknownModel = "unknown";
-  wl_output_send_geometry(resource(), rect_.x(), rect_.y(), 0, 0, 0,
-                          kUnknownMake, kUnknownModel, 0);
-  wl_output_send_mode(resource(), WL_OUTPUT_MODE_CURRENT, rect_.width(),
-                      rect_.height(), 0);
+  if (pending_rect_) {
+    rect_ = std::move(pending_rect_.value());
+    wl_output_send_geometry(resource(), rect_.x(), rect_.y(),
+                            0 /* physical_width */, 0 /* physical_height */,
+                            0 /* subpixel */, kUnknownMake, kUnknownModel,
+                            0 /* transform */);
+    wl_output_send_mode(resource(), WL_OUTPUT_MODE_CURRENT, rect_.width(),
+                        rect_.height(), 0);
+  }
+
+  if (pending_scale_) {
+    scale_ = std::move(pending_scale_.value());
+    wl_output_send_scale(resource(), scale_);
+  }
   wl_output_send_done(resource());
 }
 
-void TestOutput::SetScale(int32_t factor) {
-  wl_output_send_scale(resource(), factor);
-  wl_output_send_done(resource());
+// Notifies clients about the changes in the output configuration, if any. Doing
+// this at bind time is the most common behavior among Wayland compositors. But
+// there are some compositors that do it "lazily". An example is ChromeOS'
+// Exosphere.
+//
+// Such behavior can be emulated with this class, by just instantiating an
+// object with no setter calls. Such calls might then be done later on demand,
+// so clients get notified about such changes when Flush() is called.
+void TestOutput::OnBind() {
+  Flush();
 }
 
 }  // namespace wl

ui/ozone/platform/wayland/test/test_output.h

@@ -5,7 +5,10 @@
 #ifndef UI_OZONE_PLATFORM_WAYLAND_TEST_TEST_OUTPUT_H_
 #define UI_OZONE_PLATFORM_WAYLAND_TEST_TEST_OUTPUT_H_
 
+#include <cstdint>
+
 #include "base/macros.h"
+#include "base/optional.h"
 #include "ui/gfx/geometry/rect.h"
 #include "ui/ozone/platform/wayland/test/global_object.h"
 
@@ -16,14 +19,22 @@ class TestOutput : public GlobalObject {
  public:
   TestOutput();
   ~TestOutput() override;
-  void SetRect(const gfx::Rect rect) { rect_ = rect; }
-  const gfx::Rect GetRect() { return rect_; }
-  void OnBind() override;
 
+  const gfx::Rect GetRect() { return rect_; }
+  void SetRect(const gfx::Rect& rect);
   void SetScale(int32_t factor);
 
+  void Flush();
+
+ protected:
+  void OnBind() override;
+
  private:
   gfx::Rect rect_;
+  int32_t scale_;
+
+  base::Optional<gfx::Rect> pending_rect_ = base::nullopt;
+  base::Optional<int32_t> pending_scale_ = base::nullopt;
 
   DISALLOW_COPY_AND_ASSIGN(TestOutput);
 };

ui/ozone/platform/wayland/test/test_wayland_server_thread.cc

@@ -4,10 +4,12 @@
 
 #include "ui/ozone/platform/wayland/test/test_wayland_server_thread.h"
 
-#include <stdlib.h>
 #include <sys/socket.h>
 #include <wayland-server.h>
+
+#include <cstdlib>
 #include <memory>
+#include <utility>
 
 #include "base/bind.h"
 #include "base/files/file_util.h"
@@ -50,10 +52,6 @@ bool TestWaylandServerThread::Start(uint32_t shell_version) {
   base::ScopedFD server_fd(fd[0]);
   base::ScopedFD client_fd(fd[1]);
 
-  // If client has not specified rect before, user standard ones.
-  if (output_.GetRect().IsEmpty())
-    output_.SetRect(gfx::Rect(0, 0, 800, 600));
-
   if (wl_display_init_shm(display_.get()) < 0)
     return false;
   if (!compositor_.Initialize(display_.get()))
@@ -62,6 +60,8 @@ bool TestWaylandServerThread::Start(uint32_t shell_version) {
     return false;
   if (!output_.Initialize(display_.get()))
     return false;
+  SetupOutputs();
+
   if (!data_device_manager_.Initialize(display_.get()))
     return false;
   if (!seat_.Initialize(display_.get()))
@@ -115,6 +115,18 @@ MockWpPresentation* TestWaylandServerThread::EnsureWpPresentation() {
   return nullptr;
 }
 
+// By default, just make sure primary screen has bounds set. Otherwise delegates
+// it, making it possible to emulate different scenarios, such as, multi-screen,
+// lazy configuration, arbitrary ordering of the outputs metadata sending, etc.
+void TestWaylandServerThread::SetupOutputs() {
+  if (output_delegate_) {
+    output_delegate_->SetupOutputs(&output_);
+    return;
+  }
+  if (output_.GetRect().IsEmpty())
+    output_.SetRect(gfx::Rect{0, 0, 800, 600});
+}
+
 void TestWaylandServerThread::DoPause() {
   base::RunLoop().RunUntilIdle();
   pause_event_.Signal();

ui/ozone/platform/wayland/test/test_wayland_server_thread.h

@@ -5,9 +5,10 @@
 #ifndef UI_OZONE_PLATFORM_WAYLAND_TEST_TEST_WAYLAND_SERVER_THREAD_H_
 #define UI_OZONE_PLATFORM_WAYLAND_TEST_TEST_WAYLAND_SERVER_THREAD_H_
 
+#include <wayland-server-core.h>
+
 #include <memory>
 #include <vector>
-#include <wayland-server-core.h>
 
 #include "base/message_loop/message_pump_libevent.h"
 #include "base/synchronization/waitable_event.h"
@@ -37,6 +38,8 @@ struct DisplayDeleter {
 class TestWaylandServerThread : public base::Thread,
                                 base::MessagePumpLibevent::FdWatcher {
  public:
+  class OutputDelegate;
+
   TestWaylandServerThread();
   ~TestWaylandServerThread() override;
 
@@ -84,7 +87,12 @@ class TestWaylandServerThread : public base::Thread,
 
   wl_display* display() const { return display_.get(); }
 
+  void set_output_delegate(OutputDelegate* delegate) {
+    output_delegate_ = delegate;
+  }
+
  private:
+  void SetupOutputs();
   void DoPause();
 
   std::unique_ptr<base::MessagePump> CreateMessagePump();
@@ -116,9 +124,22 @@ class TestWaylandServerThread : public base::Thread,
 
   base::MessagePumpLibevent::FdWatchController controller_;
 
+  OutputDelegate* output_delegate_ = nullptr;
+
   DISALLOW_COPY_AND_ASSIGN(TestWaylandServerThread);
 };
 
+class TestWaylandServerThread::OutputDelegate {
+ public:
+  // Tests may implement this such that it emulates different display/output
+  // test scenarios. For example, multi-screen, lazy configuration, arbitrary
+  // ordering of the outputs metadata events, etc.
+  virtual void SetupOutputs(TestOutput* primary_output) = 0;
+
+ protected:
+  virtual ~OutputDelegate() = default;
+};
+
 }  // namespace wl
 
 #endif  // UI_OZONE_PLATFORM_WAYLAND_TEST_TEST_WAYLAND_SERVER_THREAD_H_