- Revision
- 241121
- Author
- [email protected]
- Date
- 2019-02-07 08:07:03 -0800 (Thu, 07 Feb 2019)
Log Message
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:
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (241120 => 241121)
--- trunk/Source/WebCore/ChangeLog 2019-02-07 14:42:32 UTC (rev 241120)
+++ trunk/Source/WebCore/ChangeLog 2019-02-07 16:07:03 UTC (rev 241121)
@@ -1,3 +1,49 @@
+2019-02-07 Antti Koivisto <[email protected]>
+
+ 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 <[email protected]>
[WPE] Implement GStreamer based holepunch
Modified: trunk/Source/WebCore/loader/ImageLoader.cpp (241120 => 241121)
--- trunk/Source/WebCore/loader/ImageLoader.cpp 2019-02-07 14:42:32 UTC (rev 241120)
+++ trunk/Source/WebCore/loader/ImageLoader.cpp 2019-02-07 16:07:03 UTC (rev 241121)
@@ -194,7 +194,6 @@
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
Modified: trunk/Source/WebCore/loader/cache/CachedResource.cpp (241120 => 241121)
--- trunk/Source/WebCore/loader/cache/CachedResource.cpp 2019-02-07 14:42:32 UTC (rev 241120)
+++ trunk/Source/WebCore/loader/cache/CachedResource.cpp 2019-02-07 16:07:03 UTC (rev 241121)
@@ -174,9 +174,6 @@
m_deleted = true;
cachedResourceLeakCounter.decrement();
#endif
-
- if (m_owningCachedResourceLoader)
- m_owningCachedResourceLoader->removeCachedResource(*this);
}
void CachedResource::failBeforeStarting()
Modified: trunk/Source/WebCore/loader/cache/CachedResource.h (241120 => 241121)
--- trunk/Source/WebCore/loader/cache/CachedResource.h 2019-02-07 14:42:32 UTC (rev 241120)
+++ trunk/Source/WebCore/loader/cache/CachedResource.h 2019-02-07 16:07:03 UTC (rev 241121)
@@ -234,8 +234,6 @@
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 @@
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
Modified: trunk/Source/WebCore/loader/cache/CachedResourceLoader.cpp (241120 => 241121)
--- trunk/Source/WebCore/loader/cache/CachedResourceLoader.cpp 2019-02-07 14:42:32 UTC (rev 241120)
+++ trunk/Source/WebCore/loader/cache/CachedResourceLoader.cpp 2019-02-07 16:07:03 UTC (rev 241121)
@@ -161,8 +161,6 @@
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 @@
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 @@
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> 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 @@
}
}
-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 @@
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)
Modified: trunk/Source/WebCore/loader/cache/CachedResourceLoader.h (241120 => 241121)
--- trunk/Source/WebCore/loader/cache/CachedResourceLoader.h 2019-02-07 14:42:32 UTC (rev 241120)
+++ trunk/Source/WebCore/loader/cache/CachedResourceLoader.h 2019-02-07 16:07:03 UTC (rev 241121)
@@ -130,8 +130,6 @@
void clearDocumentLoader() { m_documentLoader = nullptr; }
PAL::SessionID sessionID() const;
- void removeCachedResource(CachedResource&);
-
void loadDone(LoadCompletionType, bool shouldPerformPostLoadActions = true);
WEBCORE_EXPORT void garbageCollectDocumentResources();
