Changeset 208018 in webkit


Ignore:
Timestamp:
Oct 27, 2016 4:46:13 PM (7 years ago)
Author:
mark.lam@apple.com
Message:

JSFunction::put() should not allow caching of lazily reified properties.
​https://bugs.webkit.org/show_bug.cgi?id=164081

Reviewed by Geoffrey Garen.

It is incorrect for JSFunction::put() to return PutPropertySlots that indicates
that its lazily reified properties (e.g. .caller, and .arguments) are cacheable.
The reason for this is:

  1. Currently, a cacheable put may only consist of the following types of put operations:
    1. putting a new property at an offset in the object storage.
    2. changing the value of an existing property at an offset in the object storage.
    3. invoking the setter for a property at an offset in the object storage.

Returning a PutPropertySlot that indicates the property is cacheable means that
the property put must be one of the above operations.

For lazily reified properties, JSFunction::put() implements complex conditional
behavior that is different than the set of cacheable put operations above.
Hence, it should not claim that the property put is cacheable.


  1. Cacheable puts are cached on the original structure of the object before the put operation.

Reifying a lazy property will trigger a structure transition. Even though
subsequent puts to such a property may be cacheable after the structure
transition, it is incorrect to indicate that the property put is cacheable
because the caching is on the original structure, not the new transitioned
structure.

Also fixed some missing exception checks.

  • jit/JITOperations.cpp:
  • runtime/JSFunction.cpp:

(JSC::JSFunction::put):
(JSC::JSFunction::reifyLazyPropertyIfNeeded):
(JSC::JSFunction::reifyBoundNameIfNeeded):

  • runtime/JSFunction.h:
Location:
trunk/Source/JavaScriptCore
Files:
4 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/JavaScriptCore/ChangeLog

    r208016 r208018  
     12016-10-27  Mark Lam  <mark.lam@apple.com>
     2
     3        JSFunction::put() should not allow caching of lazily reified properties.
     4        https://bugs.webkit.org/show_bug.cgi?id=164081
     5
     6        Reviewed by Geoffrey Garen.
     7
     8        It is incorrect for JSFunction::put() to return PutPropertySlots that indicates
     9        that its lazily reified properties (e.g. .caller, and .arguments) are cacheable.
     10        The reason for this is:
     11
     12        1. Currently, a cacheable put may only consist of the following types of put
     13           operations:
     14           a. putting a new property at an offset in the object storage.
     15           b. changing the value of an existing property at an offset in the object storage.
     16           c. invoking the setter for a property at an offset in the object storage.
     17
     18           Returning a PutPropertySlot that indicates the property is cacheable means that
     19           the property put must be one of the above operations.
     20
     21           For lazily reified properties, JSFunction::put() implements complex conditional
     22           behavior that is different than the set of cacheable put operations above.
     23           Hence, it should not claim that the property put is cacheable.
     24   
     25        2. Cacheable puts are cached on the original structure of the object before the
     26           put operation.
     27
     28           Reifying a lazy property will trigger a structure transition.  Even though
     29           subsequent puts to such a property may be cacheable after the structure
     30           transition, it is incorrect to indicate that the property put is cacheable
     31           because the caching is on the original structure, not the new transitioned
     32           structure.
     33
     34        Also fixed some missing exception checks.
     35
     36        * jit/JITOperations.cpp:
     37        * runtime/JSFunction.cpp:
     38        (JSC::JSFunction::put):
     39        (JSC::JSFunction::reifyLazyPropertyIfNeeded):
     40        (JSC::JSFunction::reifyBoundNameIfNeeded):
     41        * runtime/JSFunction.h:
     42
    1432016-10-27  Caitlin Potter  <caitp@igalia.com>
    244

  • trunk/Source/JavaScriptCore/jit/JITOperations.cpp

    r207475 r208018  
    197197    VM* vm = &exec->vm();
    198198    NativeCallFrameTracer tracer(vm, exec);
     199    auto scope = DECLARE_THROW_SCOPE(*vm);
    199200    Identifier ident = Identifier::fromUid(vm, uid);
    200201
    … …  
    203204
    204205    baseValue.getPropertySlot(exec, ident, slot);
     206    RETURN_IF_EXCEPTION(scope, encodedJSValue());
     207
    205208    if (stubInfo->considerCaching(baseValue.structureOrNull()) && !slot.isTaintedByOpaqueObject() && (slot.isCacheableValue() || slot.isCacheableGetter() || slot.isUnset()))
    206209        repatchGetByID(exec, baseValue, ident, slot, *stubInfo, GetByIDKind::Pure);
    … …  
    388391    VM* vm = &exec->vm();
    389392    NativeCallFrameTracer tracer(vm, exec);
    390    
     393    auto scope = DECLARE_THROW_SCOPE(*vm);
     394
    391395    Identifier ident = Identifier::fromUid(vm, uid);
    392396    AccessType accessType = static_cast<AccessType>(stubInfo->accessType);
    … …  
    399403    Structure* structure = baseValue.isCell() ? baseValue.asCell()->structure(*vm) : nullptr;
    400404    baseValue.putInline(exec, ident, value, slot);
    401    
     405    RETURN_IF_EXCEPTION(scope, void());
     406
    402407    if (accessType != static_cast<AccessType>(stubInfo->accessType))
    403408        return;
    … …  
    413418    VM* vm = &exec->vm();
    414419    NativeCallFrameTracer tracer(vm, exec);
    415    
     420    auto scope = DECLARE_THROW_SCOPE(*vm);
     421
    416422    Identifier ident = Identifier::fromUid(vm, uid);
    417423    AccessType accessType = static_cast<AccessType>(stubInfo->accessType);
    … …  
    424430    Structure* structure = baseValue.isCell() ? baseValue.asCell()->structure(*vm) : nullptr;   
    425431    baseValue.putInline(exec, ident, value, slot);
    426    
     432    RETURN_IF_EXCEPTION(scope, void());
     433
    427434    if (accessType != static_cast<AccessType>(stubInfo->accessType))
    428435        return;
    … …  
    517524        byValInfo->tookSlowPath = true;
    518525
     526    scope.release();
    519527    PutPropertySlot slot(baseValue, callFrame->codeBlock()->isStrictMode());
    520528    baseValue.putInline(callFrame, property, value, slot);

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

    r207859 r208018  
    430430
    431431    if (thisObject->isHostOrBuiltinFunction()) {
    432         thisObject->reifyBoundNameIfNeeded(vm, exec, propertyName);
     432        LazyPropertyType propType = thisObject->reifyBoundNameIfNeeded(vm, exec, propertyName);
     433        if (propType == LazyPropertyType::IsLazyProperty)
     434            slot.disableCaching();
    433435        return Base::put(thisObject, exec, propertyName, value, slot);
    434436    }
    435437
    436438    if (propertyName == vm.propertyNames->prototype) {
     439        slot.disableCaching();
    437440        // Make sure prototype has been reified, such that it can only be overwritten
    438441        // following the rules set out in ECMA-262 8.12.9.
    439         PropertySlot slot(thisObject, PropertySlot::InternalMethodType::VMInquiry);
    440         thisObject->methodTable(vm)->getOwnPropertySlot(thisObject, exec, propertyName, slot);
     442        PropertySlot getSlot(thisObject, PropertySlot::InternalMethodType::VMInquiry);
     443        thisObject->methodTable(vm)->getOwnPropertySlot(thisObject, exec, propertyName, getSlot);
    441444        if (thisObject->m_rareData)
    442445            thisObject->m_rareData->clear("Store to prototype property of a function");
    443         // Don't allow this to be cached, since a [[Put]] must clear m_rareData.
    444         PutPropertySlot dontCache(thisObject);
    445446        scope.release();
    446         return Base::put(thisObject, exec, propertyName, value, dontCache);
    447     }
    448 
    449     if (propertyName == exec->propertyNames().arguments || propertyName == exec->propertyNames().caller) {
     447        return Base::put(thisObject, exec, propertyName, value, slot);
     448    }
     449
     450    if (propertyName == vm.propertyNames->arguments || propertyName == vm.propertyNames->caller) {
     451        slot.disableCaching();
    450452        if (!thisObject->jsExecutable()->hasCallerAndArgumentsProperties()) {
    451453            // This will trigger the property to be reified, if this is not already the case!
    … …  
    459461        return typeError(exec, scope, slot.isStrictMode(), ASCIILiteral(ReadonlyPropertyWriteError));
    460462    }
    461     thisObject->reifyLazyPropertyIfNeeded(vm, exec, propertyName);
     463    LazyPropertyType propType = thisObject->reifyLazyPropertyIfNeeded(vm, exec, propertyName);
     464    if (propType == LazyPropertyType::IsLazyProperty)
     465        slot.disableCaching();
    462466    scope.release();
    463467    return Base::put(thisObject, exec, propertyName, value, slot);
    … …  
    662666}
    663667
    664 void JSFunction::reifyLazyPropertyIfNeeded(VM& vm, ExecState* exec, PropertyName propertyName)
     668JSFunction::LazyPropertyType JSFunction::reifyLazyPropertyIfNeeded(VM& vm, ExecState* exec, PropertyName propertyName)
    665669{
    666670    if (propertyName == vm.propertyNames->length) {
    667671        if (!hasReifiedLength())
    668672            reifyLength(vm);
     673        return LazyPropertyType::IsLazyProperty;
    669674    } else if (propertyName == vm.propertyNames->name) {
    670675        if (!hasReifiedName())
    671676            reifyName(vm, exec);
    672     }
    673 }
    674 
    675 void JSFunction::reifyBoundNameIfNeeded(VM& vm, ExecState* exec, PropertyName propertyName)
     677        return LazyPropertyType::IsLazyProperty;
     678    }
     679    return LazyPropertyType::NotLazyProperty;
     680}
     681
     682JSFunction::LazyPropertyType JSFunction::reifyBoundNameIfNeeded(VM& vm, ExecState* exec, PropertyName propertyName)
    676683{
    677684    const Identifier& nameIdent = vm.propertyNames->name;
    678685    if (propertyName != nameIdent)
    679         return;
     686        return LazyPropertyType::NotLazyProperty;
    680687
    681688    if (hasReifiedName())
    682         return;
     689        return LazyPropertyType::IsLazyProperty;
    683690
    684691    if (this->inherits(JSBoundFunction::info())) {
    … …  
    689696        rareData->setHasReifiedName();
    690697    }
     698    return LazyPropertyType::IsLazyProperty;
    691699}
    692700

  • trunk/Source/JavaScriptCore/runtime/JSFunction.h

    r206525 r208018  
    191191    void reifyLength(VM&);
    192192    void reifyName(VM&, ExecState*);
    193     void reifyBoundNameIfNeeded(VM&, ExecState*, PropertyName);
    194193    void reifyName(VM&, ExecState*, String name);
    195     void reifyLazyPropertyIfNeeded(VM&, ExecState*, PropertyName propertyName);
     194
     195    enum class LazyPropertyType { NotLazyProperty, IsLazyProperty };
     196    LazyPropertyType reifyLazyPropertyIfNeeded(VM&, ExecState*, PropertyName);
     197    LazyPropertyType reifyBoundNameIfNeeded(VM&, ExecState*, PropertyName);
    196198
    197199    friend class LLIntOffsetsExtractor;
Note: See TracChangeset for help on using the changeset viewer.