Title: [164009] trunk/Source/_javascript_Core
- Revision
- 164009
- Author
- [email protected]
- Date
- 2014-02-12 20:24:41 -0800 (Wed, 12 Feb 2014)
Log Message
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:
Modified Paths
Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (164008 => 164009)
--- trunk/Source/_javascript_Core/ChangeLog 2014-02-13 04:05:10 UTC (rev 164008)
+++ trunk/Source/_javascript_Core/ChangeLog 2014-02-13 04:24:41 UTC (rev 164009)
@@ -1,3 +1,39 @@
+2014-02-12 Mark Hahnenberg <[email protected]>
+
+ 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:
+
2014-02-12 Brian Burg <[email protected]>
Web Replay: capture and replay nondeterminism of Date.now() and Math.random()
Modified: trunk/Source/_javascript_Core/heap/MarkedAllocator.cpp (164008 => 164009)
--- trunk/Source/_javascript_Core/heap/MarkedAllocator.cpp 2014-02-13 04:05:10 UTC (rev 164008)
+++ trunk/Source/_javascript_Core/heap/MarkedAllocator.cpp 2014-02-13 04:24:41 UTC (rev 164009)
@@ -105,19 +105,42 @@
}
ASSERT(m_freeList.head);
- MarkedBlock::FreeCell* head = m_freeList.head;
- m_freeList.head = head->next;
+ void* head = tryPopFreeList(bytes);
ASSERT(head);
m_markedSpace->didAllocateInBlock(m_currentBlock);
return head;
}
-
+
+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;
+}
+
inline void* MarkedAllocator::tryAllocate(size_t bytes)
{
ASSERT(!m_heap->isBusy());
m_heap->m_operationInProgress = Allocation;
void* result = tryAllocateHelper(bytes);
+
+ // 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);
+ }
+
m_heap->m_operationInProgress = NoOperation;
+ ASSERT(result || !m_currentBlock);
return result;
}
@@ -171,14 +194,11 @@
void MarkedAllocator::addBlock(MarkedBlock* block)
{
- // Satisfy the ASSERT in MarkedBlock::sweep.
- DelayedReleaseScope delayedReleaseScope(*m_markedSpace);
ASSERT(!m_currentBlock);
ASSERT(!m_freeList.head);
m_blockList.append(block);
- m_nextBlockToSweep = m_currentBlock = block;
- m_freeList = block->sweep(MarkedBlock::SweepToFreeList);
+ m_nextBlockToSweep = block;
m_markedSpace->didAddBlock(block);
}
Modified: trunk/Source/_javascript_Core/heap/MarkedAllocator.h (164008 => 164009)
--- trunk/Source/_javascript_Core/heap/MarkedAllocator.h 2014-02-13 04:05:10 UTC (rev 164008)
+++ trunk/Source/_javascript_Core/heap/MarkedAllocator.h 2014-02-13 04:24:41 UTC (rev 164009)
@@ -47,6 +47,7 @@
JS_EXPORT_PRIVATE void* allocateSlowCase(size_t);
void* tryAllocate(size_t);
void* tryAllocateHelper(size_t);
+ void* tryPopFreeList(size_t);
MarkedBlock* allocateBlock(size_t);
MarkedBlock::FreeList m_freeList;
Modified: trunk/Source/_javascript_Core/heap/MarkedSpace.cpp (164008 => 164009)
--- trunk/Source/_javascript_Core/heap/MarkedSpace.cpp 2014-02-13 04:05:10 UTC (rev 164008)
+++ trunk/Source/_javascript_Core/heap/MarkedSpace.cpp 2014-02-13 04:24:41 UTC (rev 164009)
@@ -213,7 +213,6 @@
void MarkedSpace::resumeAllocating()
{
ASSERT(isIterating());
- DelayedReleaseScope scope(*this);
forEachAllocator<ResumeAllocatingFunctor>();
}
@@ -346,6 +345,7 @@
void MarkedSpace::didFinishIterating()
{
ASSERT(isIterating());
+ DelayedReleaseScope scope(*this);
resumeAllocating();
m_isIterating = false;
}
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes
