ozone/wayland: Do not allow to attach buffers until surface's configured

This CL fixes spurious problems when new frames are sent and a buffer is
attached earlier than WaylandSurface/WaylandPopup are configured.

Thus, add a new interface into the WaylandWindowObserver that notifies
the WaylandBufferManagerHost if the window has been configured.

The manager resets |configured_| variable upon ResetWindowContents as
long as the wl_surface becomes unmapped and it is reinitialized with
new xdg_surface, xdg_toplevel or whatever only during the next Show()
call. In that case, it will be configured again and the host will
be notified about that.

Bug: 1083949
Change-Id: I9895bc8876748ba69bc0461c2aeb300175a40065
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2207172Reviewed-by: Nick Yamane <nickdiego@igalia.com>
Commit-Queue: Maksim Sisov <msisov@igalia.com>
Cr-Commit-Position: refs/heads/master@{#770567}

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

@@ -106,7 +106,10 @@ class WaylandBufferManagerHost::Surface {
     // Another case, which always happen is waiting until the frame callback is
     // completed. Thus, wait here when the Wayland compositor fires the frame
     // callback.
-    if (!buffer->wl_buffer || wl_frame_callback_) {
+    //
+    // The third case happens if the window hasn't been configured until a
+    // request to attach a buffer to its surface is sent.
+    if (!buffer->wl_buffer || wl_frame_callback_ || !configured_) {
       pending_buffer_ = buffer;
       return true;
     }
@@ -197,6 +200,12 @@ class WaylandBufferManagerHost::Surface {
     // callback. Check more comments below where the variable is declared.
     contents_reset_ = true;
 
+    // ResetSurfaceContents happens upon WaylandWindow::Hide call, which
+    // destroyes xdg_surface, xdg_popup, etc. They are going to be reinitialized
+    // once WaylandWindow::Show is called. Thus, they will have to be configured
+    // once again before buffers can be attached.
+    configured_ = false;
+
     connection_->ScheduleFlush();
   }
 
@@ -210,6 +219,14 @@ class WaylandBufferManagerHost::Surface {
   void OnWindowRemoved() { window_ = nullptr; }
   bool HasWindow() const { return !!window_; }
 
+  void OnWindowConfigured() {
+    if (configured_)
+      return;
+
+    configured_ = true;
+    ProcessPendingBuffer();
+  }
+
  private:
   struct FeedbackInfo {
     // The wayland object identifying this feedback.
@@ -313,7 +330,7 @@ class WaylandBufferManagerHost::Surface {
   }
 
   void AttachBuffer(WaylandBuffer* buffer) {
-    DCHECK(window_);
+    DCHECK(window_ && configured_);
 
     // The logic in DamageBuffer currently relies on attachment coordinates of
     // (0, 0). If this changes, then the calculation in DamageBuffer will also
@@ -598,6 +615,11 @@ class WaylandBufferManagerHost::Surface {
   // a need to call submission callback manually.
   bool contents_reset_ = false;
 
+  // If WaylandWindow has never been configured, do not try to attach
+  // buffers to its surface. Otherwise, Wayland server will drop the connection
+  // and send an error - "The surface has never been configured.".
+  bool configured_ = false;
+
   DISALLOW_COPY_AND_ASSIGN(Surface);
 };
 
@@ -632,6 +654,13 @@ void WaylandBufferManagerHost::OnWindowRemoved(WaylandWindow* window) {
     surfaces_.erase(it);
 }
 
+void WaylandBufferManagerHost::OnWindowConfigured(WaylandWindow* window) {
+  DCHECK(window);
+  auto it = surfaces_.find(window->GetWidget());
+  DCHECK(it != surfaces_.end());
+  it->second->OnWindowConfigured();
+}
+
 void WaylandBufferManagerHost::SetTerminateGpuCallback(
     base::OnceCallback<void(std::string)> terminate_callback) {
   terminate_gpu_cb_ = std::move(terminate_callback);
@@ -923,11 +952,10 @@ bool WaylandBufferManagerHost::ValidateBufferIdFromGpu(uint32_t buffer_id) {
   return true;
 }
 
-bool WaylandBufferManagerHost::ValidateDataFromGpu(
-    const base::ScopedFD& fd,
-    size_t length,
-    const gfx::Size& size,
-    uint32_t buffer_id) {
+bool WaylandBufferManagerHost::ValidateDataFromGpu(const base::ScopedFD& fd,
+                                                   size_t length,
+                                                   const gfx::Size& size,
+                                                   uint32_t buffer_id) {
   if (!ValidateBufferIdFromGpu(buffer_id))
     return false;
 
@@ -952,21 +980,20 @@ bool WaylandBufferManagerHost::ValidateDataFromGpu(
 void WaylandBufferManagerHost::OnCreateBufferComplete(
     uint32_t buffer_id,
     wl::Object<struct wl_buffer> new_buffer) {
-    auto it = anonymous_buffers_.find(buffer_id);
-    // It might have already been destroyed or stored by any of the surfaces.
-    if (it != anonymous_buffers_.end()) {
-      it->second->wl_buffer = std::move(new_buffer);
-    } else {
-      for (auto& surface : surfaces_) {
-        if (surface.second->BufferExists(buffer_id)) {
-          surface.second.get()->AttachWlBuffer(buffer_id,
-                                               std::move(new_buffer));
-          break;
-        }
+  auto it = anonymous_buffers_.find(buffer_id);
+  // It might have already been destroyed or stored by any of the surfaces.
+  if (it != anonymous_buffers_.end()) {
+    it->second->wl_buffer = std::move(new_buffer);
+  } else {
+    for (auto& surface : surfaces_) {
+      if (surface.second->BufferExists(buffer_id)) {
+        surface.second.get()->AttachWlBuffer(buffer_id, std::move(new_buffer));
+        break;
       }
     }
-    // There is no need for the buffer anymore. Let it go out of the scope and
-    // be destroyed.
+  }
+  // There is no need for the buffer anymore. Let it go out of the scope and
+  // be destroyed.
 }
 
 void WaylandBufferManagerHost::OnSubmission(

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

@@ -81,6 +81,7 @@ class WaylandBufferManagerHost : public ozone::mojom::WaylandBufferManagerHost,
   // WaylandWindowObserver implements:
   void OnWindowAdded(WaylandWindow* window) override;
   void OnWindowRemoved(WaylandWindow* window) override;
+  void OnWindowConfigured(WaylandWindow* window) override;
 
   void SetTerminateGpuCallback(
       base::OnceCallback<void(std::string)> terminate_gpu_cb);

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

@@ -114,6 +114,11 @@ void WaylandSubsurface::CreateSubsurface() {
   wl_subsurface_set_desync(subsurface_.get());
   wl_surface_commit(parent->surface());
   connection()->ScheduleFlush();
+
+  // Notify the observers the window has been configured. Please note that
+  // subsurface doesn't send ack configure events. Thus, notify the observers as
+  // soon as the subsurface is created.
+  connection()->wayland_window_manager()->NotifyWindowConfigured(this);
 }
 
 bool WaylandSubsurface::OnInitialize(PlatformWindowInitProperties properties) {

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

@@ -20,6 +20,11 @@ void WaylandWindowManager::RemoveObserver(WaylandWindowObserver* observer) {
   observers_.RemoveObserver(observer);
 }
 
+void WaylandWindowManager::NotifyWindowConfigured(WaylandWindow* window) {
+  for (WaylandWindowObserver& observer : observers_)
+    observer.OnWindowConfigured(window);
+}
+
 void WaylandWindowManager::GrabLocatedEvents(WaylandWindow* window) {
   DCHECK_NE(located_events_grabber_, window);
 

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

@@ -27,6 +27,10 @@ class WaylandWindowManager {
   void AddObserver(WaylandWindowObserver* observer);
   void RemoveObserver(WaylandWindowObserver* observer);
 
+  // Notifies observers that the Window has been ack configured and
+  // WaylandBufferManagerHost can start attaching buffers to the |surface_|.
+  void NotifyWindowConfigured(WaylandWindow* window);
+
   // Stores the window that should grab the located events.
   void GrabLocatedEvents(WaylandWindow* event_grabber);
 

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

@@ -12,4 +12,6 @@ void WaylandWindowObserver::OnWindowAdded(WaylandWindow* window) {}
 
 void WaylandWindowObserver::OnWindowRemoved(WaylandWindow* window) {}
 
+void WaylandWindowObserver::OnWindowConfigured(WaylandWindow* window) {}
+
 }  // namespace ui

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

@@ -20,6 +20,9 @@ class WaylandWindowObserver : public base::CheckedObserver {
   // Called when |window| has been removed.
   virtual void OnWindowRemoved(WaylandWindow* window);
 
+  // Called when |window| has been ack configured.
+  virtual void OnWindowConfigured(WaylandWindow* window);
+
  protected:
   ~WaylandWindowObserver() override;
 };

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

@@ -119,6 +119,8 @@ void XDGSurfaceWrapperImpl::AckConfigure() {
     zxdg_surface_v6_ack_configure(zxdg_surface_v6_.get(),
                                   pending_configure_serial_);
   }
+  connection_->wayland_window_manager()->NotifyWindowConfigured(
+      wayland_window_);
 }
 
 void XDGSurfaceWrapperImpl::SetWindowGeometry(const gfx::Rect& bounds) {

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

@@ -97,17 +97,22 @@ void WaylandTest::SendConfigureEvent(wl::MockXdgSurface* xdg_surface,
                                      int height,
                                      uint32_t serial,
                                      struct wl_array* states) {
-  DCHECK(xdg_surface->xdg_toplevel());
   // In xdg_shell_v6+, both surfaces send serial configure event and toplevel
   // surfaces send other data like states, heights and widths.
+  // Please note that toplevel surfaces may not exist if the surface was created
+  // for the popup role.
   if (GetParam() == kXdgShellV6) {
     zxdg_surface_v6_send_configure(xdg_surface->resource(), serial);
-    zxdg_toplevel_v6_send_configure(xdg_surface->xdg_toplevel()->resource(),
-                                    width, height, states);
+    if (xdg_surface->xdg_toplevel()) {
+      zxdg_toplevel_v6_send_configure(xdg_surface->xdg_toplevel()->resource(),
+                                      width, height, states);
+    }
   } else {
     xdg_surface_send_configure(xdg_surface->resource(), serial);
-    xdg_toplevel_send_configure(xdg_surface->xdg_toplevel()->resource(), width,
-                                height, states);
+    if (xdg_surface->xdg_toplevel()) {
+      xdg_toplevel_send_configure(xdg_surface->xdg_toplevel()->resource(),
+                                  width, height, states);
+    }
   }
 }
 

ui/ozone/platform/wayland/wayland_buffer_manager_unittest.cc

@@ -187,11 +187,14 @@ class WaylandBufferManagerTest : public WaylandTest {
     }
   }
 
-  std::unique_ptr<WaylandWindow> CreateWindow() {
+  std::unique_ptr<WaylandWindow> CreateWindow(
+      PlatformWindowType type = PlatformWindowType::kWindow,
+      gfx::AcceleratedWidget parent_widget = gfx::kNullAcceleratedWidget) {
     testing::Mock::VerifyAndClearExpectations(&delegate_);
     PlatformWindowInitProperties properties;
     properties.bounds = gfx::Rect(0, 0, 800, 600);
-    properties.type = PlatformWindowType::kWindow;
+    properties.type = type;
+    properties.parent_widget = parent_widget;
     auto new_window = WaylandWindow::Create(&delegate_, connection_.get(),
                                             std::move(properties));
     EXPECT_TRUE(new_window);
@@ -726,6 +729,77 @@ TEST_P(WaylandBufferManagerTest, TestCommitBufferConditions) {
                                           false /*fail*/);
 }
 
+// Tests the surface does not have buffers attached until it's configured at
+// least once.
+TEST_P(WaylandBufferManagerTest, TestCommitBufferConditionsAckConfigured) {
+  constexpr uint32_t kDmabufBufferId = 1;
+
+  // Exercise three window types that create different windows - toplevel, popup
+  // and subsurface.
+  std::vector<PlatformWindowType> window_types{PlatformWindowType::kWindow,
+                                               PlatformWindowType::kPopup,
+                                               PlatformWindowType::kTooltip};
+
+  for (const auto& type : window_types) {
+    // If the type is not kWindow, provide default created window as parent of
+    // the newly created window.
+    auto temp_window = CreateWindow(type, type != PlatformWindowType::kWindow
+                                              ? widget_
+                                              : gfx::kNullAcceleratedWidget);
+    auto widget = temp_window->GetWidget();
+
+    // Subsurface doesn't have an interface for sending configure events.
+    // Thus, WaylandSubsurface notifies the manager that the window is
+    // activated upon creation of the subsurface. Skip calling Show() and call
+    // later then.
+    if (type != PlatformWindowType::kTooltip)
+      temp_window->Show(false);
+
+    Sync();
+
+    auto* mock_surface = server_.GetObject<wl::MockSurface>(widget);
+
+    auto* linux_dmabuf = server_.zwp_linux_dmabuf_v1();
+    EXPECT_CALL(*linux_dmabuf, CreateParams(_, _, _)).Times(1);
+
+    CreateDmabufBasedBufferAndSetTerminateExpecation(false /*fail*/,
+                                                     kDmabufBufferId);
+
+    Sync();
+
+    ProcessCreatedBufferResourcesWithExpectation(1u /* expected size */,
+                                                 false /* fail */);
+
+    EXPECT_CALL(*mock_surface, Attach(_, _, _)).Times(0);
+    EXPECT_CALL(*mock_surface, Frame(_)).Times(0);
+    EXPECT_CALL(*mock_surface, Commit()).Times(0);
+
+    buffer_manager_gpu_->CommitBuffer(widget, kDmabufBufferId,
+                                      window_->GetBounds());
+    Sync();
+
+    if (type != PlatformWindowType::kTooltip) {
+      DCHECK(mock_surface->xdg_surface());
+      ActivateSurface(mock_surface->xdg_surface());
+    } else {
+      // See the comment near Show() call above.
+      temp_window->Show(false);
+    }
+
+    EXPECT_CALL(*mock_surface, Attach(_, _, _)).Times(1);
+    EXPECT_CALL(*mock_surface, Frame(_)).Times(1);
+    EXPECT_CALL(*mock_surface, Commit()).Times(1);
+
+    Sync();
+
+    temp_window.reset();
+    DestroyBufferAndSetTerminateExpectation(widget, kDmabufBufferId,
+                                            false /*fail*/);
+
+    Sync();
+  }
+}
+
 // The buffer that is not originally attached to any of the surfaces,
 // must be attached when a commit request comes. Also, it must setup a buffer
 // release listener and OnSubmission must be called for that buffer if it is