Skip to content

Commit

Permalink
Clear pending_bounds_change on remote surface destruction
Browse files Browse the repository at this point in the history 
It is possible that bounds change is scheduled to
pending_bounds_change of the RemoteShell, and then the surface
is destroyed.  In that case RemoteShell will try sending events
to already destroyed object, which will lead to a crash.

This CL unregister the entry from pending_bounds_change when
the surface is destructed.

To make this, this CL also refactors exo::ClientControlledShellSurface.
Previously it has a lot of callbacks to pass events from exo to
wayland. This CL reforms those callbacks into a single delegate
class.

Bug: 1163271
Test: none
Change-Id: I879872db344f3c129a67ec9dbdcf0704d75634cb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2707387
Commit-Queue: Jun Mukai <mukai@chromium.org>
Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
Cr-Commit-Position: refs/heads/master@{#856544}
  • Loading branch information
@jmuk
jmuk authored and Chromium LUCI CQ committed Feb 23, 2021
1 parent 04590a3 commit f586a4a
Show file tree
Hide file tree
Showing 6 changed files with 337 additions and 354 deletions.
26 changes: 13 additions & 13 deletions components/exo/client_controlled_shell_surface.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -513,8 +513,8 @@ void ClientControlledShellSurface::OnWindowStateChangeEvent(
chromeos::WindowStateType next_state) {
// Android already knows this state change. Don't send state change to Android
// that it is about to do anyway.
if (state_changed_callback_ && pending_window_state_ != next_state)
state_changed_callback_.Run(current_state, next_state);
if (delegate_ && pending_window_state_ != next_state)
delegate_->OnStateChanged(current_state, next_state);
}

void ClientControlledShellSurface::StartDrag(int component,
Expand Down Expand Up @@ -659,7 +659,7 @@ void ClientControlledShellSurface::OnBoundsChangeEvent(
if (geometry().IsEmpty() || window_bounds.IsEmpty() ||
(widget_->IsMinimized() &&
requested_state == chromeos::WindowStateType::kMinimized) ||
!bounds_changed_callback_) {
!delegate_) {
return;
}

Expand All @@ -675,8 +675,8 @@ void ClientControlledShellSurface::OnBoundsChangeEvent(
const float scale = 1.f / GetClientToDpScale();
const gfx::Rect scaled_client_bounds =
gfx::ScaleToRoundedRect(client_bounds, scale);
bounds_changed_callback_.Run(current_state, requested_state, display_id,
scaled_client_bounds, is_resize, bounds_change);
delegate_->OnBoundsChanged(current_state, requested_state, display_id,
scaled_client_bounds, is_resize, bounds_change);

auto* window_state = GetWindowState();
if (server_reparent_window_ &&
Expand All @@ -691,25 +691,25 @@ void ClientControlledShellSurface::OnBoundsChangeEvent(
}

void ClientControlledShellSurface::ChangeZoomLevel(ZoomChange change) {
if (change_zoom_level_callback_)
change_zoom_level_callback_.Run(change);
if (delegate_)
delegate_->OnZoomLevelChanged(change);
}

void ClientControlledShellSurface::OnDragStarted(int component) {
in_drag_ = true;
if (drag_started_callback_)
drag_started_callback_.Run(component);
if (delegate_)
delegate_->OnDragStarted(component);
}

void ClientControlledShellSurface::OnDragFinished(bool canceled,
const gfx::PointF& location) {
in_drag_ = false;
if (!drag_finished_callback_)
if (!delegate_)
return;

const float scale = 1.f / GetClientToDpScale();
const gfx::PointF scaled = gfx::ScalePoint(location, scale);
drag_finished_callback_.Run(scaled.x(), scaled.y(), canceled);
delegate_->OnDragFinished(scaled.x(), scaled.y(), canceled);
}

float ClientControlledShellSurface::GetClientToDpScale() const {
Expand Down Expand Up @@ -1181,13 +1181,13 @@ void ClientControlledShellSurface::OnPostWidgetCommit() {
UpdateFrame();
UpdateBackdrop();

if (geometry_changed_callback_) {
if (delegate_) {
// Since the visible bounds are in screen coordinates, do not scale these
// bounds with the display's scale before sending them.
// TODO(b/167286795): Instead of sending bounds in screen coordinates, send
// the bounds in the display along with the display information, similar to
// the bounds_changed_callback_.
geometry_changed_callback_.Run(GetVisibleBounds());
delegate_->OnGeometryChanged(GetVisibleBounds());
}

// Apply new top inset height.
Expand Down
78 changes: 22 additions & 56 deletions components/exo/client_controlled_shell_surface.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,17 +45,33 @@ enum class ZoomChange { IN, OUT, RESET };
class ClientControlledShellSurface : public ShellSurfaceBase,
public ui::CompositorLockClient {
public:
// TODO(mukai): integrate this with ShellSurfaceBase's callback.
class Delegate {
public:
virtual ~Delegate() = default;
virtual void OnGeometryChanged(const gfx::Rect& geometry) = 0;
virtual void OnStateChanged(chromeos::WindowStateType old_state_type,
chromeos::WindowStateType new_state_type) = 0;
virtual void OnBoundsChanged(chromeos::WindowStateType current_state,
chromeos::WindowStateType requested_state,
int64_t display_id,
const gfx::Rect& bounds_in_display,
bool is_resize,
int bounds_change) = 0;
virtual void OnDragStarted(int component) = 0;
virtual void OnDragFinished(int x, int y, bool canceled) = 0;
virtual void OnZoomLevelChanged(ZoomChange zoom_change) = 0;
};

ClientControlledShellSurface(Surface* surface,
bool can_minimize,
int container,
bool default_scale_cancellation);
~ClientControlledShellSurface() override;

using GeometryChangedCallback =
base::RepeatingCallback<void(const gfx::Rect& geometry)>;

void set_geometry_changed_callback(const GeometryChangedCallback& callback) {
geometry_changed_callback_ = callback;
Delegate* set_delegate(std::unique_ptr<Delegate> delegate) {
delegate_ = std::move(delegate);
return delegate_.get();
}

void set_server_reparent_window(bool reparent) {
Expand Down Expand Up @@ -92,50 +108,6 @@ class ClientControlledShellSurface : public ShellSurfaceBase,
// Called when the client was set to PIP.
void SetPip();

// Set the callback to run when the surface state changed.
using StateChangedCallback =
base::RepeatingCallback<void(chromeos::WindowStateType old_state_type,
chromeos::WindowStateType new_state_type)>;
void set_state_changed_callback(
const StateChangedCallback& state_changed_callback) {
state_changed_callback_ = state_changed_callback;
}

// Set the callback to run when the surface bounds changed.
using BoundsChangedCallback =
base::RepeatingCallback<void(chromeos::WindowStateType current_state,
chromeos::WindowStateType requested_state,
int64_t display_id,
const gfx::Rect& bounds_in_display,
bool is_resize,
int bounds_change)>;
void set_bounds_changed_callback(
const BoundsChangedCallback& bounds_changed_callback) {
bounds_changed_callback_ = bounds_changed_callback;
}

bool has_bounds_changed_callback() const {
return static_cast<bool>(bounds_changed_callback_);
}

// Set the callback to run when the drag operation started.
using DragStartedCallback = base::RepeatingCallback<void(int direction)>;
void set_drag_started_callback(const DragStartedCallback& callback) {
drag_started_callback_ = callback;
}

// Set the callback to run when the drag operation finished.
using DragFinishedCallback = base::RepeatingCallback<void(int, int, bool)>;
void set_drag_finished_callback(const DragFinishedCallback& callback) {
drag_finished_callback_ = callback;
}

// Set callback to run when user requests to change a zoom level.
using ChangeZoomLevelCallback = base::RepeatingCallback<void(ZoomChange)>;
void set_change_zoom_level_callback(const ChangeZoomLevelCallback& callback) {
change_zoom_level_callback_ = callback;
}

// Returns true if this shell surface is currently being dragged.
bool IsDragging();

Expand Down Expand Up @@ -318,8 +290,6 @@ class ClientControlledShellSurface : public ShellSurfaceBase,
const gfx::Rect& window_bounds,
chromeos::WindowStateType window_state) const;

GeometryChangedCallback geometry_changed_callback_;

int top_inset_height_ = 0;
int pending_top_inset_height_ = 0;

Expand All @@ -331,11 +301,7 @@ class ClientControlledShellSurface : public ShellSurfaceBase,
uint32_t frame_visible_button_mask_ = 0;
uint32_t frame_enabled_button_mask_ = 0;

StateChangedCallback state_changed_callback_;
BoundsChangedCallback bounds_changed_callback_;
DragStartedCallback drag_started_callback_;
DragFinishedCallback drag_finished_callback_;
ChangeZoomLevelCallback change_zoom_level_callback_;
std::unique_ptr<Delegate> delegate_;

// TODO(reveman): Use configure callbacks for orientation. crbug.com/765954
Orientation pending_orientation_ = Orientation::LANDSCAPE;
Expand Down
Loading

0 comments on commit f586a4a

Please sign in to comment.