Infinite recursion via CachedResource::~CachedResource
authorantti@apple.com <antti@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 7 Feb 2019 16:07:03 +0000 (16:07 +0000)
committerantti@apple.com <antti@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 7 Feb 2019 16:07:03 +0000 (16:07 +0000)
https://bugs.webkit.org/show_bug.cgi?id=194378
<rdar://problem/42023295>

Reviewed by Daniel Bates.

I don't know the exact steps to trigger this but the mechanism seems clear.

1) An existing resource is removed from or replaced in CachedResourceLoader::m_documentResources map.
2) This decrements the handle count of resource and causes it be deleted.
3) CachedResource::~CachedResource calls m_owningCachedResourceLoader->removeCachedResource(*this). This only happens with
   resources that are "owned" by CachedResourceLoader which is a rare special case (used by image document and if memory cache is disabled).
4) CachedResourceLoader::removeCachedResource looks up the resource from the map which causes a temporary CachedResourceHandle to be created.
   This increments the handle count of the resource from 0 back to 1.
5) When the temporary dies, CachedResource::~CachedResource is called again and we cycle back to 3).

The fix here is simply to remove CachedResourceLoader::removeCachedResource call from ~CachedResource.
It is a leftover from when the map contained raw pointers instead of owning CachedResourceHandles.

Since m_documentResources map has a handle to the resource, the only way we are in the destructor is that the resource
has been removed from the map already (or is in process of being removed like in this crash). Any call that does anything
other than bail out is going to crash.

CachedResource::n_owningCachedResourceLoader member and CachedResourceLoader::removeCachedResource function only exist to
support this erranous call so they are removed as well.

* loader/ImageLoader.cpp:
(WebCore::ImageLoader::updateFromElement):
* loader/cache/CachedResource.cpp:
(WebCore::CachedResource::~CachedResource):

This is the substantive change. The rest just removes now-dead code.

* loader/cache/CachedResource.h:
(WebCore::CachedResource::setOwningCachedResourceLoader): Deleted.
* loader/cache/CachedResourceLoader.cpp:
(WebCore::CachedResourceLoader::~CachedResourceLoader):
(WebCore::CachedResourceLoader::requestUserCSSStyleSheet):
(WebCore::CachedResourceLoader::requestResource):
(WebCore::CachedResourceLoader::loadResource):
(WebCore::CachedResourceLoader::garbageCollectDocumentResources):
(WebCore::CachedResourceLoader::removeCachedResource): Deleted.
* loader/cache/CachedResourceLoader.h:

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

Source/WebCore/ChangeLog
Source/WebCore/loader/ImageLoader.cpp
Source/WebCore/loader/cache/CachedResource.cpp
Source/WebCore/loader/cache/CachedResource.h
Source/WebCore/loader/cache/CachedResourceLoader.cpp
Source/WebCore/loader/cache/CachedResourceLoader.h

index 9a5813d..385986c 100644 (file)
@@ -1,3 +1,49 @@
+2019-02-07  Antti Koivisto  <antti@apple.com>
+
+        Infinite recursion via CachedResource::~CachedResource
+        https://bugs.webkit.org/show_bug.cgi?id=194378
+        <rdar://problem/42023295>
+
+        Reviewed by Daniel Bates.
+
+        I don't know the exact steps to trigger this but the mechanism seems clear.
+
+        1) An existing resource is removed from or replaced in CachedResourceLoader::m_documentResources map.
+        2) This decrements the handle count of resource and causes it be deleted.
+        3) CachedResource::~CachedResource calls m_owningCachedResourceLoader->removeCachedResource(*this). This only happens with
+           resources that are "owned" by CachedResourceLoader which is a rare special case (used by image document and if memory cache is disabled).
+        4) CachedResourceLoader::removeCachedResource looks up the resource from the map which causes a temporary CachedResourceHandle to be created.
+           This increments the handle count of the resource from 0 back to 1.
+        5) When the temporary dies, CachedResource::~CachedResource is called again and we cycle back to 3).
+
+        The fix here is simply to remove CachedResourceLoader::removeCachedResource call from ~CachedResource.
+        It is a leftover from when the map contained raw pointers instead of owning CachedResourceHandles.
+
+        Since m_documentResources map has a handle to the resource, the only way we are in the destructor is that the resource
+        has been removed from the map already (or is in process of being removed like in this crash). Any call that does anything
+        other than bail out is going to crash.
+
+        CachedResource::n_owningCachedResourceLoader member and CachedResourceLoader::removeCachedResource function only exist to
+        support this erranous call so they are removed as well.
+
+        * loader/ImageLoader.cpp:
+        (WebCore::ImageLoader::updateFromElement):
+        * loader/cache/CachedResource.cpp:
+        (WebCore::CachedResource::~CachedResource):
+
+        This is the substantive change. The rest just removes now-dead code.
+
+        * loader/cache/CachedResource.h:
+        (WebCore::CachedResource::setOwningCachedResourceLoader): Deleted.
+        * loader/cache/CachedResourceLoader.cpp:
+        (WebCore::CachedResourceLoader::~CachedResourceLoader):
+        (WebCore::CachedResourceLoader::requestUserCSSStyleSheet):
+        (WebCore::CachedResourceLoader::requestResource):
+        (WebCore::CachedResourceLoader::loadResource):
+        (WebCore::CachedResourceLoader::garbageCollectDocumentResources):
+        (WebCore::CachedResourceLoader::removeCachedResource): Deleted.
+        * loader/cache/CachedResourceLoader.h:
+
 2019-02-07  Miguel Gomez  <magomez@igalia.com>
 
         [WPE] Implement GStreamer based holepunch
index 3f1ff32..03fbb31 100644 (file)
@@ -194,7 +194,6 @@ void ImageLoader::updateFromElement()
             newImage = new CachedImage(WTFMove(request), page->sessionID(), &page->cookieJar());
             newImage->setStatus(CachedResource::Pending);
             newImage->setLoading(true);
-            newImage->setOwningCachedResourceLoader(&document.cachedResourceLoader());
             document.cachedResourceLoader().m_documentResources.set(newImage->url(), newImage.get());
             document.cachedResourceLoader().setAutoLoadImages(autoLoadOtherImages);
         } else
index 025e555..d29e4ad 100644 (file)
@@ -174,9 +174,6 @@ CachedResource::~CachedResource()
     m_deleted = true;
     cachedResourceLeakCounter.decrement();
 #endif
-
-    if (m_owningCachedResourceLoader)
-        m_owningCachedResourceLoader->removeCachedResource(*this);
 }
 
 void CachedResource::failBeforeStarting()
index 9b83848..aa8530d 100644 (file)
@@ -234,8 +234,6 @@ public:
 
     virtual void destroyDecodedData() { }
 
-    void setOwningCachedResourceLoader(CachedResourceLoader* cachedResourceLoader) { m_owningCachedResourceLoader = cachedResourceLoader; }
-
     bool isPreloaded() const { return m_preloadCount; }
     void increasePreloadCount() { ++m_preloadCount; }
     void decreasePreloadCount() { ASSERT(m_preloadCount); --m_preloadCount; }
@@ -333,8 +331,6 @@ private:
 
     Vector<std::pair<String, String>> m_varyingHeaderValues;
 
-    CachedResourceLoader* m_owningCachedResourceLoader { nullptr }; // only non-null for resources that are not in the cache
-
     // If this field is non-null we are using the resource as a proxy for checking whether an existing resource is still up to date
     // using HTTP If-Modified-Since/If-None-Match headers. If the response is 304 all clients of this resource are moved
     // to to be clients of m_resourceToRevalidate and the resource is deleted. If not, the field is zeroed and this
index dcae23b..f426303 100644 (file)
@@ -161,8 +161,6 @@ CachedResourceLoader::~CachedResourceLoader()
     m_document = nullptr;
 
     clearPreloads(ClearPreloadsMode::ClearAllPreloads);
-    for (auto& resource : m_documentResources.values())
-        resource->setOwningCachedResourceLoader(nullptr);
 
     // Make sure no requests still point to this CachedResourceLoader
     ASSERT(m_requestCount == 0);
@@ -258,7 +256,6 @@ CachedResourceHandle<CachedCSSStyleSheet> CachedResourceLoader::requestUserCSSSt
 
     if (userSheet->allowsCaching())
         memoryCache.add(*userSheet);
-    // FIXME: loadResource calls setOwningCachedResourceLoader() if the resource couldn't be added to cache. Does this function need to call it, too?
 
     userSheet->load(*this);
     return userSheet;
@@ -861,10 +858,8 @@ ResourceErrorOr<CachedResourceHandle<CachedResource>> CachedResourceLoader::requ
         updateHTTPRequestHeaders(type, request);
 
     auto& memoryCache = MemoryCache::singleton();
-    if (request.allowsCaching() && memoryCache.disabled()) {
-        if (auto handle = m_documentResources.take(url.string()))
-            handle->setOwningCachedResourceLoader(nullptr);
-    }
+    if (request.allowsCaching() && memoryCache.disabled())
+        m_documentResources.remove(url.string());
 
     // See if we can use an existing resource from the cache.
     CachedResourceHandle<CachedResource> resource;
@@ -1013,8 +1008,8 @@ CachedResourceHandle<CachedResource> CachedResourceLoader::loadResource(CachedRe
 
     CachedResourceHandle<CachedResource> resource = createResource(type, WTFMove(request), sessionID(), cookieJar);
 
-    if (resource->allowsCaching() && !memoryCache.add(*resource))
-        resource->setOwningCachedResourceLoader(this);
+    if (resource->allowsCaching())
+        memoryCache.add(*resource);
 
     if (RuntimeEnabledFeatures::sharedFeatures().resourceTimingEnabled())
         m_resourceTimingInfo.storeResourceTimingInitiatorInformation(resource, resource->initiatorName(), frame());
@@ -1302,13 +1297,6 @@ CachePolicy CachedResourceLoader::cachePolicy(CachedResource::Type type, const U
     }
 }
 
-void CachedResourceLoader::removeCachedResource(CachedResource& resource)
-{
-    if (m_documentResources.contains(resource.url()) && m_documentResources.get(resource.url()).get() != &resource)
-        return;
-    m_documentResources.remove(resource.url());
-}
-
 void CachedResourceLoader::loadDone(LoadCompletionType type, bool shouldPerformPostLoadActions)
 {
     RefPtr<DocumentLoader> protectDocumentLoader(m_documentLoader);
@@ -1342,10 +1330,8 @@ void CachedResourceLoader::garbageCollectDocumentResources()
     for (auto& resource : m_documentResources) {
         LOG(ResourceLoading, "  cached resource %p - hasOneHandle %d", resource.value.get(), resource.value->hasOneHandle());
 
-        if (resource.value->hasOneHandle()) {
+        if (resource.value->hasOneHandle())
             resourcesToDelete.append(resource.key);
-            resource.value->setOwningCachedResourceLoader(nullptr);
-        }
     }
 
     for (auto& resource : resourcesToDelete)
index daac861..d5248cc 100644 (file)
@@ -130,8 +130,6 @@ public:
     void clearDocumentLoader() { m_documentLoader = nullptr; }
     PAL::SessionID sessionID() const;
 
-    void removeCachedResource(CachedResource&);
-
     void loadDone(LoadCompletionType, bool shouldPerformPostLoadActions = true);
 
     WEBCORE_EXPORT void garbageCollectDocumentResources();