- Revision
- 222671
- Author
- [email protected]
- Date
- 2017-09-29 16:48:10 -0700 (Fri, 29 Sep 2017)
Log 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/_javascript_Core:
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):
Modified Paths
Added Paths
Diff
Modified: trunk/JSTests/ChangeLog (222670 => 222671)
--- trunk/JSTests/ChangeLog 2017-09-29 23:42:03 UTC (rev 222670)
+++ trunk/JSTests/ChangeLog 2017-09-29 23:48:10 UTC (rev 222671)
@@ -1,3 +1,16 @@
+2017-09-29 Saam Barati <[email protected]>
+
+ 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 <[email protected]>
Unreviewed, rolling out r222563, r222565, and r222581.
Added: trunk/JSTests/stress/custom-get-set-inline-caching-one-level-up-proto-chain.js (0 => 222671)
--- trunk/JSTests/stress/custom-get-set-inline-caching-one-level-up-proto-chain.js (rev 0)
+++ trunk/JSTests/stress/custom-get-set-inline-caching-one-level-up-proto-chain.js 2017-09-29 23:48:10 UTC (rev 222671)
@@ -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);
+ }
+}
Modified: trunk/Source/_javascript_Core/ChangeLog (222670 => 222671)
--- trunk/Source/_javascript_Core/ChangeLog 2017-09-29 23:42:03 UTC (rev 222670)
+++ trunk/Source/_javascript_Core/ChangeLog 2017-09-29 23:48:10 UTC (rev 222671)
@@ -1,3 +1,60 @@
+2017-09-29 Saam Barati <[email protected]>
+
+ 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 <[email protected]>
Unreviewed, rolling out r222563, r222565, and r222581.
Modified: trunk/Source/_javascript_Core/bytecode/ObjectPropertyConditionSet.cpp (222670 => 222671)
--- trunk/Source/_javascript_Core/bytecode/ObjectPropertyConditionSet.cpp 2017-09-29 23:42:03 UTC (rev 222670)
+++ trunk/Source/_javascript_Core/bytecode/ObjectPropertyConditionSet.cpp 2017-09-29 23:48:10 UTC (rev 222671)
@@ -361,24 +361,6 @@
});
}
-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)
{
Modified: trunk/Source/_javascript_Core/bytecode/ObjectPropertyConditionSet.h (222670 => 222671)
--- trunk/Source/_javascript_Core/bytecode/ObjectPropertyConditionSet.h 2017-09-29 23:42:03 UTC (rev 222670)
+++ trunk/Source/_javascript_Core/bytecode/ObjectPropertyConditionSet.h 2017-09-29 23:48:10 UTC (rev 222671)
@@ -165,9 +165,6 @@
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,
Modified: trunk/Source/_javascript_Core/jit/Repatch.cpp (222670 => 222671)
--- trunk/Source/_javascript_Core/jit/Repatch.cpp 2017-09-29 23:42:03 UTC (rev 222670)
+++ trunk/Source/_javascript_Core/jit/Repatch.cpp 2017-09-29 23:48:10 UTC (rev 222671)
@@ -446,7 +446,7 @@
if (slot.base() != baseValue) {
conditionSet =
- generateConditionsForPrototypePropertyHitCustom(
+ generateConditionsForPrototypePropertyHit(
vm, codeBlock, exec, structure, slot.base(), ident.impl());
if (!conditionSet.isValid())
return GiveUpOnCache;
Modified: trunk/Source/_javascript_Core/jsc.cpp (222670 => 222671)
--- trunk/Source/_javascript_Core/jsc.cpp 2017-09-29 23:42:03 UTC (rev 222670)
+++ trunk/Source/_javascript_Core/jsc.cpp 2017-09-29 23:48:10 UTC (rev 222671)
@@ -1057,6 +1057,84 @@
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 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 @@
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 @@
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) { }
Modified: trunk/Source/_javascript_Core/runtime/PropertySlot.h (222670 => 222671)
--- trunk/Source/_javascript_Core/runtime/PropertySlot.h 2017-09-29 23:42:03 UTC (rev 222670)
+++ trunk/Source/_javascript_Core/runtime/PropertySlot.h 2017-09-29 23:48:10 UTC (rev 222671)
@@ -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 @@
}
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 @@
{
ASSERT(attributes == attributesForStructure(attributes));
+ disableCaching();
+
ASSERT(getterSetter);
m_data.customAccessor.getterSetter = getterSetter;
m_attributes = attributes;
