⚠ Archived content — this site is no longer maintained.   Current WebKit documentation is at docs.webkit.org.

Changeset 179728 in webkit


Ignore:
Timestamp:
Feb 5, 2015, 5:12:00 PM (12 years ago)
Author:
msaboff@apple.com
Message:

CodeCache is not thread safe when adding the same source from two different threads
​https://bugs.webkit.org/show_bug.cgi?id=141275

Reviewed by Mark Lam.

The issue for this bug is that one thread, takes a cache miss in CodeCache::getGlobalCodeBlock,
but in the process creates a cache entry with a nullptr UnlinkedCodeBlockType* which it
will fill in later in the function. During the body of that function, it allocates
objects that may garbage collect. During that garbage collection, we drop the all locks.
While the locks are released by the first thread, another thread can enter the VM and might
have exactly the same source and enter CodeCache::getGlobalCodeBlock() itself. When it
looks up the code block, it sees it as a cache it and uses the nullptr UnlinkedCodeBlockType*
and crashes. This fixes the problem by not dropping the locks during garbage collection.
There are other likely scenarios where we have a data structure like this code cache in an
unsafe state for arbitrary reentrance.

Moved the functionality of DelayedReleaseScope directly into Heap. Changed it into
a simple list that is cleared with the new function Heap::releaseDelayedReleasedObjects.
Now we accumulate objects to be released and release them when all locks are dropped or
when destroying the Heap. This eliminated the dropping and reaquiring of locks associated
with the old scope form of this list.

Given that all functionality of DelayedReleaseScope is now used and referenced by Heap
and the lock management no longer needs to be done, just made the list a member of Heap.
We do need to guard against the case that releasing an object can create more objects
by calling into JS. That is why releaseDelayedReleasedObjects() is written to remove
an object to release so that we aren't recursively in Vector code. The other thing we
do in releaseDelayedReleasedObjects() is to guard against recursive calls to itself using
the m_delayedReleaseRecursionCount. We only release at the first entry into the function.
This case is already tested by testapi.mm.

  • heap/DelayedReleaseScope.h: Removed file
  • API/JSAPIWrapperObject.mm:
  • API/ObjCCallbackFunction.mm:
  • JavaScriptCore.vcxproj/JavaScriptCore.vcxproj:
  • JavaScriptCore.vcxproj/JavaScriptCore.vcxproj.filters:
  • JavaScriptCore.xcodeproj/project.pbxproj:
  • heap/IncrementalSweeper.cpp:

(JSC::IncrementalSweeper::doSweep):

  • heap/MarkedAllocator.cpp:

(JSC::MarkedAllocator::tryAllocateHelper):
(JSC::MarkedAllocator::tryAllocate):

  • heap/MarkedBlock.cpp:

(JSC::MarkedBlock::sweep):

  • heap/MarkedSpace.cpp:

(JSC::MarkedSpace::MarkedSpace):
(JSC::MarkedSpace::lastChanceToFinalize):
(JSC::MarkedSpace::didFinishIterating):

  • heap/MarkedSpace.h:
  • heap/Heap.cpp:

(JSC::Heap::collectAllGarbage):
(JSC::Heap::zombifyDeadObjects):
Removed references to DelayedReleaseScope and DelayedReleaseScope.h.

  • heap/Heap.cpp:

(JSC::Heap::Heap): Initialized m_delayedReleaseRecursionCount.
(JSC::Heap::lastChanceToFinalize): Call releaseDelayedObjectsNow() as the VM is going away.
(JSC::Heap::releaseDelayedReleasedObjects): New function that released the accumulated
delayed release objects.

  • heap/Heap.h:

(JSC::Heap::m_delayedReleaseObjects): List of objects to be released later.
(JSC::Heap::m_delayedReleaseRecursionCount): Counter to indicate that
releaseDelayedReleasedObjects is being called recursively.

  • heap/HeapInlines.h:

(JSC::Heap::releaseSoon): Changed location of list to add delayed release objects.

  • runtime/JSLock.cpp:

(JSC::JSLock::willReleaseLock):
Call Heap::releaseDelayedObjectsNow() when releasing the lock.

Location:
trunk/Source/JavaScriptCore
Files:
1 deleted
15 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/JavaScriptCore/API/JSAPIWrapperObject.mm

    r171939 r179728  
    2727#include "JSAPIWrapperObject.h"
    2828
    29 #include "DelayedReleaseScope.h"
    3029#include "JSCInlines.h"
    3130#include "JSCallbackObject.h"

  • trunk/Source/JavaScriptCore/API/ObjCCallbackFunction.mm

    r173410 r179728  
    3131#import "APICallbackFunction.h"
    3232#import "APICast.h"
    33 #import "DelayedReleaseScope.h"
    3433#import "Error.h"
    3534#import "JSCJSValueInlines.h"

  • trunk/Source/JavaScriptCore/ChangeLog

    r179687 r179728  
     12015-02-05  Michael Saboff  <msaboff@apple.com>
     2
     3        CodeCache is not thread safe when adding the same source from two different threads
     4        https://bugs.webkit.org/show_bug.cgi?id=141275
     5
     6        Reviewed by Mark Lam.
     7
     8        The issue for this bug is that one thread, takes a cache miss in CodeCache::getGlobalCodeBlock,
     9        but in the process creates a cache entry with a nullptr UnlinkedCodeBlockType* which it
     10        will fill in later in the function.  During the body of that function, it allocates
     11        objects that may garbage collect.  During that garbage collection, we drop the all locks.
     12        While the locks are released by the first thread, another thread can enter the VM and might
     13        have exactly the same source and enter CodeCache::getGlobalCodeBlock() itself.  When it
     14        looks up the code block, it sees it as a cache it and uses the nullptr UnlinkedCodeBlockType*
     15        and crashes.  This fixes the problem by not dropping the locks during garbage collection.
     16        There are other likely scenarios where we have a data structure like this code cache in an
     17        unsafe state for arbitrary reentrance.
     18
     19        Moved the functionality of DelayedReleaseScope directly into Heap.  Changed it into
     20        a simple list that is cleared with the new function Heap::releaseDelayedReleasedObjects.
     21        Now we accumulate objects to be released and release them when all locks are dropped or
     22        when destroying the Heap.  This eliminated the dropping and reaquiring of locks associated
     23        with the old scope form of this list.
     24
     25        Given that all functionality of DelayedReleaseScope is now used and referenced by Heap
     26        and the lock management no longer needs to be done, just made the list a member of Heap.
     27        We do need to guard against the case that releasing an object can create more objects
     28        by calling into JS.  That is why releaseDelayedReleasedObjects() is written to remove
     29        an object to release so that we aren't recursively in Vector code.  The other thing we
     30        do in releaseDelayedReleasedObjects() is to guard against recursive calls to itself using
     31        the m_delayedReleaseRecursionCount.  We only release at the first entry into the function.
     32        This case is already tested by testapi.mm.
     33
     34        * heap/DelayedReleaseScope.h: Removed file
     35
     36        * API/JSAPIWrapperObject.mm:
     37        * API/ObjCCallbackFunction.mm:
     38        * JavaScriptCore.vcxproj/JavaScriptCore.vcxproj:
     39        * JavaScriptCore.vcxproj/JavaScriptCore.vcxproj.filters:
     40        * JavaScriptCore.xcodeproj/project.pbxproj:
     41        * heap/IncrementalSweeper.cpp:
     42        (JSC::IncrementalSweeper::doSweep):
     43        * heap/MarkedAllocator.cpp:
     44        (JSC::MarkedAllocator::tryAllocateHelper):
     45        (JSC::MarkedAllocator::tryAllocate):
     46        * heap/MarkedBlock.cpp:
     47        (JSC::MarkedBlock::sweep):
     48        * heap/MarkedSpace.cpp:
     49        (JSC::MarkedSpace::MarkedSpace):
     50        (JSC::MarkedSpace::lastChanceToFinalize):
     51        (JSC::MarkedSpace::didFinishIterating):
     52        * heap/MarkedSpace.h:
     53        * heap/Heap.cpp:
     54        (JSC::Heap::collectAllGarbage):
     55        (JSC::Heap::zombifyDeadObjects):
     56        Removed references to DelayedReleaseScope and DelayedReleaseScope.h.
     57
     58        * heap/Heap.cpp:
     59        (JSC::Heap::Heap): Initialized m_delayedReleaseRecursionCount.
     60        (JSC::Heap::lastChanceToFinalize): Call releaseDelayedObjectsNow() as the VM is going away.
     61        (JSC::Heap::releaseDelayedReleasedObjects): New function that released the accumulated
     62        delayed release objects.
     63
     64        * heap/Heap.h:
     65        (JSC::Heap::m_delayedReleaseObjects): List of objects to be released later.
     66        (JSC::Heap::m_delayedReleaseRecursionCount): Counter to indicate that
     67        releaseDelayedReleasedObjects is being called recursively.
     68        * heap/HeapInlines.h:
     69        (JSC::Heap::releaseSoon): Changed location of list to add delayed release objects.
     70       
     71        * runtime/JSLock.cpp:
     72        (JSC::JSLock::willReleaseLock):
     73        Call Heap::releaseDelayedObjectsNow() when releasing the lock.
     74
    1752015-02-05  Youenn Fablet  <youenn.fablet@crf.canon.fr> and Xabier Rodriguez Calvar <calvaris@igalia.com>
    276

  • trunk/Source/JavaScriptCore/JavaScriptCore.vcxproj/JavaScriptCore.vcxproj

    r179552 r179728  
    12311231    <ClInclude Include="..\heap\CopyWriteBarrier.h" />
    12321232    <ClInclude Include="..\heap\DeferGC.h" />
    1233     <ClInclude Include="..\heap\DelayedReleaseScope.h" />
    12341233    <ClInclude Include="..\heap\EdenGCActivityCallback.h" />
    12351234    <ClInclude Include="..\heap\FullGCActivityCallback.h" />

  • trunk/Source/JavaScriptCore/JavaScriptCore.vcxproj/JavaScriptCore.vcxproj.filters

    r179552 r179728  
    37693769      <Filter>runtime</Filter>
    37703770    </ClInclude>
    3771     <ClInclude Include="..\heap\DelayedReleaseScope.h">
    3772       <Filter>heap</Filter>
    3773     </ClInclude>
    37743771    <ClInclude Include="..\bytecode\VariableWatchpointSet.h">
    37753772      <Filter>bytecode</Filter>

  • trunk/Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj

    r179503 r179728  
    865865                2A111245192FCE79005EE18D /* CustomGetterSetter.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 2A111243192FCE79005EE18D /* CustomGetterSetter.cpp */; };
    866866                2A111246192FCE79005EE18D /* CustomGetterSetter.h in Headers */ = {isa = PBXBuildFile; fileRef = 2A111244192FCE79005EE18D /* CustomGetterSetter.h */; settings = {ATTRIBUTES = (Private, ); }; };
    867                 2A2825D018341F2D0087FBA9 /* DelayedReleaseScope.h in Headers */ = {isa = PBXBuildFile; fileRef = 2A2825CF18341F2D0087FBA9 /* DelayedReleaseScope.h */; };
    868867                2A48D1911772365B00C65A5F /* APICallbackFunction.h in Headers */ = {isa = PBXBuildFile; fileRef = C211B574176A224D000E2A23 /* APICallbackFunction.h */; };
    869868                2A4BB7F318A41179008A0FCD /* JSManagedValueInternal.h in Headers */ = {isa = PBXBuildFile; fileRef = 2A4BB7F218A41179008A0FCD /* JSManagedValueInternal.h */; };
    … …  
    25002499                2A111243192FCE79005EE18D /* CustomGetterSetter.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = CustomGetterSetter.cpp; sourceTree = "<group>"; };
    25012500                2A111244192FCE79005EE18D /* CustomGetterSetter.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = CustomGetterSetter.h; sourceTree = "<group>"; };
    2502                 2A2825CF18341F2D0087FBA9 /* DelayedReleaseScope.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = DelayedReleaseScope.h; sourceTree = "<group>"; };
    25032501                2A343F7418A1748B0039B085 /* GCSegmentedArray.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = GCSegmentedArray.h; sourceTree = "<group>"; };
    25042502                2A343F7718A1749D0039B085 /* GCSegmentedArrayInlines.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = GCSegmentedArrayInlines.h; sourceTree = "<group>"; };
    … …  
    38493847                                2A7A58EE1808A4C40020BDF7 /* DeferGC.cpp */,
    38503848                                0F136D4B174AD69B0075B354 /* DeferGC.h */,
    3851                                 2A2825CF18341F2D0087FBA9 /* DelayedReleaseScope.h */,
    38523849                                BCBE2CAD14E985AA000593AD /* GCAssertions.h */,
    38533850                                0F2B66A817B6B53D00A7AE3F /* GCIncomingRefCounted.h */,
    … …  
    55365533                                0F136D4D174AD69E0075B354 /* DeferGC.h in Headers */,
    55375534                                0FC712DF17CD877C008CC93C /* DeferredCompilationCallback.h in Headers */,
    5538                                 2A2825D018341F2D0087FBA9 /* DelayedReleaseScope.h in Headers */,
    55395535                                A77A423E17A0BBFD00A8DB81 /* DFGAbstractHeap.h in Headers */,
    55405536                                A5EA70E819F5B1010098F5EC /* AugmentableInspectorControllerClient.h in Headers */,

  • trunk/Source/JavaScriptCore/heap/Heap.cpp

    r179478 r179728  
    2828#include "CopyVisitorInlines.h"
    2929#include "DFGWorklist.h"
    30 #include "DelayedReleaseScope.h"
    3130#include "EdenGCActivityCallback.h"
    3231#include "FullGCActivityCallback.h"
    … …  
    338337#endif
    339338    , m_deferralDepth(0)
     339#if USE(CF)
     340    , m_delayedReleaseRecursionCount(0)
     341#endif
    340342{
    341343    m_storageSpace.init();
    … …  
    361363
    362364    m_objectSpace.lastChanceToFinalize();
     365    releaseDelayedReleasedObjects();
     366}
     367
     368void Heap::releaseDelayedReleasedObjects()
     369{
     370#if USE(CF)
     371    if (!m_delayedReleaseRecursionCount++) {
     372        while (!m_delayedReleaseObjects.isEmpty()) {
     373            RetainPtr<CFTypeRef> objectToRelease = m_delayedReleaseObjects.takeLast();
     374            objectToRelease.clear();
     375        }
     376    }
     377    m_delayedReleaseRecursionCount--;
     378#endif
    363379}
    364380
    … …  
    967983
    968984    SamplingRegion samplingRegion("Garbage Collection: Sweeping");
    969     DelayedReleaseScope delayedReleaseScope(m_objectSpace);
    970985    m_objectSpace.sweep();
    971986    m_objectSpace.shrink();
    … …  
    13791394    {
    13801395        SamplingRegion samplingRegion("Garbage Collection: Sweeping");
    1381         DelayedReleaseScope delayedReleaseScope(m_objectSpace);
    13821396        m_objectSpace.zombifySweep();
    13831397    }

  • trunk/Source/JavaScriptCore/heap/Heap.h

    r179211 r179728  
    116116    ~Heap();
    117117    JS_EXPORT_PRIVATE void lastChanceToFinalize();
     118    void releaseDelayedReleasedObjects();
    118119
    119120    VM* vm() const { return m_vm; }
    … …  
    232233    friend class DeferGC;
    233234    friend class DeferGCForAWhile;
    234     friend class DelayedReleaseScope;
    235235    friend class GCAwareJITStubRoutine;
    236236    friend class GCLogging;
    … …  
    388388
    389389    std::unique_ptr<HeapVerifier> m_verifier;
     390#if USE(CF)
     391    Vector<RetainPtr<CFTypeRef>> m_delayedReleaseObjects;
     392    unsigned m_delayedReleaseRecursionCount;
     393#endif
    390394};
    391395

  • trunk/Source/JavaScriptCore/heap/HeapInlines.h

    r179211 r179728  
    250250inline void Heap::releaseSoon(RetainPtr<T>&& object)
    251251{
    252     m_objectSpace.releaseSoon(WTF::move(object));
     252    m_delayedReleaseObjects.append(WTF::move(object));
    253253}
    254254#endif

  • trunk/Source/JavaScriptCore/heap/IncrementalSweeper.cpp

    r176000 r179728  
    2727#include "IncrementalSweeper.h"
    2828
    29 #include "DelayedReleaseScope.h"
    3029#include "Heap.h"
    3130#include "JSObject.h"
    … …  
    6968void IncrementalSweeper::doSweep(double sweepBeginTime)
    7069{
    71     DelayedReleaseScope scope(m_vm->heap.m_objectSpace);
    7270    while (m_currentBlockToSweepIndex < m_blocksToSweep.size()) {
    7371        sweepNextBlock();

  • trunk/Source/JavaScriptCore/heap/MarkedAllocator.cpp

    r179211 r179728  
    2727#include "MarkedAllocator.h"
    2828
    29 #include "DelayedReleaseScope.h"
    3029#include "GCActivityCallback.h"
    3130#include "Heap.h"
    … …  
    6362inline void* MarkedAllocator::tryAllocateHelper(size_t bytes)
    6463{
    65     // We need a while loop to check the free list because the DelayedReleaseScope
    66     // could cause arbitrary code to execute and exhaust the free list that we
    67     // thought had elements in it.
    68     while (!m_freeList.head) {
    69         DelayedReleaseScope delayedReleaseScope(*m_markedSpace);
    70         if (m_currentBlock) {
    71             ASSERT(m_currentBlock == m_nextBlockToSweep);
    72             m_currentBlock->didConsumeFreeList();
    73             m_nextBlockToSweep = m_currentBlock->next();
     64    if (m_currentBlock) {
     65        ASSERT(m_currentBlock == m_nextBlockToSweep);
     66        m_currentBlock->didConsumeFreeList();
     67        m_nextBlockToSweep = m_currentBlock->next();
     68    }
     69
     70    MarkedBlock* next;
     71    for (MarkedBlock*& block = m_nextBlockToSweep; block; block = next) {
     72        next = block->next();
     73
     74        MarkedBlock::FreeList freeList = block->sweep(MarkedBlock::SweepToFreeList);
     75       
     76        double utilization = ((double)MarkedBlock::blockSize - (double)freeList.bytes) / (double)MarkedBlock::blockSize;
     77        if (utilization >= Options::minMarkedBlockUtilization()) {
     78            ASSERT(freeList.bytes || !freeList.head);
     79            m_blockList.remove(block);
     80            m_retiredBlocks.push(block);
     81            block->didRetireBlock(freeList);
     82            continue;
    7483        }
    7584
    76         MarkedBlock* next;
    77         for (MarkedBlock*& block = m_nextBlockToSweep; block; block = next) {
    78             next = block->next();
    79 
    80             MarkedBlock::FreeList freeList = block->sweep(MarkedBlock::SweepToFreeList);
    81            
    82             double utilization = ((double)MarkedBlock::blockSize - (double)freeList.bytes) / (double)MarkedBlock::blockSize;
    83             if (utilization >= Options::minMarkedBlockUtilization()) {
    84                 ASSERT(freeList.bytes || !freeList.head);
    85                 m_blockList.remove(block);
    86                 m_retiredBlocks.push(block);
    87                 block->didRetireBlock(freeList);
    88                 continue;
    89             }
    90 
    91             if (bytes > block->cellSize()) {
    92                 block->stopAllocating(freeList);
    93                 continue;
    94             }
    95 
    96             m_currentBlock = block;
    97             m_freeList = freeList;
    98             break;
     85        if (bytes > block->cellSize()) {
     86            block->stopAllocating(freeList);
     87            continue;
    9988        }
    100        
    101         if (!m_freeList.head) {
    102             m_currentBlock = 0;
    103             return 0;
    104         }
     89
     90        m_currentBlock = block;
     91        m_freeList = freeList;
     92        break;
     93    }
     94   
     95    if (!m_freeList.head) {
     96        m_currentBlock = 0;
     97        return 0;
    10598    }
    10699
    … …  
    128121    m_heap->m_operationInProgress = Allocation;
    129122    void* result = tryAllocateHelper(bytes);
    130 
    131     // Due to the DelayedReleaseScope in tryAllocateHelper, some other thread might have
    132     // created a new block after we thought we didn't find any free cells.
    133     while (!result && m_currentBlock) {
    134         // A new block was added by another thread so try popping the free list.
    135         result = tryPopFreeList(bytes);
    136         if (result)
    137             break;
    138         // The free list was empty, so call tryAllocateHelper to do the normal sweeping stuff.
    139         result = tryAllocateHelper(bytes);
    140     }
    141123
    142124    m_heap->m_operationInProgress = NoOperation;

  • trunk/Source/JavaScriptCore/heap/MarkedBlock.cpp

    r179211 r179728  
    2727#include "MarkedBlock.h"
    2828
    29 #include "DelayedReleaseScope.h"
    3029#include "IncrementalSweeper.h"
    3130#include "JSCell.h"
    … …  
    111110MarkedBlock::FreeList MarkedBlock::sweep(SweepMode sweepMode)
    112111{
    113     ASSERT(DelayedReleaseScope::isInEffectFor(heap()->m_objectSpace));
    114112    HEAP_LOG_BLOCK_STATE_TRANSITION(this);
    115113

  • trunk/Source/JavaScriptCore/heap/MarkedSpace.cpp

    r179211 r179728  
    2222#include "MarkedSpace.h"
    2323
    24 #include "DelayedReleaseScope.h"
    2524#include "IncrementalSweeper.h"
    2625#include "JSGlobalObject.h"
    … …  
    8382    , m_capacity(0)
    8483    , m_isIterating(false)
    85     , m_currentDelayedReleaseScope(nullptr)
    8684{
    8785    for (size_t cellSize = preciseStep; cellSize <= preciseCutoff; cellSize += preciseStep) {
    … …  
    115113void MarkedSpace::lastChanceToFinalize()
    116114{
    117     DelayedReleaseScope delayedReleaseScope(*this);
    118115    stopAllocating();
    119116    forEachAllocator<LastChanceToFinalize>();
    … …  
    363360{
    364361    ASSERT(isIterating());
    365     DelayedReleaseScope scope(*this);
    366362    resumeAllocating();
    367363    m_isIterating = false;

  • trunk/Source/JavaScriptCore/heap/MarkedSpace.h

    r179211 r179728  
    3737namespace JSC {
    3838
    39 class DelayedReleaseScope;
    4039class Heap;
    4140class HeapIterationScope;
    … …  
    162161
    163162private:
    164     friend class DelayedReleaseScope;
    165163    friend class LLIntOffsetsExtractor;
    166164    friend class JIT;
    … …  
    178176    MarkedBlockSet m_blocks;
    179177    Vector<MarkedBlock*> m_blocksWithNewObjects;
    180 
    181     DelayedReleaseScope* m_currentDelayedReleaseScope;
    182178};
    183179

  • trunk/Source/JavaScriptCore/runtime/JSLock.cpp

    r171558 r179728  
    172172void JSLock::willReleaseLock()
    173173{
    174     if (m_vm)
     174    if (m_vm) {
     175        m_vm->heap.releaseDelayedReleasedObjects();
    175176        m_vm->setStackPointerAtVMEntry(nullptr);
     177    }
    176178
    177179    if (m_entryAtomicStringTable) {
Note: See TracChangeset for help on using the changeset viewer.