Changeset 154113 in webkit for trunk/Source/JavaScriptCore

Timestamp:

Aug 15, 2013, 11:49:05 AM (12 years ago)

Author:

[email protected]

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)

Location:

trunk/Source/JavaScriptCore

Files:

• API/JSCallbackObjectFunctions.h (modified) (2 diffs)
• ChangeLog (modified) (1 diff)
• JSCTypedArrayStubs.h (modified) (2 diffs)
• runtime/Arguments.cpp (modified) (3 diffs)
• runtime/JSActivation.cpp (modified) (2 diffs)
• runtime/JSArray.cpp (modified) (1 diff)
• runtime/JSObject.cpp (modified) (3 diffs)
• runtime/JSString.h (modified) (3 diffs)
• runtime/JSSymbolTableObject.h (modified) (2 diffs)
• runtime/PropertySlot.h (modified) (2 diffs)
• runtime/SparseArrayValueMap.cpp (modified) (1 diff)

trunk/Source/JavaScriptCore/API/JSCallbackObjectFunctions.h

@@ -152 +152 @@
                 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;
                 }
@@ -165 +165 @@
                     JSValue value = thisObject->getStaticValue(exec, propertyName);
                     if (value) {
-                        slot.setValue(value);
+                        slot.setValue(thisObject, value);
                         return true;
                     }

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+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]>
 

trunk/Source/JavaScriptCore/JSCTypedArrayStubs.h

@@ -108 +108 @@
     if (index < thisObject->m_storageLength) {\
         ASSERT(index != PropertyName::NotAnIndex);\
-        slot.setValue(thisObject->getByIndex(exec, index));\
+        slot.setValue(thisObject, thisObject->getByIndex(exec, index));\
         return true;\
     }\
@@ -132 +132 @@
     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;\
     }\

trunk/Source/JavaScriptCore/runtime/Arguments.cpp

@@ -95 +95 @@
     Arguments* thisObject = jsCast<Arguments*>(object);
     if (JSValue value = thisObject->tryGetArgument(i)) {
-        slot.setValue(value);
+        slot.setValue(thisObject, value);
         return true;
     }
@@ -130 +130 @@
     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;
     }
@@ -141 +141 @@
     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;
         }

trunk/Source/JavaScriptCore/runtime/JSActivation.cpp

@@ -67 +67 @@
         return false;
 
-    slot.setValue(registerAt(entry.getIndex()).get());
+    slot.setValue(this, registerAt(entry.getIndex()).get());
     return true;
 }
@@ -167 +167 @@
 
     if (JSValue value = thisObject->getDirect(exec->vm(), propertyName)) {
-        slot.setValue(value);
+        slot.setValue(thisObject, value);
         return true;
     }

trunk/Source/JavaScriptCore/runtime/JSArray.cpp

@@ -182 +182 @@
     JSArray* thisObject = jsCast<JSArray*>(object);
     if (propertyName == exec->propertyNames().length) {
-        slot.setValue(jsNumber(thisObject->length()));
+        slot.setValue(thisObject, jsNumber(thisObject->length()));
         return true;
     }

trunk/Source/JavaScriptCore/runtime/JSObject.cpp

@@ -290 +290 @@
         JSValue value = butterfly->contiguous()[i].get();
         if (value) {
-            slot.setValue(value);
+            slot.setValue(thisObject, value);
             return true;
         }
@@ -304 +304 @@
         double value = butterfly->contiguousDouble()[i];
         if (value == value) {
-            slot.setValue(JSValue(JSValue::EncodeAsDouble, value));
+            slot.setValue(thisObject, JSValue(JSValue::EncodeAsDouble, value));
             return true;
         }
@@ -319 +319 @@
             JSValue value = storage->m_vector[i].get();
             if (value) {
-                slot.setValue(value);
+                slot.setValue(thisObject, value);
                 return true;
             }

trunk/Source/JavaScriptCore/runtime/JSString.h

@@ -474 +474 @@
     {
         if (propertyName == exec->propertyNames().length) {
-            slot.setValue(jsNumber(m_length));
+            slot.setValue(this, jsNumber(m_length));
             return true;
         }
@@ -481 +481 @@
         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;
         }
@@ -491 +491 @@
     {
         if (propertyName < m_length) {
-            slot.setValue(getIndex(exec, propertyName));
+            slot.setValue(this, getIndex(exec, propertyName));
             return true;
         }

trunk/Source/JavaScriptCore/runtime/JSSymbolTableObject.h

@@ -80 +80 @@
     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;
 }
@@ -112 +112 @@
     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;

trunk/Source/JavaScriptCore/runtime/PropertySlot.h

@@ -102 +102 @@
     }
 
-    void setValue(JSValue value)
+    void setValue(JSString*, JSValue value)
     {
         ASSERT(value);
@@ -170 +170 @@
     void setUndefined()
     {
-        setValue(jsUndefined());
+        m_data.value = JSValue::encode(jsUndefined());
+
+        m_slotBase = 0;
+        m_propertyType = TypeValue;
+        m_offset = invalidOffset;
     }
 

trunk/Source/JavaScriptCore/runtime/SparseArrayValueMap.cpp

@@ -129 +129 @@
 
     if (LIKELY(!value.isGetterSetter())) {
-        slot.setValue(value);
+        slot.setValue(thisObject, value);
         return;
     }