Added a compile time assertion that forbids classes that offer WeakPtr

with no corresponding RefPtr or CheckedPtr
https://bugs.webkit.org/show_bug.cgi?id=273569
rdar://127296969

Reviewed by Ryosuke Niwa.

An object that offers WeakPtr with no corresponding RefPtr or CheckedPtr
is a dangerous contradiction. On the one hand, we know that it can be
deleted at any time. On the other hand, when we go to use it, we do
nothing to ensure its lifetime. This has been a source of use after
free / security bugs.

This patch adds a compile time assertion against future uses of this
anti-pattern. An explicit template specialization allow list maintains
existing uses. The allow list is a todo list for future deployment of
RefPtr or CheckedPtr.

This patch does not enforce correct usage of RefPtr or CheckedPtr; it
only enforces a rule that a class must at least offer RefPtr or
CheckedPtr if it offers WeakPtr.

In order to facilitate the allow list, I needed to
    * Move some nested classes out-of-line, since C++ does not support
    forward declaration or template specialization of nested classes

    * Move some destructors out-of-line, since our compile time
    assertion requires a complete type definition, and I didn't want to
    increase #includes in headers

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

Source/JavaScriptCore/inspector/InspectorTarget.h

@@ -29,6 +29,15 @@
 #include <wtf/WeakPtr.h>
 #include <wtf/text/WTFString.h>
 
+namespace Inspector {
+class InspectorTarget;
+}
+
+namespace WTF {
+template<typename T> struct IsDeprecatedWeakRefSmartPointerException;
+template<> struct IsDeprecatedWeakRefSmartPointerException<Inspector::InspectorTarget> : std::true_type { };
+}
+
 namespace Inspector {
 
 // FIXME: Add DedicatedWorker Inspector Targets

Source/JavaScriptCore/runtime/ConsoleClient.h

@@ -29,6 +29,15 @@
 #include <wtf/Forward.h>
 #include <wtf/WeakPtr.h>
 
+namespace JSC {
+class ConsoleClient;
+}
+
+namespace WTF {
+template<typename T> struct IsDeprecatedWeakRefSmartPointerException;
+template<> struct IsDeprecatedWeakRefSmartPointerException<JSC::ConsoleClient> : std::true_type { };
+}
+
 namespace Inspector {
 class ScriptArguments;
 }

Source/JavaScriptCore/runtime/JSGlobalObject.cpp

@@ -3158,6 +3158,11 @@ void JSGlobalObject::setConsoleClient(WeakPtr<ConsoleClient>&& consoleClient)
     m_consoleClient = WTFMove(consoleClient);
 }
 
+WeakPtr<ConsoleClient> JSGlobalObject::consoleClient() const
+{
+    return m_consoleClient;
+}
+
 void JSGlobalObject::setDebugger(Debugger* debugger)
 {
     m_debugger = debugger;

Source/JavaScriptCore/runtime/JSGlobalObject.h

@@ -894,7 +894,7 @@ class JSGlobalObject : public JSSegmentedVariableObject {
     unsigned* addressOfGlobalLexicalBindingEpoch() { return &m_globalLexicalBindingEpoch; }
 
     JS_EXPORT_PRIVATE void setConsoleClient(WeakPtr<ConsoleClient>&&);
-    WeakPtr<ConsoleClient> consoleClient() const { return m_consoleClient; }
+    JS_EXPORT_PRIVATE WeakPtr<ConsoleClient> consoleClient() const;
 
     void setName(const String&);
     const String& name() const { return m_name; }

Source/WTF/wtf/CancellableTask.h

@@ -29,37 +29,55 @@
 #include <wtf/UniqueRef.h>
 #include <wtf/WeakPtr.h>
 
+namespace WTF {
+class TaskCancellationGroup;
+class TaskCancellationGroupImpl;
+}
+
+namespace WTF {
+template<typename T> struct IsDeprecatedWeakRefSmartPointerException;
+template<> struct IsDeprecatedWeakRefSmartPointerException<WTF::TaskCancellationGroup> : std::true_type { };
+template<> struct IsDeprecatedWeakRefSmartPointerException<WTF::TaskCancellationGroupImpl> : std::true_type { };
+}
+
 namespace WTF {
 
 class CancellableTask;
 
+class TaskCancellationGroupImpl : public CanMakeWeakPtr<TaskCancellationGroupImpl> {
+    WTF_MAKE_FAST_ALLOCATED;
+public:
+    void cancel() { weakPtrFactory().revokeAll(); }
+    bool hasPendingTask() const { return weakPtrFactory().weakPtrCount(); }
+};
+
+class TaskCancellationGroupHandle {
+public:
+    bool isCancelled() const { return !m_impl; }
+    void clear() { m_impl = nullptr; }
+private:
+    friend class TaskCancellationGroup;
+    explicit TaskCancellationGroupHandle(TaskCancellationGroupImpl& impl)
+        : m_impl(impl)
+    {
+    }
+    WeakPtr<TaskCancellationGroupImpl> m_impl;
+};
+
 class TaskCancellationGroup : public CanMakeWeakPtr<TaskCancellationGroup> {
 public:
-    TaskCancellationGroup() : m_impl(makeUniqueRef<Impl>()) { }
+    TaskCancellationGroup()
+        : m_impl(makeUniqueRef<TaskCancellationGroupImpl>())
+    {
+    }
     void cancel() { m_impl->cancel(); }
     bool hasPendingTask() const { return m_impl->hasPendingTask(); }
 
 private:
     friend class CancellableTask;
-    class Impl : public CanMakeWeakPtr<Impl> {
-        WTF_MAKE_FAST_ALLOCATED;
-    public:
-        void cancel() { weakPtrFactory().revokeAll(); }
-        bool hasPendingTask() const { return weakPtrFactory().weakPtrCount(); }
-    };
-
-    class Handle {
-    public:
-        bool isCancelled() const { return !m_impl; }
-        void clear() { m_impl = nullptr; }
-    private:
-        friend class TaskCancellationGroup;
-        explicit Handle(Impl& impl) : m_impl(impl) { }
-        WeakPtr<Impl> m_impl;
-    };
-    Handle createHandle() { return Handle { m_impl }; }
-
-    UniqueRef<Impl> m_impl;
+    TaskCancellationGroupHandle createHandle() { return TaskCancellationGroupHandle { m_impl }; }
+
+    UniqueRef<TaskCancellationGroupImpl> m_impl;
 };
 
 class CancellableTask {
@@ -68,7 +86,7 @@ class CancellableTask {
     void operator()();
 
 private:
-    TaskCancellationGroup::Handle m_cancellationGroup;
+    TaskCancellationGroupHandle m_cancellationGroup;
     Function<void()> m_task;
 };
 

Source/WTF/wtf/ListHashSet.h

@@ -27,6 +27,19 @@
 #include <wtf/WeakPtr.h>
 #endif
 
+namespace WTF {
+template<typename Value, typename HashFunctions> class ListHashSet;
+template<typename ValueArg> struct ListHashSetNode;
+template<typename ValueArg, typename HashArg> class ListHashSetConstIterator;
+}
+
+namespace WTF {
+template<typename T> struct IsDeprecatedWeakRefSmartPointerException;
+template<typename Value, typename HashFunctions> struct IsDeprecatedWeakRefSmartPointerException<WTF::ListHashSet<Value, HashFunctions>> : std::true_type { };
+template<typename ValueArg> struct IsDeprecatedWeakRefSmartPointerException<WTF::ListHashSetNode<ValueArg>> : std::true_type { };
+template<typename ValueArg, typename HashArg> struct IsDeprecatedWeakRefSmartPointerException<WTF::ListHashSetConstIterator<ValueArg, HashArg>> : std::true_type { };
+}
+
 namespace WTF {
 
 // ListHashSet: Just like HashSet, this class provides a Set

Source/WTF/wtf/NativePromise.h

@@ -51,6 +51,15 @@
 #include <wtf/Vector.h>
 #include <wtf/WeakPtr.h>
 
+namespace WTF {
+class NativePromiseRequest;
+}
+
+namespace WTF {
+template<typename T> struct IsDeprecatedWeakRefSmartPointerException;
+template<> struct IsDeprecatedWeakRefSmartPointerException<WTF::NativePromiseRequest> : std::true_type { };
+}
+
 namespace WTF {
 
 /*

Source/WTF/wtf/Observer.h

@@ -29,6 +29,15 @@
 #include <wtf/Noncopyable.h>
 #include <wtf/WeakPtr.h>
 
+namespace WTF {
+template<typename> class Observer;
+}
+
+namespace WTF {
+template<typename T> struct IsDeprecatedWeakRefSmartPointerException;
+template<typename Out, typename... In> struct IsDeprecatedWeakRefSmartPointerException<WTF::Observer<Out(In...)>> : std::true_type { };
+}
+
 namespace WTF {
 
 template<typename> class Observer;

Source/WTF/wtf/TypeTraits.h

@@ -36,6 +36,14 @@
 #include <wtf/Ref.h>
 #include <wtf/RefPtr.h>
 
+// SFINAE depends on overload resolution. We indicate the overload we'd prefer
+// (if it can compile) using a higher priorty type (int), and the overload
+// to fall back to using a lower priority type (long). 0 can convert to int
+// or long, so we can trigger overload resolution using 0. C++ is awesome!
+#define SFINAE_OVERLOAD 0
+#define SFINAE_OVERLOAD_DEFAULT long
+#define SFINAE_OVERLOAD_PREFERRED int
+
 namespace WTF {
 
 namespace detail {
@@ -92,34 +100,49 @@ struct RemoveSmartPointerHelper<T, Ref<Pointee>> {
 template<typename T>
 struct RemoveSmartPointer : detail::RemoveSmartPointerHelper<T, std::remove_cv_t<T>> { };
 
-// HasRefCountMethods implementation
+// HasRefPtrMethods implementation
 namespace detail {
 
 template<typename>
 struct SFINAE1True : std::true_type { };
 
 template<class T>
-static auto HasRefCountMethodsTest(int) -> SFINAE1True<decltype(std::declval<T>().ref(), std::declval<T>().deref())>;
+static auto HasRefPtrMethodsTest(SFINAE_OVERLOAD_PREFERRED) -> SFINAE1True<decltype(&T::ref, &T::deref)>;
+template<class>
+static auto HasRefPtrMethodsTest(SFINAE_OVERLOAD_DEFAULT) -> std::false_type;
+
+} // namespace detail
+
+template<class T>
+struct HasRefPtrMethods : decltype(detail::HasRefPtrMethodsTest<T>(SFINAE_OVERLOAD)) { };
+
+// HasCheckedPtrMethods implementation
+namespace detail {
+
+template<class T>
+static auto HasCheckedPtrMethodsTest(SFINAE_OVERLOAD_PREFERRED) -> SFINAE1True<decltype(&T::incrementPtrCount, &T::decrementPtrCount)>;
 template<class>
-static auto HasRefCountMethodsTest(long) -> std::false_type;
+static auto HasCheckedPtrMethodsTest(SFINAE_OVERLOAD_DEFAULT) -> std::false_type;
 
 } // namespace detail
 
 template<class T>
-struct HasRefCountMethods : decltype(detail::HasRefCountMethodsTest<T>(0)) { };
+struct HasCheckedPtrMethods : decltype(detail::HasCheckedPtrMethodsTest<T>(SFINAE_OVERLOAD)) { };
 
 // HasIsolatedCopy()
 namespace detail {
 
+// FIXME: This test is incorrectly false for RefCounted objects because
+// substitution for std::declval<T>() fails when the constructor is private.
 template<class T>
-static auto HasIsolatedCopyTest(int) -> SFINAE1True<decltype(std::declval<T>().isolatedCopy())>;
+static auto HasIsolatedCopyTest(SFINAE_OVERLOAD_PREFERRED) -> SFINAE1True<decltype(std::declval<T>().isolatedCopy())>;
 template<class>
-static auto HasIsolatedCopyTest(long) -> std::false_type;
+static auto HasIsolatedCopyTest(SFINAE_OVERLOAD_DEFAULT) -> std::false_type;
 
 } // namespace detail
 
 template<class T>
-struct HasIsolatedCopy : decltype(detail::HasIsolatedCopyTest<T>(0)) { };
+struct HasIsolatedCopy : decltype(detail::HasIsolatedCopyTest<T>(SFINAE_OVERLOAD)) { };
 
 // LooksLikeRCSerialDispatcher implementation
 namespace detail {
@@ -128,16 +151,16 @@ template <bool b, typename>
 struct SFINAE1If : std::integral_constant<bool, b> { };
 
 template <bool b, class T>
-static auto LooksLikeRCSerialDispatcherTest(int)
+static auto LooksLikeRCSerialDispatcherTest(SFINAE_OVERLOAD_PREFERRED)
     -> SFINAE1If<b, decltype(std::declval<T>().ref(), std::declval<T>().deref())>;
 
 template <bool, typename>
-static auto LooksLikeRCSerialDispatcherTest(long) -> std::false_type;
+static auto LooksLikeRCSerialDispatcherTest(SFINAE_OVERLOAD_DEFAULT) -> std::false_type;
 
 } // namespace detail
 
 template <class T>
-struct LooksLikeRCSerialDispatcher : decltype(detail::LooksLikeRCSerialDispatcherTest<std::is_base_of_v<SerialFunctionDispatcher, T>, T>(0)) { };
+struct LooksLikeRCSerialDispatcher : decltype(detail::LooksLikeRCSerialDispatcherTest<std::is_base_of_v<SerialFunctionDispatcher, T>, T>(SFINAE_OVERLOAD)) { };
 
 class NativePromiseBase;
 class ConvertibleToNativePromise;

Source/WTF/wtf/WeakPtr.h

@@ -83,6 +83,13 @@ template<typename T, typename WeakPtrImpl, typename PtrTraits> class WeakPtr {
     {
     }
 
+    ~WeakPtr()
+    {
+        static_assert(
+            HasRefPtrMethods<T>::value || HasCheckedPtrMethods<T>::value || IsDeprecatedWeakRefSmartPointerException<std::remove_const_t<T>>::value,
+            "Classes that offer weak pointers should also offer RefPtr or CheckedPtr.");
+    }
+
     RefPtr<WeakPtrImpl, PtrTraits> releaseImpl() { return WTFMove(m_impl); }
 
     T* get() const

Source/WTF/wtf/WeakRef.h

@@ -31,9 +31,12 @@
 #include <wtf/ThreadSafeRefCounted.h>
 #include <wtf/Threading.h>
 #include <wtf/TypeCasts.h>
+#include <wtf/TypeTraits.h>
 #include <wtf/WeakPtrImpl.h>
 
 namespace WTF {
+// Classes that offer weak pointers should also offer RefPtr or CheckedPtr. Please do not add new exceptions.
+template<typename T> struct IsDeprecatedWeakRefSmartPointerException : std::false_type { };
 
 enum class EnableWeakPtrThreadingAssertions : bool { No, Yes };
 
@@ -64,6 +67,13 @@ class WeakRef {
     WeakRef(HashTableDeletedValueType) : m_impl(HashTableDeletedValue) { }
     WeakRef(HashTableEmptyValueType) : m_impl(HashTableEmptyValue) { }
 
+    ~WeakRef()
+    {
+        static_assert(
+            HasRefPtrMethods<T>::value || HasCheckedPtrMethods<T>::value || IsDeprecatedWeakRefSmartPointerException<std::remove_const_t<T>>::value,
+            "Classes that offer weak pointers should also offer RefPtr or CheckedPtr. Please do not add new exceptions.");
+    }
+
     bool isHashTableDeletedValue() const { return m_impl.isHashTableDeletedValue(); }
     bool isHashTableEmptyValue() const { return m_impl.isHashTableEmptyValue(); }
 

Source/WebCore/Modules/WebGPU/InternalAPI/WebGPUBufferBinding.h

@@ -25,15 +25,14 @@
 
 #pragma once
 
+#include "WebGPUBuffer.h"
 #include "WebGPUIntegralTypes.h"
 #include <optional>
 #include <wtf/Ref.h>
 #include <wtf/WeakRef.h>
 
 namespace WebCore::WebGPU {
 
-class Buffer;
-
 struct BufferBinding {
     WeakRef<Buffer> buffer;
     Size64 offset { 0 };

Source/WebCore/Modules/WebGPU/InternalAPI/WebGPUCanvasConfiguration.h

@@ -26,6 +26,7 @@
 #pragma once
 
 #include "WebGPUCanvasAlphaMode.h"
+#include "WebGPUDevice.h"
 #include "WebGPUPredefinedColorSpace.h"
 #include "WebGPUTextureFormat.h"
 #include "WebGPUTextureUsage.h"
@@ -34,8 +35,6 @@
 
 namespace WebCore::WebGPU {
 
-class Device;
-
 struct CanvasConfiguration {
     WeakRef<Device> device;
     TextureFormat format { TextureFormat::R8unorm };

Source/WebCore/Modules/WebGPU/InternalAPI/WebGPUImageCopyTexture.h

@@ -27,6 +27,7 @@
 
 #include "WebGPUIntegralTypes.h"
 #include "WebGPUOrigin3D.h"
+#include "WebGPUTexture.h"
 #include "WebGPUTextureAspect.h"
 #include <optional>
 #include <wtf/Ref.h>

Source/WebCore/Modules/WebGPU/InternalAPI/WebGPUPipelineDescriptorBase.h

@@ -26,12 +26,11 @@
 #pragma once
 
 #include "WebGPUObjectDescriptorBase.h"
+#include "WebGPUPipelineLayout.h"
 #include <wtf/WeakPtr.h>
 
 namespace WebCore::WebGPU {
 
-class PipelineLayout;
-
 struct PipelineDescriptorBase : public ObjectDescriptorBase {
     WeakPtr<PipelineLayout> layout;
 };

Source/WebCore/Modules/WebGPU/InternalAPI/WebGPUProgrammableStage.h

@@ -25,15 +25,14 @@
 
 #pragma once
 
+#include "WebGPUShaderModule.h"
 #include <wtf/KeyValuePair.h>
 #include <wtf/Ref.h>
 #include <wtf/Vector.h>
 #include <wtf/WeakRef.h>
 
 namespace WebCore::WebGPU {
 
-class ShaderModule;
-
 using PipelineConstantValue = double; // May represent WGSL’s bool, f32, i32, u32.
 
 struct ProgrammableStage {