Title: [292826] branches/safari-613.2.6.1-branch/Source/WebCore
Revision
292826
Author
kocsen_ch...@apple.com
Date
2022-04-13 13:32:01 -0700 (Wed, 13 Apr 2022)

Log Message

Cherry-pick r292801. rdar://problem/91346216

    Crash under CachedResourceClientWalker<WebCore::CachedImageClient>::next()
    https://bugs.webkit.org/show_bug.cgi?id=239253
    <rdar://91346216>

    Reviewed by Simon Fraser and Brent Fulgham.

    I haven't been able to reproduce the issue or figure out why this is happening so I am doing
    some hardening and adding assertions to help catch the underlying bug.

    * loader/ImageLoader.cpp:
    (WebCore::ImageLoader::didUpdateCachedImage):
    There is some speculation that r291141 could have caused this because of the timing of when
    this patch landed and the fact that this patch modifies ImageLoader, which is a CachedImageClient.
    I couldn't see anything wrong with the change. However, I did notice that we were calling
    didUpdateCachedImage() twice with the same CachedImage now for lazy loading (once initially and then
    another time when the image actually starts lazily). This was intentional. However, the registering
    again as a client of the CachedImage (and then unregistering right away) was not. Technically, this
    should be fine since CachedResource is using a HashCountedSet for m_clients locally. However, for
    the sake of safety, I am now not doing this redundant registering/unregistering as a client of
    the CachedImage when this image has not changed.

    * loader/cache/CachedCSSStyleSheet.cpp:
    (WebCore::CachedCSSStyleSheet::checkNotify):
    * loader/cache/CachedFont.cpp:
    (WebCore::CachedFont::checkNotify):
    * loader/cache/CachedImage.cpp:
    (WebCore::CachedImage::load):
    (WebCore::CachedImage::addClientWaitingForAsyncDecoding):
    (WebCore::CachedImage::notifyObservers):
    (WebCore::CachedImage::canDestroyDecodedData):
    (WebCore::CachedImage::imageFrameAvailable):
    (WebCore::CachedImage::scheduleRenderingUpdate):
    (WebCore::CachedImage::isVisibleInViewport):
    (WebCore::CachedImage::isVisibleInViewport const): Deleted.
    * loader/cache/CachedImage.h:
    * loader/cache/CachedRawResource.cpp:
    (WebCore::CachedRawResource::notifyClientsDataWasReceived):
    (WebCore::iterateRedirects):

    (WebCore::CachedRawResource::redirectReceived):
    The new assertions found a bug here where we were capturing the CachedRawResourceClient by value in the lambda and thus
    making a copy of it (even though this is a polymorphic class). I fixed the bug and marked CachedResourceClient as non
    copyable to avoid issues like these.

    (WebCore::CachedRawResource::responseReceived):
    (WebCore::CachedRawResource::shouldCacheResponse):
    (WebCore::CachedRawResource::didSendData):
    (WebCore::CachedRawResource::finishedTimingForWorkerLoad):
    (WebCore::CachedRawResource::previewResponseReceived):

    * loader/cache/CachedResource.cpp:
    (WebCore::CachedResource::checkNotify):
    (WebCore::CachedResource::didAddClient):
    (WebCore::CachedResource::addClientToSet):
    (WebCore::CachedResource::removeClient):
    * loader/cache/CachedResource.h:
    * loader/cache/CachedResourceClient.h:
    (WebCore::CachedResourceClient::~CachedResourceClient):
    (WebCore::CachedResourceClient::addAssociatedResource):
    (WebCore::CachedResourceClient::removeAssociatedResource):
    Add new assertions to make sure that a CachedResourceClient is no longer associated (i.e. marked as a client of) with
    any CachedResource at the time it is destroyed. Hopefully, this will catch the issue right away and give us a useful
    stack trace, instead of crashing later on when iterating over the clients of a CachedResource.

    * loader/cache/CachedResourceClientWalker.h:
    (WebCore::CachedResourceClientWalker::CachedResourceClientWalker):
    (WebCore::CachedResourceClientWalker::next):
    CachedResourceClientWalker is meant to be a safe way of iterating over the clients of a CachedResource, allowing clients
    to unregister themselves as we iterate. However, when clients unregister themselves, it could in theory cause the
    CachedResource itself to get destroyed. In such cases, the CachedResourceClientWalker would not be safe since its
    m_clientSet data member would come from a dead CachedResource. To address the issue, the walker now keeps a handle to
    the cached resource, instead of the reference to the CachedResource's clients set. The handle will ensure the cached
    resource stays alive.

    * loader/cache/CachedScript.cpp:
    * loader/cache/CachedTextTrack.cpp:
    (WebCore::CachedTextTrack::doUpdateBuffer):
    * loader/cache/CachedXSLStyleSheet.cpp:
    (WebCore::CachedXSLStyleSheet::checkNotify):
    * rendering/RenderObject.cpp:

    git-svn-id: https://svn.webkit.org/repository/webkit/trunk@292801 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Modified Paths

Diff

Modified: branches/safari-613.2.6.1-branch/Source/WebCore/ChangeLog (292825 => 292826)


--- branches/safari-613.2.6.1-branch/Source/WebCore/ChangeLog	2022-04-13 20:11:05 UTC (rev 292825)
+++ branches/safari-613.2.6.1-branch/Source/WebCore/ChangeLog	2022-04-13 20:32:01 UTC (rev 292826)
@@ -1,3 +1,173 @@
+2022-04-13  Alan Coon  <alanc...@apple.com>
+
+        Cherry-pick r292801. rdar://problem/91346216
+
+    Crash under CachedResourceClientWalker<WebCore::CachedImageClient>::next()
+    https://bugs.webkit.org/show_bug.cgi?id=239253
+    <rdar://91346216>
+    
+    Reviewed by Simon Fraser and Brent Fulgham.
+    
+    I haven't been able to reproduce the issue or figure out why this is happening so I am doing
+    some hardening and adding assertions to help catch the underlying bug.
+    
+    * loader/ImageLoader.cpp:
+    (WebCore::ImageLoader::didUpdateCachedImage):
+    There is some speculation that r291141 could have caused this because of the timing of when
+    this patch landed and the fact that this patch modifies ImageLoader, which is a CachedImageClient.
+    I couldn't see anything wrong with the change. However, I did notice that we were calling
+    didUpdateCachedImage() twice with the same CachedImage now for lazy loading (once initially and then
+    another time when the image actually starts lazily). This was intentional. However, the registering
+    again as a client of the CachedImage (and then unregistering right away) was not. Technically, this
+    should be fine since CachedResource is using a HashCountedSet for m_clients locally. However, for
+    the sake of safety, I am now not doing this redundant registering/unregistering as a client of
+    the CachedImage when this image has not changed.
+    
+    * loader/cache/CachedCSSStyleSheet.cpp:
+    (WebCore::CachedCSSStyleSheet::checkNotify):
+    * loader/cache/CachedFont.cpp:
+    (WebCore::CachedFont::checkNotify):
+    * loader/cache/CachedImage.cpp:
+    (WebCore::CachedImage::load):
+    (WebCore::CachedImage::addClientWaitingForAsyncDecoding):
+    (WebCore::CachedImage::notifyObservers):
+    (WebCore::CachedImage::canDestroyDecodedData):
+    (WebCore::CachedImage::imageFrameAvailable):
+    (WebCore::CachedImage::scheduleRenderingUpdate):
+    (WebCore::CachedImage::isVisibleInViewport):
+    (WebCore::CachedImage::isVisibleInViewport const): Deleted.
+    * loader/cache/CachedImage.h:
+    * loader/cache/CachedRawResource.cpp:
+    (WebCore::CachedRawResource::notifyClientsDataWasReceived):
+    (WebCore::iterateRedirects):
+    
+    (WebCore::CachedRawResource::redirectReceived):
+    The new assertions found a bug here where we were capturing the CachedRawResourceClient by value in the lambda and thus
+    making a copy of it (even though this is a polymorphic class). I fixed the bug and marked CachedResourceClient as non
+    copyable to avoid issues like these.
+    
+    (WebCore::CachedRawResource::responseReceived):
+    (WebCore::CachedRawResource::shouldCacheResponse):
+    (WebCore::CachedRawResource::didSendData):
+    (WebCore::CachedRawResource::finishedTimingForWorkerLoad):
+    (WebCore::CachedRawResource::previewResponseReceived):
+    
+    * loader/cache/CachedResource.cpp:
+    (WebCore::CachedResource::checkNotify):
+    (WebCore::CachedResource::didAddClient):
+    (WebCore::CachedResource::addClientToSet):
+    (WebCore::CachedResource::removeClient):
+    * loader/cache/CachedResource.h:
+    * loader/cache/CachedResourceClient.h:
+    (WebCore::CachedResourceClient::~CachedResourceClient):
+    (WebCore::CachedResourceClient::addAssociatedResource):
+    (WebCore::CachedResourceClient::removeAssociatedResource):
+    Add new assertions to make sure that a CachedResourceClient is no longer associated (i.e. marked as a client of) with
+    any CachedResource at the time it is destroyed. Hopefully, this will catch the issue right away and give us a useful
+    stack trace, instead of crashing later on when iterating over the clients of a CachedResource.
+    
+    * loader/cache/CachedResourceClientWalker.h:
+    (WebCore::CachedResourceClientWalker::CachedResourceClientWalker):
+    (WebCore::CachedResourceClientWalker::next):
+    CachedResourceClientWalker is meant to be a safe way of iterating over the clients of a CachedResource, allowing clients
+    to unregister themselves as we iterate. However, when clients unregister themselves, it could in theory cause the
+    CachedResource itself to get destroyed. In such cases, the CachedResourceClientWalker would not be safe since its
+    m_clientSet data member would come from a dead CachedResource. To address the issue, the walker now keeps a handle to
+    the cached resource, instead of the reference to the CachedResource's clients set. The handle will ensure the cached
+    resource stays alive.
+    
+    * loader/cache/CachedScript.cpp:
+    * loader/cache/CachedTextTrack.cpp:
+    (WebCore::CachedTextTrack::doUpdateBuffer):
+    * loader/cache/CachedXSLStyleSheet.cpp:
+    (WebCore::CachedXSLStyleSheet::checkNotify):
+    * rendering/RenderObject.cpp:
+    
+    git-svn-id: https://svn.webkit.org/repository/webkit/trunk@292801 268f45cc-cd09-0410-ab3c-d52691b4dbfc
+
+    2022-04-12  Chris Dumez  <cdu...@apple.com>
+
+            Crash under CachedResourceClientWalker<WebCore::CachedImageClient>::next()
+            https://bugs.webkit.org/show_bug.cgi?id=239253
+            <rdar://91346216>
+
+            Reviewed by Simon Fraser and Brent Fulgham.
+
+            I haven't been able to reproduce the issue or figure out why this is happening so I am doing
+            some hardening and adding assertions to help catch the underlying bug.
+
+            * loader/ImageLoader.cpp:
+            (WebCore::ImageLoader::didUpdateCachedImage):
+            There is some speculation that r291141 could have caused this because of the timing of when
+            this patch landed and the fact that this patch modifies ImageLoader, which is a CachedImageClient.
+            I couldn't see anything wrong with the change. However, I did notice that we were calling
+            didUpdateCachedImage() twice with the same CachedImage now for lazy loading (once initially and then
+            another time when the image actually starts lazily). This was intentional. However, the registering
+            again as a client of the CachedImage (and then unregistering right away) was not. Technically, this
+            should be fine since CachedResource is using a HashCountedSet for m_clients locally. However, for
+            the sake of safety, I am now not doing this redundant registering/unregistering as a client of
+            the CachedImage when this image has not changed.
+
+            * loader/cache/CachedCSSStyleSheet.cpp:
+            (WebCore::CachedCSSStyleSheet::checkNotify):
+            * loader/cache/CachedFont.cpp:
+            (WebCore::CachedFont::checkNotify):
+            * loader/cache/CachedImage.cpp:
+            (WebCore::CachedImage::load):
+            (WebCore::CachedImage::addClientWaitingForAsyncDecoding):
+            (WebCore::CachedImage::notifyObservers):
+            (WebCore::CachedImage::canDestroyDecodedData):
+            (WebCore::CachedImage::imageFrameAvailable):
+            (WebCore::CachedImage::scheduleRenderingUpdate):
+            (WebCore::CachedImage::isVisibleInViewport):
+            (WebCore::CachedImage::isVisibleInViewport const): Deleted.
+            * loader/cache/CachedImage.h:
+            * loader/cache/CachedRawResource.cpp:
+            (WebCore::CachedRawResource::notifyClientsDataWasReceived):
+            (WebCore::iterateRedirects):
+
+            (WebCore::CachedRawResource::redirectReceived):
+            The new assertions found a bug here where we were capturing the CachedRawResourceClient by value in the lambda and thus
+            making a copy of it (even though this is a polymorphic class). I fixed the bug and marked CachedResourceClient as non
+            copyable to avoid issues like these.
+
+            (WebCore::CachedRawResource::responseReceived):
+            (WebCore::CachedRawResource::shouldCacheResponse):
+            (WebCore::CachedRawResource::didSendData):
+            (WebCore::CachedRawResource::finishedTimingForWorkerLoad):
+            (WebCore::CachedRawResource::previewResponseReceived):
+
+            * loader/cache/CachedResource.cpp:
+            (WebCore::CachedResource::checkNotify):
+            (WebCore::CachedResource::didAddClient):
+            (WebCore::CachedResource::addClientToSet):
+            (WebCore::CachedResource::removeClient):
+            * loader/cache/CachedResource.h:
+            * loader/cache/CachedResourceClient.h:
+            (WebCore::CachedResourceClient::~CachedResourceClient):
+            (WebCore::CachedResourceClient::addAssociatedResource):
+            (WebCore::CachedResourceClient::removeAssociatedResource):
+            Add new assertions to make sure that a CachedResourceClient is no longer associated (i.e. marked as a client of) with
+            any CachedResource at the time it is destroyed. Hopefully, this will catch the issue right away and give us a useful
+            stack trace, instead of crashing later on when iterating over the clients of a CachedResource.
+
+            * loader/cache/CachedResourceClientWalker.h:
+            (WebCore::CachedResourceClientWalker::CachedResourceClientWalker):
+            (WebCore::CachedResourceClientWalker::next):
+            CachedResourceClientWalker is meant to be a safe way of iterating over the clients of a CachedResource, allowing clients
+            to unregister themselves as we iterate. However, when clients unregister themselves, it could in theory cause the
+            CachedResource itself to get destroyed. In such cases, the CachedResourceClientWalker would not be safe since its
+            m_clientSet data member would come from a dead CachedResource. To address the issue, the walker now keeps a handle to
+            the cached resource, instead of the reference to the CachedResource's clients set. The handle will ensure the cached
+            resource stays alive.
+
+            * loader/cache/CachedScript.cpp:
+            * loader/cache/CachedTextTrack.cpp:
+            (WebCore::CachedTextTrack::doUpdateBuffer):
+            * loader/cache/CachedXSLStyleSheet.cpp:
+            (WebCore::CachedXSLStyleSheet::checkNotify):
+            * rendering/RenderObject.cpp:
+
 2022-04-07  Alan Coon  <alanc...@apple.com>
 
         Cherry-pick r291629. rdar://problem/91446360

Modified: branches/safari-613.2.6.1-branch/Source/WebCore/loader/ImageLoader.cpp (292825 => 292826)


--- branches/safari-613.2.6.1-branch/Source/WebCore/loader/ImageLoader.cpp	2022-04-13 20:11:05 UTC (rev 292825)
+++ branches/safari-613.2.6.1-branch/Source/WebCore/loader/ImageLoader.cpp	2022-04-13 20:32:01 UTC (rev 292826)
@@ -299,11 +299,14 @@
             // If newImage is cached, addClient() will result in the load event
             // being queued to fire. Ensure this happens after beforeload is
             // dispatched.
-            newImage->addClient(*this);
+            if (!loadHasNowLazilyStarted)
+                newImage->addClient(*this);
         } else
             resetLazyImageLoading(element().document());
+
         if (oldImage) {
-            oldImage->removeClient(*this);
+            if (!loadHasNowLazilyStarted)
+                oldImage->removeClient(*this);
             updateRenderer();
         }
     }

Modified: branches/safari-613.2.6.1-branch/Source/WebCore/loader/cache/CachedCSSStyleSheet.cpp (292825 => 292826)


--- branches/safari-613.2.6.1-branch/Source/WebCore/loader/cache/CachedCSSStyleSheet.cpp	2022-04-13 20:11:05 UTC (rev 292825)
+++ branches/safari-613.2.6.1-branch/Source/WebCore/loader/cache/CachedCSSStyleSheet.cpp	2022-04-13 20:32:01 UTC (rev 292826)
@@ -120,8 +120,8 @@
     if (isLoading())
         return;
 
-    CachedResourceClientWalker<CachedStyleSheetClient> w(m_clients);
-    while (CachedStyleSheetClient* c = w.next())
+    CachedResourceClientWalker<CachedStyleSheetClient> walker(*this);
+    while (CachedStyleSheetClient* c = walker.next())
         c->setCSSStyleSheet(m_resourceRequest.url().string(), m_response.url(), m_decoder->encoding().name(), this);
 }
 

Modified: branches/safari-613.2.6.1-branch/Source/WebCore/loader/cache/CachedFont.cpp (292825 => 292826)


--- branches/safari-613.2.6.1-branch/Source/WebCore/loader/cache/CachedFont.cpp	2022-04-13 20:11:05 UTC (rev 292825)
+++ branches/safari-613.2.6.1-branch/Source/WebCore/loader/cache/CachedFont.cpp	2022-04-13 20:32:01 UTC (rev 292826)
@@ -145,7 +145,7 @@
     if (isLoading())
         return;
 
-    CachedResourceClientWalker<CachedFontClient> walker(m_clients);
+    CachedResourceClientWalker<CachedFontClient> walker(*this);
     while (CachedFontClient* client = walker.next())
         client->fontLoaded(*this);
 }

Modified: branches/safari-613.2.6.1-branch/Source/WebCore/loader/cache/CachedImage.cpp (292825 => 292826)


--- branches/safari-613.2.6.1-branch/Source/WebCore/loader/cache/CachedImage.cpp	2022-04-13 20:11:05 UTC (rev 292825)
+++ branches/safari-613.2.6.1-branch/Source/WebCore/loader/cache/CachedImage.cpp	2022-04-13 20:32:01 UTC (rev 292826)
@@ -101,7 +101,7 @@
     m_skippingRevalidationDocument = loader.document();
     if (loader.shouldPerformImageLoad(url())) {
         CachedResource::load(loader);
-        CachedResourceClientWalker<CachedImageClient> walker(m_clients);
+        CachedResourceClientWalker<CachedImageClient> walker(*this);
         while (auto* client = walker.next())
             client->didStartLoading();
     } else
@@ -173,7 +173,7 @@
         // a client to m_clientsWaitingForAsyncDecoding unless it is one of the m_clients, we are going
         // to cancel the repaint optimization we do in CachedImage::imageFrameAvailable() by adding
         // all the m_clients to m_clientsWaitingForAsyncDecoding.
-        CachedResourceClientWalker<CachedImageClient> walker(m_clients);
+        CachedResourceClientWalker<CachedImageClient> walker(*this);
         while (auto* client = walker.next())
             m_clientsWaitingForAsyncDecoding.add(client);
     } else
@@ -342,8 +342,8 @@
 
 void CachedImage::notifyObservers(const IntRect* changeRect)
 {
-    CachedResourceClientWalker<CachedImageClient> w(m_clients);
-    while (CachedImageClient* c = w.next())
+    CachedResourceClientWalker<CachedImageClient> walker(*this);
+    while (CachedImageClient* c = walker.next())
         c->imageChanged(this, changeRect);
 }
 
@@ -656,8 +656,8 @@
     if (&image != m_image)
         return false;
 
-    CachedResourceClientWalker<CachedImageClient> clientWalker(m_clients);
-    while (CachedImageClient* client = clientWalker.next()) {
+    CachedResourceClientWalker<CachedImageClient> walker(*this);
+    while (CachedImageClient* client = walker.next()) {
         if (!client->canDestroyDecodedData())
             return false;
     }
@@ -670,10 +670,10 @@
     if (&image != m_image)
         return;
 
-    CachedResourceClientWalker<CachedImageClient> clientWalker(m_clients);
+    CachedResourceClientWalker<CachedImageClient> walker(*this);
     VisibleInViewportState visibleState = VisibleInViewportState::No;
 
-    while (CachedImageClient* client = clientWalker.next()) {
+    while (CachedImageClient* client = walker.next()) {
         // All the clients of animated images have to be notified. The new frame has to be drawn in all of them.
         if (animatingState == ImageAnimatingState::No && !m_clientsWaitingForAsyncDecoding.contains(client))
             continue;
@@ -700,7 +700,7 @@
     if (&image != m_image)
         return;
 
-    CachedResourceClientWalker<CachedImageClient> walker(m_clients);
+    CachedResourceClientWalker<CachedImageClient> walker(*this);
     while (auto* client = walker.next())
         client->scheduleRenderingUpdateForImage(*this);
 }
@@ -744,7 +744,7 @@
 
 bool CachedImage::isVisibleInViewport(const Document& document) const
 {
-    CachedResourceClientWalker<CachedImageClient> walker(m_clients);
+    CachedResourceClientWalker<CachedImageClient> walker(*this);
     while (auto* client = walker.next()) {
         if (client->imageVisibleInViewport(document) == VisibleInViewportState::Yes)
             return true;

Modified: branches/safari-613.2.6.1-branch/Source/WebCore/loader/cache/CachedRawResource.cpp (292825 => 292826)


--- branches/safari-613.2.6.1-branch/Source/WebCore/loader/cache/CachedRawResource.cpp	2022-04-13 20:11:05 UTC (rev 292825)
+++ branches/safari-613.2.6.1-branch/Source/WebCore/loader/cache/CachedRawResource.cpp	2022-04-13 20:32:01 UTC (rev 292826)
@@ -140,8 +140,8 @@
         return;
 
     CachedResourceHandle<CachedRawResource> protectedThis(this);
-    CachedResourceClientWalker<CachedRawResourceClient> w(m_clients);
-    while (CachedRawResourceClient* c = w.next())
+    CachedResourceClientWalker<CachedRawResourceClient> walker(*this);
+    while (CachedRawResourceClient* c = walker.next())
         c->dataReceived(*this, buffer);
 }
 
@@ -150,10 +150,10 @@
     if (!handle->hasClient(client) || redirectsInReverseOrder.isEmpty())
         return completionHandler({ });
     auto redirectPair = redirectsInReverseOrder.takeLast();
-    client.redirectReceived(*handle, WTFMove(redirectPair.first), WTFMove(redirectPair.second), [handle = WTFMove(handle), client, redirectsInReverseOrder = WTFMove(redirectsInReverseOrder), completionHandler = WTFMove(completionHandler)] (ResourceRequest&&) mutable {
+    client.redirectReceived(*handle, WTFMove(redirectPair.first), WTFMove(redirectPair.second), [handle = WTFMove(handle), client = &client, redirectsInReverseOrder = WTFMove(redirectsInReverseOrder), completionHandler = WTFMove(completionHandler)] (ResourceRequest&&) mutable {
         // Ignore the new request because we can't do anything with it.
         // We're just replying a redirect chain that has already happened.
-        iterateRedirects(WTFMove(handle), client, WTFMove(redirectsInReverseOrder), WTFMove(completionHandler));
+        iterateRedirects(WTFMove(handle), *client, WTFMove(redirectsInReverseOrder), WTFMove(completionHandler));
     });
 }
 
@@ -222,7 +222,7 @@
         CachedResource::redirectReceived(WTFMove(request), response, WTFMove(completionHandler));
     else {
         m_redirectChain.append(RedirectPair(request, response));
-        iterateClients(CachedResourceClientWalker<CachedRawResourceClient>(m_clients), CachedResourceHandle<CachedRawResource>(this), WTFMove(request), makeUnique<ResourceResponse>(response), [this, protectedThis = CachedResourceHandle<CachedRawResource>(this), completionHandler = WTFMove(completionHandler), response] (ResourceRequest&& request) mutable {
+        iterateClients(CachedResourceClientWalker<CachedRawResourceClient>(*this), CachedResourceHandle<CachedRawResource>(this), WTFMove(request), makeUnique<ResourceResponse>(response), [this, protectedThis = CachedResourceHandle<CachedRawResource>(this), completionHandler = WTFMove(completionHandler), response] (ResourceRequest&& request) mutable {
             CachedResource::redirectReceived(WTFMove(request), response, WTFMove(completionHandler));
         });
     }
@@ -234,15 +234,15 @@
     if (!m_identifier)
         m_identifier = m_loader->identifier();
     CachedResource::responseReceived(response);
-    CachedResourceClientWalker<CachedRawResourceClient> w(m_clients);
-    while (CachedRawResourceClient* c = w.next())
+    CachedResourceClientWalker<CachedRawResourceClient> walker(*this);
+    while (CachedRawResourceClient* c = walker.next())
         c->responseReceived(*this, m_response, nullptr);
 }
 
 bool CachedRawResource::shouldCacheResponse(const ResourceResponse& response)
 {
-    CachedResourceClientWalker<CachedRawResourceClient> w(m_clients);
-    while (CachedRawResourceClient* c = w.next()) {
+    CachedResourceClientWalker<CachedRawResourceClient> walker(*this);
+    while (CachedRawResourceClient* c = walker.next()) {
         if (!c->shouldCacheResponse(*this, response))
             return false;
     }
@@ -251,15 +251,15 @@
 
 void CachedRawResource::didSendData(unsigned long long bytesSent, unsigned long long totalBytesToBeSent)
 {
-    CachedResourceClientWalker<CachedRawResourceClient> w(m_clients);
-    while (CachedRawResourceClient* c = w.next())
+    CachedResourceClientWalker<CachedRawResourceClient> walker(*this);
+    while (CachedRawResourceClient* c = walker.next())
         c->dataSent(*this, bytesSent, totalBytesToBeSent);
 }
 
 void CachedRawResource::finishedTimingForWorkerLoad(ResourceTiming&& resourceTiming)
 {
-    CachedResourceClientWalker<CachedRawResourceClient> w(m_clients);
-    while (CachedRawResourceClient* c = w.next())
+    CachedResourceClientWalker<CachedRawResourceClient> walker(*this);
+    while (CachedRawResourceClient* c = walker.next())
         c->finishedTimingForWorkerLoad(*this, resourceTiming);
 }
 
@@ -360,8 +360,8 @@
 {
     CachedResourceHandle<CachedRawResource> protectedThis(this);
     CachedResource::previewResponseReceived(response);
-    CachedResourceClientWalker<CachedRawResourceClient> w(m_clients);
-    while (CachedRawResourceClient* c = w.next())
+    CachedResourceClientWalker<CachedRawResourceClient> walker(*this);
+    while (CachedRawResourceClient* c = walker.next())
         c->previewResponseReceived(*this, m_response);
 }
 

Modified: branches/safari-613.2.6.1-branch/Source/WebCore/loader/cache/CachedResource.cpp (292825 => 292826)


--- branches/safari-613.2.6.1-branch/Source/WebCore/loader/cache/CachedResource.cpp	2022-04-13 20:11:05 UTC (rev 292825)
+++ branches/safari-613.2.6.1-branch/Source/WebCore/loader/cache/CachedResource.cpp	2022-04-13 20:32:01 UTC (rev 292826)
@@ -331,7 +331,7 @@
     if (isLoading() || stillNeedsLoad())
         return;
 
-    CachedResourceClientWalker<CachedResourceClient> walker(m_clients);
+    CachedResourceClientWalker<CachedResourceClient> walker(*this);
     while (CachedResourceClient* client = walker.next())
         client->notifyFinished(*this, metrics);
 }
@@ -511,8 +511,12 @@
     if (m_decodedDataDeletionTimer.isActive())
         m_decodedDataDeletionTimer.stop();
 
-    if (m_clientsAwaitingCallback.remove(&client))
+    if (m_clientsAwaitingCallback.remove(&client)) {
         m_clients.add(&client);
+#if ASSERT_ENABLED
+        client.addAssociatedResource(*this);
+#endif
+    }
 
     // FIXME: Make calls to notifyFinished async
     if (!isLoading() && !stillNeedsLoad())
@@ -543,6 +547,9 @@
     }
 
     m_clients.add(&client);
+#if ASSERT_ENABLED
+    client.addAssociatedResource(*this);
+#endif
     return true;
 }
 
@@ -556,6 +563,10 @@
     } else {
         ASSERT(m_clients.contains(&client));
         m_clients.remove(&client);
+#if ASSERT_ENABLED
+        if (!m_clients.contains(&client))
+            client.removeAssociatedResource(*this);
+#endif
         didRemoveClient(client);
     }
 

Modified: branches/safari-613.2.6.1-branch/Source/WebCore/loader/cache/CachedResource.h (292825 => 292826)


--- branches/safari-613.2.6.1-branch/Source/WebCore/loader/cache/CachedResource.h	2022-04-13 20:11:05 UTC (rev 292825)
+++ branches/safari-613.2.6.1-branch/Source/WebCore/loader/cache/CachedResource.h	2022-04-13 20:32:01 UTC (rev 292826)
@@ -310,6 +310,7 @@
 
 private:
     class Callback;
+    template<typename T> friend class CachedResourceClientWalker;
 
     void deleteThis();
 

Modified: branches/safari-613.2.6.1-branch/Source/WebCore/loader/cache/CachedResourceClient.h (292825 => 292826)


--- branches/safari-613.2.6.1-branch/Source/WebCore/loader/cache/CachedResourceClient.h	2022-04-13 20:11:05 UTC (rev 292825)
+++ branches/safari-613.2.6.1-branch/Source/WebCore/loader/cache/CachedResourceClient.h	2022-04-13 20:32:01 UTC (rev 292826)
@@ -21,8 +21,12 @@
     This class provides all functionality needed for loading images, style sheets and html
     pages from the web. It has a memory cache for these objects.
 */
+
 #pragma once
 
+#include <wtf/HashSet.h>
+#include <wtf/Noncopyable.h>
+
 namespace WebCore {
 
 class CachedResource;
@@ -29,6 +33,7 @@
 class NetworkLoadMetrics;
 
 class CachedResourceClient {
+    WTF_MAKE_NONCOPYABLE(CachedResourceClient);
 public:
     enum CachedResourceClientType {
         BaseResourceType,
@@ -39,7 +44,11 @@
         RawResourceType
     };
 
-    virtual ~CachedResourceClient() = default;
+    virtual ~CachedResourceClient()
+    {
+        ASSERT(m_associatedResources.isEmpty());
+    }
+
     virtual void notifyFinished(CachedResource&, const NetworkLoadMetrics&) { }
     virtual void deprecatedDidReceiveCachedResource(CachedResource&) { }
 
@@ -47,8 +56,25 @@
     virtual CachedResourceClientType resourceClientType() const { return expectedType(); }
     virtual bool shouldMarkAsReferenced() const { return true; }
 
+#if ASSERT_ENABLED
+    void addAssociatedResource(CachedResource& resource)
+    {
+        m_associatedResources.add(&resource);
+    }
+
+    void removeAssociatedResource(CachedResource& resource)
+    {
+        m_associatedResources.remove(&resource);
+    }
+#endif
+
 protected:
     CachedResourceClient() = default;
+
+private:
+#if ASSERT_ENABLED
+    HashSet<CachedResource*> m_associatedResources;
+#endif
 };
 
 }

Modified: branches/safari-613.2.6.1-branch/Source/WebCore/loader/cache/CachedResourceClientWalker.h (292825 => 292826)


--- branches/safari-613.2.6.1-branch/Source/WebCore/loader/cache/CachedResourceClientWalker.h	2022-04-13 20:11:05 UTC (rev 292825)
+++ branches/safari-613.2.6.1-branch/Source/WebCore/loader/cache/CachedResourceClientWalker.h	2022-04-13 20:32:01 UTC (rev 292826)
@@ -24,9 +24,11 @@
 
 #pragma once
 
+#include "CachedResource.h"
 #include "CachedResourceClient.h"
+#include "CachedResourceHandle.h"
+#include <wtf/FixedVector.h>
 #include <wtf/HashCountedSet.h>
-#include <wtf/Vector.h>
 
 namespace WebCore {
 
@@ -35,12 +37,12 @@
 template<typename T>
 class CachedResourceClientWalker {
 public:
-    CachedResourceClientWalker(const HashCountedSet<CachedResourceClient*>& clientSet)
-        : m_clientSet(clientSet)
-        , m_clientVector(clientSet.size())
+    CachedResourceClientWalker(const CachedResource& resource)
+        : m_resource(const_cast<CachedResource*>(&resource))
+        , m_clientVector(resource.m_clients.size())
     {
         size_t clientIndex = 0;
-        for (const auto& client : clientSet)
+        for (const auto& client : resource.m_clients)
             m_clientVector[clientIndex++] = client.key;
     }
 
@@ -49,7 +51,7 @@
         size_t size = m_clientVector.size();
         while (m_index < size) {
             CachedResourceClient* next = m_clientVector[m_index++];
-            if (m_clientSet.contains(next)) {
+            if (m_resource->m_clients.contains(next)) {
                 RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(T::expectedType() == CachedResourceClient::expectedType() || next->resourceClientType() == T::expectedType());
                 return static_cast<T*>(next);
             }
@@ -56,9 +58,10 @@
         }
         return nullptr;
     }
+
 private:
-    const HashCountedSet<CachedResourceClient*>& m_clientSet;
-    Vector<CachedResourceClient*> m_clientVector;
+    CachedResourceHandle<CachedResource> m_resource;
+    FixedVector<CachedResourceClient*> m_clientVector;
     size_t m_index { 0 };
 };
 

Modified: branches/safari-613.2.6.1-branch/Source/WebCore/loader/cache/CachedScript.cpp (292825 => 292826)


--- branches/safari-613.2.6.1-branch/Source/WebCore/loader/cache/CachedScript.cpp	2022-04-13 20:11:05 UTC (rev 292825)
+++ branches/safari-613.2.6.1-branch/Source/WebCore/loader/cache/CachedScript.cpp	2022-04-13 20:32:01 UTC (rev 292826)
@@ -28,7 +28,6 @@
 #include "CachedScript.h"
 
 #include "CachedResourceClient.h"
-#include "CachedResourceClientWalker.h"
 #include "CachedResourceRequest.h"
 #include "RuntimeApplicationChecks.h"
 #include "SharedBuffer.h"

Modified: branches/safari-613.2.6.1-branch/Source/WebCore/loader/cache/CachedTextTrack.cpp (292825 => 292826)


--- branches/safari-613.2.6.1-branch/Source/WebCore/loader/cache/CachedTextTrack.cpp	2022-04-13 20:11:05 UTC (rev 292825)
+++ branches/safari-613.2.6.1-branch/Source/WebCore/loader/cache/CachedTextTrack.cpp	2022-04-13 20:32:01 UTC (rev 292826)
@@ -46,7 +46,7 @@
     m_data = data ? data->makeContiguous() : RefPtr<SharedBuffer>();
     setEncodedSize(data ? data->size() : 0);
 
-    CachedResourceClientWalker<CachedResourceClient> walker(m_clients);
+    CachedResourceClientWalker<CachedResourceClient> walker(*this);
     while (CachedResourceClient* client = walker.next())
         client->deprecatedDidReceiveCachedResource(*this);
 }

Modified: branches/safari-613.2.6.1-branch/Source/WebCore/loader/cache/CachedXSLStyleSheet.cpp (292825 => 292826)


--- branches/safari-613.2.6.1-branch/Source/WebCore/loader/cache/CachedXSLStyleSheet.cpp	2022-04-13 20:11:05 UTC (rev 292825)
+++ branches/safari-613.2.6.1-branch/Source/WebCore/loader/cache/CachedXSLStyleSheet.cpp	2022-04-13 20:32:01 UTC (rev 292826)
@@ -81,8 +81,8 @@
     if (isLoading())
         return;
     
-    CachedResourceClientWalker<CachedStyleSheetClient> w(m_clients);
-    while (CachedStyleSheetClient* c = w.next())
+    CachedResourceClientWalker<CachedStyleSheetClient> walker(*this);
+    while (CachedStyleSheetClient* c = walker.next())
         c->setXSLStyleSheet(m_resourceRequest.url().string(), m_response.url(), m_sheet);
 }
 

Modified: branches/safari-613.2.6.1-branch/Source/WebCore/rendering/RenderObject.cpp (292825 => 292826)


--- branches/safari-613.2.6.1-branch/Source/WebCore/rendering/RenderObject.cpp	2022-04-13 20:11:05 UTC (rev 292825)
+++ branches/safari-613.2.6.1-branch/Source/WebCore/rendering/RenderObject.cpp	2022-04-13 20:32:01 UTC (rev 292826)
@@ -111,6 +111,7 @@
     virtual ~SameSizeAsRenderObject() = default; // Allocate vtable pointer.
 #if ASSERT_ENABLED
     bool weakPtrFactorWasConstructedOnMainThread;
+    HashSet<void*> cachedResourceClientAssociatedResources;
 #endif
     void* pointers[5];
 #if ASSERT_ENABLED
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to