Title: [193690] branches/safari-601.1.46-branch/Source/WebCore
- Revision
- 193690
- Author
- ddkil...@apple.com
- Date
- 2015-12-07 21:05:32 -0800 (Mon, 07 Dec 2015)
Log Message
Merge r193635. rdar://problem/23785592
Modified Paths
Diff
Modified: branches/safari-601.1.46-branch/Source/WebCore/ChangeLog (193689 => 193690)
--- branches/safari-601.1.46-branch/Source/WebCore/ChangeLog 2015-12-08 04:59:08 UTC (rev 193689)
+++ branches/safari-601.1.46-branch/Source/WebCore/ChangeLog 2015-12-08 05:05:32 UTC (rev 193690)
@@ -1,3 +1,42 @@
+2015-12-07 David Kilzer <ddkil...@apple.com>
+
+ Merge r193635. rdar://problem/23785592
+
+ 2015-12-07 Chris Dumez <cdu...@apple.com>
+
+ Crash in MemoryCache::pruneDeadResourcesToSize()
+ https://bugs.webkit.org/show_bug.cgi?id=151833
+ <rdar://problem/22392235>
+
+ Reviewed by David Kilzer.
+
+ MemoryCache::pruneDeadResourcesToSize() is iterating over m_allResources
+ (which is a vector of LRUList). It first destroys decoded data for each
+ resource in the LRUList. Then, if it does not suffice to reach the
+ target size, and starts actually removing resources from the cache.
+
+ The issue is that this code alters m_allResources (and its LRULists) as
+ it is iterating over it. We tried to deal with this in various ways:
+ 1. Increment the iterator before removing the resource pointed by the
+ iterator.
+ 2. Protect the next resource in the LRUList and abort early if it is no
+ longer in the cache.
+
+ This adds code complexity and apparently does not correctly handle all
+ the edge cases as we still see crashes in this code. In particular, I
+ suspect that 2. may not be sufficient if it is possible for the next
+ resource to be moved to another LRUList (in which case, next->inCache()
+ would still return true but the iterator would however become invalid).
+
+ To make the code simpler and more robust, this patch copies the LRUList
+ (and refs the CachedResources) before iterating over it. This is a lot
+ safer and should hopefully fix the crashes we see in this function.
+
+ No new tests, no reproduction case.
+
+ * loader/cache/MemoryCache.cpp:
+ (WebCore::MemoryCache::pruneDeadResourcesToSize):
+
2015-12-06 Babak Shafiei <bshaf...@apple.com>
Merge r193599.
Modified: branches/safari-601.1.46-branch/Source/WebCore/loader/cache/MemoryCache.cpp (193689 => 193690)
--- branches/safari-601.1.46-branch/Source/WebCore/loader/cache/MemoryCache.cpp 2015-12-08 04:59:08 UTC (rev 193689)
+++ branches/safari-601.1.46-branch/Source/WebCore/loader/cache/MemoryCache.cpp 2015-12-08 05:05:32 UTC (rev 193690)
@@ -348,55 +348,39 @@
bool canShrinkLRULists = true;
for (int i = m_allResources.size() - 1; i >= 0; i--) {
- LRUList& list = *m_allResources[i];
+ // Make a copy of the LRUList first (and ref the resources) as calling
+ // destroyDecodedData() can alter the LRUList.
+ Vector<CachedResourceHandle<CachedResource>> lruList;
+ copyToVector(*m_allResources[i], lruList);
// First flush all the decoded data in this queue.
// Remove from the head, since this is the least frequently accessed of the objects.
- auto it = list.begin();
- while (it != list.end()) {
- CachedResource& current = **it;
+ for (auto& resource : lruList) {
+ if (!resource->inCache())
+ continue;
- // Increment the iterator now as the call to destroyDecodedData() below may
- // invalidate the current iterator.
- ++it;
-
- // Protect 'next' so it can't get deleted during destroyDecodedData().
- CachedResourceHandle<CachedResource> next = it != list.end() ? *it : nullptr;
- ASSERT(!next || next->inCache());
- if (!current.hasClients() && !current.isPreloaded() && current.isLoaded()) {
+ if (!resource->hasClients() && !resource->isPreloaded() && resource->isLoaded()) {
// Destroy our decoded data. This will remove us from
// m_liveDecodedResources, and possibly move us to a different
// LRU list in m_allResources.
- current.destroyDecodedData();
+ resource->destroyDecodedData();
if (targetSize && m_deadSize <= targetSize)
return;
}
- // Decoded data may reference other resources. Stop iterating if 'next' somehow got
- // kicked out of cache during destroyDecodedData().
- if (next && !next->inCache())
- break;
}
// Now evict objects from this list.
// Remove from the head, since this is the least frequently accessed of the objects.
- it = list.begin();
- while (it != list.end()) {
- CachedResource& current = **it;
+ for (auto& resource : lruList) {
+ if (!resource->inCache())
+ continue;
- // Increment the iterator now as the call to remove() below will
- // invalidate the current iterator.
- ++it;
-
- CachedResourceHandle<CachedResource> next = it != list.end() ? *it : nullptr;
- ASSERT(!next || next->inCache());
- if (!current.hasClients() && !current.isPreloaded() && !current.isCacheValidator()) {
- remove(current);
+ if (!resource->hasClients() && !resource->isPreloaded() && !resource->isCacheValidator()) {
+ remove(*resource);
if (targetSize && m_deadSize <= targetSize)
return;
}
- if (next && !next->inCache())
- break;
}
// Shrink the vector back down so we don't waste time inspecting
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes
