Custom GetterSetterAccessCase does not use the correct slotBase when making call
authorsbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 29 Sep 2017 23:48:10 +0000 (23:48 +0000)
committersbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 29 Sep 2017 23:48:10 +0000 (23:48 +0000)
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):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@222671 268f45cc-cd09-0410-ab3c-d52691b4dbfc

JSTests/ChangeLog
JSTests/stress/custom-get-set-inline-caching-one-level-up-proto-chain.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/bytecode/ObjectPropertyConditionSet.cpp
Source/JavaScriptCore/bytecode/ObjectPropertyConditionSet.h
Source/JavaScriptCore/jit/Repatch.cpp
Source/JavaScriptCore/jsc.cpp
Source/JavaScriptCore/runtime/PropertySlot.h

index 830dff9..98bafd9 100644 (file)
@@ -1,3 +1,16 @@
+2017-09-29  Saam Barati  <sbarati@apple.com>
+
+        Custom GetterSetterAccessCase does not use the correct slotBase when making call
+        https://bugs.webkit.org/show_bug.cgi?id=177639
+
+        Reviewed by Geoffrey Garen.
+
+        * stress/custom-get-set-inline-caching-one-level-up-proto-chain.js: Added.
+        (assert):
+        (Class):
+        (items.forEach):
+        (set get for):
+
 2017-09-29  Commit Queue  <commit-queue@webkit.org>
 
         Unreviewed, rolling out r222563, r222565, and r222581.
diff --git a/JSTests/stress/custom-get-set-inline-caching-one-level-up-proto-chain.js b/JSTests/stress/custom-get-set-inline-caching-one-level-up-proto-chain.js
new file mode 100644 (file)
index 0000000..da6900c
--- /dev/null
@@ -0,0 +1,67 @@
+function assert(b, m) {
+    if (!b)
+        throw new Error("Bad:" + m);
+}
+
+class Class { };
+
+let items = [
+    new Class,
+    new Class,
+    new Class,
+    new Class,
+];
+
+let customGetterSetter = createCustomTestGetterSetter();
+items.forEach((x) => {
+    x.__proto__ = customGetterSetter;
+    assert(x.__proto__ === customGetterSetter);
+});
+
+function validate(x, valueResult, accessorResult) {
+    assert(x.customValue === valueResult);
+
+    assert(x.customAccessor === accessorResult);
+
+    let o = {};
+    x.customValue = o;
+    assert(o.result === valueResult);
+
+    o = {};
+    x.customAccessor = o;
+    assert(o.result === accessorResult);
+
+    assert(x.randomProp === 42 || x.randomProp === undefined);
+}
+noInline(validate);
+
+
+let start = Date.now();
+for (let i = 0; i < 10000; ++i) {
+    for (let i = 0; i < items.length; ++i) {
+        validate(items[i], customGetterSetter, items[i]);
+    }
+}
+
+customGetterSetter.randomProp = 42;
+
+for (let i = 0; i < 10000; ++i) {
+    for (let i = 0; i < items.length; ++i) {
+        validate(items[i], customGetterSetter, items[i]);
+    }
+}
+
+items.forEach((x) => {
+    Reflect.setPrototypeOf(x, {
+        get customValue() { return 42; },
+        get customAccessor() { return 22; },
+        set customValue(x) { x.result = 42; },
+        set customAccessor(x) { x.result = 22; },
+    });
+});
+
+for (let i = 0; i < 10000; ++i) {
+    for (let i = 0; i < items.length; ++i) {
+        validate(items[i], 42, 22);
+    }
+}
index bb216b4..deac1ff 100644 (file)
@@ -1,3 +1,60 @@
+2017-09-29  Saam Barati  <sbarati@apple.com>
+
+        Custom GetterSetterAccessCase does not use the correct slotBase when making call
+        https://bugs.webkit.org/show_bug.cgi?id=177639
+
+        Reviewed by Geoffrey Garen.
+
+        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):
+
 2017-09-29  Commit Queue  <commit-queue@webkit.org>
 
         Unreviewed, rolling out r222563, r222565, and r222581.
index 9679523..446684e 100644 (file)
@@ -361,24 +361,6 @@ ObjectPropertyConditionSet generateConditionsForPrototypePropertyHit(
         });
 }
 
-ObjectPropertyConditionSet generateConditionsForPrototypePropertyHitCustom(
-    VM& vm, JSCell* owner, ExecState* exec, Structure* headStructure, JSObject* prototype,
-    UniquedStringImpl* uid)
-{
-    return generateConditions(
-        vm, exec->lexicalGlobalObject(), headStructure, prototype,
-        [&] (Vector<ObjectPropertyCondition>& conditions, JSObject* object) -> bool {
-            if (object == prototype)
-                return true;
-            ObjectPropertyCondition result =
-                generateCondition(vm, owner, object, uid, PropertyCondition::Absence);
-            if (!result)
-                return false;
-            conditions.append(result);
-            return true;
-        });
-}
-
 ObjectPropertyConditionSet generateConditionsForPrototypeEquivalenceConcurrently(
     VM& vm, JSGlobalObject* globalObject, Structure* headStructure, JSObject* prototype, UniquedStringImpl* uid)
 {
index 4af18fe..10d18cb 100644 (file)
@@ -165,9 +165,6 @@ ObjectPropertyConditionSet generateConditionsForPropertySetterMiss(
 ObjectPropertyConditionSet generateConditionsForPrototypePropertyHit(
     VM&, JSCell* owner, ExecState*, Structure* headStructure, JSObject* prototype,
     UniquedStringImpl* uid);
-ObjectPropertyConditionSet generateConditionsForPrototypePropertyHitCustom(
-    VM&, JSCell* owner, ExecState*, Structure* headStructure, JSObject* prototype,
-    UniquedStringImpl* uid);
 
 ObjectPropertyConditionSet generateConditionsForPrototypeEquivalenceConcurrently(
     VM&, JSGlobalObject*, Structure* headStructure, JSObject* prototype,
index f4eb3fe..19b7f18 100644 (file)
@@ -446,7 +446,7 @@ static InlineCacheAction tryCachePutByID(const GCSafeConcurrentJSLocker& locker,
 
             if (slot.base() != baseValue) {
                 conditionSet =
-                    generateConditionsForPrototypePropertyHitCustom(
+                    generateConditionsForPrototypePropertyHit(
                         vm, codeBlock, exec, structure, slot.base(), ident.impl());
                 if (!conditionSet.isValid())
                     return GiveUpOnCache;
index e9a0d2a..181182e 100644 (file)
@@ -1057,6 +1057,84 @@ private:
     Deque<String> m_reports;
 };
 
+class JSTestCustomGetterSetter : public JSNonFinalObject {
+public:
+    using Base = JSNonFinalObject;
+    static const unsigned StructureFlags = Base::StructureFlags;
+
+    JSTestCustomGetterSetter(VM& vm, Structure* structure)
+        : Base(vm, structure)
+    { }
+
+    static JSTestCustomGetterSetter* create(VM& vm, JSGlobalObject*, Structure* structure)
+    {
+        JSTestCustomGetterSetter* result = new (NotNull, allocateCell<JSTestCustomGetterSetter>(vm.heap, sizeof(JSTestCustomGetterSetter))) JSTestCustomGetterSetter(vm, structure);
+        result->finishCreation(vm);
+        return result;
+    }
+
+    void finishCreation(VM& vm);
+
+    static Structure* createStructure(VM& vm, JSGlobalObject* globalObject)
+    {
+        return Structure::create(vm, globalObject, globalObject->objectPrototype(), TypeInfo(ObjectType, StructureFlags), info());
+    }
+
+    DECLARE_INFO;
+};
+
+
+static EncodedJSValue customGetAccessor(ExecState*, EncodedJSValue thisValue, PropertyName)
+{
+    // Passed |this|
+    return thisValue;
+}
+
+static EncodedJSValue customGetValue(ExecState* exec, EncodedJSValue slotValue, PropertyName)
+{
+    RELEASE_ASSERT(JSValue::decode(slotValue).inherits(exec->vm(), JSTestCustomGetterSetter::info()));
+    // Passed property holder.
+    return slotValue;
+}
+
+static bool customSetAccessor(ExecState* exec, EncodedJSValue thisObject, EncodedJSValue encodedValue)
+{
+    VM& vm = exec->vm();
+
+    JSValue value = JSValue::decode(encodedValue);
+    RELEASE_ASSERT(value.isObject());
+    JSObject* object = asObject(value);
+    PutPropertySlot slot(object);
+    object->put(object, exec, Identifier::fromString(&vm, "result"), JSValue::decode(thisObject), slot);
+
+    return true;
+}
+
+static bool customSetValue(ExecState* exec, EncodedJSValue slotValue, EncodedJSValue encodedValue)
+{
+    VM& vm = exec->vm();
+
+    RELEASE_ASSERT(JSValue::decode(slotValue).inherits(exec->vm(), JSTestCustomGetterSetter::info()));
+
+    JSValue value = JSValue::decode(encodedValue);
+    RELEASE_ASSERT(value.isObject());
+    JSObject* object = asObject(value);
+    PutPropertySlot slot(object);
+    object->put(object, exec, Identifier::fromString(&vm, "result"), JSValue::decode(slotValue), slot);
+
+    return true;
+}
+
+void JSTestCustomGetterSetter::finishCreation(VM& vm)
+{
+    putDirectCustomAccessor(vm, Identifier::fromString(&vm, "customValue"),
+        CustomGetterSetter::create(vm, customGetValue, customSetValue), 0);
+    putDirectCustomAccessor(vm, Identifier::fromString(&vm, "customAccessor"),
+        CustomGetterSetter::create(vm, customGetAccessor, customSetAccessor), static_cast<unsigned>(PropertyAttribute::CustomAccessor));
+}
+
+const ClassInfo JSTestCustomGetterSetter::s_info = { "JSTestCustomGetterSetter", &Base::s_info, nullptr, nullptr, CREATE_METHOD_TABLE(JSTestCustomGetterSetter) };
+
 static EncodedJSValue JSC_HOST_CALL functionCreateProxy(ExecState*);
 static EncodedJSValue JSC_HOST_CALL functionCreateRuntimeArray(ExecState*);
 static EncodedJSValue JSC_HOST_CALL functionCreateImpureGetter(ExecState*);
@@ -1177,6 +1255,8 @@ static EncodedJSValue JSC_HOST_CALL functionDollarAgentLeaving(ExecState*);
 static EncodedJSValue JSC_HOST_CALL functionWaitForReport(ExecState*);
 static EncodedJSValue JSC_HOST_CALL functionHeapCapacity(ExecState*);
 static EncodedJSValue JSC_HOST_CALL functionFlashHeapAccess(ExecState*);
+static EncodedJSValue JSC_HOST_CALL functionLoadGetterFromGetterSetter(ExecState*);
+static EncodedJSValue JSC_HOST_CALL functionCreateCustomTestGetterSetter(ExecState*);
 
 struct Script {
     enum class StrictMode {
@@ -1466,6 +1546,9 @@ protected:
 
         addFunction(vm, "heapCapacity", functionHeapCapacity, 0);
         addFunction(vm, "flashHeapAccess", functionFlashHeapAccess, 0);
+
+        addFunction(vm, "loadGetterFromGetterSetter", functionLoadGetterFromGetterSetter, 1);
+        addFunction(vm, "createCustomTestGetterSetter", functionCreateCustomTestGetterSetter, 1);
     }
     
     void addFunction(VM& vm, JSObject* object, const char* name, NativeFunction function, unsigned arguments)
@@ -2832,6 +2915,24 @@ EncodedJSValue JSC_HOST_CALL functionFlashHeapAccess(ExecState* exec)
     return JSValue::encode(jsUndefined());
 }
 
+EncodedJSValue JSC_HOST_CALL functionLoadGetterFromGetterSetter(ExecState* exec)
+{
+    VM& vm = exec->vm();
+    RELEASE_ASSERT(exec->argumentCount() >= 1);
+    GetterSetter* getterSetter = jsDynamicCast<GetterSetter*>(vm, exec->argument(0));
+    RELEASE_ASSERT(getterSetter);
+    JSObject* getter = getterSetter->getter();
+    RELEASE_ASSERT(getter);
+    return JSValue::encode(getter);
+}
+
+EncodedJSValue JSC_HOST_CALL functionCreateCustomTestGetterSetter(ExecState* exec)
+{
+    VM& vm = exec->vm();
+    JSGlobalObject* globalObject = exec->lexicalGlobalObject();
+    return JSValue::encode(JSTestCustomGetterSetter::create(vm, globalObject, JSTestCustomGetterSetter::createStructure(vm, globalObject)));
+}
+
 template<typename ValueType>
 typename std::enable_if<!std::is_fundamental<ValueType>::value>::type addOption(VM&, JSObject*, Identifier, ValueType) { }
 
index d03175a..acb9b31 100644 (file)
@@ -26,6 +26,7 @@
 #include "PropertyOffset.h"
 #include "ScopeOffset.h"
 #include <wtf/Assertions.h>
+#include <wtf/ForbidHeapAllocation.h>
 
 namespace JSC {
 class ExecState;
@@ -80,6 +81,12 @@ inline unsigned attributesForStructure(unsigned attributes)
 }
 
 class PropertySlot {
+
+    // We rely on PropertySlot being stack allocated when used. This is needed
+    // because we rely on some of its fields being a GC root. For example, it
+    // may be the only thing that points to the CustomGetterSetter property it has.
+    WTF_FORBID_HEAP_ALLOCATION;
+
     enum PropertyType : uint8_t {
         TypeUnset,
         TypeValue,
@@ -291,6 +298,8 @@ public:
     {
         ASSERT(attributes == attributesForStructure(attributes));
 
+        disableCaching();
+
         ASSERT(getterSetter);
         m_data.customAccessor.getterSetter = getterSetter;
         m_attributes = attributes;