1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
|
From 5e3a738b1204941aab9f15c0eb3d06e20fefd96e Mon Sep 17 00:00:00 2001
From: Scott Violet <sky@chromium.org>
Date: Mon, 8 Mar 2021 21:07:39 +0000
Subject: [PATCH] x11/ozone: fix two edge cases
WindowTreeHost::OnHostMovedInPixels() may trigger a nested message
loop (tab dragging), which when the stack unravels means this may
be deleted. This adds an early out if this happens.
X11WholeScreenMoveLoop has a similar issue, in so far as notifying
the delegate may delete this.
BUG=1185482
TEST=WindowTreeHostPlatform.DeleteHostFromOnHostMovedInPixels
Change-Id: Ieca1c90b3e4358da50b332abe2941fdbb50c5c25
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2743555
Reviewed-by: Thomas Anderson <thomasanderson@chromium.org>
Commit-Queue: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#860852}
---
ui/aura/window_tree_host_platform.cc | 10 ++++-
ui/aura/window_tree_host_platform_unittest.cc | 40 ++++++++++++++++++-
ui/base/x/x11_whole_screen_move_loop.cc | 4 ++
3 files changed, 51 insertions(+), 3 deletions(-)
diff --git a/ui/aura/window_tree_host_platform.cc b/ui/aura/window_tree_host_platform.cc
index ce8395fe07..7589542026 100644
--- a/ui/aura/window_tree_host_platform.cc
+++ b/ui/aura/window_tree_host_platform.cc
@@ -214,13 +214,21 @@ void WindowTreeHostPlatform::OnBoundsChanged(const gfx::Rect& new_bounds) {
float current_scale = compositor()->device_scale_factor();
float new_scale = ui::GetScaleFactorForNativeView(window());
gfx::Rect old_bounds = bounds_in_pixels_;
+ auto weak_ref = GetWeakPtr();
bounds_in_pixels_ = new_bounds;
- if (bounds_in_pixels_.origin() != old_bounds.origin())
+ if (bounds_in_pixels_.origin() != old_bounds.origin()) {
OnHostMovedInPixels(bounds_in_pixels_.origin());
+ // Changing the bounds may destroy this.
+ if (!weak_ref)
+ return;
+ }
if (bounds_in_pixels_.size() != old_bounds.size() ||
current_scale != new_scale) {
pending_size_ = gfx::Size();
OnHostResizedInPixels(bounds_in_pixels_.size());
+ // Changing the size may destroy this.
+ if (!weak_ref)
+ return;
}
DCHECK_GT(on_bounds_changed_recursion_depth_, 0);
if (--on_bounds_changed_recursion_depth_ == 0) {
diff --git a/ui/aura/window_tree_host_platform_unittest.cc b/ui/aura/window_tree_host_platform_unittest.cc
index eda14e2f0c..4de039c88a 100644
--- a/ui/aura/window_tree_host_platform_unittest.cc
+++ b/ui/aura/window_tree_host_platform_unittest.cc
@@ -34,7 +34,7 @@ class TestWindowTreeHost : public WindowTreeHostPlatform {
// OnHostWill/DidProcessBoundsChange. Additionally, this triggers a bounds
// change from within OnHostResized(). Such a scenario happens in production
// code.
-class TestWindowTreeHostObserver : public aura::WindowTreeHostObserver {
+class TestWindowTreeHostObserver : public WindowTreeHostObserver {
public:
TestWindowTreeHostObserver(WindowTreeHostPlatform* host,
ui::PlatformWindow* platform_window)
@@ -51,7 +51,7 @@ class TestWindowTreeHostObserver : public aura::WindowTreeHostObserver {
return on_host_will_process_bounds_change_count_;
}
- // aura::WindowTreeHostObserver:
+ // WindowTreeHostObserver:
void OnHostResized(WindowTreeHost* host) override {
if (!should_change_bounds_in_on_resized_)
return;
@@ -92,5 +92,41 @@ TEST_F(WindowTreeHostPlatformTest, HostWillProcessBoundsChangeRecursion) {
EXPECT_EQ(1, observer.on_host_will_process_bounds_change_count());
}
+// Deletes WindowTreeHostPlatform from OnHostMovedInPixels().
+class DeleteHostWindowTreeHostObserver : public WindowTreeHostObserver {
+ public:
+ explicit DeleteHostWindowTreeHostObserver(
+ std::unique_ptr<TestWindowTreeHost> host)
+ : host_(std::move(host)) {
+ host_->AddObserver(this);
+ }
+ ~DeleteHostWindowTreeHostObserver() override = default;
+
+ TestWindowTreeHost* host() { return host_.get(); }
+
+ // WindowTreeHostObserver:
+ void OnHostMovedInPixels(WindowTreeHost* host,
+ const gfx::Point& new_origin_in_pixels) override {
+ host_->RemoveObserver(this);
+ host_.reset();
+ }
+
+ private:
+ std::unique_ptr<TestWindowTreeHost> host_;
+
+ DISALLOW_COPY_AND_ASSIGN(DeleteHostWindowTreeHostObserver);
+};
+
+// Verifies WindowTreeHostPlatform can be safely deleted when calling
+// OnHostMovedInPixels().
+// Regression test for https://crbug.com/1185482
+TEST_F(WindowTreeHostPlatformTest, DeleteHostFromOnHostMovedInPixels) {
+ std::unique_ptr<TestWindowTreeHost> host =
+ std::make_unique<TestWindowTreeHost>();
+ DeleteHostWindowTreeHostObserver observer(std::move(host));
+ observer.host()->SetBoundsInPixels(gfx::Rect(1, 2, 3, 4));
+ EXPECT_EQ(nullptr, observer.host());
+}
+
} // namespace
} // namespace aura
diff --git a/ui/base/x/x11_whole_screen_move_loop.cc b/ui/base/x/x11_whole_screen_move_loop.cc
index 5ed215db66..db678799db 100644
--- a/ui/base/x/x11_whole_screen_move_loop.cc
+++ b/ui/base/x/x11_whole_screen_move_loop.cc
@@ -78,9 +78,13 @@ X11WholeScreenMoveLoop::~X11WholeScreenMoveLoop() {
void X11WholeScreenMoveLoop::DispatchMouseMovement() {
if (!last_motion_in_screen_)
return;
+ auto weak_ref = weak_factory_.GetWeakPtr();
delegate_->OnMouseMovement(last_motion_in_screen_->root_location(),
last_motion_in_screen_->flags(),
last_motion_in_screen_->time_stamp());
+ // The delegate may delete this during dispatch.
+ if (!weak_ref)
+ return;
last_motion_in_screen_.reset();
}
|