WeakPtr functions crash when created with default constructor
authormmaxfield@apple.com <mmaxfield@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 17 Jan 2015 00:39:57 +0000 (00:39 +0000)
committermmaxfield@apple.com <mmaxfield@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 17 Jan 2015 00:39:57 +0000 (00:39 +0000)
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.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@178615 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WTF/ChangeLog
Source/WTF/wtf/Ref.h
Source/WTF/wtf/WeakPtr.h
Tools/ChangeLog
Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj
Tools/TestWebKitAPI/Tests/WTF/Ref.cpp
Tools/TestWebKitAPI/Tests/WTF/WeakPtr.cpp [new file with mode: 0644]

index cf27d6651e66ffb73d75fbeb654ffac62232a07d..9eade76c3e8deeebf124eaddcd3e340cac4951c7 100644 (file)
@@ -1,3 +1,47 @@
+2015-01-16  Myles C. Maxfield  <mmaxfield@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.tea@gmail.com>
 
         std::all_of requires complete C++ iterators in GCC 4.8
index 79f69603b505033ae1f30d62b87b6836fab7fb9e..28d64a40833367de0390b04105fd170cf0419cb1 100644 (file)
@@ -52,15 +52,21 @@ public:
         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 @@ public:
         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 @@ public:
         ASSERT(m_ptr);
         m_ptr->deref();
         m_ptr = &reference.leakRef();
+        ASSERT(m_ptr);
         return *this;
     }
 
index 4fd6f71fef9f79f386700c73a160eb12953fc1e8..45ec180ca46375ec4667eddaaf317917cdb094c8 100644 (file)
@@ -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>
 
 
 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,20 +62,14 @@ public:
 #else
         ASSERT(m_boundThread == currentThread());
 #endif
-        m_ptr = 0;
-    }
-
-    void bindTo(T* ptr)
-    {
-        ASSERT(!m_ptr);
-#ifndef NDEBUG
-        m_boundThread = currentThread();
-#endif
-        m_ptr = ptr;
+        m_ptr = nullptr;
     }
 
 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)
@@ -96,16 +89,23 @@ template<typename T>
 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 @@ class WeakPtrFactory {
 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 @@ public:
     }
 
 private:
-    RefPtr<WeakReference<T>> m_ref;
+    Ref<WeakReference<T>> m_ref;
 };
 
 } // namespace WTF
index 72adce411bdb89d865d4a3bdf3008afa8c5edb23..d9dd804d044f67f1ac6c506971f8040a1886caf3 100644 (file)
@@ -1,3 +1,16 @@
+2015-01-16  Myles C. Maxfield  <mmaxfield@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  <cdumez@apple.com>
 
         Regression(r178586): Caused webkitpy.w3c.test_converter_unittest.W3CTestConverterTest.test_convert_prefixed_properties to fail
index 53a3ee815e05b818c73944680df1aab83d744171..700122a62fea8d87c06c6d542039e3ee78220243 100644 (file)
@@ -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 */; };
                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>"; };
                                0BCD85691485C98B00EA2003 /* TemporaryChange.cpp */,
                                BC55F5F814AD78EE00484BE1 /* Vector.cpp */,
                                265AF54F15D1E48A00B0CB4A /* WTFString.cpp */,
+                               1CB9BC371A67482300FE5678 /* WeakPtr.cpp */,
                        );
                        path = WTF;
                        sourceTree = "<group>";
                        isa = PBXSourcesBuildPhase;
                        buildActionMask = 2147483647;
                        files = (
+                               1CB9BC381A67482300FE5678 /* WeakPtr.cpp in Sources */,
                                2E7765CD16C4D80A00BA2BB1 /* mainIOS.mm in Sources */,
                                2E7765CF16C4D81100BA2BB1 /* mainMac.mm in Sources */,
                        );
index 0227b5b0a7b3cc189075eb222214533560dcdfab..8c622d07b76d0cfee040aaf97496ad19e5606e33 100644 (file)
@@ -151,7 +151,7 @@ TEST(WTF_Ref, ReturnValue)
 
     {
         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());
     }
diff --git a/Tools/TestWebKitAPI/Tests/WTF/WeakPtr.cpp b/Tools/TestWebKitAPI/Tests/WTF/WeakPtr.cpp
new file mode 100644 (file)
index 0000000..82ca456
--- /dev/null
@@ -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