- Revision
- 154113
- Author
- [email protected]
- Date
- 2013-08-15 11:49:05 -0700 (Thu, 15 Aug 2013)
Log Message
https://bugs.webkit.org/show_bug.cgi?id=119843
PropertySlot::setValue is ambiguous
Reviewed by Geoff Garen.
There are three different versions of PropertySlot::setValue, one for cacheable properties, and two that are used interchangeably and inconsistently.
The problematic variants are the ones that just take a value, and one that takes a value and also the object containing the property.
Unify on always providing the object, and remove the version that just takes a value.
This always works except for JSString, where we optimize out the object (logically we should be instantiating a temporary StringObject on every property access).
Provide a version of setValue that takes a JSString as the owner of the property.
We won't store this, but it makes it clear that this interface should only be used from JSString.
* API/JSCallbackObjectFunctions.h:
(JSC::::getOwnPropertySlot):
* JSCTypedArrayStubs.h:
* runtime/Arguments.cpp:
(JSC::Arguments::getOwnPropertySlotByIndex):
(JSC::Arguments::getOwnPropertySlot):
* runtime/JSActivation.cpp:
(JSC::JSActivation::symbolTableGet):
(JSC::JSActivation::getOwnPropertySlot):
* runtime/JSArray.cpp:
(JSC::JSArray::getOwnPropertySlot):
* runtime/JSObject.cpp:
(JSC::JSObject::getOwnPropertySlotByIndex):
* runtime/JSString.h:
(JSC::JSString::getStringPropertySlot):
* runtime/JSSymbolTableObject.h:
(JSC::symbolTableGet):
* runtime/SparseArrayValueMap.cpp:
(JSC::SparseArrayEntry::get):
- Pass object containing property to PropertySlot::setValue
* runtime/PropertySlot.h:
(JSC::PropertySlot::setValue):
- Logically, the base of a string property access is a temporary StringObject, but we optimize that away.
(JSC::PropertySlot::setUndefined):
- removed setValue(JSValue), added setValue(JSString*, JSValue)
Modified Paths
Diff
Modified: trunk/Source/_javascript_Core/API/JSCallbackObjectFunctions.h (154112 => 154113)
--- trunk/Source/_javascript_Core/API/JSCallbackObjectFunctions.h 2013-08-15 18:48:27 UTC (rev 154112)
+++ trunk/Source/_javascript_Core/API/JSCallbackObjectFunctions.h 2013-08-15 18:49:05 UTC (rev 154113)
@@ -151,11 +151,11 @@
}
if (exception) {
throwError(exec, toJS(exec, exception));
- slot.setValue(jsUndefined());
+ slot.setValue(thisObject, jsUndefined());
return true;
}
if (value) {
- slot.setValue(toJS(exec, value));
+ slot.setValue(thisObject, toJS(exec, value));
return true;
}
}
@@ -164,7 +164,7 @@
if (staticValues->contains(name)) {
JSValue value = thisObject->getStaticValue(exec, propertyName);
if (value) {
- slot.setValue(value);
+ slot.setValue(thisObject, value);
return true;
}
}
Modified: trunk/Source/_javascript_Core/ChangeLog (154112 => 154113)
--- trunk/Source/_javascript_Core/ChangeLog 2013-08-15 18:48:27 UTC (rev 154112)
+++ trunk/Source/_javascript_Core/ChangeLog 2013-08-15 18:49:05 UTC (rev 154113)
@@ -1,3 +1,43 @@
+2013-08-15 Gavin Barraclough <[email protected]>
+
+ https://bugs.webkit.org/show_bug.cgi?id=119843
+ PropertySlot::setValue is ambiguous
+
+ Reviewed by Geoff Garen.
+
+ There are three different versions of PropertySlot::setValue, one for cacheable properties, and two that are used interchangeably and inconsistently.
+ The problematic variants are the ones that just take a value, and one that takes a value and also the object containing the property.
+ Unify on always providing the object, and remove the version that just takes a value.
+ This always works except for JSString, where we optimize out the object (logically we should be instantiating a temporary StringObject on every property access).
+ Provide a version of setValue that takes a JSString as the owner of the property.
+ We won't store this, but it makes it clear that this interface should only be used from JSString.
+
+ * API/JSCallbackObjectFunctions.h:
+ (JSC::::getOwnPropertySlot):
+ * JSCTypedArrayStubs.h:
+ * runtime/Arguments.cpp:
+ (JSC::Arguments::getOwnPropertySlotByIndex):
+ (JSC::Arguments::getOwnPropertySlot):
+ * runtime/JSActivation.cpp:
+ (JSC::JSActivation::symbolTableGet):
+ (JSC::JSActivation::getOwnPropertySlot):
+ * runtime/JSArray.cpp:
+ (JSC::JSArray::getOwnPropertySlot):
+ * runtime/JSObject.cpp:
+ (JSC::JSObject::getOwnPropertySlotByIndex):
+ * runtime/JSString.h:
+ (JSC::JSString::getStringPropertySlot):
+ * runtime/JSSymbolTableObject.h:
+ (JSC::symbolTableGet):
+ * runtime/SparseArrayValueMap.cpp:
+ (JSC::SparseArrayEntry::get):
+ - Pass object containing property to PropertySlot::setValue
+ * runtime/PropertySlot.h:
+ (JSC::PropertySlot::setValue):
+ - Logically, the base of a string property access is a temporary StringObject, but we optimize that away.
+ (JSC::PropertySlot::setUndefined):
+ - removed setValue(JSValue), added setValue(JSString*, JSValue)
+
2013-08-15 Oliver Hunt <[email protected]>
Remove bogus assertion.
Modified: trunk/Source/_javascript_Core/JSCTypedArrayStubs.h (154112 => 154113)
--- trunk/Source/_javascript_Core/JSCTypedArrayStubs.h 2013-08-15 18:48:27 UTC (rev 154112)
+++ trunk/Source/_javascript_Core/JSCTypedArrayStubs.h 2013-08-15 18:49:05 UTC (rev 154113)
@@ -107,7 +107,7 @@
unsigned index = propertyName.asIndex();\
if (index < thisObject->m_storageLength) {\
ASSERT(index != PropertyName::NotAnIndex);\
- slot.setValue(thisObject->getByIndex(exec, index));\
+ slot.setValue(thisObject, thisObject->getByIndex(exec, index));\
return true;\
}\
return Base::getOwnPropertySlot(object, exec, propertyName, slot);\
@@ -131,7 +131,7 @@
JS##name##Array* thisObject = jsCast<JS##name##Array*>(object);\
ASSERT_GC_OBJECT_INHERITS(thisObject, info());\
if (propertyName < thisObject->m_storageLength) {\
- slot.setValue(thisObject->getByIndex(exec, propertyName));\
+ slot.setValue(thisObject, thisObject->getByIndex(exec, propertyName));\
return true;\
}\
return thisObject->methodTable()->getOwnPropertySlot(thisObject, exec, Identifier::from(exec, propertyName), slot);\
Modified: trunk/Source/_javascript_Core/runtime/Arguments.cpp (154112 => 154113)
--- trunk/Source/_javascript_Core/runtime/Arguments.cpp 2013-08-15 18:48:27 UTC (rev 154112)
+++ trunk/Source/_javascript_Core/runtime/Arguments.cpp 2013-08-15 18:49:05 UTC (rev 154113)
@@ -94,7 +94,7 @@
{
Arguments* thisObject = jsCast<Arguments*>(object);
if (JSValue value = thisObject->tryGetArgument(i)) {
- slot.setValue(value);
+ slot.setValue(thisObject, value);
return true;
}
@@ -129,18 +129,18 @@
unsigned i = propertyName.asIndex();
if (JSValue value = thisObject->tryGetArgument(i)) {
RELEASE_ASSERT(i < PropertyName::NotAnIndex);
- slot.setValue(value);
+ slot.setValue(thisObject, value);
return true;
}
if (propertyName == exec->propertyNames().length && LIKELY(!thisObject->m_overrodeLength)) {
- slot.setValue(jsNumber(thisObject->m_numArguments));
+ slot.setValue(thisObject, jsNumber(thisObject->m_numArguments));
return true;
}
if (propertyName == exec->propertyNames().callee && LIKELY(!thisObject->m_overrodeCallee)) {
if (!thisObject->m_isStrictMode) {
- slot.setValue(thisObject->m_callee.get());
+ slot.setValue(thisObject, thisObject->m_callee.get());
return true;
}
thisObject->createStrictModeCalleeIfNecessary(exec);
Modified: trunk/Source/_javascript_Core/runtime/JSActivation.cpp (154112 => 154113)
--- trunk/Source/_javascript_Core/runtime/JSActivation.cpp 2013-08-15 18:48:27 UTC (rev 154112)
+++ trunk/Source/_javascript_Core/runtime/JSActivation.cpp 2013-08-15 18:49:05 UTC (rev 154113)
@@ -66,7 +66,7 @@
if (isTornOff() && !isValid(entry))
return false;
- slot.setValue(registerAt(entry.getIndex()).get());
+ slot.setValue(this, registerAt(entry.getIndex()).get());
return true;
}
@@ -166,7 +166,7 @@
return true;
if (JSValue value = thisObject->getDirect(exec->vm(), propertyName)) {
- slot.setValue(value);
+ slot.setValue(thisObject, value);
return true;
}
Modified: trunk/Source/_javascript_Core/runtime/JSArray.cpp (154112 => 154113)
--- trunk/Source/_javascript_Core/runtime/JSArray.cpp 2013-08-15 18:48:27 UTC (rev 154112)
+++ trunk/Source/_javascript_Core/runtime/JSArray.cpp 2013-08-15 18:49:05 UTC (rev 154113)
@@ -181,7 +181,7 @@
{
JSArray* thisObject = jsCast<JSArray*>(object);
if (propertyName == exec->propertyNames().length) {
- slot.setValue(jsNumber(thisObject->length()));
+ slot.setValue(thisObject, jsNumber(thisObject->length()));
return true;
}
Modified: trunk/Source/_javascript_Core/runtime/JSObject.cpp (154112 => 154113)
--- trunk/Source/_javascript_Core/runtime/JSObject.cpp 2013-08-15 18:48:27 UTC (rev 154112)
+++ trunk/Source/_javascript_Core/runtime/JSObject.cpp 2013-08-15 18:49:05 UTC (rev 154113)
@@ -289,7 +289,7 @@
JSValue value = butterfly->contiguous()[i].get();
if (value) {
- slot.setValue(value);
+ slot.setValue(thisObject, value);
return true;
}
@@ -303,7 +303,7 @@
double value = butterfly->contiguousDouble()[i];
if (value == value) {
- slot.setValue(JSValue(JSValue::EncodeAsDouble, value));
+ slot.setValue(thisObject, JSValue(JSValue::EncodeAsDouble, value));
return true;
}
@@ -318,7 +318,7 @@
if (i < storage->vectorLength()) {
JSValue value = storage->m_vector[i].get();
if (value) {
- slot.setValue(value);
+ slot.setValue(thisObject, value);
return true;
}
} else if (SparseArrayValueMap* map = storage->m_sparseMap.get()) {
Modified: trunk/Source/_javascript_Core/runtime/JSString.h (154112 => 154113)
--- trunk/Source/_javascript_Core/runtime/JSString.h 2013-08-15 18:48:27 UTC (rev 154112)
+++ trunk/Source/_javascript_Core/runtime/JSString.h 2013-08-15 18:49:05 UTC (rev 154113)
@@ -473,14 +473,14 @@
ALWAYS_INLINE bool JSString::getStringPropertySlot(ExecState* exec, PropertyName propertyName, PropertySlot& slot)
{
if (propertyName == exec->propertyNames().length) {
- slot.setValue(jsNumber(m_length));
+ slot.setValue(this, jsNumber(m_length));
return true;
}
unsigned i = propertyName.asIndex();
if (i < m_length) {
ASSERT(i != PropertyName::NotAnIndex); // No need for an explicit check, the above test would always fail!
- slot.setValue(getIndex(exec, i));
+ slot.setValue(this, getIndex(exec, i));
return true;
}
@@ -490,7 +490,7 @@
ALWAYS_INLINE bool JSString::getStringPropertySlot(ExecState* exec, unsigned propertyName, PropertySlot& slot)
{
if (propertyName < m_length) {
- slot.setValue(getIndex(exec, propertyName));
+ slot.setValue(this, getIndex(exec, propertyName));
return true;
}
Modified: trunk/Source/_javascript_Core/runtime/JSSymbolTableObject.h (154112 => 154113)
--- trunk/Source/_javascript_Core/runtime/JSSymbolTableObject.h 2013-08-15 18:48:27 UTC (rev 154112)
+++ trunk/Source/_javascript_Core/runtime/JSSymbolTableObject.h 2013-08-15 18:49:05 UTC (rev 154113)
@@ -79,7 +79,7 @@
return false;
SymbolTableEntry::Fast entry = iter->value;
ASSERT(!entry.isNull());
- slot.setValue(object->registerAt(entry.getIndex()).get());
+ slot.setValue(object, object->registerAt(entry.getIndex()).get());
return true;
}
@@ -111,7 +111,7 @@
return false;
SymbolTableEntry::Fast entry = iter->value;
ASSERT(!entry.isNull());
- slot.setValue(object->registerAt(entry.getIndex()).get());
+ slot.setValue(object, object->registerAt(entry.getIndex()).get());
slotIsWriteable = !entry.isReadOnly();
return true;
}
Modified: trunk/Source/_javascript_Core/runtime/PropertySlot.h (154112 => 154113)
--- trunk/Source/_javascript_Core/runtime/PropertySlot.h 2013-08-15 18:48:27 UTC (rev 154112)
+++ trunk/Source/_javascript_Core/runtime/PropertySlot.h 2013-08-15 18:49:05 UTC (rev 154113)
@@ -101,7 +101,7 @@
m_offset = offset;
}
- void setValue(JSValue value)
+ void setValue(JSString*, JSValue value)
{
ASSERT(value);
m_data.value = JSValue::encode(value);
@@ -169,7 +169,11 @@
void setUndefined()
{
- setValue(jsUndefined());
+ m_data.value = JSValue::encode(jsUndefined());
+
+ m_slotBase = 0;
+ m_propertyType = TypeValue;
+ m_offset = invalidOffset;
}
private:
Modified: trunk/Source/_javascript_Core/runtime/SparseArrayValueMap.cpp (154112 => 154113)
--- trunk/Source/_javascript_Core/runtime/SparseArrayValueMap.cpp 2013-08-15 18:48:27 UTC (rev 154112)
+++ trunk/Source/_javascript_Core/runtime/SparseArrayValueMap.cpp 2013-08-15 18:49:05 UTC (rev 154113)
@@ -128,7 +128,7 @@
ASSERT(value);
if (LIKELY(!value.isGetterSetter())) {
- slot.setValue(value);
+ slot.setValue(thisObject, value);
return;
}
