Attachment 348019 Details for Bug 188921 – Patch

[patch] Patch

bug-188921-20180824101438.patch (text/plain), 8.38 KB, created by Jer Noble on 2018-08-24 10:14:38 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Jer Noble

Created: 2018-08-24 10:14:38 PDT

Size: 8.38 KB

patch

obsolete

>Subversion Revision: 235089
>diff --git a/Source/WTF/ChangeLog b/Source/WTF/ChangeLog
>index da17134ad663a74a4ec10943ca11dc06609e0a33..9a9dfdfa6cc3a6951423f48a23b02220d6ae8010 100644
>--- a/Source/WTF/ChangeLog
>+++ b/Source/WTF/ChangeLog
>@@ -1,3 +1,54 @@
>+2018-08-24  Jer Noble  <jer.noble@apple.com>
>+
>+        Allow classes to safely return references to Ref<> ivars.
>+        https://bugs.webkit.org/show_bug.cgi?id=188921
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        When vending access to an internal ivar stored as a Ref<>, a developer is left with two options:
>+
>+        Option 1) return a pointer reference to the ivar, i.e. "Type& getValue() const { return m_value; }"
>+
>+        This allows callers to use the returned value as a temporary from the call site, i.e.
>+        "return document().settings()". But when the returned value is stored as a reference, callers
>+        can potentially access deleted memory. I.e.:
>+
>+            RefPtr<Object> object = Object::create();
>+            Type& value = object->getValue();
>+            object = nullptr;
>+            log(value.toString());
>+
>+        Option 2) return a new Ref<>, i.e. "Ref<Type> getValue() const { return ref.copyRef(); }"
>+
>+        This guarantees that callers must reference-count the returned value, but churns the ref-count
>+        for simple, temporary-access use cases. I.e.: "return document()->settings();" will call ref()
>+        and deref() on Document.
>+
>+        To allow classes to vend references to internal objects without churning the ref-count, add
>+        a new class TempRef<T>. TempRefs are not moveable, assignable, or constructable, except by
>+        Refs, and then only through an explicit call.  They do not give access to their underlying
>+        pointer reference except via the arrow operator. This guarantees that neither TempRefs nor their
>+        pointers can be stored as a non-temporary variable. I.e., given:
>+
>+            TempRef<Type> getValue() const { return ref.tempRef(); }
>+
>+        That the following is legal:
>+
>+            RefPtr<Object> object = Object::create();
>+            log(object.getValue()->toString);
>+
>+        But that the following is illegal:
>+
>+            RefPtr<Object> object = Object::create();
>+            auto& value = object.getValue();
>+
>+        * wtf/Ref.h:
>+        (WTF::Ref::Ref):
>+        (WTF::Ref::tempRef const):
>+        (WTF::TempRef::operator-> const):
>+        (WTF::TempRef::TempRef):
>+        (WTF::TempRef::get const):
>+
> 2018-08-22  Jer Noble  <jer.noble@apple.com>
> 
>         Refactoring: eliminate raw pointer usage in Fullscreen code
>diff --git a/Source/WTF/wtf/Ref.h b/Source/WTF/wtf/Ref.h
>index 3b3158dfb6efd4060728f4a7e015e3fa77fa5fe7..e6872269532a7e35197c9a32c1ca496b2382da66 100644
>--- a/Source/WTF/wtf/Ref.h
>+++ b/Source/WTF/wtf/Ref.h
>@@ -45,6 +45,7 @@ inline void adopted(const void*) { }
> 
> template<typename T, typename PtrTraits> class Ref;
> template<typename T, typename PtrTraits = DumbPtrTraits<T>> Ref<T, PtrTraits> adoptRef(T&);
>+template<typename T, typename PtrTraits> class TempRef;
> 
> template<typename T, typename PtrTraits>
> class Ref {
>@@ -84,6 +85,12 @@ public:
>         ASSERT(m_ptr);
>     }
> 
>+    Ref(TempRef<T, PtrTraits>&& other)
>+        : m_ptr(&other.get())
>+    {
>+        other.get().ref();
>+    }
>+
>     Ref& operator=(T&);
>     Ref& operator=(Ref&&);
>     template<typename X, typename Y> Ref& operator=(Ref<X, Y>&&);
>@@ -128,6 +135,8 @@ public:
>     Ref copyRef() && = delete;
>     Ref copyRef() const & WARN_UNUSED_RETURN { return Ref(*m_ptr); }
> 
>+    TempRef<T, PtrTraits> tempRef() const { return TempRef<T, PtrTraits>(*this); }
>+
>     T& leakRef() WARN_UNUSED_RETURN
>     {
>         ASSERT(m_ptr);
>@@ -152,6 +161,27 @@ private:
>     typename PtrTraits::StorageType m_ptr;
> };
> 
>+template<typename T, typename PtrTraits>
>+class TempRef {
>+public:
>+    TempRef(const TempRef&) = delete;
>+    TempRef(TempRef&&) = delete;
>+
>+    T* operator->() const { ASSERT(m_ptr); return PtrTraits::unwrap(m_ptr); }
>+
>+private:
>+    template<typename X, typename Y> friend class Ref;
>+
>+    TempRef(const Ref<T, PtrTraits>& other)
>+        : m_ptr(other.ptr())
>+    {
>+    }
>+
>+    T& get() const { ASSERT(m_ptr); return *PtrTraits::unwrap(m_ptr); }
>+
>+    typename PtrTraits::StorageType m_ptr;
>+};
>+
> template<typename T, typename U> Ref<T, U> adoptRef(T&);
> template<typename T> Ref<T> makeRef(T&);
> 
>@@ -277,6 +307,7 @@ using PoisonedRef = Ref<T, PoisonedPtrTraits<Poison, T>>;
> 
> using WTF::PoisonedRef;
> using WTF::Ref;
>+using WTF::TempRef;
> using WTF::adoptRef;
> using WTF::makeRef;
> using WTF::static_reference_cast;
>diff --git a/Tools/ChangeLog b/Tools/ChangeLog
>index cb2ec2b264af8b241d9fd523f875c1e49842c4c8..018c977801a18d8745aa858b1b932d816a186b88 100644
>--- a/Tools/ChangeLog
>+++ b/Tools/ChangeLog
>@@ -1,3 +1,17 @@
>+2018-08-24  Jer Noble  <jer.noble@apple.com>
>+
>+        Allow classes to safely return references to Ref<> ivars.
>+        https://bugs.webkit.org/show_bug.cgi?id=188921
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * TestWebKitAPI/Tests/WTF/Ref.cpp:
>+        (TestWebKitAPI::CallableRefCheckingRefLogger::CallableRefCheckingRefLogger):
>+        (TestWebKitAPI::CallableRefCheckingRefLogger::call const):
>+        (TestWebKitAPI::CallableRefCheckingRefLogger::call):
>+        (TestWebKitAPI::ContainsRefButReturnsTempRef::getTemp):
>+        (TestWebKitAPI::TEST):
>+
> 2018-08-20  Thomas Denney  <tdenney@apple.com>
> 
>         Added Thomas Denney to contributors.json.
>diff --git a/Tools/TestWebKitAPI/Tests/WTF/Ref.cpp b/Tools/TestWebKitAPI/Tests/WTF/Ref.cpp
>index a5c7bd1541603bcdbc29bb7a7d21a04c1643533d..8c208eb0a8cc53b6ea57e1c6213d22687b654859 100644
>--- a/Tools/TestWebKitAPI/Tests/WTF/Ref.cpp
>+++ b/Tools/TestWebKitAPI/Tests/WTF/Ref.cpp
>@@ -272,4 +272,62 @@ TEST(WTF_Ref, AssignBeforeDeref)
>     EXPECT_STREQ("ref(a) | slot=c deref(a) | deref(c) ", takeLogStr().c_str());
> }
> 
>+struct CallableRefCheckingRefLogger : RefCheckingRefLogger {
>+    CallableRefCheckingRefLogger(const char* name)
>+        : RefCheckingRefLogger(name)
>+    {
>+    }
>+
>+    void call() const { log() << "call() const "; }
>+    void call() { log() << "call() "; }
>+};
>+
>+template <typename T>
>+struct ContainsRefButReturnsTempRef {
>+    Ref<T> ref;
>+    TempRef<T> getTemp() { return ref.tempRef(); }
>+};
>+
>+TEST(WTF_Ref, TempRef)
>+{
>+    static_assert(!std::is_default_constructible<TempRef<RefLogger>>::value, "TempRef should not be default constructable");
>+    static_assert(!std::is_copy_assignable<TempRef<RefLogger>>::value, "TempRef should not be copy assignable");
>+    static_assert(!std::is_copy_constructible<TempRef<RefLogger>>::value, "TempRef should not be copy constructable");
>+    static_assert(!std::is_move_assignable<TempRef<RefLogger>>::value, "TempRef should not be move assignable");
>+    static_assert(!std::is_move_constructible<TempRef<RefLogger>>::value, "TempRef should not be move constructable");
>+    static_assert(!std::is_assignable<TempRef<RefLogger>, TempRef<RefLogger>>::value, "TempRef should not be self assignable");
>+    static_assert(!std::is_assignable<TempRef<RefLogger>, RefLogger&>::value, "TempRef should not be pointer assignable");
>+    static_assert(!std::is_assignable<TempRef<RefLogger>, RefLogger*>::value, "TempRef should not be pointer assignable");
>+    static_assert(!std::is_convertible<TempRef<RefLogger>, TempRef<RefLogger>>::value, "Ref should not be convertable to TempRef");
>+
>+    static_assert(std::is_assignable<Ref<RefLogger>, TempRef<RefLogger>>::value, "TempRef should be assignable to Ref");
>+
>+    CallableRefCheckingRefLogger a("a");
>+    {
>+        Ref<CallableRefCheckingRefLogger> ref(a);
>+        EXPECT_EQ(&a, ref.ptr());
>+        log() << "| ";
>+        ref.tempRef()->call();
>+        log() << "| ";
>+    }
>+    EXPECT_STREQ("ref(a) | call() | deref(a) ", takeLogStr().c_str());
>+    {
>+        ContainsRefButReturnsTempRef<CallableRefCheckingRefLogger> object { a };
>+        EXPECT_EQ(&a, object.ref.ptr());
>+        log() << "| ";
>+        object.getTemp()->call();
>+        log() << "| ";
>+    }
>+    EXPECT_STREQ("ref(a) | call() | deref(a) ", takeLogStr().c_str());
>+    {
>+        ContainsRefButReturnsTempRef<CallableRefCheckingRefLogger> object { a };
>+        EXPECT_EQ(&a, object.ref.ptr());
>+        log() << "| ";
>+        Ref<CallableRefCheckingRefLogger> secondRef = object.getTemp();
>+        secondRef->call();
>+        log() << "| ";
>+    }
>+    EXPECT_STREQ("ref(a) | ref(a) call() | deref(a) deref(a) ", takeLogStr().c_str());
>+}
>+
> } // namespace TestWebKitAPI

Actions: View | Formatted Diff | Diff

Attachments on bug 188921: 348019