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

Changeset 222671 in webkit


Ignore:
Timestamp:
Sep 29, 2017, 4:48:10 PM (9 years ago)
Author:
sbarati@apple.com
Message:

Custom GetterSetterAccessCase does not use the correct slotBase when making call
​https://bugs.webkit.org/show_bug.cgi?id=177639

Reviewed by Geoffrey Garen.

JSTests:

  • stress/custom-get-set-inline-caching-one-level-up-proto-chain.js: Added.

(assert):
(Class):
(items.forEach):
(set get for):

Source/JavaScriptCore:

The bug occurred when you had a custom set value. Custom set/get
values are passed the property holder, not the base of the access.
If we had an object chain like this:
o = {proto: thingWithCustomSetValue}

We would end up not providing thingWithCustomSetValue as the argument
to the PutValueFunc. The reason is, we would use generateConditionsForPrototypePropertyHitCustom
for custom sets. This would return to us an empty ConditionSet, because
the property holder was only one level up the prototype chain. The reason
is, it didn't generate a condition for the slot holder, because the
protocol for custom set/get is that if an object responds to a custom
setter/getter, it will continue to respond to that getter/setter for
the lifetime of that object. Therefore, it's not strictly necessary to
generate an OPC for the slot base for custom accesses. However, AccessCase
uses !m_conditionSet.isEmtpy() to indicate that the IC is doing a prototype
access. With the above object "o", we were doing a prototype access, but we
had an empty condition set. This lead us to passing the base instead of
the property holder to the custom set value function, which is incorrect.

With custom getters, we never called to into the generateConditionsForPrototypePropertyHitCustom
API. Gets would always call into generateConditionsForPrototypePropertyHit, which
will generate an OPC on the slot base, even if it isn't strictly necessary for custom accessors.
This patch simply removes generateConditionsForPrototypePropertyHitCustom
and aligns the set case with the get case. It makes us properly detect
when we're doing a prototype access with the above object "o". If we find
that generateConditionsForPrototypePropertyHitCustom was a worthwhile
optimization to have, we can re-introduce it. We'll just need to pipe through
a new notion of when we're doing prototype accesses that doesn't rely solely
on !m_conditionSet.isEmpty().

  • bytecode/ObjectPropertyConditionSet.cpp:

(JSC::generateConditionsForPrototypePropertyHitCustom): Deleted.

  • bytecode/ObjectPropertyConditionSet.h:
  • jit/Repatch.cpp:

(JSC::tryCachePutByID):

  • jsc.cpp:

(JSTestCustomGetterSetter::JSTestCustomGetterSetter):
(JSTestCustomGetterSetter::create):
(JSTestCustomGetterSetter::createStructure):
(customGetAccessor):
(customGetValue):
(customSetAccessor):
(customSetValue):
(JSTestCustomGetterSetter::finishCreation):
(GlobalObject::finishCreation):
(functionLoadGetterFromGetterSetter):
(functionCreateCustomTestGetterSetter):

  • runtime/PropertySlot.h:

(JSC::PropertySlot::setCustomGetterSetter):

Location:
trunk
Files:
1 added
7 edited

Legend:

Unmodified
Added
Removed

  • trunk/JSTests/ChangeLog

    r222658 r222671  
     12017-09-29  Saam Barati  <sbarati@apple.com>
     2
     3        Custom GetterSetterAccessCase does not use the correct slotBase when making call
     4        https://bugs.webkit.org/show_bug.cgi?id=177639
     5
     6        Reviewed by Geoffrey Garen.
     7
     8        * stress/custom-get-set-inline-caching-one-level-up-proto-chain.js: Added.
     9        (assert):
     10        (Class):
     11        (items.forEach):
     12        (set get for):
     13
    1142017-09-29  Commit Queue  <commit-queue@webkit.org>
    215

  • trunk/Source/JavaScriptCore/ChangeLog

    r222658 r222671  
     12017-09-29  Saam Barati  <sbarati@apple.com>
     2
     3        Custom GetterSetterAccessCase does not use the correct slotBase when making call
     4        https://bugs.webkit.org/show_bug.cgi?id=177639
     5
     6        Reviewed by Geoffrey Garen.
     7
     8        The bug occurred when you had a custom set value. Custom set/get
     9        values are passed the property holder, not the base of the access.
     10        If we had an object chain like this:
     11        o = {__proto__: thingWithCustomSetValue}
     12       
     13        We would end up not providing thingWithCustomSetValue as the argument
     14        to the PutValueFunc. The reason is, we would use generateConditionsForPrototypePropertyHitCustom
     15        for custom sets. This would return to us an empty ConditionSet, because
     16        the property holder was only one level up the prototype chain. The reason
     17        is, it didn't generate a condition for the slot holder, because the
     18        protocol for custom set/get is that if an object responds to a custom
     19        setter/getter, it will continue to respond to that getter/setter for
     20        the lifetime of that object. Therefore, it's not strictly necessary to
     21        generate an OPC for the slot base for custom accesses. However, AccessCase
     22        uses !m_conditionSet.isEmtpy() to indicate that the IC is doing a prototype
     23        access. With the above object "o", we were doing a prototype access, but we
     24        had an empty condition set. This lead us to passing the base instead of
     25        the property holder to the custom set value function, which is incorrect.
     26       
     27        With custom getters, we never called to into the generateConditionsForPrototypePropertyHitCustom
     28        API. Gets would always call into generateConditionsForPrototypePropertyHit, which
     29        will generate an OPC on the slot base, even if it isn't strictly necessary for custom accessors.
     30        This patch simply removes generateConditionsForPrototypePropertyHitCustom
     31        and aligns the set case with the get case. It makes us properly detect
     32        when we're doing a prototype access with the above object "o". If we find
     33        that generateConditionsForPrototypePropertyHitCustom was a worthwhile
     34        optimization to have, we can re-introduce it. We'll just need to pipe through
     35        a new notion of when we're doing prototype accesses that doesn't rely solely
     36        on !m_conditionSet.isEmpty().
     37
     38        * bytecode/ObjectPropertyConditionSet.cpp:
     39        (JSC::generateConditionsForPrototypePropertyHitCustom): Deleted.
     40        * bytecode/ObjectPropertyConditionSet.h:
     41        * jit/Repatch.cpp:
     42        (JSC::tryCachePutByID):
     43        * jsc.cpp:
     44        (JSTestCustomGetterSetter::JSTestCustomGetterSetter):
     45        (JSTestCustomGetterSetter::create):
     46        (JSTestCustomGetterSetter::createStructure):
     47        (customGetAccessor):
     48        (customGetValue):
     49        (customSetAccessor):
     50        (customSetValue):
     51        (JSTestCustomGetterSetter::finishCreation):
     52        (GlobalObject::finishCreation):
     53        (functionLoadGetterFromGetterSetter):
     54        (functionCreateCustomTestGetterSetter):
     55        * runtime/PropertySlot.h:
     56        (JSC::PropertySlot::setCustomGetterSetter):
     57
    1582017-09-29  Commit Queue  <commit-queue@webkit.org>
    259

  • trunk/Source/JavaScriptCore/bytecode/ObjectPropertyConditionSet.cpp

    r221954 r222671  
    362362}
    363363
    364 ObjectPropertyConditionSet generateConditionsForPrototypePropertyHitCustom(
    365     VM& vm, JSCell* owner, ExecState* exec, Structure* headStructure, JSObject* prototype,
    366     UniquedStringImpl* uid)
    367 {
    368     return generateConditions(
    369         vm, exec->lexicalGlobalObject(), headStructure, prototype,
    370         [&] (Vector<ObjectPropertyCondition>& conditions, JSObject* object) -> bool {
    371             if (object == prototype)
    372                 return true;
    373             ObjectPropertyCondition result =
    374                 generateCondition(vm, owner, object, uid, PropertyCondition::Absence);
    375             if (!result)
    376                 return false;
    377             conditions.append(result);
    378             return true;
    379         });
    380 }
    381 
    382364ObjectPropertyConditionSet generateConditionsForPrototypeEquivalenceConcurrently(
    383365    VM& vm, JSGlobalObject* globalObject, Structure* headStructure, JSObject* prototype, UniquedStringImpl* uid)

  • trunk/Source/JavaScriptCore/bytecode/ObjectPropertyConditionSet.h

    r218794 r222671  
    166166    VM&, JSCell* owner, ExecState*, Structure* headStructure, JSObject* prototype,
    167167    UniquedStringImpl* uid);
    168 ObjectPropertyConditionSet generateConditionsForPrototypePropertyHitCustom(
    169     VM&, JSCell* owner, ExecState*, Structure* headStructure, JSObject* prototype,
    170     UniquedStringImpl* uid);
    171168
    172169ObjectPropertyConditionSet generateConditionsForPrototypeEquivalenceConcurrently(

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

    r222473 r222671  
    447447            if (slot.base() != baseValue) {
    448448                conditionSet =
    449                     generateConditionsForPrototypePropertyHitCustom(
     449                    generateConditionsForPrototypePropertyHit(
    450450                        vm, codeBlock, exec, structure, slot.base(), ident.impl());
    451451                if (!conditionSet.isValid())

  • trunk/Source/JavaScriptCore/jsc.cpp

    r222473 r222671  
    10581058};
    10591059
     1060class JSTestCustomGetterSetter : public JSNonFinalObject {
     1061public:
     1062    using Base = JSNonFinalObject;
     1063    static const unsigned StructureFlags = Base::StructureFlags;
     1064
     1065    JSTestCustomGetterSetter(VM& vm, Structure* structure)
     1066        : Base(vm, structure)
     1067    { }
     1068
     1069    static JSTestCustomGetterSetter* create(VM& vm, JSGlobalObject*, Structure* structure)
     1070    {
     1071        JSTestCustomGetterSetter* result = new (NotNull, allocateCell<JSTestCustomGetterSetter>(vm.heap, sizeof(JSTestCustomGetterSetter))) JSTestCustomGetterSetter(vm, structure);
     1072        result->finishCreation(vm);
     1073        return result;
     1074    }
     1075
     1076    void finishCreation(VM& vm);
     1077
     1078    static Structure* createStructure(VM& vm, JSGlobalObject* globalObject)
     1079    {
     1080        return Structure::create(vm, globalObject, globalObject->objectPrototype(), TypeInfo(ObjectType, StructureFlags), info());
     1081    }
     1082
     1083    DECLARE_INFO;
     1084};
     1085
     1086
     1087static EncodedJSValue customGetAccessor(ExecState*, EncodedJSValue thisValue, PropertyName)
     1088{
     1089    // Passed |this|
     1090    return thisValue;
     1091}
     1092
     1093static EncodedJSValue customGetValue(ExecState* exec, EncodedJSValue slotValue, PropertyName)
     1094{
     1095    RELEASE_ASSERT(JSValue::decode(slotValue).inherits(exec->vm(), JSTestCustomGetterSetter::info()));
     1096    // Passed property holder.
     1097    return slotValue;
     1098}
     1099
     1100static bool customSetAccessor(ExecState* exec, EncodedJSValue thisObject, EncodedJSValue encodedValue)
     1101{
     1102    VM& vm = exec->vm();
     1103
     1104    JSValue value = JSValue::decode(encodedValue);
     1105    RELEASE_ASSERT(value.isObject());
     1106    JSObject* object = asObject(value);
     1107    PutPropertySlot slot(object);
     1108    object->put(object, exec, Identifier::fromString(&vm, "result"), JSValue::decode(thisObject), slot);
     1109
     1110    return true;
     1111}
     1112
     1113static bool customSetValue(ExecState* exec, EncodedJSValue slotValue, EncodedJSValue encodedValue)
     1114{
     1115    VM& vm = exec->vm();
     1116
     1117    RELEASE_ASSERT(JSValue::decode(slotValue).inherits(exec->vm(), JSTestCustomGetterSetter::info()));
     1118
     1119    JSValue value = JSValue::decode(encodedValue);
     1120    RELEASE_ASSERT(value.isObject());
     1121    JSObject* object = asObject(value);
     1122    PutPropertySlot slot(object);
     1123    object->put(object, exec, Identifier::fromString(&vm, "result"), JSValue::decode(slotValue), slot);
     1124
     1125    return true;
     1126}
     1127
     1128void JSTestCustomGetterSetter::finishCreation(VM& vm)
     1129{
     1130    putDirectCustomAccessor(vm, Identifier::fromString(&vm, "customValue"),
     1131        CustomGetterSetter::create(vm, customGetValue, customSetValue), 0);
     1132    putDirectCustomAccessor(vm, Identifier::fromString(&vm, "customAccessor"),
     1133        CustomGetterSetter::create(vm, customGetAccessor, customSetAccessor), static_cast<unsigned>(PropertyAttribute::CustomAccessor));
     1134}
     1135
     1136const ClassInfo JSTestCustomGetterSetter::s_info = { "JSTestCustomGetterSetter", &Base::s_info, nullptr, nullptr, CREATE_METHOD_TABLE(JSTestCustomGetterSetter) };
     1137
    10601138static EncodedJSValue JSC_HOST_CALL functionCreateProxy(ExecState*);
    10611139static EncodedJSValue JSC_HOST_CALL functionCreateRuntimeArray(ExecState*);
    … …  
    11781256static EncodedJSValue JSC_HOST_CALL functionHeapCapacity(ExecState*);
    11791257static EncodedJSValue JSC_HOST_CALL functionFlashHeapAccess(ExecState*);
     1258static EncodedJSValue JSC_HOST_CALL functionLoadGetterFromGetterSetter(ExecState*);
     1259static EncodedJSValue JSC_HOST_CALL functionCreateCustomTestGetterSetter(ExecState*);
    11801260
    11811261struct Script {
    … …  
    14671547        addFunction(vm, "heapCapacity", functionHeapCapacity, 0);
    14681548        addFunction(vm, "flashHeapAccess", functionFlashHeapAccess, 0);
     1549
     1550        addFunction(vm, "loadGetterFromGetterSetter", functionLoadGetterFromGetterSetter, 1);
     1551        addFunction(vm, "createCustomTestGetterSetter", functionCreateCustomTestGetterSetter, 1);
    14691552    }
    14701553   
    … …  
    28332916}
    28342917
     2918EncodedJSValue JSC_HOST_CALL functionLoadGetterFromGetterSetter(ExecState* exec)
     2919{
     2920    VM& vm = exec->vm();
     2921    RELEASE_ASSERT(exec->argumentCount() >= 1);
     2922    GetterSetter* getterSetter = jsDynamicCast<GetterSetter*>(vm, exec->argument(0));
     2923    RELEASE_ASSERT(getterSetter);
     2924    JSObject* getter = getterSetter->getter();
     2925    RELEASE_ASSERT(getter);
     2926    return JSValue::encode(getter);
     2927}
     2928
     2929EncodedJSValue JSC_HOST_CALL functionCreateCustomTestGetterSetter(ExecState* exec)
     2930{
     2931    VM& vm = exec->vm();
     2932    JSGlobalObject* globalObject = exec->lexicalGlobalObject();
     2933    return JSValue::encode(JSTestCustomGetterSetter::create(vm, globalObject, JSTestCustomGetterSetter::createStructure(vm, globalObject)));
     2934}
     2935
    28352936template<typename ValueType>
    28362937typename std::enable_if<!std::is_fundamental<ValueType>::value>::type addOption(VM&, JSObject*, Identifier, ValueType) { }

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

    r222473 r222671  
    2727#include "ScopeOffset.h"
    2828#include <wtf/Assertions.h>
     29#include <wtf/ForbidHeapAllocation.h>
    2930
    3031namespace JSC {
    … …  
    8182
    8283class PropertySlot {
     84
     85    // We rely on PropertySlot being stack allocated when used. This is needed
     86    // because we rely on some of its fields being a GC root. For example, it
     87    // may be the only thing that points to the CustomGetterSetter property it has.
     88    WTF_FORBID_HEAP_ALLOCATION;
     89
    8390    enum PropertyType : uint8_t {
    8491        TypeUnset,
    … …  
    291298    {
    292299        ASSERT(attributes == attributesForStructure(attributes));
     300
     301        disableCaching();
    293302
    294303        ASSERT(getterSetter);
Note: See TracChangeset for help on using the changeset viewer.