From c644b4aba05233a8a78d7708a98d9a583ff78a27 Mon Sep 17 00:00:00 2001 From: Maksim Sisov Date: Tue, 30 Jul 2019 05:53:48 +0000 Subject: [PATCH 2/5] [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/+/1719013 Reviewed-by: Robert Kroeger Commit-Queue: Maksim Sisov Cr-Commit-Position: refs/heads/master@{#682157} --- ui/ozone/platform/wayland/BUILD.gn | 3 ++ .../wayland/gpu/wayland_surface_gpu.h | 2 + .../host/wayland_buffer_manager_host.cc | 25 +++++++-- .../platform/wayland/test/mock_surface.cc | 44 ++++++++++++++- ui/ozone/platform/wayland/test/mock_surface.h | 14 +++++ .../wayland/test/mock_wp_presentation.cc | 53 +++++++++++++++++++ .../wayland/test/mock_wp_presentation.h | 47 ++++++++++++++++ .../test/test_wayland_server_thread.cc | 6 +++ .../wayland/test/test_wayland_server_thread.h | 5 ++ 9 files changed, 194 insertions(+), 5 deletions(-) create mode 100644 ui/ozone/platform/wayland/test/mock_wp_presentation.cc create mode 100644 ui/ozone/platform/wayland/test/mock_wp_presentation.h diff --git a/ui/ozone/platform/wayland/BUILD.gn b/ui/ozone/platform/wayland/BUILD.gn index 303173b4ba6c..686138bb614a 100644 --- a/ui/ozone/platform/wayland/BUILD.gn +++ b/ui/ozone/platform/wayland/BUILD.gn @@ -173,6 +173,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", @@ -225,6 +227,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", diff --git a/ui/ozone/platform/wayland/gpu/wayland_surface_gpu.h b/ui/ozone/platform/wayland/gpu/wayland_surface_gpu.h index f3593766eed8..ace5279e838e 100644 --- a/ui/ozone/platform/wayland/gpu/wayland_surface_gpu.h +++ b/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, diff --git a/ui/ozone/platform/wayland/host/wayland_buffer_manager_host.cc b/ui/ozone/platform/wayland/host/wayland_buffer_manager_host.cc index bbcfa84016f9..5f7efebac400 100644 --- a/ui/ozone/platform/wayland/host/wayland_buffer_manager_host.cc +++ b/ui/ozone/platform/wayland/host/wayland_buffer_manager_host.cc @@ -207,6 +207,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); @@ -334,6 +340,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 @@ -349,14 +360,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); } diff --git a/ui/ozone/platform/wayland/test/mock_surface.cc b/ui/ozone/platform/wayland/test/mock_surface.cc index 9d2333683a41..6ee1c0a9c543 100644 --- a/ui/ozone/platform/wayland/test/mock_surface.cc +++ b/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(resource)->Attach(buffer_resource, x, y); + auto* surface = GetUserDataAs(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(resource)->Frame(callback); + auto* surface = GetUserDataAs(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) { @@ -85,4 +92,37 @@ MockSurface* MockSurface::FromResource(wl_resource* resource) { return GetUserDataAs(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 diff --git a/ui/ozone/platform/wayland/test/mock_surface.h b/ui/ozone/platform/wayland/test/mock_surface.h index 1ea9c52dea27..0b44ba090187 100644 --- a/ui/ozone/platform/wayland/test/mock_surface.h +++ b/ui/ozone/platform/wayland/test/mock_surface.h @@ -50,10 +50,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 xdg_surface_; std::unique_ptr xdg_popup_; + wl_resource* frame_callback_ = nullptr; + + wl_resource* attached_buffer_ = nullptr; + wl_resource* prev_attached_buffer_ = nullptr; + DISALLOW_COPY_AND_ASSIGN(MockSurface); }; diff --git a/ui/ozone/platform/wayland/test/mock_wp_presentation.cc b/ui/ozone/platform/wayland/test/mock_wp_presentation.cc new file mode 100644 index 000000000000..d24c13658aa4 --- /dev/null +++ b/ui/ozone/platform/wayland/test/mock_wp_presentation.cc @@ -0,0 +1,53 @@ +// 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 + +#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(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 diff --git a/ui/ozone/platform/wayland/test/mock_wp_presentation.h b/ui/ozone/platform/wayland/test/mock_wp_presentation.h new file mode 100644 index 000000000000..a1bb344fd5f1 --- /dev/null +++ b/ui/ozone/platform/wayland/test/mock_wp_presentation.h @@ -0,0 +1,47 @@ +// 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 + +#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_ diff --git a/ui/ozone/platform/wayland/test/test_wayland_server_thread.cc b/ui/ozone/platform/wayland/test/test_wayland_server_thread.cc index 22b1776ebc4e..1101cccb6922 100644 --- a/ui/ozone/platform/wayland/test/test_wayland_server_thread.cc +++ b/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(); diff --git a/ui/ozone/platform/wayland/test/test_wayland_server_thread.h b/ui/ozone/platform/wayland/test/test_wayland_server_thread.h index fddd426db690..110e3ffbc1c2 100644 --- a/ui/ozone/platform/wayland/test/test_wayland_server_thread.h +++ b/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 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> globals_; -- 2.22.0