[ozone/wayland] Sway: avoid sending presentation early.

In Sway, presentation callbacks may come much earlier than we send
submission callbacks. That results in unexpected crashes in
GbmSurfacelessWayland, because of early presentation callbacks.
Briefly, a submitted frame may not receive the submission
callback and not be moved to the list waiting for presentation
frames. That means GbmSurfacelessWayland::OnPresentation accesses
an invalid pointer to a frame when the presentation feedback comes.

Bug: 974456
Change-Id: Iae7ab57a00d06f0dde6057ed05df885239e099bd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1719013Reviewed-by: Robert Kroeger <rjkroege@chromium.org>
Commit-Queue: Maksim Sisov <msisov@igalia.com>
Cr-Commit-Position: refs/heads/master@{#682157}

ui/ozone/platform/wayland/BUILD.gn

@@ -177,6 +177,8 @@ source_set("test_support") {
     "test/mock_pointer.h",
     "test/mock_surface.cc",
     "test/mock_surface.h",
+    "test/mock_wp_presentation.cc",
+    "test/mock_wp_presentation.h",
     "test/mock_xdg_popup.cc",
     "test/mock_xdg_popup.h",
     "test/mock_xdg_shell.cc",
@@ -229,6 +231,7 @@ source_set("test_support") {
     "//testing/gmock",
     "//third_party/wayland:wayland_server",
     "//third_party/wayland-protocols:linux_dmabuf_protocol",
+    "//third_party/wayland-protocols:presentation_time_protocol",
     "//third_party/wayland-protocols:text_input_protocol",
     "//third_party/wayland-protocols:xdg_shell_protocol",
     "//ui/gfx/geometry:geometry",

ui/ozone/platform/wayland/gpu/wayland_surface_gpu.h

@@ -22,6 +22,8 @@ namespace ui {
 // the buffer.
 class WaylandSurfaceGpu {
  public:
+  virtual ~WaylandSurfaceGpu() {}
+
   // Tells the surface the result of the last swap of buffer with the
   // |buffer_id|.
   virtual void OnSubmission(uint32_t buffer_id,

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

@@ -228,6 +228,12 @@ class WaylandBufferManagerHost::Surface {
     // surface can tell the gpu about successful swap.
     bool released = true;
 
+    // In some cases, a presentation feedback can come earlier than we fire a
+    // submission callback. Thus, instead of sending it immediately to the GPU
+    // process, we store it and fire as soon as the submission callback is
+    // fired.
+    bool needs_send_feedback = false;
+
     gfx::PresentationFeedback feedback;
 
     DISALLOW_COPY_AND_ASSIGN(WaylandBuffer);
@@ -355,6 +361,11 @@ class WaylandBufferManagerHost::Surface {
   void CompleteSubmission() {
     DCHECK(submitted_buffer_);
     auto id = submitted_buffer_->buffer_id;
+
+    auto feedback = std::move(submitted_buffer_->feedback);
+    bool needs_send_feedback = submitted_buffer_->needs_send_feedback;
+    submitted_buffer_->needs_send_feedback = false;
+
     prev_submitted_buffer_ = submitted_buffer_;
     submitted_buffer_ = nullptr;
     // We can now complete the latest submission. We had to wait for this
@@ -370,14 +381,22 @@ class WaylandBufferManagerHost::Surface {
       OnPresentation(id, gfx::PresentationFeedback(
                              base::TimeTicks::Now(), base::TimeDelta(),
                              GetPresentationKindFlags(0)));
+    } else if (needs_send_feedback) {
+      OnPresentation(id, std::move(feedback));
     }
   }
 
   void OnPresentation(uint32_t buffer_id,
                       const gfx::PresentationFeedback& feedback) {
-    // The order of submission and presentation callbacks is checked on the GPU
-    // side, but it must never happen, because the Submission is called
-    // immediately after the buffer is swapped.
+    // The order of submission and presentation callbacks cannot be controlled.
+    // Some Wayland compositors may fire presentation callbacks earlier than we
+    // are able to send submission callbacks and this is bad. Thus, handle it here.
+    if (submitted_buffer_ && submitted_buffer_->buffer_id == buffer_id) {
+      submitted_buffer_->needs_send_feedback = true;
+      submitted_buffer_->feedback = feedback;
+      return;
+    }
+
     buffer_manager_->OnPresentation(window_->GetWidget(), buffer_id, feedback);
   }
 

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

@@ -13,7 +13,8 @@ void Attach(wl_client* client,
             wl_resource* buffer_resource,
             int32_t x,
             int32_t y) {
-  GetUserDataAs<MockSurface>(resource)->Attach(buffer_resource, x, y);
+  auto* surface = GetUserDataAs<MockSurface>(resource);
+  surface->AttachNewBuffer(buffer_resource, x, y);
 }
 
 void SetOpaqueRegion(wl_client* client,
@@ -40,7 +41,13 @@ void Damage(wl_client* client,
 void Frame(struct wl_client* client,
            struct wl_resource* resource,
            uint32_t callback) {
-  GetUserDataAs<MockSurface>(resource)->Frame(callback);
+  auto* surface = GetUserDataAs<MockSurface>(resource);
+
+  wl_resource* callback_resource =
+      wl_resource_create(client, &wl_callback_interface, 1, callback);
+  surface->set_frame_callback(callback_resource);
+
+  surface->Frame(callback);
 }
 
 void Commit(wl_client* client, wl_resource* resource) {
@@ -89,4 +96,37 @@ MockSurface* MockSurface::FromResource(wl_resource* resource) {
   return GetUserDataAs<MockSurface>(resource);
 }
 
+void MockSurface::AttachNewBuffer(wl_resource* buffer_resource,
+                                  int32_t x,
+                                  int32_t y) {
+  if (attached_buffer_) {
+    DCHECK(!prev_attached_buffer_);
+    prev_attached_buffer_ = attached_buffer_;
+  }
+  attached_buffer_ = buffer_resource;
+
+  Attach(buffer_resource, x, y);
+}
+
+void MockSurface::ReleasePrevAttachedBuffer() {
+  if (!prev_attached_buffer_)
+    return;
+
+  wl_buffer_send_release(prev_attached_buffer_);
+  wl_client_flush(wl_resource_get_client(prev_attached_buffer_));
+  prev_attached_buffer_ = nullptr;
+}
+
+void MockSurface::SendFrameCallback() {
+  if (!frame_callback_)
+    return;
+
+  wl_callback_send_done(
+      frame_callback_,
+      0 /* trequest-specific data for the callback. not used */);
+  wl_client_flush(wl_resource_get_client(frame_callback_));
+  wl_resource_destroy(frame_callback_);
+  frame_callback_ = nullptr;
+}
+
 }  // namespace wl

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

@@ -51,10 +51,24 @@ class MockSurface : public ServerObject {
   }
   MockXdgPopup* xdg_popup() const { return xdg_popup_.get(); }
 
+  void set_frame_callback(wl_resource* callback_resource) {
+    DCHECK(!frame_callback_);
+    frame_callback_ = callback_resource;
+  }
+
+  void AttachNewBuffer(wl_resource* buffer_resource, int32_t x, int32_t y);
+  void ReleasePrevAttachedBuffer();
+  void SendFrameCallback();
+
  private:
   std::unique_ptr<MockXdgSurface> xdg_surface_;
   std::unique_ptr<MockXdgPopup> xdg_popup_;
 
+  wl_resource* frame_callback_ = nullptr;
+
+  wl_resource* attached_buffer_ = nullptr;
+  wl_resource* prev_attached_buffer_ = nullptr;
+
   DISALLOW_COPY_AND_ASSIGN(MockSurface);
 };
 

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

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "ui/ozone/platform/wayland/test/mock_wp_presentation.h"
+
+#include <wayland-server-core.h>
+
+#include "ui/ozone/platform/wayland/test/server_object.h"
+
+namespace wl {
+
+namespace {
+
+void Feedback(struct wl_client* client,
+              struct wl_resource* resource,
+              struct wl_resource* surface,
+              uint32_t callback) {
+  auto* wp_presentation = GetUserDataAs<MockWpPresentation>(resource);
+  wl_resource* presentation_feedback_resource =
+      wl_resource_create(client, &wp_presentation_feedback_interface,
+                         wl_resource_get_version(resource), callback);
+  wp_presentation->set_presentation_callback(presentation_feedback_resource);
+  wp_presentation->Feedback(client, resource, surface, callback);
+}
+
+}  // namespace
+
+const struct wp_presentation_interface kMockWpPresentationImpl = {
+    &DestroyResource,  // destroy
+    &Feedback,         // feedback
+};
+
+MockWpPresentation::MockWpPresentation()
+    : GlobalObject(&wp_presentation_interface, &kMockWpPresentationImpl, 1) {}
+
+MockWpPresentation::~MockWpPresentation() {}
+
+void MockWpPresentation::SendPresentationCallback() {
+  if (!presentation_callback_)
+    return;
+
+  // TODO(msisov): add support for test provided presentation feedback values.
+  wp_presentation_feedback_send_presented(
+      presentation_callback_, 0 /* tv_sec_hi */, 0 /* tv_sec_lo */,
+      0 /* tv_nsec */, 0 /* refresh */, 0 /* seq_hi */, 0 /* seq_lo */,
+      0 /* flags */);
+  wl_client_flush(wl_resource_get_client(presentation_callback_));
+  wl_resource_destroy(presentation_callback_);
+  presentation_callback_ = nullptr;
+}
+
+}  // namespace wl

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

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef UI_OZONE_PLATFORM_WAYLAND_TEST_MOCK_WP_PRESENTATION_H_
+#define UI_OZONE_PLATFORM_WAYLAND_TEST_MOCK_WP_PRESENTATION_H_
+
+#include <presentation-time-server-protocol.h>
+
+#include "base/logging.h"
+#include "base/macros.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "ui/ozone/platform/wayland/test/global_object.h"
+
+namespace wl {
+
+extern const struct wp_presentation_interface kMockWpPresentationImpl;
+
+class MockWpPresentation : public GlobalObject {
+ public:
+  MockWpPresentation();
+  ~MockWpPresentation() override;
+
+  MOCK_METHOD2(Destroy,
+               void(struct wl_client* client, struct wl_resource* resource));
+  MOCK_METHOD4(Feedback,
+               void(struct wl_client* client,
+                    struct wl_resource* resource,
+                    struct wl_resource* surface,
+                    uint32_t callback));
+
+  void set_presentation_callback(wl_resource* callback_resource) {
+    DCHECK(!presentation_callback_);
+    presentation_callback_ = callback_resource;
+  }
+
+  void SendPresentationCallback();
+
+ private:
+  wl_resource* presentation_callback_ = nullptr;
+
+  DISALLOW_COPY_AND_ASSIGN(MockWpPresentation);
+};
+
+}  // namespace wl
+
+#endif  // UI_OZONE_PLATFORM_WAYLAND_TEST_MOCK_WP_PRESENTATION_H_

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

@@ -107,6 +107,12 @@ void TestWaylandServerThread::Resume() {
   resume_event_.Signal();
 }
 
+MockWpPresentation* TestWaylandServerThread::EnsureWpPresentation() {
+  if (wp_presentation_.Initialize(display_.get()))
+    return &wp_presentation_;
+  return nullptr;
+}
+
 void TestWaylandServerThread::DoPause() {
   base::RunLoop().RunUntilIdle();
   pause_event_.Signal();

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

@@ -13,6 +13,7 @@
 #include "base/synchronization/waitable_event.h"
 #include "base/threading/thread.h"
 #include "ui/ozone/platform/wayland/test/global_object.h"
+#include "ui/ozone/platform/wayland/test/mock_wp_presentation.h"
 #include "ui/ozone/platform/wayland/test/mock_xdg_shell.h"
 #include "ui/ozone/platform/wayland/test/mock_zwp_linux_dmabuf.h"
 #include "ui/ozone/platform/wayland/test/test_compositor.h"
@@ -53,6 +54,9 @@ class TestWaylandServerThread : public base::Thread,
   // Resumes the server thread after flushing client connections.
   void Resume();
 
+  // Initializes and returns WpPresentation.
+  MockWpPresentation* EnsureWpPresentation();
+
   template <typename T>
   T* GetObject(uint32_t id) {
     wl_resource* resource = wl_client_get_object(client_, id);
@@ -104,6 +108,7 @@ class TestWaylandServerThread : public base::Thread,
   MockZxdgShellV6 zxdg_shell_v6_;
   TestZwpTextInputManagerV1 zwp_text_input_manager_v1_;
   MockZwpLinuxDmabufV1 zwp_linux_dmabuf_v1_;
+  MockWpPresentation wp_presentation_;
 
   std::vector<std::unique_ptr<GlobalObject>> globals_;
 

ui/ozone/platform/wayland/wayland_buffer_manager_unittest.cc

@@ -14,6 +14,8 @@
 #include "mojo/public/cpp/system/platform_handle.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "ui/ozone/platform/wayland/gpu/wayland_surface_gpu.h"
+#include "ui/ozone/platform/wayland/test/mock_surface.h"
 #include "ui/ozone/platform/wayland/test/wayland_test.h"
 
 using testing::_;
@@ -38,6 +40,21 @@ struct InputData {
   uint32_t buffer_id = 0;
 };
 
+class MockSurfaceGpu : public WaylandSurfaceGpu {
+ public:
+  MockSurfaceGpu() = default;
+  ~MockSurfaceGpu() override = default;
+
+  MOCK_METHOD2(OnSubmission,
+               void(uint32_t buffer_id, const gfx::SwapResult& swap_result));
+  MOCK_METHOD2(OnPresentation,
+               void(uint32_t buffer_id,
+                    const gfx::PresentationFeedback& feedback));
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(MockSurfaceGpu);
+};
+
 }  // namespace
 
 class WaylandBufferManagerTest : public WaylandTest {
@@ -261,6 +278,108 @@ TEST_P(WaylandBufferManagerTest, CreateAndDestroyBuffer) {
   }
 }
 
+TEST_P(WaylandBufferManagerTest, EnsureCorrectOrderOfCallbacks) {
+  constexpr uint32_t kBufferId1 = 1;
+  constexpr uint32_t kBufferId2 = 2;
+
+  const gfx::AcceleratedWidget widget = window_->GetWidget();
+  const gfx::Rect bounds = gfx::Rect({0, 0}, kDefaultSize);
+  window_->SetBounds(bounds);
+
+  MockSurfaceGpu mock_surface_gpu;
+  buffer_manager_gpu_->RegisterSurface(widget, &mock_surface_gpu);
+
+  EXPECT_CALL(*server_.zwp_linux_dmabuf_v1(), CreateParams(_, _, _)).Times(2);
+  CreateDmabufBasedBufferAndSetTerminateExpecation(false /*fail*/, widget,
+                                                   kBufferId1);
+  CreateDmabufBasedBufferAndSetTerminateExpecation(false /*fail*/, widget,
+                                                   kBufferId2);
+
+  Sync();
+
+  auto* mock_surface = server_.GetObject<wl::MockSurface>(widget);
+
+  constexpr uint32_t kNumberOfCommits = 3;
+  EXPECT_CALL(*mock_surface, Attach(_, _, _)).Times(kNumberOfCommits);
+  EXPECT_CALL(*mock_surface, Frame(_)).Times(kNumberOfCommits);
+  EXPECT_CALL(*mock_surface, Commit()).Times(kNumberOfCommits);
+
+  // All the other expectations must come in order.
+  ::testing::InSequence sequence;
+  EXPECT_CALL(mock_surface_gpu,
+              OnSubmission(kBufferId1, gfx::SwapResult::SWAP_ACK))
+      .Times(1);
+  // wp_presentation must not exist now. This means that the buffer
+  // manager must send synthetized presentation feedbacks.
+  ASSERT_TRUE(!connection_->presentation());
+  EXPECT_CALL(mock_surface_gpu, OnPresentation(kBufferId1, _)).Times(1);
+
+  buffer_manager_gpu_->CommitBuffer(widget, kBufferId1, bounds);
+
+  Sync();
+
+  // As long as there hasn't any previous buffer attached (nothing to release
+  // yet), it must be enough to just send a frame callback back.
+  mock_surface->SendFrameCallback();
+
+  Sync();
+
+  // Commit second buffer now.
+  buffer_manager_gpu_->CommitBuffer(widget, kBufferId2, bounds);
+
+  Sync();
+
+  EXPECT_CALL(mock_surface_gpu,
+              OnSubmission(kBufferId2, gfx::SwapResult::SWAP_ACK))
+      .Times(1);
+  EXPECT_CALL(mock_surface_gpu, OnPresentation(kBufferId2, _)).Times(1);
+
+  mock_surface->ReleasePrevAttachedBuffer();
+  mock_surface->SendFrameCallback();
+
+  Sync();
+
+  // wp_presentation is available now.
+  auto* mock_wp_presentation = server_.EnsureWpPresentation();
+  ASSERT_TRUE(mock_wp_presentation);
+
+  Sync();
+
+  // Now, the wp_presentation object exists and there must be a real feedback
+  // sent. Ensure the order now.
+  ASSERT_TRUE(connection_->presentation());
+
+  EXPECT_CALL(*mock_wp_presentation,
+              Feedback(_, _, mock_surface->resource(), _))
+      .Times(1);
+
+  // Commit second buffer now.
+  buffer_manager_gpu_->CommitBuffer(widget, kBufferId1, bounds);
+
+  Sync();
+
+  // Even though, the server send the presentation feeedback, the host manager
+  // must make sure the order of the submission and presentation callbacks is
+  // correct. Thus, no callbacks must be received by the MockSurfaceGpu.
+  EXPECT_CALL(mock_surface_gpu, OnSubmission(_, _)).Times(0);
+  EXPECT_CALL(mock_surface_gpu, OnPresentation(_, _)).Times(0);
+
+  mock_wp_presentation->SendPresentationCallback();
+
+  Sync();
+
+  EXPECT_CALL(mock_surface_gpu,
+              OnSubmission(kBufferId1, gfx::SwapResult::SWAP_ACK))
+      .Times(1);
+  EXPECT_CALL(mock_surface_gpu, OnPresentation(kBufferId1, _)).Times(1);
+
+  // Now, send the release callback. The host manager must send the submission
+  // and presentation callbacks in correct order.
+  mock_surface->ReleasePrevAttachedBuffer();
+
+  Sync();
+}
+
 INSTANTIATE_TEST_SUITE_P(XdgVersionV5Test,
                          WaylandBufferManagerTest,
                          ::testing::Values(kXdgShellV5));