ThreadSafeWeakHashSet::contains should return false if object has beg…

…un destruction

https://bugs.webkit.org/show_bug.cgi?id=259966
rdar://113608976

Reviewed by Chris Dumez.

In #16413 we found that it returns true, but only if the amortized
cleanup hasn't removed its null ThreadSafeWeakPtr from the map yet.  This is undesirable because the
amortized cleanup shouldn't affect behavior.  To fix this, just make it return false in this case.

* Source/WTF/wtf/ThreadSafeWeakHashSet.h:
* Tools/TestWebKitAPI/Tests/WTF/WeakPtr.cpp:
(TestWebKitAPI::ObjectAddingAndRemovingItself::~ObjectAddingAndRemovingItself):

Canonical link: https://commits.webkit.org/266764@main

Source/WTF/wtf/ThreadSafeWeakHashSet.h

@@ -108,7 +108,12 @@ class ThreadSafeWeakHashSet final {
     {
         Locker locker { m_lock };
         amortizedCleanupIfNeeded();
-        return m_map.remove(static_cast<const T*>(&value));
+        auto it = m_map.find(static_cast<const T*>(&value));
+        if (it == m_map.end())
+            return false;
+        bool wasDeleted = it->value && it->value->objectHasStartedDeletion();
+        bool result = m_map.remove(it);
+        return !wasDeleted && result;
     }
 
     void clear()
@@ -123,7 +128,10 @@ class ThreadSafeWeakHashSet final {
     {
         Locker locker { m_lock };
         amortizedCleanupIfNeeded();
-        return m_map.contains(static_cast<const T*>(&value));
+        auto it = m_map.find(static_cast<const T*>(&value));
+        if (it == m_map.end())
+            return false;
+        return it->value && !it->value->objectHasStartedDeletion();
     }
 
     bool isEmptyIgnoringNullReferences() const

Tools/TestWebKitAPI/Tests/WTF/WeakPtr.cpp

@@ -2897,15 +2897,16 @@ class ObjectAddingAndRemovingItself : public ThreadSafeRefCountedAndCanMakeThrea
 
     ~ObjectAddingAndRemovingItself()
     {
+        EXPECT_FALSE(m_set.contains(*this));
+        auto sizeBefore = m_set.sizeIncludingEmptyEntriesForTesting();
+        EXPECT_FALSE(m_set.remove(*this));
+        auto sizeAfter = m_set.sizeIncludingEmptyEntriesForTesting();
         static size_t i { 0 };
-        i++;
-        if (i == 8) {
-            // amortized cleanup of the set in contains makes this return false sometimes,
-            // but only in the destructor, where contains is less meaningful.
-            EXPECT_FALSE(m_set.contains(*this));
+        if (++i == 8) {
+            // Amortized cleanup gets this one during the contains call.
+            EXPECT_EQ(sizeBefore, sizeAfter);
         } else
-            EXPECT_TRUE(m_set.contains(*this));
-        m_set.remove(*this);
+            EXPECT_EQ(sizeBefore, sizeAfter + 1);
         EXPECT_FALSE(m_set.contains(*this));
     }