Crash under WebCore: WebCore::CachedResourceClientWalker<WebCore::Cac…

…hedImageClient>::next()

https://bugs.webkit.org/show_bug.cgi?id=240072
<rdar://92717726>

Reviewed by Geoff Garen.

Have CachedResource and CachedResourceClientWalker hold the clients via WeakPtrs instead of
raw pointers and null check them before usage. This is a lot safer.

* Source/WTF/WTF.xcodeproj/project.pbxproj:
* Source/WTF/wtf/WeakHashCountedSet.h: Added.
(WTF::WeakHashCountedSet::begin):
(WTF::WeakHashCountedSet::end):
(WTF::WeakHashCountedSet::begin const):
(WTF::WeakHashCountedSet::end const):
(WTF::WeakHashCountedSet::find):
(WTF::WeakHashCountedSet::find const):
(WTF::WeakHashCountedSet::contains const):
(WTF::WeakHashCountedSet::computeSize const):
(WTF::WeakHashCountedSet::clear):
(WTF::Counter>::add):
(WTF::Counter>::remove):
* Source/WebCore/html/HTMLLinkElement.h:
* Source/WebCore/loader/DocumentThreadableLoader.h:
* Source/WebCore/loader/ImageLoader.h:
* Source/WebCore/loader/LinkLoader.h:
* Source/WebCore/loader/cache/CachedImage.cpp:
(WebCore::CachedImage::addClientWaitingForAsyncDecoding):
* Source/WebCore/loader/cache/CachedResource.cpp:
(WebCore::CachedResource::didAddClient):
(WebCore::CachedResource::addClientToSet):
(WebCore::CachedResource::removeClient):
(WebCore::CachedResource::switchClientsToRevalidatedResource):
* Source/WebCore/loader/cache/CachedResource.h:
(WebCore::CachedResource::hasClients const):
(WebCore::CachedResource::hasClient):
(WebCore::CachedResource::numberOfClients const):
* Source/WebCore/loader/cache/CachedResourceClient.h:
* Source/WebCore/loader/cache/CachedResourceClientWalker.h:
(WebCore::CachedResourceClientWalker::CachedResourceClientWalker):
(WebCore::CachedResourceClientWalker::next):
* Source/WebCore/rendering/RenderObject.h:

Canonical link: https://commits.webkit.org/250278@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@293804 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WTF/WTF.xcodeproj/project.pbxproj

@@ -59,6 +59,7 @@
 		337B2D6A26546EB300DDFD3D /* LikelyDenseUnsignedIntegerSet.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 337B2D6826546EAA00DDFD3D /* LikelyDenseUnsignedIntegerSet.cpp */; };
 		339B7B1127C45EF50072BF9A /* FixedWidthDouble.h in Headers */ = {isa = PBXBuildFile; fileRef = 33479C1C27236F2000B2E1B7 /* FixedWidthDouble.h */; settings = {ATTRIBUTES = (Private, ); }; };
 		4427C5AA21F6D6C300A612A4 /* ASCIICType.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 4427C5A921F6D6C300A612A4 /* ASCIICType.cpp */; };
+		46449E8B2822E5680005A8BC /* WeakHashCountedSet.h in Headers */ = {isa = PBXBuildFile; fileRef = 46449E8A2822E5670005A8BC /* WeakHashCountedSet.h */; settings = {ATTRIBUTES = (Private, ); }; };
 		4655743627F5FC4A002D5522 /* ASCIILiteralCocoa.mm in Sources */ = {isa = PBXBuildFile; fileRef = 4655743527F5FC4A002D5522 /* ASCIILiteralCocoa.mm */; };
 		46BEB6EB22FFE24900269867 /* RefCounted.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 46BEB6E922FFDDD500269867 /* RefCounted.cpp */; };
 		46E93049271F1205005BA6E5 /* SafeStrerror.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 46E43647271F10AA00C88C90 /* SafeStrerror.cpp */; };
@@ -1076,6 +1077,7 @@
 		4468567225094FE8008CCA05 /* ThreadSanitizerSupport.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = ThreadSanitizerSupport.h; sourceTree = "<group>"; };
 		44CDE4D226EE6CDA009F6ACB /* TypeCastsCocoa.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = TypeCastsCocoa.h; sourceTree = "<group>"; };
 		46209A27266D543A007F8F4A /* CancellableTask.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = CancellableTask.h; sourceTree = "<group>"; };
+		46449E8A2822E5670005A8BC /* WeakHashCountedSet.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = WeakHashCountedSet.h; sourceTree = "<group>"; };
 		4655743527F5FC4A002D5522 /* ASCIILiteralCocoa.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = ASCIILiteralCocoa.mm; sourceTree = "<group>"; };
 		46BA9EAB1F4CD61E009A2BBC /* CompletionHandler.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = CompletionHandler.h; sourceTree = "<group>"; };
 		46BEB6E922FFDDD500269867 /* RefCounted.cpp */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.cpp; path = RefCounted.cpp; sourceTree = "<group>"; };
@@ -2299,6 +2301,7 @@
 				A8A47372151A825B004123FF /* VMTags.h */,
 				0F66B2881DC97BAB004A1D3F /* WallTime.cpp */,
 				0F66B2891DC97BAB004A1D3F /* WallTime.h */,
+				46449E8A2822E5670005A8BC /* WeakHashCountedSet.h */,
 				9BE153352671F00F00C7D096 /* WeakHashMap.h */,
 				9B67F3F12228D5310030DE9C /* WeakHashSet.h */,
 				83ABB3C020B3823200BA3306 /* WeakObjCPtr.h */,
@@ -3221,6 +3224,7 @@
 				DD3DC95327A4BF8E007E5B61 /* VectorTraits.h in Headers */,
 				DD3DC95127A4BF8E007E5B61 /* VMTags.h in Headers */,
 				DD3DC8EE27A4BF8E007E5B61 /* WallTime.h in Headers */,
+				46449E8B2822E5680005A8BC /* WeakHashCountedSet.h in Headers */,
 				DD3DC8E527A4BF8E007E5B61 /* WeakHashMap.h in Headers */,
 				DD3DC85E27A4BF8E007E5B61 /* WeakHashSet.h in Headers */,
 				DDF3070627C086CC006A526F /* WeakLinking.h in Headers */,

Source/WTF/wtf/CMakeLists.txt

@@ -320,6 +320,7 @@ set(WTF_PUBLIC_HEADERS
     WTFConfig.h
     WTFSemaphore.h
     WallTime.h
+    WeakHashCountedSet.h
     WeakHashMap.h
     WeakHashSet.h
     WeakPtr.h

Source/WTF/wtf/WeakHashCountedSet.h

@@ -0,0 +1,115 @@
+/*
+ * Copyright (C) 2022 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.
+ */
+
+#pragma once
+
+#include <wtf/WeakHashMap.h>
+
+namespace WTF {
+
+template<typename Value, typename Counter = EmptyCounter>
+class WeakHashCountedSet {
+    WTF_MAKE_FAST_ALLOCATED;
+private:
+    using ImplType = WeakHashMap<Value, unsigned, Counter>;
+public:
+    using ValueType = Value;
+    using iterator = typename ImplType::iterator;
+    using const_iterator = typename ImplType::const_iterator;
+    using AddResult = typename ImplType::AddResult;
+
+    // Iterators iterate over pairs of values and counts.
+    iterator begin() { return m_impl.begin(); }
+    iterator end() { return m_impl.end(); }
+    const_iterator begin() const { return m_impl.begin(); }
+    const_iterator end() const { return m_impl.end(); }
+
+    iterator find(const ValueType& value) { return m_impl.find(value); }
+    const_iterator find(const ValueType& value) const { return m_impl.find(value); }
+    bool contains(const ValueType& value) const { return m_impl.contains(value); }
+
+    unsigned computeSize() const { return m_impl.computeSize(); }
+
+    // Increments the count if an equal value is already present.
+    // The return value includes both an iterator to the value's location,
+    // and an isNewEntry bool that indicates whether it is a new or existing entry.
+    AddResult add(const ValueType&);
+    AddResult add(ValueType&&);
+
+    // Decrements the count of the value, and removes it if count goes down to zero.
+    // Returns true if the value is removed.
+    bool remove(const ValueType&);
+    bool remove(iterator);
+
+    // Clears the whole set.
+    void clear() { m_impl.clear(); }
+
+private:
+    WeakHashMap<Value, unsigned, Counter> m_impl;
+};
+
+template<typename Value, typename Counter>
+inline auto WeakHashCountedSet<Value, Counter>::add(const ValueType &value) -> AddResult
+{
+    auto result = m_impl.add(value, 0);
+    ++result.iterator->value;
+    return result;
+}
+
+template<typename Value, typename Counter>
+inline auto WeakHashCountedSet<Value, Counter>::add(ValueType&& value) -> AddResult
+{
+    auto result = m_impl.add(std::forward<Value>(value), 0);
+    ++result.iterator->value;
+    return result;
+}
+
+template<typename Value, typename Counter>
+inline bool WeakHashCountedSet<Value, Counter>::remove(const ValueType& value)
+{
+    return remove(find(value));
+}
+
+template<typename Value, typename Counter>
+inline bool WeakHashCountedSet<Value, Counter>::remove(iterator it)
+{
+    if (it == end())
+        return false;
+
+    unsigned oldVal = it->value;
+    ASSERT(oldVal);
+    unsigned newVal = oldVal - 1;
+    if (newVal) {
+        it->value = newVal;
+        return false;
+    }
+
+    m_impl.remove(it);
+    return true;
+}
+
+} // namespace WTF
+
+using WTF::WeakHashCountedSet;

Source/WebCore/html/HTMLLinkElement.h

@@ -45,6 +45,9 @@ using LinkEventSender = EventSender<HTMLLinkElement>;
 class HTMLLinkElement final : public HTMLElement, public CachedStyleSheetClient, public LinkLoaderClient {
     WTF_MAKE_ISO_ALLOCATED(HTMLLinkElement);
 public:
+    using WeakValueType = HTMLElement::WeakValueType;
+    using HTMLElement::weakPtrFactory;
+
     static Ref<HTMLLinkElement> create(const QualifiedName&, Document&, bool createdByParser);
     virtual ~HTMLLinkElement();
 

Source/WebCore/loader/DocumentThreadableLoader.h

@@ -45,7 +45,7 @@ namespace WebCore {
     class Document;
     class ThreadableLoaderClient;
 
-    class DocumentThreadableLoader : public RefCounted<DocumentThreadableLoader>, public ThreadableLoader, public CanMakeWeakPtr<DocumentThreadableLoader>, private CachedRawResourceClient {
+    class DocumentThreadableLoader : public RefCounted<DocumentThreadableLoader>, public ThreadableLoader, public CachedRawResourceClient {
         WTF_MAKE_FAST_ALLOCATED;
     public:
         static void loadResourceSynchronously(Document&, ResourceRequest&&, ThreadableLoaderClient&, const ThreadableLoaderOptions&, RefPtr<SecurityOrigin>&&, std::unique_ptr<ContentSecurityPolicy>&&, std::optional<CrossOriginEmbedderPolicy>&&);

Source/WebCore/loader/ImageLoader.h

@@ -42,7 +42,7 @@ using ImageEventSender = EventSender<ImageLoader>;
 
 enum class RelevantMutation : bool { No, Yes };
 
-class ImageLoader : public CachedImageClient, public CanMakeWeakPtr<ImageLoader> {
+class ImageLoader : public CachedImageClient {
     WTF_MAKE_FAST_ALLOCATED;
 public:
     virtual ~ImageLoader();

Source/WebCore/loader/LinkLoader.h

@@ -38,8 +38,6 @@
 #include "LinkRelAttribute.h"
 #include "ReferrerPolicy.h"
 
-#include <wtf/WeakPtr.h>
-
 namespace WebCore {
 
 class Document;
@@ -57,7 +55,7 @@ struct LinkLoadParameters {
     ReferrerPolicy referrerPolicy { ReferrerPolicy::EmptyString };
 };
 
-class LinkLoader : private CachedResourceClient, public CanMakeWeakPtr<LinkLoader> {
+class LinkLoader : public CachedResourceClient {
 public:
     explicit LinkLoader(LinkLoaderClient&);
     virtual ~LinkLoader();

Source/WebCore/loader/cache/CachedImage.cpp

@@ -166,7 +166,7 @@ void CachedImage::addClientWaitingForAsyncDecoding(CachedImageClient& client)
     ASSERT(client.resourceClientType() == CachedImageClient::expectedType());
     if (m_clientsWaitingForAsyncDecoding.contains(&client))
         return;
-    if (!m_clients.contains(&client)) {
+    if (!m_clients.contains(client)) {
         // If the <html> element does not have its own background specified, painting the root box
         // renderer uses the style of the <body> element, see RenderView::rendererForRootBackground().
         // In this case, the client we are asked to add is the root box renderer. Since we can't add

Source/WebCore/loader/cache/CachedResource.cpp

@@ -511,8 +511,8 @@ void CachedResource::didAddClient(CachedResourceClient& client)
     if (m_decodedDataDeletionTimer.isActive())
         m_decodedDataDeletionTimer.stop();
 
-    if (m_clientsAwaitingCallback.remove(&client)) {
-        m_clients.add(&client);
+    if (m_clientsAwaitingCallback.remove(client)) {
+        m_clients.add(client);
 #if ASSERT_ENABLED
         client.addAssociatedResource(*this);
 #endif
@@ -541,12 +541,12 @@ bool CachedResource::addClientToSet(CachedResourceClient& client)
         // synchronously (e.g., scripts may not have set all the state they need to handle the load).
         // Therefore, rather than immediately sending callbacks on a cache hit like other CachedResources,
         // we schedule the callbacks and ensure we never finish synchronously.
-        ASSERT(!m_clientsAwaitingCallback.contains(&client));
-        m_clientsAwaitingCallback.add(&client, makeUnique<Callback>(*this, client));
+        ASSERT(!m_clientsAwaitingCallback.contains(client));
+        m_clientsAwaitingCallback.add(client, makeUnique<Callback>(*this, client));
         return false;
     }
 
-    m_clients.add(&client);
+    m_clients.add(client);
 #if ASSERT_ENABLED
     client.addAssociatedResource(*this);
 #endif
@@ -555,16 +555,16 @@ bool CachedResource::addClientToSet(CachedResourceClient& client)
 
 void CachedResource::removeClient(CachedResourceClient& client)
 {
-    auto callback = m_clientsAwaitingCallback.take(&client);
+    auto callback = m_clientsAwaitingCallback.take(client);
     if (callback) {
-        ASSERT(!m_clients.contains(&client));
+        ASSERT(!m_clients.contains(client));
         callback->cancel();
         callback = nullptr;
     } else {
-        ASSERT(m_clients.contains(&client));
-        m_clients.remove(&client);
+        ASSERT(m_clients.contains(client));
+        m_clients.remove(client);
 #if ASSERT_ENABLED
-        if (!m_clients.contains(&client))
+        if (!m_clients.contains(client))
             client.removeAssociatedResource(*this);
 #endif
         didRemoveClient(client);
@@ -779,28 +779,32 @@ void CachedResource::switchClientsToRevalidatedResource()
     ASSERT(!m_handleCount);
     m_handlesToRevalidate.clear();
 
-    Vector<CachedResourceClient*> clientsToMove;
-    for (auto& entry : m_clients) {
-        CachedResourceClient* client = entry.key;
+    Vector<WeakPtr<CachedResourceClient>> clientsToMove;
+    for (auto entry : m_clients) {
+        auto& client = entry.key;
         unsigned count = entry.value;
         while (count) {
             clientsToMove.append(client);
             --count;
         }
     }
 
-    for (auto& client : clientsToMove)
-        removeClient(*client);
-    ASSERT(m_clients.isEmpty());
+    for (auto& client : clientsToMove) {
+        if (client)
+            removeClient(*client);
+    }
+    ASSERT(!m_clients.computeSize());
 
-    for (auto& client : clientsToMove)
-        m_resourceToRevalidate->addClientToSet(*client);
+    for (auto& client : clientsToMove) {
+        if (client)
+            m_resourceToRevalidate->addClientToSet(*client);
+    }
     for (auto& client : clientsToMove) {
         // Calling didAddClient may do anything, including trying to cancel revalidation.
         // Assert that it didn't succeed.
         ASSERT(m_resourceToRevalidate);
         // Calling didAddClient for a client may end up removing another client. In that case it won't be in the set anymore.
-        if (m_resourceToRevalidate->m_clients.contains(client))
+        if (client && m_resourceToRevalidate->m_clients.contains(*client))
             m_resourceToRevalidate->didAddClient(*client);
     }
     m_switchingClientsToRevalidatedResource = false;

Source/WebCore/loader/cache/CachedResource.h

@@ -23,6 +23,7 @@
 #pragma once
 
 #include "CacheValidation.h"
+#include "CachedResourceClient.h"
 #include "FrameLoaderTypes.h"
 #include "ResourceError.h"
 #include "ResourceLoadPriority.h"
@@ -37,6 +38,8 @@
 #include <wtf/HashSet.h>
 #include <wtf/TypeCasts.h>
 #include <wtf/Vector.h>
+#include <wtf/WeakHashCountedSet.h>
+#include <wtf/WeakHashMap.h>
 #include <wtf/text/WTFString.h>
 
 namespace WebCore {
@@ -138,8 +141,8 @@ class CachedResource {
 
     WEBCORE_EXPORT void addClient(CachedResourceClient&);
     WEBCORE_EXPORT void removeClient(CachedResourceClient&);
-    bool hasClients() const { return !m_clients.isEmpty() || !m_clientsAwaitingCallback.isEmpty(); }
-    bool hasClient(CachedResourceClient& client) { return m_clients.contains(&client) || m_clientsAwaitingCallback.contains(&client); }
+    bool hasClients() const { return m_clients.computeSize() || m_clientsAwaitingCallback.computeSize(); }
+    bool hasClient(const CachedResourceClient& client) { return m_clients.contains(client) || m_clientsAwaitingCallback.contains(client); }
     bool deleteIfPossible();
 
     enum class PreloadResult : uint8_t {
@@ -157,7 +160,7 @@ class CachedResource {
     virtual void allClientsRemoved();
     void destroyDecodedDataIfNeeded();
 
-    unsigned numberOfClients() const { return m_clients.size(); }
+    unsigned numberOfClients() const { return m_clients.computeSize(); }
 
     Status status() const { return static_cast<Status>(m_status); }
     void setStatus(Status status)
@@ -335,7 +338,7 @@ class CachedResource {
     DeferrableOneShotTimer m_decodedDataDeletionTimer;
 
     // FIXME: Make the rest of these data members private and use functions in derived classes instead.
-    HashCountedSet<CachedResourceClient*> m_clients;
+    WeakHashCountedSet<CachedResourceClient> m_clients;
     std::unique_ptr<ResourceRequest> m_originalRequest; // Needed by Ping loads.
     RefPtr<SubresourceLoader> m_loader;
     RefPtr<FragmentedSharedBuffer> m_data;
@@ -347,7 +350,7 @@ class CachedResource {
     WallTime m_responseTimestamp;
     ResourceLoaderIdentifier m_identifierForLoadWithoutResourceLoader;
 
-    HashMap<CachedResourceClient*, std::unique_ptr<Callback>> m_clientsAwaitingCallback;
+    WeakHashMap<CachedResourceClient, std::unique_ptr<Callback>> m_clientsAwaitingCallback;
 
     // These handles will need to be updated to point to the m_resourceToRevalidate in case we get 304 response.
     HashSet<CachedResourceHandleBase*> m_handlesToRevalidate;

Source/WebCore/loader/cache/CachedResourceClient.h

@@ -26,13 +26,14 @@
 
 #include <wtf/HashSet.h>
 #include <wtf/Noncopyable.h>
+#include <wtf/WeakPtr.h>
 
 namespace WebCore {
 
 class CachedResource;
 class NetworkLoadMetrics;
 
-class CachedResourceClient {
+class CachedResourceClient : public CanMakeWeakPtr<CachedResourceClient> {
     WTF_MAKE_NONCOPYABLE(CachedResourceClient);
 public:
     enum CachedResourceClientType {