Skip to content

Commit 7e77f1e

Browse files
author
[email protected]
committed
Fix Ref to deref before assignment, add tests for this to RefPtr, Ref, Function
https://bugs.webkit.org/show_bug.cgi?id=173526 Reviewed by Sam Weinig. Source/WTF: * wtf/Ref.h: Changed operator= to not be defined inside the class definition. Added swap functions. Changed operator= implementations to use swap in the conventional manner, the same way that RefPtr does. Tools: * TestWebKitAPI/CMakeLists.txt: Added Function.cpp. * TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj: Ditto. * TestWebKitAPI/Tests/WTF/Function.cpp: Added. Contains basic tests and some tests for assignment before destruction ones. * TestWebKitAPI/Tests/WTF/MoveOnly.h: Added a () operator so this can be used as a function, so it can be used in WTF::Function tests. * TestWebKitAPI/Tests/WTF/Ref.cpp: Use EXPECT macros insead of ASSERT. Added tests for swap and for assignment before deref. * TestWebKitAPI/Tests/WTF/RefLogger.cpp: Stopped using inlining; no good reason to inline everything. Also removed the unnecessary clearing of the log every time the DerivedRefLogger constructor is called. * TestWebKitAPI/Tests/WTF/RefLogger.h: Ditto. * TestWebKitAPI/Tests/WTF/RefPtr.cpp: Use EXPECT macros instead of ASSERT. Added tests for assignment before deref and similar for releaseNonNull. git-svn-id: http://svn.webkit.org/repository/webkit/trunk@218496 268f45cc-cd09-0410-ab3c-d52691b4dbfc
1 parent b6fb690 commit 7e77f1e

File tree

11 files changed

+621
-202
lines changed

11 files changed

+621
-202
lines changed

‎Source/WTF/ChangeLog

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,14 @@
1+
2017-06-18 Darin Adler <[email protected]>
2+
3+
Fix Ref to deref before assignment, add tests for this to RefPtr, Ref, Function
4+
https://bugs.webkit.org/show_bug.cgi?id=173526
5+
6+
Reviewed by Sam Weinig.
7+
8+
* wtf/Ref.h: Changed operator= to not be defined inside the class definition.
9+
Added swap functions. Changed operator= implementations to use swap in the
10+
conventional manner, the same way that RefPtr does.
11+
112
2017-06-18 Chris Dumez <[email protected]>
213

314
Use WTF::Function instead of std::function in WTF/

‎Source/WTF/wtf/Ref.h

Lines changed: 41 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -82,37 +82,15 @@ template<typename T> class Ref {
8282
ASSERT(m_ptr);
8383
}
8484

85-
Ref& operator=(T& object)
86-
{
87-
ASSERT(m_ptr);
88-
object.ref();
89-
m_ptr->deref();
90-
m_ptr = &object;
91-
ASSERT(m_ptr);
92-
return *this;
93-
}
85+
Ref& operator=(T&);
86+
Ref& operator=(Ref&&);
87+
template<typename U> Ref& operator=(Ref<U>&&);
9488

9589
// Use copyRef() and the move assignment operators instead.
96-
Ref& operator=(const Ref& reference) = delete;
97-
template<typename U> Ref& operator=(const Ref<U>& reference) = delete;
98-
99-
Ref& operator=(Ref&& reference)
100-
{
101-
ASSERT(m_ptr);
102-
m_ptr->deref();
103-
m_ptr = &reference.leakRef();
104-
ASSERT(m_ptr);
105-
return *this;
106-
}
90+
Ref& operator=(const Ref&) = delete;
91+
template<typename U> Ref& operator=(const Ref<U>&) = delete;
10792

108-
template<typename U> Ref& operator=(Ref<U>&& reference)
109-
{
110-
ASSERT(m_ptr);
111-
m_ptr->deref();
112-
m_ptr = &reference.leakRef();
113-
ASSERT(m_ptr);
114-
return *this;
115-
}
93+
void swap(Ref&);
11694

11795
// Hash table deleted values, which are only constructed and never copied or destroyed.
11896
Ref(HashTableDeletedValueType) : m_ptr(hashTableDeletedValue()) { }
@@ -171,6 +149,41 @@ template<typename T> class Ref {
171149
T* m_ptr;
172150
};
173151

152+
template<typename T> void swap(Ref<T>&, Ref<T>&);
153+
template<typename T> Ref<T> adoptRef(T&);
154+
template<typename T> Ref<T> makeRef(T&);
155+
156+
template<typename T> inline Ref<T>& Ref<T>::operator=(T& reference)
157+
{
158+
Ref copiedReference = reference;
159+
swap(copiedReference);
160+
return *this;
161+
}
162+
163+
template<typename T> inline Ref<T>& Ref<T>::operator=(Ref&& reference)
164+
{
165+
Ref movedReference = WTFMove(reference);
166+
swap(movedReference);
167+
return *this;
168+
}
169+
170+
template<typename T> template<typename U> inline Ref<T>& Ref<T>::operator=(Ref<U>&& reference)
171+
{
172+
Ref movedReference = WTFMove(reference);
173+
swap(movedReference);
174+
return *this;
175+
}
176+
177+
template<typename T> inline void Ref<T>::swap(Ref& other)
178+
{
179+
std::swap(m_ptr, other.m_ptr);
180+
}
181+
182+
template<typename T> inline void swap(Ref<T>& a, Ref<T>& b)
183+
{
184+
a.swap(b);
185+
}
186+
174187
template<typename T> template<typename U> inline Ref<T> Ref<T>::replace(Ref<U>&& reference)
175188
{
176189
auto oldReference = adoptRef(*m_ptr);

‎Tools/ChangeLog

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,30 @@
1+
2017-06-18 Darin Adler <[email protected]>
2+
3+
Fix Ref to deref before assignment, add tests for this to RefPtr, Ref, Function
4+
https://bugs.webkit.org/show_bug.cgi?id=173526
5+
6+
Reviewed by Sam Weinig.
7+
8+
* TestWebKitAPI/CMakeLists.txt: Added Function.cpp.
9+
* TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj: Ditto.
10+
11+
* TestWebKitAPI/Tests/WTF/Function.cpp: Added. Contains basic tests and some
12+
tests for assignment before destruction ones.
13+
14+
* TestWebKitAPI/Tests/WTF/MoveOnly.h: Added a () operator so this can be used
15+
as a function, so it can be used in WTF::Function tests.
16+
17+
* TestWebKitAPI/Tests/WTF/Ref.cpp: Use EXPECT macros insead of ASSERT.
18+
Added tests for swap and for assignment before deref.
19+
20+
* TestWebKitAPI/Tests/WTF/RefLogger.cpp: Stopped using inlining; no good reason
21+
to inline everything. Also removed the unnecessary clearing of the log every time
22+
the DerivedRefLogger constructor is called.
23+
* TestWebKitAPI/Tests/WTF/RefLogger.h: Ditto.
24+
25+
* TestWebKitAPI/Tests/WTF/RefPtr.cpp: Use EXPECT macros instead of ASSERT.
26+
Added tests for assignment before deref and similar for releaseNonNull.
27+
128
2017-06-19 Sam Weinig <[email protected]>
229

330
[WebIDL] Properly model buffer source / typed arrays as their own IDL types

‎Tools/TestWebKitAPI/CMakeLists.txt

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,7 @@ set(TestWTF_SOURCES
5050
${TESTWEBKITAPI_DIR}/Tests/WTF/Deque.cpp
5151
${TESTWEBKITAPI_DIR}/Tests/WTF/EnumTraits.cpp
5252
${TESTWEBKITAPI_DIR}/Tests/WTF/Expected.cpp
53+
${TESTWEBKITAPI_DIR}/Tests/WTF/Function.cpp
5354
${TESTWEBKITAPI_DIR}/Tests/WTF/HashCountedSet.cpp
5455
${TESTWEBKITAPI_DIR}/Tests/WTF/HashMap.cpp
5556
${TESTWEBKITAPI_DIR}/Tests/WTF/HashSet.cpp

‎Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -489,6 +489,7 @@
489489
83DE134D1EF1C50800C1B355 /* ResponsivenessTimer.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 83DE134C1EF1C4FE00C1B355 /* ResponsivenessTimer.cpp */; };
490490
8E4A85371E1D1AB200F53B0F /* GridPosition.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 8E4A85361E1D1AA100F53B0F /* GridPosition.cpp */; };
491491
930AD402150698D00067970F /* lots-of-text.html in Copy Resources */ = {isa = PBXBuildFile; fileRef = 930AD401150698B30067970F /* lots-of-text.html */; };
492+
9310CD381EF708FB0050FFE0 /* Function.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 9310CD361EF708FB0050FFE0 /* Function.cpp */; };
492493
9329AA291DE3F81E003ABD07 /* TextBreakIterator.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 9329AA281DE3F81E003ABD07 /* TextBreakIterator.cpp */; };
493494
932AE53D1D371047005DFFAF /* focus-inputs.html in Copy Resources */ = {isa = PBXBuildFile; fileRef = 93575C551D30366E000D604D /* focus-inputs.html */; };
494495
9361002914DC95A70061379D /* lots-of-iframes.html in Copy Resources */ = {isa = PBXBuildFile; fileRef = 9361002814DC957B0061379D /* lots-of-iframes.html */; };
@@ -1333,6 +1334,7 @@
13331334
8DD76FA10486AA7600D96B5E /* TestWebKitAPI */ = {isa = PBXFileReference; explicitFileType = "compiled.mach-o.executable"; includeInIndex = 0; path = TestWebKitAPI; sourceTree = BUILT_PRODUCTS_DIR; };
13341335
8E4A85361E1D1AA100F53B0F /* GridPosition.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = GridPosition.cpp; sourceTree = "<group>"; };
13351336
930AD401150698B30067970F /* lots-of-text.html */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.html; path = "lots-of-text.html"; sourceTree = "<group>"; };
1337+
9310CD361EF708FB0050FFE0 /* Function.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = Function.cpp; sourceTree = "<group>"; };
13361338
9329AA281DE3F81E003ABD07 /* TextBreakIterator.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = TextBreakIterator.cpp; sourceTree = "<group>"; };
13371339
9331407B17B4419000F083B1 /* DidNotHandleKeyDown.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = DidNotHandleKeyDown.cpp; sourceTree = "<group>"; };
13381340
93575C551D30366E000D604D /* focus-inputs.html */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.html; path = "focus-inputs.html"; sourceTree = "<group>"; };
@@ -2308,6 +2310,7 @@
23082310
E4A757D3178AEA5B00B5D7A4 /* Deque.cpp */,
23092311
1AF7B21D1D6CD12E008C126C /* EnumTraits.cpp */,
23102312
AD7C434C1DD2A5470026888B /* Expected.cpp */,
2313+
9310CD361EF708FB0050FFE0 /* Function.cpp */,
23112314
7A38D7E51C752D5F004F157D /* HashCountedSet.cpp */,
23122315
0BCD833414857CE400EA2003 /* HashMap.cpp */,
23132316
26B2DFF815BDE599004F691D /* HashSet.cpp */,
@@ -2846,6 +2849,7 @@
28462849
7C83DE991D0A590C00FEBCF3 /* AtomicString.cpp in Sources */,
28472850
1ADAD1501D77A9F600212586 /* BlockPtr.mm in Sources */,
28482851
7C83DE9C1D0A590C00FEBCF3 /* BloomFilter.cpp in Sources */,
2852+
9310CD381EF708FB0050FFE0 /* Function.cpp in Sources */,
28492853
7C83DEA01D0A590C00FEBCF3 /* CheckedArithmeticOperations.cpp in Sources */,
28502854
7C83DEC31D0A590C00FEBCF3 /* Condition.cpp in Sources */,
28512855
7C83DEA61D0A590C00FEBCF3 /* Counters.cpp in Sources */,

‎Tools/TestWebKitAPI/Tests/WTF/Function.cpp

Lines changed: 104 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@
2525

2626
#include "config.h"
2727

28-
#include "Test.h"
28+
#include "MoveOnly.h"
2929
#include <wtf/Function.h>
3030

3131
namespace TestWebKitAPI {
@@ -137,4 +137,107 @@ TEST(WTF_Function, assignNullReEntersAssignLamda)
137137
EXPECT_EQ(-1, function_for_reentrancy_test());
138138
}
139139

140+
TEST(WTF_Function, Basics)
141+
{
142+
Function<unsigned()> a;
143+
EXPECT_FALSE(static_cast<bool>(a));
144+
EXPECT_EQ(0U, a());
145+
146+
a = [] {
147+
return 1U;
148+
};
149+
EXPECT_TRUE(static_cast<bool>(a));
150+
EXPECT_EQ(1U, a());
151+
152+
a = nullptr;
153+
EXPECT_FALSE(static_cast<bool>(a));
154+
EXPECT_EQ(0U, a());
155+
156+
a = MoveOnly { 2 };
157+
EXPECT_TRUE(static_cast<bool>(a));
158+
EXPECT_EQ(2U, a());
159+
160+
Function<unsigned()> b = WTFMove(a);
161+
EXPECT_TRUE(static_cast<bool>(b));
162+
EXPECT_EQ(2U, b());
163+
EXPECT_FALSE(static_cast<bool>(a));
164+
EXPECT_EQ(0U, a());
165+
166+
a = MoveOnly { 3 };
167+
Function<unsigned()> c = WTFMove(a);
168+
EXPECT_TRUE(static_cast<bool>(c));
169+
EXPECT_EQ(3U, c());
170+
EXPECT_FALSE(static_cast<bool>(a));
171+
EXPECT_EQ(0U, a());
172+
173+
b = WTFMove(c);
174+
EXPECT_TRUE(static_cast<bool>(b));
175+
EXPECT_EQ(3U, b());
176+
EXPECT_FALSE(static_cast<bool>(c));
177+
EXPECT_EQ(0U, c());
178+
179+
Function<unsigned()> d = nullptr;
180+
EXPECT_FALSE(static_cast<bool>(d));
181+
EXPECT_EQ(0U, d());
182+
}
183+
184+
struct FunctionDestructionChecker {
185+
FunctionDestructionChecker(Function<unsigned()>& function)
186+
: function { function }
187+
{
188+
}
189+
190+
~FunctionDestructionChecker()
191+
{
192+
functionAsBool = static_cast<bool>(function);
193+
functionResult = function();
194+
}
195+
196+
unsigned operator()() const
197+
{
198+
return 10;
199+
}
200+
201+
Function<unsigned()>& function;
202+
static std::optional<bool> functionAsBool;
203+
static std::optional<unsigned> functionResult;
204+
};
205+
206+
std::optional<bool> FunctionDestructionChecker::functionAsBool;
207+
std::optional<unsigned> FunctionDestructionChecker::functionResult;
208+
209+
TEST(WTF_Function, AssignBeforeDestroy)
210+
{
211+
Function<unsigned()> a;
212+
213+
a = FunctionDestructionChecker(a);
214+
a = [] {
215+
return 1U;
216+
};
217+
EXPECT_TRUE(static_cast<bool>(FunctionDestructionChecker::functionAsBool));
218+
EXPECT_TRUE(static_cast<bool>(FunctionDestructionChecker::functionResult));
219+
EXPECT_TRUE(FunctionDestructionChecker::functionAsBool.value());
220+
EXPECT_EQ(1U, FunctionDestructionChecker::functionResult.value());
221+
FunctionDestructionChecker::functionAsBool = std::nullopt;
222+
FunctionDestructionChecker::functionResult = std::nullopt;
223+
224+
a = FunctionDestructionChecker(a);
225+
a = nullptr;
226+
EXPECT_TRUE(static_cast<bool>(FunctionDestructionChecker::functionAsBool));
227+
EXPECT_TRUE(static_cast<bool>(FunctionDestructionChecker::functionResult));
228+
EXPECT_FALSE(FunctionDestructionChecker::functionAsBool.value());
229+
EXPECT_EQ(0U, FunctionDestructionChecker::functionResult.value());
230+
FunctionDestructionChecker::functionAsBool = std::nullopt;
231+
FunctionDestructionChecker::functionResult = std::nullopt;
232+
233+
a = FunctionDestructionChecker(a);
234+
a = MoveOnly { 2 };
235+
EXPECT_TRUE(static_cast<bool>(FunctionDestructionChecker::functionAsBool));
236+
EXPECT_TRUE(static_cast<bool>(FunctionDestructionChecker::functionResult));
237+
EXPECT_TRUE(FunctionDestructionChecker::functionAsBool.value());
238+
EXPECT_EQ(2U, FunctionDestructionChecker::functionResult.value());
239+
FunctionDestructionChecker::functionAsBool = std::nullopt;
240+
FunctionDestructionChecker::functionResult = std::nullopt;
140241
}
242+
243+
} // namespace TestWebKitAPI

‎Tools/TestWebKitAPI/Tests/WTF/MoveOnly.h

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,11 @@ class MoveOnly {
4646
return m_value;
4747
}
4848

49+
unsigned operator()() const
50+
{
51+
return m_value;
52+
}
53+
4954
MoveOnly(MoveOnly&& other)
5055
: m_value(other.m_value)
5156
{

0 commit comments

Comments
 (0)