Changeset 250762 in webkit

Timestamp:

Oct 4, 2019 6:59:35 PM (5 years ago)

Author:

rniwa@webkit.org

Message:

Make a ThreadSafeRefCounted object safe to ref & deref inside its destructor
​https://bugs.webkit.org/show_bug.cgi?id=201576

Reviewed by Geoffrey Garen.

This patch leaves m_refCount 1 inside the last deref call to ThreadSafeRefCounted
such that ref'ing and deref'ing it again inside its destructor would never try
to double delete the object.

Also added m_deletionHasBegun like RefCounted.

• wtf/ThreadSafeRefCounted.h:

(WTF::ThreadSafeRefCountedBase::ref const):
(WTF::ThreadSafeRefCountedBase::hasOneRef const):
(WTF::ThreadSafeRefCountedBase::derefBase const):

Location:

trunk/Source/WTF

Files:

• ChangeLog (modified) (1 diff)
• wtf/ThreadSafeRefCounted.h (modified) (4 diffs)

trunk/Source/WTF/ChangeLog

@@ +1 @@
+2019-10-04  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Make a ThreadSafeRefCounted object safe to ref & deref inside its destructor
+        https://bugs.webkit.org/show_bug.cgi?id=201576
+
+        Reviewed by Geoffrey Garen.
+
+        This patch leaves m_refCount 1 inside the last deref call to ThreadSafeRefCounted
+        such that ref'ing and deref'ing it again inside its destructor would never try
+        to double delete the object.
+
+        Also added m_deletionHasBegun like RefCounted.
+
+        * wtf/ThreadSafeRefCounted.h:
+        (WTF::ThreadSafeRefCountedBase::ref const):
+        (WTF::ThreadSafeRefCountedBase::hasOneRef const):
+        (WTF::ThreadSafeRefCountedBase::derefBase const):
+
 2019-10-04  Heiko Becker  <heirecka@exherbo.org>
 

trunk/Source/WTF/wtf/ThreadSafeRefCounted.h

@@ -33 +33 @@
 namespace WTF {
 
+#if defined(NDEBUG) && !ENABLE(SECURITY_ASSERTIONS)
+#define CHECK_THREAD_SAFE_REF_COUNTED_LIFECYCLE 0
+#else
+#define CHECK_THREAD_SAFE_REF_COUNTED_LIFECYCLE 1
+#endif
+
 class ThreadSafeRefCountedBase {
     WTF_MAKE_NONCOPYABLE(ThreadSafeRefCountedBase);
@@ -39 +45 @@
     ThreadSafeRefCountedBase() = default;
 
+#if CHECK_THREAD_SAFE_REF_COUNTED_LIFECYCLE
+    ~ThreadSafeRefCountedBase()
+    {
+        // When this ThreadSafeRefCounted object is a part of another object, derefBase() is never called on this object.
+        m_deletionHasBegun = true;
+    }
+#endif
+
     void ref() const
     {
+#if CHECK_THREAD_SAFE_REF_COUNTED_LIFECYCLE
+        ASSERT_WITH_SECURITY_IMPLICATION(!m_deletionHasBegun);
+#endif
         ++m_refCount;
     }
@@ -46 +63 @@
     bool hasOneRef() const
     {
+#if CHECK_THREAD_SAFE_REF_COUNTED_LIFECYCLE
+        ASSERT(!m_deletionHasBegun);
+#endif
         return refCount() == 1;
     }
@@ -58 +78 @@
     bool derefBase() const
     {
-        return !--m_refCount;
+        ASSERT(m_refCount);
+
+#if CHECK_THREAD_SAFE_REF_COUNTED_LIFECYCLE
+        ASSERT_WITH_SECURITY_IMPLICATION(!m_deletionHasBegun);
+#endif
+
+        if (UNLIKELY(!--m_refCount)) {
+            // Setting m_refCount to 1 here prevents double delete within the destructor but not from another thread
+            // since such a thread could have ref'ed this object long after it had been deleted. See webkit.org/b/201576.
+            m_refCount = 1;
+#if CHECK_THREAD_SAFE_REF_COUNTED_LIFECYCLE
+            m_deletionHasBegun = true;
+#endif
+            return true;
+        }
+
+        return false;
     }
 
 private:
     mutable std::atomic<unsigned> m_refCount { 1 };
+
+#if CHECK_THREAD_SAFE_REF_COUNTED_LIFECYCLE
+    mutable std::atomic<bool> m_deletionHasBegun { false };
+#endif
 };