<!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>[164009] trunk/Source/JavaScriptCore</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/164009">164009</a></dd>
<dt>Author</dt> <dd>mhahnenberg@apple.com</dd>
<dt>Date</dt> <dd>2014-02-12 20:24:41 -0800 (Wed, 12 Feb 2014)</dd>
</dl>
<h3>Log Message</h3>
<pre>DelayedReleaseScope in MarkedAllocator::tryAllocateHelper is wrong
https://bugs.webkit.org/show_bug.cgi?id=128641
Reviewed by Michael Saboff.
We were improperly handling the case where the DelayedReleaseScope
in tryAllocateHelper would cause us to drop the API lock, allowing
another thread to sneak in and allocate a new block after we had already
concluded that there were no more blocks to allocate out of.
The fix is to call tryAllocateHelper in a loop until we know for sure
that this did not happen.
There was also a race condition with the DelayedReleaseScope in addBlock.
We would add the block to the MarkedBlock's list, sweep it, and then return,
causing us to drop the API lock momentarily. Another thread could then
grab the lock, and allocate out of the new block to the point where the
free list was empty. Then we would return to the original thread, who thinks
it's impossible to not allocate successfully at this point.
Instead we should just let tryAllocate do all the hard work with correctly
sweeping and getting a valid result.
There was another race condition in didFinishIterating. We would call resumeAllocating,
which would create a DelayedReleaseScope. The DelayedReleaseScope would then release
API lock before we set m_isIterating back to false, which would potentially confuse
other threads.
* heap/MarkedAllocator.cpp:
(JSC::MarkedAllocator::tryAllocateHelper):
(JSC::MarkedAllocator::tryPopFreeList):
(JSC::MarkedAllocator::tryAllocate):
(JSC::MarkedAllocator::addBlock):
* heap/MarkedAllocator.h:</pre>
<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkSourceJavaScriptCoreChangeLog">trunk/Source/JavaScriptCore/ChangeLog</a></li>
<li><a href="#trunkSourceJavaScriptCoreheapMarkedAllocatorcpp">trunk/Source/JavaScriptCore/heap/MarkedAllocator.cpp</a></li>
<li><a href="#trunkSourceJavaScriptCoreheapMarkedAllocatorh">trunk/Source/JavaScriptCore/heap/MarkedAllocator.h</a></li>
<li><a href="#trunkSourceJavaScriptCoreheapMarkedSpacecpp">trunk/Source/JavaScriptCore/heap/MarkedSpace.cpp</a></li>
</ul>
</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkSourceJavaScriptCoreChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/ChangeLog (164008 => 164009)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/ChangeLog 2014-02-13 04:05:10 UTC (rev 164008)
+++ trunk/Source/JavaScriptCore/ChangeLog 2014-02-13 04:24:41 UTC (rev 164009)
</span><span class="lines">@@ -1,3 +1,39 @@
</span><ins>+2014-02-12 Mark Hahnenberg <mhahnenberg@apple.com>
+
+ DelayedReleaseScope in MarkedAllocator::tryAllocateHelper is wrong
+ https://bugs.webkit.org/show_bug.cgi?id=128641
+
+ Reviewed by Michael Saboff.
+
+ We were improperly handling the case where the DelayedReleaseScope
+ in tryAllocateHelper would cause us to drop the API lock, allowing
+ another thread to sneak in and allocate a new block after we had already
+ concluded that there were no more blocks to allocate out of.
+
+ The fix is to call tryAllocateHelper in a loop until we know for sure
+ that this did not happen.
+
+ There was also a race condition with the DelayedReleaseScope in addBlock.
+ We would add the block to the MarkedBlock's list, sweep it, and then return,
+ causing us to drop the API lock momentarily. Another thread could then
+ grab the lock, and allocate out of the new block to the point where the
+ free list was empty. Then we would return to the original thread, who thinks
+ it's impossible to not allocate successfully at this point.
+ Instead we should just let tryAllocate do all the hard work with correctly
+ sweeping and getting a valid result.
+
+ There was another race condition in didFinishIterating. We would call resumeAllocating,
+ which would create a DelayedReleaseScope. The DelayedReleaseScope would then release
+ API lock before we set m_isIterating back to false, which would potentially confuse
+ other threads.
+
+ * heap/MarkedAllocator.cpp:
+ (JSC::MarkedAllocator::tryAllocateHelper):
+ (JSC::MarkedAllocator::tryPopFreeList):
+ (JSC::MarkedAllocator::tryAllocate):
+ (JSC::MarkedAllocator::addBlock):
+ * heap/MarkedAllocator.h:
+
</ins><span class="cx"> 2014-02-12 Brian Burg <bburg@apple.com>
</span><span class="cx">
</span><span class="cx"> Web Replay: capture and replay nondeterminism of Date.now() and Math.random()
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoreheapMarkedAllocatorcpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/heap/MarkedAllocator.cpp (164008 => 164009)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/heap/MarkedAllocator.cpp 2014-02-13 04:05:10 UTC (rev 164008)
+++ trunk/Source/JavaScriptCore/heap/MarkedAllocator.cpp 2014-02-13 04:24:41 UTC (rev 164009)
</span><span class="lines">@@ -105,19 +105,42 @@
</span><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> ASSERT(m_freeList.head);
</span><del>- MarkedBlock::FreeCell* head = m_freeList.head;
- m_freeList.head = head->next;
</del><ins>+ void* head = tryPopFreeList(bytes);
</ins><span class="cx"> ASSERT(head);
</span><span class="cx"> m_markedSpace->didAllocateInBlock(m_currentBlock);
</span><span class="cx"> return head;
</span><span class="cx"> }
</span><del>-
</del><ins>+
+inline void* MarkedAllocator::tryPopFreeList(size_t bytes)
+{
+ ASSERT(m_currentBlock);
+ if (bytes > m_currentBlock->cellSize())
+ return 0;
+
+ MarkedBlock::FreeCell* head = m_freeList.head;
+ m_freeList.head = head->next;
+ return head;
+}
+
</ins><span class="cx"> inline void* MarkedAllocator::tryAllocate(size_t bytes)
</span><span class="cx"> {
</span><span class="cx"> ASSERT(!m_heap->isBusy());
</span><span class="cx"> m_heap->m_operationInProgress = Allocation;
</span><span class="cx"> void* result = tryAllocateHelper(bytes);
</span><ins>+
+ // Due to the DelayedReleaseScope in tryAllocateHelper, some other thread might have
+ // created a new block after we thought we didn't find any free cells.
+ while (!result && m_currentBlock) {
+ // A new block was added by another thread so try popping the free list.
+ result = tryPopFreeList(bytes);
+ if (result)
+ break;
+ // The free list was empty, so call tryAllocateHelper to do the normal sweeping stuff.
+ result = tryAllocateHelper(bytes);
+ }
+
</ins><span class="cx"> m_heap->m_operationInProgress = NoOperation;
</span><ins>+ ASSERT(result || !m_currentBlock);
</ins><span class="cx"> return result;
</span><span class="cx"> }
</span><span class="cx">
</span><span class="lines">@@ -171,14 +194,11 @@
</span><span class="cx">
</span><span class="cx"> void MarkedAllocator::addBlock(MarkedBlock* block)
</span><span class="cx"> {
</span><del>- // Satisfy the ASSERT in MarkedBlock::sweep.
- DelayedReleaseScope delayedReleaseScope(*m_markedSpace);
</del><span class="cx"> ASSERT(!m_currentBlock);
</span><span class="cx"> ASSERT(!m_freeList.head);
</span><span class="cx">
</span><span class="cx"> m_blockList.append(block);
</span><del>- m_nextBlockToSweep = m_currentBlock = block;
- m_freeList = block->sweep(MarkedBlock::SweepToFreeList);
</del><ins>+ m_nextBlockToSweep = block;
</ins><span class="cx"> m_markedSpace->didAddBlock(block);
</span><span class="cx"> }
</span><span class="cx">
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoreheapMarkedAllocatorh"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/heap/MarkedAllocator.h (164008 => 164009)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/heap/MarkedAllocator.h 2014-02-13 04:05:10 UTC (rev 164008)
+++ trunk/Source/JavaScriptCore/heap/MarkedAllocator.h 2014-02-13 04:24:41 UTC (rev 164009)
</span><span class="lines">@@ -47,6 +47,7 @@
</span><span class="cx"> JS_EXPORT_PRIVATE void* allocateSlowCase(size_t);
</span><span class="cx"> void* tryAllocate(size_t);
</span><span class="cx"> void* tryAllocateHelper(size_t);
</span><ins>+ void* tryPopFreeList(size_t);
</ins><span class="cx"> MarkedBlock* allocateBlock(size_t);
</span><span class="cx">
</span><span class="cx"> MarkedBlock::FreeList m_freeList;
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoreheapMarkedSpacecpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/heap/MarkedSpace.cpp (164008 => 164009)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/heap/MarkedSpace.cpp 2014-02-13 04:05:10 UTC (rev 164008)
+++ trunk/Source/JavaScriptCore/heap/MarkedSpace.cpp 2014-02-13 04:24:41 UTC (rev 164009)
</span><span class="lines">@@ -213,7 +213,6 @@
</span><span class="cx"> void MarkedSpace::resumeAllocating()
</span><span class="cx"> {
</span><span class="cx"> ASSERT(isIterating());
</span><del>- DelayedReleaseScope scope(*this);
</del><span class="cx"> forEachAllocator<ResumeAllocatingFunctor>();
</span><span class="cx"> }
</span><span class="cx">
</span><span class="lines">@@ -346,6 +345,7 @@
</span><span class="cx"> void MarkedSpace::didFinishIterating()
</span><span class="cx"> {
</span><span class="cx"> ASSERT(isIterating());
</span><ins>+ DelayedReleaseScope scope(*this);
</ins><span class="cx"> resumeAllocating();
</span><span class="cx"> m_isIterating = false;
</span><span class="cx"> }
</span></span></pre>
</div>
</div>
</body>
</html>