- Revision
- 178615
- Author
- mmaxfi...@apple.com
- Date
- 2015-01-16 16:39:57 -0800 (Fri, 16 Jan 2015)
Log Message
WeakPtr functions crash when created with default constructor
https://bugs.webkit.org/show_bug.cgi?id=140479
Reviewed by Andreas Kling.
Source/WTF:
This patch rearranges how WeakPtr works, and has the following ideas behind it:
1. WeakPtr should use Ref internally. This solves the crash by always having a
valid WeakReference.
2. Clients should not be able to construct WeakReferences directly. Instead,
only WeakPtrFactory (and WeakPtr's default constructor) should be able to
construct them. They are considered an implementation detail of WeakPtr.
3. Except for the default constructor, clients should not be able to construct
WeakPtrs directly. Instead, the WeakPtrFactory must construct them. This
guarantees that the WeakPtrs all reference the same WeakReference.
4. Clients can construct a WeakPtr using its default constructor, and then
use the assignment operator to make it non-null. (Or they could use
WeakPtrFactory to make it non-null at creation-time.)
5. No one was using WeakReference::bindTo(), and it doesn't seem useful, so I
removed it.
Tests: WTF_WeakPtr API tests
* wtf/Ref.h:
(WTF::Ref::Ref): Added extra ASSERT()s, and explicitly deleted copy
constructors with a comment.
(WTF::Ref::operator=): Added extra ASSERT()s, and explicitly deleted copy
assignment operators with a comment.
* wtf/WeakPtr.h:
(WTF::WeakReference::clear): Used nullptr.
(WTF::WeakReference::create): Moved to private:
(WTF::WeakPtr::WeakPtr): For the default constructor, initialized the Ref with
a new WeakReference. For the other constructor, moved it to private:. Also added
copy constructors and copy assignment operators (since Ref doesn't have them but
RefPtr does). These constructors/operators are relied upon in various places
throughout WebCore.
(WTF::WeakPtr::operator bool): Made non-explicit.
(WTF::WeakReference::createUnbound): Deleted.
(WTF::WeakReference::bindTo): Deleted.
(WTF::WeakReference::WeakReference): Deleted.
(WTF::WeakPtrFactory::WeakPtrFactory): Deleted.
Tools:
Add WeakPtr API tests.
* TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
* TestWebKitAPI/Tests/WTF/WeakPtr.cpp: Added.
* TestWebKitAPI/Tests/WTF/Ref.cpp: Update to not use copy constructor.
Modified Paths
Added Paths
Diff
Modified: trunk/Source/WTF/ChangeLog (178614 => 178615)
--- trunk/Source/WTF/ChangeLog 2015-01-17 00:37:08 UTC (rev 178614)
+++ trunk/Source/WTF/ChangeLog 2015-01-17 00:39:57 UTC (rev 178615)
@@ -1,3 +1,47 @@
+2015-01-16 Myles C. Maxfield <mmaxfi...@apple.com>
+
+ WeakPtr functions crash when created with default constructor
+ https://bugs.webkit.org/show_bug.cgi?id=140479
+
+ Reviewed by Andreas Kling.
+
+ This patch rearranges how WeakPtr works, and has the following ideas behind it:
+
+ 1. WeakPtr should use Ref internally. This solves the crash by always having a
+ valid WeakReference.
+ 2. Clients should not be able to construct WeakReferences directly. Instead,
+ only WeakPtrFactory (and WeakPtr's default constructor) should be able to
+ construct them. They are considered an implementation detail of WeakPtr.
+ 3. Except for the default constructor, clients should not be able to construct
+ WeakPtrs directly. Instead, the WeakPtrFactory must construct them. This
+ guarantees that the WeakPtrs all reference the same WeakReference.
+ 4. Clients can construct a WeakPtr using its default constructor, and then
+ use the assignment operator to make it non-null. (Or they could use
+ WeakPtrFactory to make it non-null at creation-time.)
+ 5. No one was using WeakReference::bindTo(), and it doesn't seem useful, so I
+ removed it.
+
+ Tests: WTF_WeakPtr API tests
+
+ * wtf/Ref.h:
+ (WTF::Ref::Ref): Added extra ASSERT()s, and explicitly deleted copy
+ constructors with a comment.
+ (WTF::Ref::operator=): Added extra ASSERT()s, and explicitly deleted copy
+ assignment operators with a comment.
+ * wtf/WeakPtr.h:
+ (WTF::WeakReference::clear): Used nullptr.
+ (WTF::WeakReference::create): Moved to private:
+ (WTF::WeakPtr::WeakPtr): For the default constructor, initialized the Ref with
+ a new WeakReference. For the other constructor, moved it to private:. Also added
+ copy constructors and copy assignment operators (since Ref doesn't have them but
+ RefPtr does). These constructors/operators are relied upon in various places
+ throughout WebCore.
+ (WTF::WeakPtr::operator bool): Made non-explicit.
+ (WTF::WeakReference::createUnbound): Deleted.
+ (WTF::WeakReference::bindTo): Deleted.
+ (WTF::WeakReference::WeakReference): Deleted.
+ (WTF::WeakPtrFactory::WeakPtrFactory): Deleted.
+
2015-01-16 Yusuke Suzuki <utatane....@gmail.com>
std::all_of requires complete C++ iterators in GCC 4.8
Modified: trunk/Source/WTF/wtf/Ref.h (178614 => 178615)
--- trunk/Source/WTF/wtf/Ref.h 2015-01-17 00:37:08 UTC (rev 178614)
+++ trunk/Source/WTF/wtf/Ref.h 2015-01-17 00:39:57 UTC (rev 178615)
@@ -52,15 +52,21 @@
m_ptr->ref();
}
+ // Use copyRef() instead.
+ Ref(const Ref& other) = delete;
+ template<typename U> Ref(const Ref<U>& other) = delete;
+
Ref(Ref&& other)
: m_ptr(&other.leakRef())
{
+ ASSERT(m_ptr);
}
template<typename U>
Ref(Ref<U>&& other)
: m_ptr(&other.leakRef())
{
+ ASSERT(m_ptr);
}
Ref& operator=(T& object)
@@ -69,14 +75,20 @@
object.ref();
m_ptr->deref();
m_ptr = &object;
+ ASSERT(m_ptr);
return *this;
}
+ // Use copyRef() and the move assignment operators instead.
+ Ref& operator=(const Ref& reference) = delete;
+ template<typename U> Ref& operator=(const Ref<U>& reference) = delete;
+
Ref& operator=(Ref&& reference)
{
ASSERT(m_ptr);
m_ptr->deref();
m_ptr = &reference.leakRef();
+ ASSERT(m_ptr);
return *this;
}
@@ -85,6 +97,7 @@
ASSERT(m_ptr);
m_ptr->deref();
m_ptr = &reference.leakRef();
+ ASSERT(m_ptr);
return *this;
}
Modified: trunk/Source/WTF/wtf/WeakPtr.h (178614 => 178615)
--- trunk/Source/WTF/wtf/WeakPtr.h 2015-01-17 00:37:08 UTC (rev 178614)
+++ trunk/Source/WTF/wtf/WeakPtr.h 2015-01-17 00:39:57 UTC (rev 178615)
@@ -27,8 +27,7 @@
#define WTF_WeakPtr_h
#include <wtf/Noncopyable.h>
-#include <wtf/PassRefPtr.h>
-#include <wtf/RefPtr.h>
+#include <wtf/Ref.h>
#include <wtf/ThreadSafeRefCounted.h>
#include <wtf/Threading.h>
@@ -38,14 +37,14 @@
namespace WTF {
+template<typename T> class WeakPtr;
+template<typename T> class WeakPtrFactory;
+
template<typename T>
class WeakReference : public ThreadSafeRefCounted<WeakReference<T>> {
WTF_MAKE_NONCOPYABLE(WeakReference<T>);
WTF_MAKE_FAST_ALLOCATED;
public:
- static PassRefPtr<WeakReference<T>> create(T* ptr) { return adoptRef(new WeakReference(ptr)); }
- static PassRefPtr<WeakReference<T>> createUnbound() { return adoptRef(new WeakReference()); }
-
T* get() const
{
#if USE(WEB_THREAD)
@@ -63,21 +62,15 @@
#else
ASSERT(m_boundThread == currentThread());
#endif
- m_ptr = 0;
+ m_ptr = nullptr;
}
- void bindTo(T* ptr)
- {
- ASSERT(!m_ptr);
-#ifndef NDEBUG
- m_boundThread = currentThread();
-#endif
- m_ptr = ptr;
- }
-
private:
- WeakReference() : m_ptr(0) { }
+ friend class WeakPtr<T>;
+ friend class WeakPtrFactory<T>;
+ static Ref<WeakReference<T>> create(T* ptr) { return adoptRef(*new WeakReference(ptr)); }
+
explicit WeakReference(T* ptr)
: m_ptr(ptr)
#ifndef NDEBUG
@@ -96,16 +89,23 @@
class WeakPtr {
WTF_MAKE_FAST_ALLOCATED;
public:
- WeakPtr() { }
- WeakPtr(PassRefPtr<WeakReference<T>> ref) : m_ref(ref) { }
+ WeakPtr() : m_ref(WeakReference<T>::create(nullptr)) { }
+ WeakPtr(const WeakPtr& o) : m_ref(o.m_ref.copyRef()) { }
+ template<typename U> WeakPtr(const WeakPtr<U>& o) : m_ref(o.m_ref.copyRef()) { }
T* get() const { return m_ref->get(); }
- explicit operator bool() const { return m_ref->get(); }
+ operator bool() const { return m_ref->get(); }
+ WeakPtr& operator=(const WeakPtr& o) { m_ref = o.m_ref.copyRef(); return *this; }
+ template<typename U> WeakPtr& operator=(const WeakPtr<U>& o) { m_ref = o.m_ref.copyRef(); return *this; }
+
T* operator->() const { return m_ref->get(); }
private:
- RefPtr<WeakReference<T>> m_ref;
+ friend class WeakPtrFactory<T>;
+ WeakPtr(Ref<WeakReference<T>>&& ref) : m_ref(std::forward<Ref<WeakReference<T>>>(ref)) { }
+
+ Ref<WeakReference<T>> m_ref;
};
template<typename T>
@@ -115,16 +115,10 @@
public:
explicit WeakPtrFactory(T* ptr) : m_ref(WeakReference<T>::create(ptr)) { }
- WeakPtrFactory(PassRefPtr<WeakReference<T>> ref, T* ptr)
- : m_ref(ref)
- {
- m_ref->bindTo(ptr);
- }
-
~WeakPtrFactory() { m_ref->clear(); }
// We should consider having createWeakPtr populate m_ref the first time createWeakPtr is called.
- WeakPtr<T> createWeakPtr() { return WeakPtr<T>(m_ref); }
+ WeakPtr<T> createWeakPtr() { return WeakPtr<T>(m_ref.copyRef()); }
void revokeAll()
{
@@ -135,7 +129,7 @@
}
private:
- RefPtr<WeakReference<T>> m_ref;
+ Ref<WeakReference<T>> m_ref;
};
} // namespace WTF
Modified: trunk/Tools/ChangeLog (178614 => 178615)
--- trunk/Tools/ChangeLog 2015-01-17 00:37:08 UTC (rev 178614)
+++ trunk/Tools/ChangeLog 2015-01-17 00:39:57 UTC (rev 178615)
@@ -1,3 +1,16 @@
+2015-01-16 Myles C. Maxfield <mmaxfi...@apple.com>
+
+ WeakPtr functions crash when created with default constructor
+ https://bugs.webkit.org/show_bug.cgi?id=140479
+
+ Reviewed by Andreas Kling.
+
+ Add WeakPtr API tests.
+
+ * TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
+ * TestWebKitAPI/Tests/WTF/WeakPtr.cpp: Added.
+ * TestWebKitAPI/Tests/WTF/Ref.cpp: Update to not use copy constructor.
+
2015-01-16 Chris Dumez <cdu...@apple.com>
Regression(r178586): Caused webkitpy.w3c.test_converter_unittest.W3CTestConverterTest.test_convert_prefixed_properties to fail
Modified: trunk/Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj (178614 => 178615)
--- trunk/Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj 2015-01-17 00:37:08 UTC (rev 178614)
+++ trunk/Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj 2015-01-17 00:39:57 UTC (rev 178615)
@@ -17,6 +17,7 @@
1A9E52C913E65EF4006917F5 /* 18-characters.html in Copy Resources */ = {isa = PBXBuildFile; fileRef = C045F9461385C2F800C0F3CD /* 18-characters.html */; };
1ADBEFE3130C6AA100D61D19 /* simple-accelerated-compositing.html in Copy Resources */ = {isa = PBXBuildFile; fileRef = 1ADBEFBC130C6A0100D61D19 /* simple-accelerated-compositing.html */; };
1AEDE22613E5E7E700E62FE8 /* InjectedBundleControllerMac.mm in Sources */ = {isa = PBXBuildFile; fileRef = 1AEDE22413E5E7A000E62FE8 /* InjectedBundleControllerMac.mm */; };
+ 1CB9BC381A67482300FE5678 /* WeakPtr.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 1CB9BC371A67482300FE5678 /* WeakPtr.cpp */; };
26DF5A6315A2A27E003689C2 /* CancelLoadFromResourceLoadDelegate.html in Copy Resources */ = {isa = PBXBuildFile; fileRef = 26DF5A6115A2A22B003689C2 /* CancelLoadFromResourceLoadDelegate.html */; };
26F52EAD1828827B0023D412 /* geolocationGetCurrentPosition.html in Copy Resources */ = {isa = PBXBuildFile; fileRef = 26F52EAC1828820E0023D412 /* geolocationGetCurrentPosition.html */; };
26F52EAF18288C230023D412 /* geolocationGetCurrentPositionWithHighAccuracy.html in Copy Resources */ = {isa = PBXBuildFile; fileRef = 26F52EAE18288C040023D412 /* geolocationGetCurrentPositionWithHighAccuracy.html */; };
@@ -404,6 +405,7 @@
1AEDE22413E5E7A000E62FE8 /* InjectedBundleControllerMac.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = InjectedBundleControllerMac.mm; sourceTree = "<group>"; };
1AEF994817A09F5300998EF0 /* GetPIDAfterAbortedProcessLaunch.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = GetPIDAfterAbortedProcessLaunch.cpp; sourceTree = "<group>"; };
1AFDE6541953B2C000C48FFA /* Optional.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = Optional.cpp; sourceTree = "<group>"; };
+ 1CB9BC371A67482300FE5678 /* WeakPtr.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = WeakPtr.cpp; sourceTree = "<group>"; };
261516D515B0E60500A2C201 /* SetAndUpdateCacheModel.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = SetAndUpdateCacheModel.mm; sourceTree = "<group>"; };
26300B1716755CD90066886D /* ListHashSet.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = ListHashSet.cpp; sourceTree = "<group>"; };
265AF54F15D1E48A00B0CB4A /* WTFString.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = WTFString.cpp; sourceTree = "<group>"; };
@@ -1020,6 +1022,7 @@
0BCD85691485C98B00EA2003 /* TemporaryChange.cpp */,
BC55F5F814AD78EE00484BE1 /* Vector.cpp */,
265AF54F15D1E48A00B0CB4A /* WTFString.cpp */,
+ 1CB9BC371A67482300FE5678 /* WeakPtr.cpp */,
);
path = WTF;
sourceTree = "<group>";
@@ -1513,6 +1516,7 @@
isa = PBXSourcesBuildPhase;
buildActionMask = 2147483647;
files = (
+ 1CB9BC381A67482300FE5678 /* WeakPtr.cpp in Sources */,
2E7765CD16C4D80A00BA2BB1 /* mainIOS.mm in Sources */,
2E7765CF16C4D81100BA2BB1 /* mainMac.mm in Sources */,
);
Modified: trunk/Tools/TestWebKitAPI/Tests/WTF/Ref.cpp (178614 => 178615)
--- trunk/Tools/TestWebKitAPI/Tests/WTF/Ref.cpp 2015-01-17 00:37:08 UTC (rev 178614)
+++ trunk/Tools/TestWebKitAPI/Tests/WTF/Ref.cpp 2015-01-17 00:39:57 UTC (rev 178615)
@@ -151,7 +151,7 @@
{
Ref<DerivedRefLogger> derivedReference(a);
- Ref<RefLogger> baseReference(passWithRef(Ref<RefLogger>(derivedReference)));
+ Ref<RefLogger> baseReference(passWithRef(derivedReference.copyRef()));
ASSERT_EQ(&a, derivedReference.ptr());
ASSERT_EQ(&a, baseReference.ptr());
}
Added: trunk/Tools/TestWebKitAPI/Tests/WTF/WeakPtr.cpp (0 => 178615)
--- trunk/Tools/TestWebKitAPI/Tests/WTF/WeakPtr.cpp (rev 0)
+++ trunk/Tools/TestWebKitAPI/Tests/WTF/WeakPtr.cpp 2015-01-17 00:39:57 UTC (rev 178615)
@@ -0,0 +1,122 @@
+/*
+ * Copyright (C) 2015 Apple Inc. All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in the
+ * documentation and/or other materials provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS''
+ * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+ * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
+ * PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL APPLE INC. OR ITS CONTRIBUTORS
+ * BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+ * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+ * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+ * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF
+ * THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#include "config.h"
+#include "test.h"
+
+#include <wtf/WeakPtr.h>
+
+namespace TestWebKitAPI {
+
+TEST(WTF_WeakPtr, Basic)
+{
+ int dummy = 5;
+ WeakPtrFactory<int>* factory = new WeakPtrFactory<int>(&dummy);
+ WeakPtr<int> weakPtr1 = factory->createWeakPtr();
+ WeakPtr<int> weakPtr2 = factory->createWeakPtr();
+ WeakPtr<int> weakPtr3 = factory->createWeakPtr();
+ EXPECT_EQ(weakPtr1.get(), &dummy);
+ EXPECT_EQ(weakPtr2.get(), &dummy);
+ EXPECT_EQ(weakPtr3.get(), &dummy);
+ EXPECT_TRUE(weakPtr1);
+ EXPECT_TRUE(weakPtr2);
+ EXPECT_TRUE(weakPtr3);
+ delete factory;
+ EXPECT_NULL(weakPtr1.get());
+ EXPECT_NULL(weakPtr2.get());
+ EXPECT_NULL(weakPtr3.get());
+ EXPECT_FALSE(weakPtr1);
+ EXPECT_FALSE(weakPtr2);
+ EXPECT_FALSE(weakPtr3);
+}
+
+TEST(WTF_WeakPtr, Assignment)
+{
+ int dummy = 5;
+ WeakPtr<int> weakPtr;
+ {
+ WeakPtrFactory<int> factory(&dummy);
+ EXPECT_NULL(weakPtr.get());
+ weakPtr = factory.createWeakPtr();
+ EXPECT_EQ(weakPtr.get(), &dummy);
+ }
+ EXPECT_NULL(weakPtr.get());
+}
+
+TEST(WTF_WeakPtr, MultipleFactories)
+{
+ int dummy1 = 5;
+ int dummy2 = 7;
+ WeakPtrFactory<int>* factory1 = new WeakPtrFactory<int>(&dummy1);
+ WeakPtrFactory<int>* factory2 = new WeakPtrFactory<int>(&dummy2);
+ WeakPtr<int> weakPtr1 = factory1->createWeakPtr();
+ WeakPtr<int> weakPtr2 = factory2->createWeakPtr();
+ EXPECT_EQ(weakPtr1.get(), &dummy1);
+ EXPECT_EQ(weakPtr2.get(), &dummy2);
+ delete factory1;
+ EXPECT_NULL(weakPtr1.get());
+ EXPECT_EQ(weakPtr2.get(), &dummy2);
+ delete factory2;
+ EXPECT_NULL(weakPtr2.get());
+}
+
+TEST(WTF_WeakPtr, RevokeAll)
+{
+ int dummy = 5;
+ WeakPtrFactory<int> factory(&dummy);
+ WeakPtr<int> weakPtr1 = factory.createWeakPtr();
+ WeakPtr<int> weakPtr2 = factory.createWeakPtr();
+ WeakPtr<int> weakPtr3 = factory.createWeakPtr();
+ EXPECT_EQ(weakPtr1.get(), &dummy);
+ EXPECT_EQ(weakPtr2.get(), &dummy);
+ EXPECT_EQ(weakPtr3.get(), &dummy);
+ factory.revokeAll();
+ EXPECT_NULL(weakPtr1.get());
+ EXPECT_NULL(weakPtr2.get());
+ EXPECT_NULL(weakPtr3.get());
+}
+
+TEST(WTF_WeakPtr, NullFactory)
+{
+ WeakPtrFactory<int> factory(nullptr);
+ WeakPtr<int> weakPtr = factory.createWeakPtr();
+ EXPECT_NULL(weakPtr.get());
+ factory.revokeAll();
+ EXPECT_NULL(weakPtr.get());
+}
+
+struct Foo {
+ void bar() { };
+};
+
+TEST(WTF_WeakPtr, Dereference)
+{
+ Foo f;
+ WeakPtrFactory<Foo> factory(&f);
+ WeakPtr<Foo> weakPtr = factory.createWeakPtr();
+ weakPtr->bar();
+}
+
+} // namespace TestWebKitAPI
