Attachment 361387 Details for Bug 194378 – patch

[patch] patch

no-removeCachedResource.patch (text/plain), 9.21 KB, created by Antti Koivisto on 2019-02-07 02:35:12 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Antti Koivisto

Created: 2019-02-07 02:35:12 PST

Size: 9.21 KB

patch

obsolete

>Index: Source/WebCore/ChangeLog
>===================================================================
>--- Source/WebCore/ChangeLog	(revision 241118)
>+++ Source/WebCore/ChangeLog	(working copy)
>@@ -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 NOBODY (OOPS!).
>+
>+        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-06  Benjamin Poulain  <benjamin@webkit.org>
> 
>         Unreviewed, rolling out r240759 and r240944.
>Index: Source/WebCore/loader/ImageLoader.cpp
>===================================================================
>--- Source/WebCore/loader/ImageLoader.cpp	(revision 241114)
>+++ Source/WebCore/loader/ImageLoader.cpp	(working copy)
>@@ -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: Source/WebCore/loader/cache/CachedResource.cpp
>===================================================================
>--- Source/WebCore/loader/cache/CachedResource.cpp	(revision 241114)
>+++ Source/WebCore/loader/cache/CachedResource.cpp	(working copy)
>@@ -174,9 +174,6 @@ CachedResource::~CachedResource()
>     m_deleted = true;
>     cachedResourceLeakCounter.decrement();
> #endif
>-
>-    if (m_owningCachedResourceLoader)
>-        m_owningCachedResourceLoader->removeCachedResource(*this);
> }
> 
> void CachedResource::failBeforeStarting()
>Index: Source/WebCore/loader/cache/CachedResource.h
>===================================================================
>--- Source/WebCore/loader/cache/CachedResource.h	(revision 241114)
>+++ Source/WebCore/loader/cache/CachedResource.h	(working copy)
>@@ -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: Source/WebCore/loader/cache/CachedResourceLoader.cpp
>===================================================================
>--- Source/WebCore/loader/cache/CachedResourceLoader.cpp	(revision 241114)
>+++ Source/WebCore/loader/cache/CachedResourceLoader.cpp	(working copy)
>@@ -161,8 +161,6 @@ CachedResourceLoader::~CachedResourceLoa
>     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
> 
>     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<Cac
>         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> Cac
> 
>     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::cacheP
>     }
> }
> 
>-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::garbageCollec
>     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: Source/WebCore/loader/cache/CachedResourceLoader.h
>===================================================================
>--- Source/WebCore/loader/cache/CachedResourceLoader.h	(revision 241114)
>+++ Source/WebCore/loader/cache/CachedResourceLoader.h	(working copy)
>@@ -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();

Actions: View | Formatted Diff | Diff

Attachments on bug 194378: 361387