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
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
|
commit c5af1605ca8e
Author: Emilio Cobos Álvarez <emilio@crisal.io>
Date: Mon Feb 5 15:27:35 2018 -0500
Bug 1433671 - Make AccessibleCaretManager flushes a bit more sound. r=TYLin, a=RyanVM
The accessible caret manager is owned by the event hub, that is owned by the
shell.
All the callers of methods that call FlushLayout on the AccessibleCaretManager
should hold an external reference to the event hub.
Flushing pending notifications can run arbitrary script, that can call Destroy()
on the pres shell (and thus tear down the accessible caret event hub, and the
manager with him).
I don't know why before my change this wasn't crashing badly, but the code as it
was just doesn't look sound to me at all either (maybe I'm misunderstanding
something and I should just revert that patch and give up on having nice
invariants during our flushes..., but I don't think it's the case).
This also adds some sanity-checking that we don't die under our flush.
MozReview-Commit-ID: 4s0UT0fD3TI
---
layout/base/AccessibleCaretEventHub.cpp | 6 +++--
layout/base/AccessibleCaretManager.cpp | 29 +++++++++++++-----------
layout/base/AccessibleCaretManager.h | 17 ++++++++++----
layout/base/gtest/TestAccessibleCaretManager.cpp | 4 ++--
4 files changed, 34 insertions(+), 22 deletions(-)
diff --git layout/base/AccessibleCaretEventHub.cpp layout/base/AccessibleCaretEventHub.cpp
index 2c777396c857..05da6cad3369 100644
--- layout/base/AccessibleCaretEventHub.cpp
+++ layout/base/AccessibleCaretEventHub.cpp
@@ -677,7 +677,8 @@ AccessibleCaretEventHub::CancelLongTapInjector()
AccessibleCaretEventHub::FireLongTap(nsITimer* aTimer,
void* aAccessibleCaretEventHub)
{
- auto* self = static_cast<AccessibleCaretEventHub*>(aAccessibleCaretEventHub);
+ RefPtr<AccessibleCaretEventHub> self =
+ static_cast<AccessibleCaretEventHub*>(aAccessibleCaretEventHub);
self->mState->OnLongTap(self, self->mPressPoint);
}
@@ -779,7 +780,8 @@ AccessibleCaretEventHub::CancelScrollEndInjector()
AccessibleCaretEventHub::FireScrollEnd(nsITimer* aTimer,
void* aAccessibleCaretEventHub)
{
- auto* self = static_cast<AccessibleCaretEventHub*>(aAccessibleCaretEventHub);
+ RefPtr<AccessibleCaretEventHub> self =
+ static_cast<AccessibleCaretEventHub*>(aAccessibleCaretEventHub);
self->mState->OnScrollEnd(self);
}
diff --git layout/base/AccessibleCaretManager.cpp layout/base/AccessibleCaretManager.cpp
index 19ca4e3da9cc..994396785ba6 100644
--- layout/base/AccessibleCaretManager.cpp
+++ layout/base/AccessibleCaretManager.cpp
@@ -124,6 +124,7 @@ AccessibleCaretManager::AccessibleCaretManager(nsIPresShell* aPresShell)
AccessibleCaretManager::~AccessibleCaretManager()
{
+ MOZ_RELEASE_ASSERT(!mFlushingLayout, "Going away in FlushLayout? Bad!");
}
void
@@ -221,8 +222,7 @@ AccessibleCaretManager::HideCarets()
void
AccessibleCaretManager::UpdateCarets(UpdateCaretsHintSet aHint)
{
- FlushLayout();
- if (IsTerminated()) {
+ if (!FlushLayout()) {
return;
}
@@ -389,8 +389,7 @@ AccessibleCaretManager::UpdateCaretsForSelectionMode(UpdateCaretsHintSet aHints)
if (firstCaretResult == PositionChangedResult::Changed ||
secondCaretResult == PositionChangedResult::Changed) {
// Flush layout to make the carets intersection correct.
- FlushLayout();
- if (IsTerminated()) {
+ if (!FlushLayout()) {
return;
}
}
@@ -1022,12 +1021,17 @@ AccessibleCaretManager::ClearMaintainedSelection() const
}
}
-void
-AccessibleCaretManager::FlushLayout() const
+bool
+AccessibleCaretManager::FlushLayout()
{
if (mPresShell) {
+ AutoRestore<bool> flushing(mFlushingLayout);
+ mFlushingLayout = true;
+
mPresShell->FlushPendingNotifications(FlushType::Layout);
}
+
+ return !IsTerminated();
}
nsIFrame*
@@ -1380,14 +1384,9 @@ AccessibleCaretManager::AdjustDragBoundary(const nsPoint& aPoint) const
}
void
-AccessibleCaretManager::DispatchCaretStateChangedEvent(CaretChangedReason aReason) const
+AccessibleCaretManager::DispatchCaretStateChangedEvent(CaretChangedReason aReason)
{
- if (!mPresShell) {
- return;
- }
-
- FlushLayout();
- if (IsTerminated()) {
+ if (!FlushLayout()) {
return;
}
diff --git layout/base/AccessibleCaretManager.h layout/base/AccessibleCaretManager.h
index 2de756d4699b..4805b519585d 100644
--- layout/base/AccessibleCaretManager.h
+++ layout/base/AccessibleCaretManager.h
@@ -193,9 +193,13 @@ protected:
nsPoint AdjustDragBoundary(const nsPoint& aPoint) const;
void ClearMaintainedSelection() const;
- // Caller is responsible to use IsTerminated() to check whether PresShell is
- // still valid.
- void FlushLayout() const;
+ // This method could kill the shell, so callers to methods that call
+ // FlushLayout should ensure the event hub that owns us is still alive.
+ //
+ // See the mRefCnt assertions in AccessibleCaretEventHub.
+ //
+ // Returns whether mPresShell we're holding is still valid.
+ MOZ_MUST_USE bool FlushLayout();
dom::Element* GetEditingHostForFrame(nsIFrame* aFrame) const;
dom::Selection* GetSelection() const;
@@ -251,7 +255,7 @@ protected:
// This function will flush layout, so caller must ensure the PresShell is
// still valid after calling this method.
- virtual void DispatchCaretStateChangedEvent(dom::CaretChangedReason aReason) const;
+ virtual void DispatchCaretStateChangedEvent(dom::CaretChangedReason aReason);
// ---------------------------------------------------------------------------
// Member variables
@@ -295,6 +299,9 @@ protected:
// Set to true in OnScrollStart() and set to false in OnScrollEnd().
bool mIsScrollStarted = false;
+ // Whether we're flushing layout, used for sanity-checking.
+ bool mFlushingLayout = false;
+
static const int32_t kAutoScrollTimerDelay = 30;
// Clicking on the boundary of input or textarea will move the caret to the
diff --git layout/base/gtest/TestAccessibleCaretManager.cpp layout/base/gtest/TestAccessibleCaretManager.cpp
index 1d3a1c039404..118e5cbcf5fe 100644
--- layout/base/gtest/TestAccessibleCaretManager.cpp
+++ layout/base/gtest/TestAccessibleCaretManager.cpp
@@ -108,8 +108,8 @@ public:
virtual bool IsTerminated() const override { return false; }
MOCK_CONST_METHOD0(GetCaretMode, CaretMode());
- MOCK_CONST_METHOD1(DispatchCaretStateChangedEvent,
- void(CaretChangedReason aReason));
+ MOCK_METHOD1(DispatchCaretStateChangedEvent,
+ void(CaretChangedReason aReason));
MOCK_CONST_METHOD1(HasNonEmptyTextContent, bool(nsINode* aNode));
}; // class MockAccessibleCaretManager
|
