Bug 128641

Summary: DelayedReleaseScope in MarkedAllocator::tryAllocateHelper is racy
Product: WebKit Reporter: Mark Hahnenberg <mhahnenberg>
Component: JavaScriptCoreAssignee: Mark Hahnenberg <mhahnenberg>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, ggaren, mark.lam
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Mark Hahnenberg
Reported 2014-02-11 17:52:14 PST
It causes the VM to drop the lock in the middle of adding a new block to the allocator, so another thread could come in and add a block before we're done.
Attachments
Patch (4.01 KB, patch)
2014-02-11 20:57 PST, Mark Hahnenberg 		no flags
DetailsFormatted DiffDiff
Patch (4.10 KB, patch)
2014-02-11 22:24 PST, Mark Hahnenberg 		no flags
DetailsFormatted DiffDiff
Patch (3.65 KB, patch)
2014-02-11 22:26 PST, Mark Hahnenberg 		no flags
DetailsFormatted DiffDiff
Patch (4.53 KB, patch)
2014-02-12 07:05 PST, Mark Hahnenberg 		no flags
DetailsFormatted DiffDiff
Patch (5.50 KB, patch)
2014-02-12 12:50 PST, Mark Hahnenberg 		no flags
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Mark Hahnenberg
Comment 1 2014-02-11 20:45:42 PST
(In reply to comment #0) > It causes the VM to drop the lock in the middle of adding a new block to the allocator, so another thread could come in and add a block before we're done. Actually, it was the DelayedReleaseScope in tryAllocateHelper that was wrong.
Mark Hahnenberg
Comment 2 2014-02-11 20:57:32 PST
Created attachment 223936 [details] Patch
Mark Hahnenberg
Comment 3 2014-02-11 21:05:16 PST
Comment on attachment 223936 [details] Patch This still has issues.
Mark Hahnenberg
Comment 4 2014-02-11 22:24:35 PST
Created attachment 223941 [details] Patch
Mark Hahnenberg
Comment 5 2014-02-11 22:26:09 PST
Created attachment 223942 [details] Patch
Mark Hahnenberg
Comment 6 2014-02-12 07:05:26 PST
Created attachment 223969 [details] Patch
Mark Lam
Comment 7 2014-02-12 10:08:40 PST
Comment on attachment 223969 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=223969&action=review > Source/JavaScriptCore/heap/MarkedAllocator.cpp:109 > + void* head = tryPopFreeList(bytes); > ASSERT(head); According to tryPopFreeList() below, it can return a 0. So, is this assertion still valid?
Mark Hahnenberg
Comment 8 2014-02-12 10:09:55 PST
(In reply to comment #7) > (From update of attachment 223969 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=223969&action=review > > > Source/JavaScriptCore/heap/MarkedAllocator.cpp:109 > > + void* head = tryPopFreeList(bytes); > > ASSERT(head); > > According to tryPopFreeList() below, it can return a 0. So, is this assertion still valid? Yes. We will loop around the while loop until m_freeList.head is not null. If m_freeList.head is not null then tryPopFreeList *must* succeed.
Oliver Hunt
Comment 9 2014-02-12 10:42:40 PST
Comment on attachment 223969 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=223969&action=review > Source/JavaScriptCore/heap/MarkedAllocator.cpp:116 > + ASSERT(m_currentBlock); RELEASE_ASSERT? > Source/JavaScriptCore/heap/MarkedAllocator.cpp:127 > ASSERT(!m_heap->isBusy()); Can we RELEASE_ASSERT here without hurting perf?
Mark Hahnenberg
Comment 10 2014-02-12 12:50:57 PST
Created attachment 223996 [details] Patch
Michael Saboff
Comment 11 2014-02-12 15:40:01 PST
Comment on attachment 223996 [details] Patch r=me
WebKit Commit Bot
Comment 12 2014-02-12 20:19:38 PST
Comment on attachment 223996 [details] Patch Clearing flags on attachment: 223996 Committed r164009: <http://trac.webkit.org/changeset/164009>
WebKit Commit Bot
Comment 13 2014-02-12 20:19:42 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.