Skip to content

Commit

Permalink
ozone/wayland: do not depend on WaylandScreen on buffer scale updates
Browse files Browse the repository at this point in the history
This CL fixes a segfault when a WaylandScreen is created after a
WaylandWindow..

Basically, when WaylandScreen is created, it gets the list of
existing outputs, which it uses to create displays. In its own turn,
it also checks what WaylandWindows are on those displays and
calls WaylandWindow::UpdateBufferScale. This operation also
results in SetBounds to be called, which calls to
PlatformWindowDelegate::GetMinimumSizeForWindow, which requires
ScreenOzone instance to have PlatformScreen (WaylandScreen in this
case) to be set.

But as long as creation of the screen, updating buffer scale, and
setting updated bounds based on new scale value happens in the
same sequence of calls, ScreenOzone just doesn't have the PlatformScreen
set, and it results in a crash. See the stacktrace below -

#3 0x7f9c30bba4c0 (/lib/x86_64-linux-gnu/libc-2.23.so+0x354bf)
#4 0x556900be4bad aura::ScreenOzone::GetPrimaryDisplay()
#5 0x5568ff6aebff views::DesktopWindowTreeHostPlatform::GetRootTransform()
#6 0x5568ff6af1ba views::DesktopWindowTreeHostPlatform::GetMinimumSizeForWindow()
#7 0x5568faeb9aaf ui::WaylandWindow::SetBounds()
#8 0x5568faeb961e ui::WaylandWindow::UpdateBufferScale()
#9 0x5568faeb473d ui::WaylandScreen::AddOrUpdateDisplay()
#10 0x5568faeb3023 ui::WaylandOutputManager::CreateWaylandScreen()
#11 0x5568faec7fa2 ui::(anonymous namespace)::OzonePlatformWayland::CreateScreen()
#12 0x556900be480a aura::ScreenOzone::ScreenOzone()
#13 0x5568ff6aa10e views::DesktopScreenOzone::DesktopScreenOzone()

This only happens in test environments (interactive_ui_tests, when
Ozone is initialized, WaylandWindows are created, but the screen
instance is created and set a bit later.

Thus, make WaylandWindow use existing information about preferred
outputs that it has.

Bug: 1134495
Change-Id: I3f2f9349adc85684d5cf4f5e2bca8497c326bb9b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2773276
Commit-Queue: Maksim Sisov <msisov@igalia.com>
Reviewed-by: Robert Kroeger <rjkroege@chromium.org>
Cr-Commit-Position: refs/heads/master@{#865569}
  • Loading branch information
msisov authored and Chromium LUCI CQ committed Mar 23, 2021
1 parent 4eb379a commit e71070a
Show file tree
Hide file tree
Showing 11 changed files with 57 additions and 38 deletions.
3 changes: 1 addition & 2 deletions ui/ozone/platform/wayland/fuzzer/wayland_buffer_fuzzer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -116,8 +116,7 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) {
std::make_unique<ui::WaylandConnection>();
CHECK(connection->Initialize());

auto screen = connection->wayland_output_manager()->CreateWaylandScreen(
connection.get());
auto screen = connection->wayland_output_manager()->CreateWaylandScreen();

MockPlatformWindowDelegate delegate;
gfx::AcceleratedWidget widget = gfx::kNullAcceleratedWidget;
Expand Down
2 changes: 1 addition & 1 deletion ui/ozone/platform/wayland/host/wayland_connection.cc
Original file line number Diff line number Diff line change
Expand Up @@ -382,7 +382,7 @@ void WaylandConnection::Global(void* data,

if (!connection->wayland_output_manager_) {
connection->wayland_output_manager_ =
std::make_unique<WaylandOutputManager>();
std::make_unique<WaylandOutputManager>(connection);
}
connection->wayland_output_manager_->AddWaylandOutput(name,
output.release());
Expand Down
7 changes: 7 additions & 0 deletions ui/ozone/platform/wayland/host/wayland_output.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

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

#include "ui/display/display.h"
#include "ui/gfx/color_space.h"
#include "ui/ozone/platform/wayland/host/wayland_connection.h"

Expand Down Expand Up @@ -33,6 +34,12 @@ void WaylandOutput::Initialize(Delegate* delegate) {
wl_output_add_listener(output_.get(), &output_listener, this);
}

float WaylandOutput::GetUIScaleFactor() const {
return display::Display::HasForceDeviceScaleFactor()
? display::Display::GetForcedDeviceScaleFactor()
: scale_factor();
}

void WaylandOutput::TriggerDelegateNotifications() const {
DCHECK(!rect_in_physical_pixels_.IsEmpty());
delegate_->OnOutputHandleMetrics(output_id_, rect_in_physical_pixels_,
Expand Down
2 changes: 2 additions & 0 deletions ui/ozone/platform/wayland/host/wayland_output.h
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ class WaylandOutput {

void Initialize(Delegate* delegate);

float GetUIScaleFactor() 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_; }
Expand Down
18 changes: 14 additions & 4 deletions ui/ozone/platform/wayland/host/wayland_output_manager.cc
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,12 @@

#include "ui/ozone/platform/wayland/host/wayland_connection.h"
#include "ui/ozone/platform/wayland/host/wayland_output.h"
#include "ui/ozone/platform/wayland/host/wayland_window.h"

namespace ui {

WaylandOutputManager::WaylandOutputManager() = default;
WaylandOutputManager::WaylandOutputManager(WaylandConnection* connection)
: connection_(connection) {}

WaylandOutputManager::~WaylandOutputManager() = default;

Expand Down Expand Up @@ -56,9 +58,8 @@ void WaylandOutputManager::RemoveWaylandOutput(uint32_t output_id) {
output_list_.erase(output_it);
}

std::unique_ptr<WaylandScreen> WaylandOutputManager::CreateWaylandScreen(
WaylandConnection* connection) {
auto wayland_screen = std::make_unique<WaylandScreen>(connection);
std::unique_ptr<WaylandScreen> WaylandOutputManager::CreateWaylandScreen() {
auto wayland_screen = std::make_unique<WaylandScreen>(connection_);
wayland_screen_ = wayland_screen->GetWeakPtr();

// As long as |wl_output| sends geometry and other events asynchronously (that
Expand Down Expand Up @@ -86,13 +87,22 @@ WaylandOutput* WaylandOutputManager::GetOutput(uint32_t id) const {
return output_it->get();
}

WaylandOutput* WaylandOutputManager::GetPrimaryOutput() const {
if (wayland_screen_)
return GetOutput(wayland_screen_->GetPrimaryDisplay().id());
return nullptr;
}

void WaylandOutputManager::OnOutputHandleMetrics(uint32_t output_id,
const gfx::Rect& new_bounds,
int32_t scale_factor) {
if (wayland_screen_) {
wayland_screen_->OnOutputAddedOrUpdated(output_id, new_bounds,
scale_factor);
}
auto* wayland_window_manager = connection_->wayland_window_manager();
for (auto* window : wayland_window_manager->GetWindowsOnOutput(output_id))
window->UpdateBufferScale(true);
}

WaylandOutputManager::OutputList::const_iterator
Expand Down
8 changes: 5 additions & 3 deletions ui/ozone/platform/wayland/host/wayland_output_manager.h
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ class WaylandOutput;

class WaylandOutputManager : public WaylandOutput::Delegate {
public:
WaylandOutputManager();
explicit WaylandOutputManager(WaylandConnection* connection);
~WaylandOutputManager() override;

// Says if at least one output has already been announced by a Wayland
Expand All @@ -35,10 +35,10 @@ class WaylandOutputManager : public WaylandOutput::Delegate {
void RemoveWaylandOutput(const uint32_t output_id);

// Creates a platform screen and feeds it with existing outputs.
std::unique_ptr<WaylandScreen> CreateWaylandScreen(
WaylandConnection* connection);
std::unique_ptr<WaylandScreen> CreateWaylandScreen();

WaylandOutput* GetOutput(uint32_t id) const;
WaylandOutput* GetPrimaryOutput() const;

WaylandScreen* wayland_screen() const { return wayland_screen_.get(); }

Expand All @@ -54,6 +54,8 @@ class WaylandOutputManager : public WaylandOutput::Delegate {

OutputList output_list_;

WaylandConnection* const connection_;

// Non-owned wayland screen instance.
base::WeakPtr<WaylandScreen> wayland_screen_;

Expand Down
4 changes: 0 additions & 4 deletions ui/ozone/platform/wayland/host/wayland_screen.cc
Original file line number Diff line number Diff line change
Expand Up @@ -129,10 +129,6 @@ void WaylandScreen::AddOrUpdateDisplay(uint32_t output_id,
}

display_list_.AddOrUpdateDisplay(changed_display, type);

auto* wayland_window_manager = connection_->wayland_window_manager();
for (auto* window : wayland_window_manager->GetWindowsOnOutput(output_id))
window->UpdateBufferScale(true);
}

void WaylandScreen::OnTabletStateChanged(display::TabletState tablet_state) {
Expand Down
2 changes: 1 addition & 1 deletion ui/ozone/platform/wayland/host/wayland_screen_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ class WaylandScreenTest : public WaylandTest {
ASSERT_TRUE(output_manager_);

EXPECT_TRUE(output_manager_->IsOutputReady());
platform_screen_ = output_manager_->CreateWaylandScreen(connection_.get());
platform_screen_ = output_manager_->CreateWaylandScreen();
}

protected:
Expand Down
43 changes: 24 additions & 19 deletions ui/ozone/platform/wayland/host/wayland_window.cc
Original file line number Diff line number Diff line change
Expand Up @@ -87,32 +87,37 @@ void WaylandWindow::OnWindowLostCapture() {

void WaylandWindow::UpdateBufferScale(bool update_bounds) {
DCHECK(connection_->wayland_output_manager());
const auto* screen = connection_->wayland_output_manager()->wayland_screen();

// The client might not create screen at all.
if (!screen)
return;

int32_t new_scale = 0;
if (parent_window_) {
new_scale = parent_window_->buffer_scale();
ui_scale_ = parent_window_->ui_scale_;
} else {
const auto display = screen->GetDisplayForAcceleratedWidget(GetWidget());
new_scale = connection_->wayland_output_manager()
->GetOutput(display.id())
->scale_factor();

if (display::Display::HasForceDeviceScaleFactor())
ui_scale_ = display::Display::GetForcedDeviceScaleFactor();
else
ui_scale_ = display.device_scale_factor();
auto preferred_outputs_id = GetPreferredEnteredOutputId();
if (preferred_outputs_id == 0) {
// If non of the output are entered, use primary output. This is what
// WaylandScreen returns back to ScreenOzone.
auto* primary_output =
connection_->wayland_output_manager()->GetPrimaryOutput();
// We don't know our primary output - WaylandScreen hasn't been created
// yet.
if (!primary_output)
return;
preferred_outputs_id = primary_output->output_id();
}

auto* output =
connection_->wayland_output_manager()->GetOutput(preferred_outputs_id);
// Sanity check. WaylandConnection always waits for at least one output to
// become ready. See WaylandConnection::Initialize.
DCHECK(output);
int32_t new_scale = output->scale_factor();
ui_scale_ = output->GetUIScaleFactor();

int32_t old_scale = buffer_scale();
root_surface_->SetBufferScale(new_scale, update_bounds);
// We need to keep DIP size of the window the same whenever the scale changes.
if (update_bounds)
SetBoundsDip(gfx::ScaleToRoundedRect(bounds_px_, 1.0 / old_scale));

// Propagate update to the child windows
if (child_window_)
child_window_->UpdateBufferScale(update_bounds);
}

gfx::AcceleratedWidget WaylandWindow::GetWidget() const {
Expand Down
3 changes: 1 addition & 2 deletions ui/ozone/platform/wayland/ozone_platform_wayland.cc
Original file line number Diff line number Diff line change
Expand Up @@ -111,8 +111,7 @@ class OzonePlatformWayland : public OzonePlatform {
// The WaylandConnection and the WaylandOutputManager must be created
// before PlatformScreen.
DCHECK(connection_ && connection_->wayland_output_manager());
return connection_->wayland_output_manager()->CreateWaylandScreen(
connection_.get());
return connection_->wayland_output_manager()->CreateWaylandScreen();
}

PlatformClipboard* GetPlatformClipboard() override {
Expand Down
3 changes: 1 addition & 2 deletions ui/ozone/platform/wayland/test/wayland_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,7 @@ void WaylandTest::SetUp() {

ASSERT_TRUE(server_.Start(GetParam()));
ASSERT_TRUE(connection_->Initialize());
screen_ = connection_->wayland_output_manager()->CreateWaylandScreen(
connection_.get());
screen_ = connection_->wayland_output_manager()->CreateWaylandScreen();
EXPECT_CALL(delegate_, OnAcceleratedWidgetAvailable(_))
.WillOnce(SaveArg<0>(&widget_));
PlatformWindowInitProperties properties;
Expand Down

0 comments on commit e71070a

Please sign in to comment.