Ignore:
Timestamp:
Oct 16, 2013, 4:47:45 PM (12 years ago)
Author:
[email protected]
Message:

llint_slow_path_put_by_id can deadlock on a ConcurrentJITLock
​https://bugs.webkit.org/show_bug.cgi?id=122667

Reviewed by Geoffrey Garen.

The issue this patch is attempting to fix is that there are places in our codebase
where we acquire the ConcurrentJITLock for a particular CodeBlock, then we do some
operations that can initiate a garbage collection. Garbage collection then calls
some methods of CodeBlock that also take the ConcurrentJITLock (because they don't
always necessarily run during garbage collection). This causes a deadlock.

To fix this issue, this patch adds a new RAII-style object (DisallowGC) that stores
into a thread-local field that indicates that it is unsafe to perform any operation
that could trigger garbage collection on the current thread. In debug builds,
ConcurrentJITLocker contains one of these DisallowGC objects so that we can eagerly
detect deadlocks.

This patch also adds a new type of ConcurrentJITLocker, GCSafeConcurrentJITLocker,
which uses the DeferGC mechanism to prevent collections from occurring while the
lock is held.

(JSC::DisallowGC::DisallowGC):
(JSC::DisallowGC::~DisallowGC):
(JSC::DisallowGC::isGCDisallowedOnCurrentThread):
(JSC::DisallowGC::initialize):

  • jit/Repatch.cpp:

(JSC::repatchPutByID):
(JSC::buildPutByIdList):

  • llint/LLIntSlowPaths.cpp:

(JSC::LLInt::LLINT_SLOW_PATH_DECL):

  • runtime/ConcurrentJITLock.h:

(JSC::ConcurrentJITLockerBase::ConcurrentJITLockerBase):
(JSC::ConcurrentJITLockerBase::~ConcurrentJITLockerBase):
(JSC::ConcurrentJITLockerBase::unlockEarly):
(JSC::GCSafeConcurrentJITLocker::GCSafeConcurrentJITLocker):
(JSC::GCSafeConcurrentJITLocker::~GCSafeConcurrentJITLocker):
(JSC::GCSafeConcurrentJITLocker::NoDefer::NoDefer):
(JSC::ConcurrentJITLocker::ConcurrentJITLocker):

  • runtime/InitializeThreading.cpp:

(JSC::initializeThreadingOnce):

  • runtime/JSCellInlines.h:

(JSC::allocateCell):

  • runtime/JSSymbolTableObject.h:

(JSC::symbolTablePut):

  • runtime/Structure.cpp: materializePropertyMapIfNecessary* now has a problem in that it

can start a garbage collection when the GCSafeConcurrentJITLocker goes out of scope, but
before the caller has a chance to use the newly created PropertyTable. The garbage collection
clears the PropertyTable, and then the caller uses it assuming it's valid. To avoid this,
we must DeferGC until the caller is done getting the newly materialized PropertyTable from
the Structure.
(JSC::Structure::materializePropertyMap):
(JSC::Structure::despecifyDictionaryFunction):
(JSC::Structure::changePrototypeTransition):
(JSC::Structure::despecifyFunctionTransition):
(JSC::Structure::attributeChangeTransition):
(JSC::Structure::toDictionaryTransition):
(JSC::Structure::preventExtensionsTransition):
(JSC::Structure::takePropertyTableOrCloneIfPinned):
(JSC::Structure::isSealed):
(JSC::Structure::isFrozen):
(JSC::Structure::addPropertyWithoutTransition):
(JSC::Structure::removePropertyWithoutTransition):
(JSC::Structure::get):
(JSC::Structure::despecifyFunction):
(JSC::Structure::despecifyAllFunctions):
(JSC::Structure::putSpecificValue):
(JSC::Structure::createPropertyMap):
(JSC::Structure::getPropertyNamesFromStructure):

  • runtime/Structure.h:

(JSC::Structure::materializePropertyMapIfNecessary):
(JSC::Structure::materializePropertyMapIfNecessaryForPinning):

  • runtime/StructureInlines.h:

(JSC::Structure::get):

  • runtime/SymbolTable.h:

(JSC::SymbolTable::find):
(JSC::SymbolTable::end):

File:
1 edited

Legend:

Unmodified
Added
Removed

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

    r157424 r157539  
    281281    // property map. We don't want getConcurrently() to see the property map in a half-baked
    282282    // state.
    283     ConcurrentJITLocker locker(m_lock);
     283    GCSafeConcurrentJITLocker locker(m_lock, vm.heap);
    284284    if (!table)
    285285        createPropertyMap(locker, vm, numberOfSlotsForLastOffset(m_offset, m_inlineCapacity));
    … …  
    314314    StringImpl* rep = propertyName.uid();
    315315
    316     materializePropertyMapIfNecessary(vm);
     316    DeferGC deferGC(vm.heap);
     317    materializePropertyMapIfNecessary(vm, deferGC);
    317318
    318319    ASSERT(isDictionary());
    … …  
    452453    transition->m_prototype.set(vm, transition, prototype);
    453454
    454     structure->materializePropertyMapIfNecessary(vm);
     455    DeferGC deferGC(vm.heap);
     456    structure->materializePropertyMapIfNecessary(vm, deferGC);
    455457    transition->propertyTable().set(vm, transition, structure->copyPropertyTableForPinning(vm, transition));
    456458    transition->m_offset = structure->m_offset;
    … …  
    468470    ++transition->m_specificFunctionThrashCount;
    469471
    470     structure->materializePropertyMapIfNecessary(vm);
     472    DeferGC deferGC(vm.heap);
     473    structure->materializePropertyMapIfNecessary(vm, deferGC);
    471474    transition->propertyTable().set(vm, transition, structure->copyPropertyTableForPinning(vm, transition));
    472475    transition->m_offset = structure->m_offset;
    … …  
    486489Structure* Structure::attributeChangeTransition(VM& vm, Structure* structure, PropertyName propertyName, unsigned attributes)
    487490{
     491    DeferGC deferGC(vm.heap);
    488492    if (!structure->isUncacheableDictionary()) {
    489493        Structure* transition = create(vm, structure);
    490494
    491         structure->materializePropertyMapIfNecessary(vm);
     495        structure->materializePropertyMapIfNecessary(vm, deferGC);
    492496        transition->propertyTable().set(vm, transition, structure->copyPropertyTableForPinning(vm, transition));
    493497        transition->m_offset = structure->m_offset;
    … …  
    512516    Structure* transition = create(vm, structure);
    513517
    514     structure->materializePropertyMapIfNecessary(vm);
     518    DeferGC deferGC(vm.heap);
     519    structure->materializePropertyMapIfNecessary(vm, deferGC);
    515520    transition->propertyTable().set(vm, transition, structure->copyPropertyTableForPinning(vm, transition));
    516521    transition->m_offset = structure->m_offset;
    … …  
    572577    // Don't set m_offset, as one can not transition to this.
    573578
    574     structure->materializePropertyMapIfNecessary(vm);
     579    DeferGC deferGC(vm.heap);
     580    structure->materializePropertyMapIfNecessary(vm, deferGC);
    575581    transition->propertyTable().set(vm, transition, structure->copyPropertyTableForPinning(vm, transition));
    576582    transition->m_offset = structure->m_offset;
    … …  
    584590PropertyTable* Structure::takePropertyTableOrCloneIfPinned(VM& vm, Structure* owner)
    585591{
    586     materializePropertyMapIfNecessaryForPinning(vm);
     592    DeferGC deferGC(vm.heap);
     593    materializePropertyMapIfNecessaryForPinning(vm, deferGC);
    587594   
    588595    if (m_isPinnedPropertyTable)
    … …  
    641648        return false;
    642649
    643     materializePropertyMapIfNecessary(vm);
     650    DeferGC deferGC(vm.heap);
     651    materializePropertyMapIfNecessary(vm, deferGC);
    644652    if (!propertyTable())
    645653        return true;
    … …  
    659667        return false;
    660668
    661     materializePropertyMapIfNecessary(vm);
     669    DeferGC deferGC(vm.heap);
     670    materializePropertyMapIfNecessary(vm, deferGC);
    662671    if (!propertyTable())
    663672        return true;
    … …  
    719728        specificValue = 0;
    720729
    721     materializePropertyMapIfNecessaryForPinning(vm);
     730    DeferGC deferGC(vm.heap);
     731    materializePropertyMapIfNecessaryForPinning(vm, deferGC);
    722732   
    723733    pin();
    … …  
    731741    ASSERT(!enumerationCache());
    732742
    733     materializePropertyMapIfNecessaryForPinning(vm);
     743    DeferGC deferGC(vm.heap);
     744    materializePropertyMapIfNecessaryForPinning(vm, deferGC);
    734745
    735746    pin();
    … …  
    841852    ASSERT(structure()->classInfo() == info());
    842853
    843     materializePropertyMapIfNecessary(vm);
     854    DeferGC deferGC(vm.heap);
     855    materializePropertyMapIfNecessary(vm, deferGC);
    844856    if (!propertyTable())
    845857        return invalidOffset;
    … …  
    856868bool Structure::despecifyFunction(VM& vm, PropertyName propertyName)
    857869{
    858     materializePropertyMapIfNecessary(vm);
     870    DeferGC deferGC(vm.heap);
     871    materializePropertyMapIfNecessary(vm, deferGC);
    859872    if (!propertyTable())
    860873        return false;
    … …  
    871884void Structure::despecifyAllFunctions(VM& vm)
    872885{
    873     materializePropertyMapIfNecessary(vm);
     886    DeferGC deferGC(vm.heap);
     887    materializePropertyMapIfNecessary(vm, deferGC);
    874888    if (!propertyTable())
    875889        return;
    … …  
    882896PropertyOffset Structure::putSpecificValue(VM& vm, PropertyName propertyName, unsigned attributes, JSCell* specificValue)
    883897{
    884     ConcurrentJITLocker locker(m_lock);
     898    GCSafeConcurrentJITLocker locker(m_lock, vm.heap);
    885899   
    886900    ASSERT(!JSC::isValidOffset(get(vm, propertyName)));
    … …  
    927941}
    928942
    929 void Structure::createPropertyMap(const ConcurrentJITLocker&, VM& vm, unsigned capacity)
     943void Structure::createPropertyMap(const GCSafeConcurrentJITLocker&, VM& vm, unsigned capacity)
    930944{
    931945    ASSERT(!propertyTable());
    … …  
    937951void Structure::getPropertyNamesFromStructure(VM& vm, PropertyNameArray& propertyNames, EnumerationMode mode)
    938952{
    939     materializePropertyMapIfNecessary(vm);
     953    DeferGC deferGC(vm.heap);
     954    materializePropertyMapIfNecessary(vm, deferGC);
    940955    if (!propertyTable())
    941956        return;
Note: See TracChangeset for help on using the changeset viewer.