Regression(266170@main) Crash under CachedRawResource::switchClientsT…

…oRevalidatedResource()

https://bugs.webkit.org/show_bug.cgi?id=259401
rdar://112663008

Reviewed by Brent Fulgham.

In 266170@main, I updated MemoryCache::revalidationSucceeded() to deal with the fact
that there may already be a resource in the MemoryCache with the same URL when the
resource revalidation finishes. I dealt with this by assuming that the resource
already in the cache was good enough and I ignored the revalidated resource, and then
transferred the clients of the revalidation resource to the resource that is already
in the cache.

However, it turns out that this isn't safe to do because the resource already in the
cache may have a different type (even though it has the same URL). In the included
test, for example, the same URL is loaded both as a RawResource and a FontResource.
This would lead to crashes later on in switchClientsToRevalidatedResource().

To address the issue, I am reverting 266170@main and dealing with the original issue
in a simpler way. Upon successful revalidation, if there is already a resource in the
memory cache with the same URL, we now remove it from the cache before proceeding.

This is much safer and matches what would happen if you tried to load URL1 as a
RawResource and then load URL2 as a FontResource. Our MemoryCache logic would remove
the existing RawResource in the cache and create a new CachedResource of FontResource
type.

* LayoutTests/http/tests/workers/memory-cache-crash2.html: Added.
* LayoutTests/http/tests/workers/memory-cache-crash2-expected.txt: Added.
* Source/WebCore/loader/cache/CachedResource.cpp:
(WebCore::CachedResource::replaceResourceToRevalidate): Deleted.
* Source/WebCore/loader/cache/CachedResource.h:
* Source/WebCore/loader/cache/MemoryCache.cpp:
(WebCore::MemoryCache::revalidationSucceeded):

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

LayoutTests/http/tests/workers/memory-cache-crash2-expected.txt

@@ -0,0 +1 @@
+This test passes if it doesn't crash.

LayoutTests/http/tests/workers/memory-cache-crash2.html

@@ -0,0 +1,20 @@
+<p>This test passes if it doesn't crash.</p>
+<script>
+  if (window.testRunner) {
+    testRunner.dumpAsText();
+    testRunner.waitUntilDone();
+  }
+
+  onload = async () => {
+    document.createElement('audio').src = 'data:';
+    for (let i = 0; i < 5; i++) {
+      new Worker('');
+      await navigator.storage.persist();
+    }
+    new FontFace('a', 'url()');
+    setTimeout(() => {
+      if (window.testRunner)
+        testRunner.notifyDone();
+    }, 50);
+  };
+</script>

Source/WebCore/loader/cache/CachedResource.cpp

@@ -755,17 +755,6 @@ void CachedResource::setResourceToRevalidate(CachedResource* resource)
     m_resourceToRevalidate = resource;
 }
 
-void CachedResource::replaceResourceToRevalidate(CachedResource& resource)
-{
-    ASSERT(m_resourceToRevalidate);
-
-    m_resourceToRevalidate->m_proxyResource = nullptr;
-    m_resourceToRevalidate->deleteIfPossible();
-    m_resourceToRevalidate = &resource;
-    ASSERT(!resource.m_proxyResource);
-    resource.m_proxyResource = this;
-}
-
 void CachedResource::clearResourceToRevalidate() 
 {
     ASSERT(m_resourceToRevalidate);

Source/WebCore/loader/cache/CachedResource.h

@@ -282,7 +282,6 @@ class CachedResource : public CanMakeWeakPtr<CachedResource> {
 
     // HTTP revalidation support methods for CachedResourceLoader.
     void setResourceToRevalidate(CachedResource*);
-    void replaceResourceToRevalidate(CachedResource&);
     virtual void switchClientsToRevalidatedResource();
     void clearResourceToRevalidate();
     void updateResponseAfterRevalidation(const ResourceResponse& validatingResponse);

Source/WebCore/loader/cache/MemoryCache.cpp

@@ -146,9 +146,9 @@ void MemoryCache::revalidationSucceeded(CachedResource& revalidatingResource, co
     ASSERT(response.source() == ResourceResponse::Source::MemoryCacheAfterValidation);
     ASSERT(revalidatingResource.resourceToRevalidate());
     CachedResourceHandle protectedRevalidatingResource { revalidatingResource };
-    auto* resource = revalidatingResource.resourceToRevalidate();
-    ASSERT(!resource->inCache());
-    ASSERT(resource->isLoaded());
+    auto& resource = *revalidatingResource.resourceToRevalidate();
+    ASSERT(!resource.inCache());
+    ASSERT(resource.isLoaded());
 
     // Calling remove() can potentially delete revalidatingResource, which we use
     // below. This mustn't be the case since revalidation means it is loaded
@@ -157,28 +157,29 @@ void MemoryCache::revalidationSucceeded(CachedResource& revalidatingResource, co
 
     remove(revalidatingResource);
 
-    auto& resources = ensureSessionResourceMap(resource->sessionID());
-    std::pair key { resource->url(), resource->cachePartition() };
-
-    auto addResult = resources.add(key, resource);
-    if (addResult.isNewEntry) {
-        resource->setInCache(true);
-        resource->updateResponseAfterRevalidation(response);
-        insertInLRUList(*resource);
-        long long delta = resource->size();
-        if (resource->decodedSize() && resource->hasClients())
-            insertInLiveDecodedResourcesList(*resource);
-        if (delta)
-            adjustSize(resource->hasClients(), delta);
-    } else {
-        // The resource was re-inserted in the cache during its revalidation.
-        // Ignore the revalidated resource and switch clients to the one that
-        // is already in the cache.
-        resource = addResult.iterator->value.get();
-        ASSERT(resource);
-        revalidatingResource.replaceResourceToRevalidate(*resource);
+    // A resource with the same URL may have been added back in the cache during revalidation.
+    // In this case, we remove the cached resource and replace it with our freshly revalidated
+    // one.
+    std::pair key { resource.url(), resource.cachePartition() };
+    if (auto* existingResources = sessionResourceMap(resource.sessionID())) {
+        if (auto existingResource = existingResources->get(key))
+            remove(*existingResource);
     }
 
+    // Don't move the call to ensureSessionResourceMap() in this function as the calls to
+    // remove() above could invalidate the reference returned by ensureSessionResourceMap().
+    auto& resources = ensureSessionResourceMap(resource.sessionID());
+    ASSERT(!resources.contains(key));
+    resources.add(key, &resource);
+    resource.setInCache(true);
+    resource.updateResponseAfterRevalidation(response);
+    insertInLRUList(resource);
+    long long delta = resource.size();
+    if (resource.decodedSize() && resource.hasClients())
+        insertInLiveDecodedResourcesList(resource);
+    if (delta)
+        adjustSize(resource.hasClients(), delta);
+
     revalidatingResource.switchClientsToRevalidatedResource();
     ASSERT(!revalidatingResource.m_deleted);
     // This deletes the revalidating resource.