<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
"http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head><meta http-equiv="content-type" content="text/html; charset=utf-8" />
<title>[193635] trunk/Source/WebCore</title>
</head>
<body>
<style type="text/css"><!--
#msg dl.meta { border: 1px #006 solid; background: #369; padding: 6px; color: #fff; }
#msg dl.meta dt { float: left; width: 6em; font-weight: bold; }
#msg dt:after { content:':';}
#msg dl, #msg dt, #msg ul, #msg li, #header, #footer, #logmsg { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; }
#msg dl a { font-weight: bold}
#msg dl a:link { color:#fc3; }
#msg dl a:active { color:#ff0; }
#msg dl a:visited { color:#cc6; }
h3 { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; font-weight: bold; }
#msg pre { overflow: auto; background: #ffc; border: 1px #fa0 solid; padding: 6px; }
#logmsg { background: #ffc; border: 1px #fa0 solid; padding: 1em 1em 0 1em; }
#logmsg p, #logmsg pre, #logmsg blockquote { margin: 0 0 1em 0; }
#logmsg p, #logmsg li, #logmsg dt, #logmsg dd { line-height: 14pt; }
#logmsg h1, #logmsg h2, #logmsg h3, #logmsg h4, #logmsg h5, #logmsg h6 { margin: .5em 0; }
#logmsg h1:first-child, #logmsg h2:first-child, #logmsg h3:first-child, #logmsg h4:first-child, #logmsg h5:first-child, #logmsg h6:first-child { margin-top: 0; }
#logmsg ul, #logmsg ol { padding: 0; list-style-position: inside; margin: 0 0 0 1em; }
#logmsg ul { text-indent: -1em; padding-left: 1em; }#logmsg ol { text-indent: -1.5em; padding-left: 1.5em; }
#logmsg > ul, #logmsg > ol { margin: 0 0 1em 0; }
#logmsg pre { background: #eee; padding: 1em; }
#logmsg blockquote { border: 1px solid #fa0; border-left-width: 10px; padding: 1em 1em 0 1em; background: white;}
#logmsg dl { margin: 0; }
#logmsg dt { font-weight: bold; }
#logmsg dd { margin: 0; padding: 0 0 0.5em 0; }
#logmsg dd:before { content:'\00bb';}
#logmsg table { border-spacing: 0px; border-collapse: collapse; border-top: 4px solid #fa0; border-bottom: 1px solid #fa0; background: #fff; }
#logmsg table th { text-align: left; font-weight: normal; padding: 0.2em 0.5em; border-top: 1px dotted #fa0; }
#logmsg table td { text-align: right; border-top: 1px dotted #fa0; padding: 0.2em 0.5em; }
#logmsg table thead th { text-align: center; border-bottom: 1px solid #fa0; }
#logmsg table th.Corner { text-align: left; }
#logmsg hr { border: none 0; border-top: 2px dashed #fa0; height: 1px; }
#header, #footer { color: #fff; background: #636; border: 1px #300 solid; padding: 6px; }
#patch { width: 100%; }
#patch h4 {font-family: verdana,arial,helvetica,sans-serif;font-size:10pt;padding:8px;background:#369;color:#fff;margin:0;}
#patch .propset h4, #patch .binary h4 {margin:0;}
#patch pre {padding:0;line-height:1.2em;margin:0;}
#patch .diff {width:100%;background:#eee;padding: 0 0 10px 0;overflow:auto;}
#patch .propset .diff, #patch .binary .diff {padding:10px 0;}
#patch span {display:block;padding:0 10px;}
#patch .modfile, #patch .addfile, #patch .delfile, #patch .propset, #patch .binary, #patch .copfile {border:1px solid #ccc;margin:10px 0;}
#patch ins {background:#dfd;text-decoration:none;display:block;padding:0 10px;}
#patch del {background:#fdd;text-decoration:none;display:block;padding:0 10px;}
#patch .lines, .info {color:#888;background:#fff;}
--></style>
<div id="msg">
<dl class="meta">
<dt>Revision</dt> <dd><a href="http://trac.webkit.org/projects/webkit/changeset/193635">193635</a></dd>
<dt>Author</dt> <dd>cdumez@apple.com</dd>
<dt>Date</dt> <dd>2015-12-07 10:15:09 -0800 (Mon, 07 Dec 2015)</dd>
</dl>
<h3>Log Message</h3>
<pre>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):</pre>
<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkSourceWebCoreChangeLog">trunk/Source/WebCore/ChangeLog</a></li>
<li><a href="#trunkSourceWebCoreloadercacheMemoryCachecpp">trunk/Source/WebCore/loader/cache/MemoryCache.cpp</a></li>
</ul>
</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkSourceWebCoreChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/ChangeLog (193634 => 193635)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebCore/ChangeLog 2015-12-07 18:02:14 UTC (rev 193634)
+++ trunk/Source/WebCore/ChangeLog 2015-12-07 18:15:09 UTC (rev 193635)
</span><span class="lines">@@ -1,3 +1,38 @@
</span><ins>+2015-12-07 Chris Dumez <cdumez@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):
+
</ins><span class="cx"> 2015-12-07 Brady Eidson <beidson@apple.com>
</span><span class="cx">
</span><span class="cx"> Modern IDB: Add some more custom exception messages, passing some more tests..
</span></span></pre></div>
<a id="trunkSourceWebCoreloadercacheMemoryCachecpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/loader/cache/MemoryCache.cpp (193634 => 193635)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebCore/loader/cache/MemoryCache.cpp 2015-12-07 18:02:14 UTC (rev 193634)
+++ trunk/Source/WebCore/loader/cache/MemoryCache.cpp 2015-12-07 18:15:09 UTC (rev 193635)
</span><span class="lines">@@ -348,55 +348,39 @@
</span><span class="cx">
</span><span class="cx"> bool canShrinkLRULists = true;
</span><span class="cx"> for (int i = m_allResources.size() - 1; i >= 0; i--) {
</span><del>- LRUList& list = *m_allResources[i];
</del><ins>+ // 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);
</ins><span class="cx">
</span><span class="cx"> // First flush all the decoded data in this queue.
</span><span class="cx"> // Remove from the head, since this is the least frequently accessed of the objects.
</span><del>- auto it = list.begin();
- while (it != list.end()) {
- CachedResource& current = **it;
</del><ins>+ for (auto& resource : lruList) {
+ if (!resource->inCache())
+ continue;
</ins><span class="cx">
</span><del>- // 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()) {
</del><ins>+ if (!resource->hasClients() && !resource->isPreloaded() && resource->isLoaded()) {
</ins><span class="cx"> // Destroy our decoded data. This will remove us from
</span><span class="cx"> // m_liveDecodedResources, and possibly move us to a different
</span><span class="cx"> // LRU list in m_allResources.
</span><del>- current.destroyDecodedData();
</del><ins>+ resource->destroyDecodedData();
</ins><span class="cx">
</span><span class="cx"> if (targetSize && m_deadSize <= targetSize)
</span><span class="cx"> return;
</span><span class="cx"> }
</span><del>- // Decoded data may reference other resources. Stop iterating if 'next' somehow got
- // kicked out of cache during destroyDecodedData().
- if (next && !next->inCache())
- break;
</del><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> // Now evict objects from this list.
</span><span class="cx"> // Remove from the head, since this is the least frequently accessed of the objects.
</span><del>- it = list.begin();
- while (it != list.end()) {
- CachedResource& current = **it;
</del><ins>+ for (auto& resource : lruList) {
+ if (!resource->inCache())
+ continue;
</ins><span class="cx">
</span><del>- // 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);
</del><ins>+ if (!resource->hasClients() && !resource->isPreloaded() && !resource->isCacheValidator()) {
+ remove(*resource);
</ins><span class="cx"> if (targetSize && m_deadSize <= targetSize)
</span><span class="cx"> return;
</span><span class="cx"> }
</span><del>- if (next && !next->inCache())
- break;
</del><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> // Shrink the vector back down so we don't waste time inspecting
</span></span></pre>
</div>
</div>
</body>
</html>