Skip to content

Commit

Permalink
wayland: no longer reset kTouchPress serial on touch-up
Browse files Browse the repository at this point in the history 
Under Wayland, 'set_selection' requests require a serial number
corresponding to a recent input event (eg: mouse press, key press, touch
tap, etc). OTOH, kTouchPress serial is reset upon wl_touch.up, which
leads to no-op when trying to write to clipboard when, handling a
touch-tap (ie: touch down followed by a touch up), for example. Another
example is when clipboard writes are triggered asynchronously, eg: touch
down coming from a renderer process.

This CL fixes it by no longer resetting kTouchPress upon touch-up
Wayland event, which should not cause issues as, nowadays, compositors
usually only check whether a received serial is "not too old". It also
should not lead to bad side effects at ozone/wayland code as long as
SerialTracker is not used to determine whether there are active touch
points, for example.

Additionally, clipboard unit tests are fixed and improved to correctly
exercise these edge cases.

R=msisov@igalia.com

Bug: 1282220
Test: Covered by WaylandClipboardTest in ozone_unittests
Change-Id: Ib6305accf1fd542b80711730c654b0a1e51b6ded
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3527605
Reviewed-by: Maksim Sisov <msisov@igalia.com>
Commit-Queue: Nick Yamane <nickdiego@igalia.com>
Cr-Commit-Position: refs/heads/main@{#983748}
  • Loading branch information
@nickdiego
nickdiego authored and Chromium LUCI CQ committed Mar 22, 2022
1 parent 38dc829 commit 097317e
Show file tree
Hide file tree
Showing 15 changed files with 198 additions and 80 deletions.
11 changes: 3 additions & 8 deletions ui/ozone/platform/wayland/host/gtk_primary_selection_device.cc
View file
Expand Up @@ -6,7 +6,6 @@

#include <gtk-primary-selection-client-protocol.h>

#include "base/logging.h"
#include "ui/ozone/platform/wayland/host/gtk_primary_selection_offer.h"
#include "ui/ozone/platform/wayland/host/wayland_connection.h"
#include "ui/ozone/platform/wayland/host/wayland_data_source.h"
Expand All @@ -27,15 +26,11 @@ GtkPrimarySelectionDevice::GtkPrimarySelectionDevice(
GtkPrimarySelectionDevice::~GtkPrimarySelectionDevice() = default;

void GtkPrimarySelectionDevice::SetSelectionSource(
GtkPrimarySelectionSource* source) {
auto serial = GetSerialForSelection();
if (!serial.has_value()) {
LOG(ERROR) << "Failed to set selection. No serial found.";
return;
}
GtkPrimarySelectionSource* source,
uint32_t serial) {
auto* data_source = source ? source->data_source() : nullptr;
gtk_primary_selection_device_set_selection(data_device_.get(), data_source,
serial->value);
serial);
connection()->ScheduleFlush();
}

Expand Down
5 changes: 3 additions & 2 deletions ui/ozone/platform/wayland/host/gtk_primary_selection_device.h
View file
Expand Up @@ -5,7 +5,8 @@
#ifndef UI_OZONE_PLATFORM_WAYLAND_HOST_GTK_PRIMARY_SELECTION_DEVICE_H_
#define UI_OZONE_PLATFORM_WAYLAND_HOST_GTK_PRIMARY_SELECTION_DEVICE_H_

#include "base/callback.h"
#include <cstdint>

#include "ui/ozone/platform/wayland/common/wayland_object.h"
#include "ui/ozone/platform/wayland/host/wayland_data_device_base.h"
#include "ui/ozone/platform/wayland/host/wayland_data_source.h"
Expand All @@ -32,7 +33,7 @@ class GtkPrimarySelectionDevice : public WaylandDataDeviceBase {
return data_device_.get();
}

void SetSelectionSource(GtkPrimarySelectionSource* source);
void SetSelectionSource(GtkPrimarySelectionSource* source, uint32_t serial);

private:
// gtk_primary_selection_device_listener callbacks
Expand Down
38 changes: 32 additions & 6 deletions ui/ozone/platform/wayland/host/wayland_clipboard.cc
View file
Expand Up @@ -4,11 +4,13 @@

#include "ui/ozone/platform/wayland/host/wayland_clipboard.h"

#include <cstdint>
#include <memory>
#include <string>

#include "base/bind.h"
#include "base/check.h"
#include "base/logging.h"
#include "base/memory/scoped_refptr.h"
#include "base/memory/weak_ptr.h"
#include "base/notreached.h"
Expand All @@ -24,6 +26,7 @@
#include "ui/ozone/platform/wayland/host/wayland_data_device_manager.h"
#include "ui/ozone/platform/wayland/host/wayland_data_offer_base.h"
#include "ui/ozone/platform/wayland/host/wayland_data_source.h"
#include "ui/ozone/platform/wayland/host/wayland_serial_tracker.h"
#include "ui/ozone/platform/wayland/host/zwp_primary_selection_device.h"
#include "ui/ozone/platform/wayland/host/zwp_primary_selection_device_manager.h"
#include "ui/ozone/public/platform_clipboard.h"
Expand Down Expand Up @@ -69,8 +72,10 @@ template <typename Manager,
typename DataDevice = typename Manager::DataDevice>
class ClipboardImpl final : public Clipboard, public DataSource::Delegate {
public:
explicit ClipboardImpl(Manager* manager, ui::ClipboardBuffer buffer)
: manager_(manager), buffer_(buffer) {
ClipboardImpl(Manager* manager,
ui::ClipboardBuffer buffer,
ui::WaylandConnection* connection)
: manager_(manager), buffer_(buffer), connection_(connection) {
GetDevice()->set_selection_offer_callback(base::BindRepeating(
&ClipboardImpl::HandleNewSelectionOffer, weak_factory_.GetWeakPtr()));
}
Expand All @@ -93,6 +98,10 @@ class ClipboardImpl final : public Clipboard, public DataSource::Delegate {
// responsible for writing the clipboard contents into the supplied fd. This
// client can only drop the clipboard contents when it receives a
// wl_data_source::cancelled event.
//
// This is supposedly responding to an input event, i.e: there is a valid
// corresponding serial number (provided by wl::SerialTracker). Otherwise,
// this function will no-op.
void Write(const ui::PlatformClipboard::DataMap* data) final {
if (!data || data->empty()) {
offered_data_.clear();
Expand All @@ -102,7 +111,21 @@ class ClipboardImpl final : public Clipboard, public DataSource::Delegate {
source_ = manager_->CreateSource(this);
source_->Offer(GetOfferedMimeTypes());
}
GetDevice()->SetSelectionSource(source_.get());

// TODO(nickdiego): This function should just no-op if no serial is found
// (ie: no recent input event has been processed yet), though several unit
// and browser tests do not satisfy this precondition so would fail [1].
// Revisit this once those tests are fixed.
//
// [1] https://chromium-review.googlesource.com/c/chromium/src/+/3527605/2
auto& serial_tracker = connection_->serial_tracker();
auto serial = serial_tracker.GetSerial({wl::SerialType::kTouchPress,
wl::SerialType::kMousePress,
wl::SerialType::kKeyPress});
if (serial.has_value())
GetDevice()->SetSelectionSource(source_.get(), serial->value);
else
LOG(WARNING) << "No serial found for selection.";

if (!clipboard_changed_callback_.is_null())
clipboard_changed_callback_.Run(buffer_);
Expand Down Expand Up @@ -180,6 +203,8 @@ class ClipboardImpl final : public Clipboard, public DataSource::Delegate {
// Notifies when clipboard data changes. Can be empty if not set.
ClipboardDataChangedCallback clipboard_changed_callback_;

ui::WaylandConnection* const connection_;

base::WeakPtrFactory<ClipboardImpl> weak_factory_{this};
};

Expand All @@ -193,7 +218,8 @@ WaylandClipboard::WaylandClipboard(WaylandConnection* connection,
copypaste_clipboard_(
std::make_unique<wl::ClipboardImpl<WaylandDataDeviceManager>>(
manager,
ClipboardBuffer::kCopyPaste)) {
ClipboardBuffer::kCopyPaste,
connection)) {
DCHECK(manager);
DCHECK(connection_);
DCHECK(copypaste_clipboard_);
Expand Down Expand Up @@ -257,7 +283,7 @@ wl::Clipboard* WaylandClipboard::GetClipboard(ClipboardBuffer buffer) {
if (!primary_selection_clipboard_) {
primary_selection_clipboard_ = std::make_unique<
wl::ClipboardImpl<ZwpPrimarySelectionDeviceManager>>(
zwp_manager, ClipboardBuffer::kSelection);
zwp_manager, ClipboardBuffer::kSelection, connection_);
}
return primary_selection_clipboard_.get();
}
Expand All @@ -266,7 +292,7 @@ wl::Clipboard* WaylandClipboard::GetClipboard(ClipboardBuffer buffer) {
if (!primary_selection_clipboard_) {
primary_selection_clipboard_ = std::make_unique<
wl::ClipboardImpl<GtkPrimarySelectionDeviceManager>>(
gtk_manager, ClipboardBuffer::kSelection);
gtk_manager, ClipboardBuffer::kSelection, connection_);
}
return primary_selection_clipboard_.get();
}
Expand Down
146 changes: 132 additions & 14 deletions ui/ozone/platform/wayland/host/wayland_clipboard_unittest.cc
View file
Expand Up @@ -2,6 +2,7 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#include <linux/input.h>
#include <wayland-server.h>

#include <cstring>
Expand All @@ -10,6 +11,7 @@
#include <vector>

#include "base/bind.h"
#include "base/callback_forward.h"
#include "base/containers/flat_set.h"
#include "base/location.h"
#include "base/run_loop.h"
Expand All @@ -22,14 +24,19 @@
#include "ui/base/clipboard/clipboard_buffer.h"
#include "ui/base/clipboard/clipboard_constants.h"
#include "ui/events/base_event_utils.h"
#include "ui/gfx/geometry/point.h"
#include "ui/ozone/platform/wayland/host/wayland_clipboard.h"
#include "ui/ozone/platform/wayland/host/wayland_connection_test_api.h"
#include "ui/ozone/platform/wayland/host/wayland_serial_tracker.h"
#include "ui/ozone/platform/wayland/test/mock_pointer.h"
#include "ui/ozone/platform/wayland/test/mock_surface.h"
#include "ui/ozone/platform/wayland/test/test_data_device.h"
#include "ui/ozone/platform/wayland/test/test_data_device_manager.h"
#include "ui/ozone/platform/wayland/test/test_data_offer.h"
#include "ui/ozone/platform/wayland/test/test_data_source.h"
#include "ui/ozone/platform/wayland/test/test_keyboard.h"
#include "ui/ozone/platform/wayland/test/test_selection_device_manager.h"
#include "ui/ozone/platform/wayland/test/test_touch.h"
#include "ui/ozone/platform/wayland/test/test_wayland_server_thread.h"
#include "ui/ozone/platform/wayland/test/wayland_test.h"
#include "ui/ozone/public/platform_clipboard.h"
Expand Down Expand Up @@ -61,6 +68,21 @@ class WaylandClipboardTestBase : public WaylandTest {
void SetUp() override {
WaylandTest::SetUp();

wl_seat_send_capabilities(server_.seat()->resource(),
WL_SEAT_CAPABILITY_POINTER |
WL_SEAT_CAPABILITY_TOUCH |
WL_SEAT_CAPABILITY_KEYBOARD);
Sync();

pointer_ = server_.seat()->pointer();
ASSERT_TRUE(pointer_);

touch_ = server_.seat()->touch();
ASSERT_TRUE(touch_);

keyboard_ = server_.seat()->keyboard();
ASSERT_TRUE(keyboard_);

// As of now, WaylandClipboard::RequestClipboardData is implemented in a
// blocking way, which requires a roundtrip before attempting the data
// from the selection fd. As Wayland events polling is single-threaded for
Expand Down Expand Up @@ -94,7 +116,43 @@ class WaylandClipboardTestBase : public WaylandTest {
base::RunLoop().RunUntilIdle();
}

void SentPointerButtonPress(const gfx::Point& location) {
wl_pointer_send_enter(pointer_->resource(), ++serial_, surface_->resource(),
location.x(), location.y());
wl_pointer_send_button(pointer_->resource(), ++serial_, ++timestamp_,
BTN_LEFT, WL_POINTER_BUTTON_STATE_PRESSED);
}
void SendTouchDown(const gfx::Point& location) {
wl_touch_send_down(touch_->resource(), ++serial_, ++timestamp_,
surface_->resource(), /*touch_id=*/0,
wl_fixed_from_int(location.x()),
wl_fixed_from_int(location.y()));
}

void SendTouchUp() {
wl_touch_send_up(touch_->resource(), ++serial_, ++timestamp_,
/*touch_id=*/0);
}

void SendKeyboardKey() {
struct wl_array empty;
wl_array_init(&empty);
wl_keyboard_send_enter(keyboard_->resource(), 1, surface_->resource(),
&empty);
wl_array_release(&empty);
wl_keyboard_send_key(keyboard_->resource(), 2, 0, 30 /* a */,
WL_KEYBOARD_KEY_STATE_PRESSED);
}

/* Server objects */
wl::MockPointer* pointer_;
wl::TestTouch* touch_;
wl::TestKeyboard* keyboard_;

WaylandClipboard* clipboard_ = nullptr;

uint32_t serial_ = 0;
uint32_t timestamp_ = 0;
};

class WaylandClipboardTest : public WaylandClipboardTestBase {
Expand Down Expand Up @@ -145,6 +203,15 @@ class WaylandClipboardTest : public WaylandClipboardTestBase {
: ClipboardBuffer::kCopyPaste;
}

void ResetServerSelectionSource() {
if (GetParam().primary_selection_protocol !=
wl::PrimarySelectionProtocol::kNone) {
server_.primary_selection_device_manager()->set_source(nullptr);
} else {
server_.data_device_manager()->set_data_source(nullptr);
}
}

// Fill the clipboard backing store with sample data.
void OfferData(ClipboardBuffer buffer,
const char* data,
Expand Down Expand Up @@ -174,22 +241,66 @@ class CopyPasteOnlyClipboardTest : public WaylandClipboardTestBase {
}
};

// Verifies that copy-to-clipboard works as expected. Actual Wayland input
// events are used in order to exercise all the components involved, e.g:
// Wayland{Pointer,Keyboard,Touch}, Serial tracker and WaylandClipboard.
//
// Regression test for https://crbug.com/1282220.
TEST_P(WaylandClipboardTest, WriteToClipboard) {
// 1. Offer sample text as selection data.
OfferData(GetBuffer(), kSampleClipboardText, {kMimeTypeTextUtf8});
Sync();

// 2. Emulate an external client requesting to read the offered data and make
// sure the appropriate string gets delivered.
std::string delivered_text;
base::MockCallback<wl::TestSelectionSource::ReadDataCallback> callback;
EXPECT_CALL(callback, Run(_)).WillOnce([&](std::vector<uint8_t>&& data) {
delivered_text = std::string(data.begin(), data.end());
});
GetServerSelectionSource()->ReadData(kMimeTypeTextUtf8, callback.Get());
const base::RepeatingClosure send_input_event_closures[]{
// Mouse button press
base::BindLambdaForTesting([&]() {
SentPointerButtonPress({10, 10});
}),
// Key press
base::BindLambdaForTesting([&]() { SendKeyboardKey(); }),
// Touch down
base::BindLambdaForTesting([&]() {
SendTouchDown({200, 200});
}),
// Touch tap (down > up)
base::BindLambdaForTesting([&]() {
SendTouchDown({300, 300});
SendTouchUp();
})};

auto* window_manager = connection_->wayland_window_manager();

// Triggering copy on touch-down event.
for (auto send_input_event : send_input_event_closures) {
ResetServerSelectionSource();

send_input_event.Run();
Sync();
auto client_selection_serial = connection_->serial_tracker().GetSerial(
{wl::SerialType::kTouchPress, wl::SerialType::kMousePress,
wl::SerialType::kKeyPress});
ASSERT_TRUE(client_selection_serial.has_value());

WaitForClipboardTasks();
ASSERT_EQ(kSampleClipboardText, delivered_text);
// 1. Offer sample text as selection data.
OfferData(GetBuffer(), kSampleClipboardText, {kMimeTypeTextUtf8});
Sync();
ASSERT_TRUE(GetServerSelectionSource());

EXPECT_EQ(client_selection_serial->value,
GetServerSelectionDevice()->selection_serial());

// 2. Emulate an external client requesting to read the offered data and
// make sure the appropriate string gets delivered.
std::string delivered_text;
base::MockCallback<wl::TestSelectionSource::ReadDataCallback> callback;
EXPECT_CALL(callback, Run(_)).WillOnce([&](std::vector<uint8_t>&& data) {
delivered_text = std::string(data.begin(), data.end());
});
GetServerSelectionSource()->ReadData(kMimeTypeTextUtf8, callback.Get());

WaitForClipboardTasks();
ASSERT_EQ(kSampleClipboardText, delivered_text);

window_manager->SetPointerFocusedWindow(nullptr);
window_manager->SetTouchFocusedWindow(nullptr);
window_manager->SetKeyboardFocusedWindow(nullptr);
}
}

TEST_P(WaylandClipboardTest, ReadFromClipboard) {
Expand Down Expand Up @@ -251,8 +362,11 @@ TEST_P(WaylandClipboardTest, ReadFromClipboardWithoutOffer) {
}

TEST_P(WaylandClipboardTest, IsSelectionOwner) {
connection_->serial_tracker().UpdateSerial(wl::SerialType::kMousePress, 1);

OfferData(GetBuffer(), kSampleClipboardText, {kMimeTypeTextUtf8});
Sync();
ASSERT_TRUE(GetServerSelectionSource());
ASSERT_TRUE(clipboard_->IsSelectionOwner(GetBuffer()));

// The compositor sends OnCancelled whenever another application on the system
Expand All @@ -262,6 +376,7 @@ TEST_P(WaylandClipboardTest, IsSelectionOwner) {
Sync();

ASSERT_FALSE(clipboard_->IsSelectionOwner(GetBuffer()));
connection_->serial_tracker().ResetSerial(wl::SerialType::kMousePress);
}

// Ensures WaylandClipboard correctly handles overlapping read requests for
Expand Down Expand Up @@ -320,10 +435,13 @@ TEST_P(WaylandClipboardTest, ClipboardChangeNotifications) {
EXPECT_FALSE(clipboard_->IsSelectionOwner(buffer));

// 2. For selection offered by Chromium.
connection_->serial_tracker().UpdateSerial(wl::SerialType::kMousePress, 1);
EXPECT_CALL(clipboard_changed_callback, Run(buffer)).Times(1);
OfferData(buffer, kSampleClipboardText, {kMimeTypeTextUtf8});
Sync();
ASSERT_TRUE(GetServerSelectionSource());
EXPECT_TRUE(clipboard_->IsSelectionOwner(buffer));
connection_->serial_tracker().ResetSerial(wl::SerialType::kMousePress);
}

// Verifies clipboard calls targeting primary selection buffer no-op and run
Expand Down

0 comments on commit 097317e

Please sign in to comment.