From ad2752807b14cf3602367b56494870a4801ba5da Mon Sep 17 00:00:00 2001 From: Mark Hahnenberg Date: Wed, 19 Jun 2013 11:43:08 +0200 Subject: get_by_pname can become confused when iterating over objects with static properties https://bugs.webkit.org/show_bug.cgi?id=113831 Reviewed by Geoffrey Garen. get_by_pname doesn't take static properties into account when using a JSPropertyNameIterator to directly access an object's backing store. One way to fix this is to not cache any properties when iterating over objects with static properties. This patch fixes the bug that was originally reported on swisscom.ch. Source/JavaScriptCore: * runtime/JSObject.cpp: (JSC::JSObject::getOwnNonIndexPropertyNames): * runtime/JSPropertyNameIterator.cpp: (JSC::JSPropertyNameIterator::create): * runtime/PropertyNameArray.h: (JSC::PropertyNameArray::PropertyNameArray): (JSC::PropertyNameArray::numCacheableSlots): (JSC::PropertyNameArray::setNumCacheableSlots): (PropertyNameArray): Change-Id: I7ae9c48eea3c5300c4825a10a660b0e2210c8862 git-svn-id: http://svn.webkit.org/repository/webkit/trunk@147570 268f45cc-cd09-0410-ab3c-d52691b4dbfc Reviewed-by: Jocelyn Turcotte --- Source/JavaScriptCore/runtime/JSObject.cpp | 3 +++ Source/JavaScriptCore/runtime/JSPropertyNameIterator.cpp | 2 +- Source/JavaScriptCore/runtime/PropertyNameArray.h | 6 ++++++ 3 files changed, 10 insertions(+), 1 deletion(-) (limited to 'Source/JavaScriptCore') diff --git a/Source/JavaScriptCore/runtime/JSObject.cpp b/Source/JavaScriptCore/runtime/JSObject.cpp index 32adefd2f..72cbb022a 100644 --- a/Source/JavaScriptCore/runtime/JSObject.cpp +++ b/Source/JavaScriptCore/runtime/JSObject.cpp @@ -1513,7 +1513,10 @@ void JSObject::getOwnPropertyNames(JSObject* object, ExecState* exec, PropertyNa void JSObject::getOwnNonIndexPropertyNames(JSObject* object, ExecState* exec, PropertyNameArray& propertyNames, EnumerationMode mode) { getClassPropertyNames(exec, object->classInfo(), propertyNames, mode, object->staticFunctionsReified()); + size_t preStructurePropertyNamesCount = propertyNames.size(); object->structure()->getPropertyNamesFromStructure(exec->globalData(), propertyNames, mode); + size_t numCacheableSlots = preStructurePropertyNamesCount ? 0 : propertyNames.size(); + propertyNames.setNumCacheableSlots(numCacheableSlots); } double JSObject::toNumber(ExecState* exec) const diff --git a/Source/JavaScriptCore/runtime/JSPropertyNameIterator.cpp b/Source/JavaScriptCore/runtime/JSPropertyNameIterator.cpp index 225401fbd..496799501 100644 --- a/Source/JavaScriptCore/runtime/JSPropertyNameIterator.cpp +++ b/Source/JavaScriptCore/runtime/JSPropertyNameIterator.cpp @@ -54,7 +54,7 @@ JSPropertyNameIterator* JSPropertyNameIterator::create(ExecState* exec, JSObject size_t numCacheableSlots = 0; if (!o->structure()->hasNonEnumerableProperties() && !o->structure()->hasGetterSetterProperties() && !o->structure()->isUncacheableDictionary() && !o->structure()->typeInfo().overridesGetPropertyNames()) - numCacheableSlots = o->structure()->totalStorageSize(); + numCacheableSlots = propertyNames.numCacheableSlots(); JSPropertyNameIterator* jsPropertyNameIterator = new (NotNull, allocateCell(*exec->heap())) JSPropertyNameIterator(exec, propertyNames.data(), numCacheableSlots); jsPropertyNameIterator->finishCreation(exec, propertyNames.data(), o); diff --git a/Source/JavaScriptCore/runtime/PropertyNameArray.h b/Source/JavaScriptCore/runtime/PropertyNameArray.h index 89b1af00b..30f439bb2 100644 --- a/Source/JavaScriptCore/runtime/PropertyNameArray.h +++ b/Source/JavaScriptCore/runtime/PropertyNameArray.h @@ -55,12 +55,14 @@ namespace JSC { PropertyNameArray(JSGlobalData* globalData) : m_data(PropertyNameArrayData::create()) , m_globalData(globalData) + , m_numCacheableSlots(0) { } PropertyNameArray(ExecState* exec) : m_data(PropertyNameArrayData::create()) , m_globalData(&exec->globalData()) + , m_numCacheableSlots(0) { } @@ -83,12 +85,16 @@ namespace JSC { const_iterator begin() const { return m_data->propertyNameVector().begin(); } const_iterator end() const { return m_data->propertyNameVector().end(); } + size_t numCacheableSlots() const { return m_numCacheableSlots; } + void setNumCacheableSlots(size_t numCacheableSlots) { m_numCacheableSlots = numCacheableSlots; } + private: typedef HashSet > IdentifierSet; RefPtr m_data; IdentifierSet m_set; JSGlobalData* m_globalData; + size_t m_numCacheableSlots; }; } // namespace JSC -- cgit v1.2.1 From 55e3e0bb1d8b1487df36219869ef5bd302b8640c Mon Sep 17 00:00:00 2001 From: Mark Hahnenberg Date: Wed, 19 Jun 2013 11:43:53 +0200 Subject: JSObject::getOwnNonIndexPropertyNames calculates numCacheableSlots incorrectly https://bugs.webkit.org/show_bug.cgi?id=114235 Reviewed by Geoffrey Garen. Due to the way that numCacheableSlots is currently calculated, checking an object's prototype for enumerable properties causes us not to cache any properties at all. We should only cache properties on the object itself since we currently don't take advantage of any sort of name caching for properties in the prototype chain. This fix undoes a ~2% SunSpider regression caused by http://trac.webkit.org/changeset/147570. * runtime/JSObject.cpp: (JSC::JSObject::getOwnNonIndexPropertyNames): Change-Id: I5853ab567cd0a8cd20aeac1372ec64fc4f25df1a git-svn-id: http://svn.webkit.org/repository/webkit/trunk@148036 268f45cc-cd09-0410-ab3c-d52691b4dbfc Reviewed-by: Jocelyn Turcotte --- Source/JavaScriptCore/runtime/JSObject.cpp | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) (limited to 'Source/JavaScriptCore') diff --git a/Source/JavaScriptCore/runtime/JSObject.cpp b/Source/JavaScriptCore/runtime/JSObject.cpp index 72cbb022a..290a3ab16 100644 --- a/Source/JavaScriptCore/runtime/JSObject.cpp +++ b/Source/JavaScriptCore/runtime/JSObject.cpp @@ -1513,10 +1513,12 @@ void JSObject::getOwnPropertyNames(JSObject* object, ExecState* exec, PropertyNa void JSObject::getOwnNonIndexPropertyNames(JSObject* object, ExecState* exec, PropertyNameArray& propertyNames, EnumerationMode mode) { getClassPropertyNames(exec, object->classInfo(), propertyNames, mode, object->staticFunctionsReified()); - size_t preStructurePropertyNamesCount = propertyNames.size(); + + bool canCachePropertiesFromStructure = !propertyNames.size(); object->structure()->getPropertyNamesFromStructure(exec->globalData(), propertyNames, mode); - size_t numCacheableSlots = preStructurePropertyNamesCount ? 0 : propertyNames.size(); - propertyNames.setNumCacheableSlots(numCacheableSlots); + + if (canCachePropertiesFromStructure) + propertyNames.setNumCacheableSlots(propertyNames.size()); } double JSObject::toNumber(ExecState* exec) const -- cgit v1.2.1 From 86a59036393fb081f094325518205e6c6067b05e Mon Sep 17 00:00:00 2001 From: Mark Hahnenberg Date: Mon, 1 Jul 2013 15:46:30 +0200 Subject: JSObject::getOwnNonIndexPropertyNames calculates numCacheableSlots incorrectly (2/2) https://bugs.webkit.org/show_bug.cgi?id=114235 Reviewed by Filip Pizlo. If the object doesn't have any properties but the prototype does, we'll assume those prototype properties are accessible in the base object's backing store, which is bad. Source/JavaScriptCore: * runtime/JSObject.cpp: (JSC::JSObject::getPropertyNames): (JSC::JSObject::getOwnNonIndexPropertyNames): * runtime/PropertyNameArray.h: (JSC::PropertyNameArray::PropertyNameArray): (JSC::PropertyNameArray::setNumCacheableSlotsForObject): (JSC::PropertyNameArray::setBaseObject): (PropertyNameArray): Change-Id: If61b609438fa1d62364bac556af635413198d8ad git-svn-id: http://svn.webkit.org/repository/webkit/trunk@148142 268f45cc-cd09-0410-ab3c-d52691b4dbfc Reviewed-by: Jocelyn Turcotte --- Source/JavaScriptCore/runtime/JSObject.cpp | 3 ++- Source/JavaScriptCore/runtime/PropertyNameArray.h | 16 +++++++++++++++- 2 files changed, 17 insertions(+), 2 deletions(-) (limited to 'Source/JavaScriptCore') diff --git a/Source/JavaScriptCore/runtime/JSObject.cpp b/Source/JavaScriptCore/runtime/JSObject.cpp index 290a3ab16..e6f95bdfa 100644 --- a/Source/JavaScriptCore/runtime/JSObject.cpp +++ b/Source/JavaScriptCore/runtime/JSObject.cpp @@ -1423,6 +1423,7 @@ bool JSObject::getPropertySpecificValue(ExecState* exec, PropertyName propertyNa void JSObject::getPropertyNames(JSObject* object, ExecState* exec, PropertyNameArray& propertyNames, EnumerationMode mode) { + propertyNames.setBaseObject(object); object->methodTable()->getOwnPropertyNames(object, exec, propertyNames, mode); if (object->prototype().isNull()) @@ -1518,7 +1519,7 @@ void JSObject::getOwnNonIndexPropertyNames(JSObject* object, ExecState* exec, Pr object->structure()->getPropertyNamesFromStructure(exec->globalData(), propertyNames, mode); if (canCachePropertiesFromStructure) - propertyNames.setNumCacheableSlots(propertyNames.size()); + propertyNames.setNumCacheableSlotsForObject(object, propertyNames.size()); } double JSObject::toNumber(ExecState* exec) const diff --git a/Source/JavaScriptCore/runtime/PropertyNameArray.h b/Source/JavaScriptCore/runtime/PropertyNameArray.h index 30f439bb2..1cdac0049 100644 --- a/Source/JavaScriptCore/runtime/PropertyNameArray.h +++ b/Source/JavaScriptCore/runtime/PropertyNameArray.h @@ -56,6 +56,7 @@ namespace JSC { : m_data(PropertyNameArrayData::create()) , m_globalData(globalData) , m_numCacheableSlots(0) + , m_baseObject(0) { } @@ -63,6 +64,7 @@ namespace JSC { : m_data(PropertyNameArrayData::create()) , m_globalData(&exec->globalData()) , m_numCacheableSlots(0) + , m_baseObject(0) { } @@ -86,7 +88,18 @@ namespace JSC { const_iterator end() const { return m_data->propertyNameVector().end(); } size_t numCacheableSlots() const { return m_numCacheableSlots; } - void setNumCacheableSlots(size_t numCacheableSlots) { m_numCacheableSlots = numCacheableSlots; } + void setNumCacheableSlotsForObject(JSObject* object, size_t numCacheableSlots) + { + if (object != m_baseObject) + return; + m_numCacheableSlots = numCacheableSlots; + } + void setBaseObject(JSObject* object) + { + if (m_baseObject) + return; + m_baseObject = object; + } private: typedef HashSet > IdentifierSet; @@ -95,6 +108,7 @@ namespace JSC { IdentifierSet m_set; JSGlobalData* m_globalData; size_t m_numCacheableSlots; + JSObject* m_baseObject; }; } // namespace JSC -- cgit v1.2.1