Log Message
JSObject::getPropertySlot - index-as-propertyname, override on prototype, & shadow https://bugs.webkit.org/show_bug.cgi?id=154416
Reviewed by Geoff Garen.
Source/_javascript_Core:
Here's the bug. Suppose you call JSObject::getOwnProperty and -
- PropertyName contains an index,
- An object on the prototype chain overrides getOwnPropertySlot, and has that index property,
- The base of the access (or another object on the prototype chain) shadows that property.
JSObject::getPropertySlot is written assuming the common case is that propertyName is not an
index, and as such walks up the prototype chain looking for non-index properties before it
tries calling parseIndex.
At the point we reach an object on the prototype chain overriding getOwnPropertySlot (which
would potentially return the property) we may have already skipped over non-overriding
objects that contain the property in index storage.
* runtime/JSObject.h:
(JSC::JSObject::getOwnNonIndexPropertySlot):
- renamed from inlineGetOwnPropertySlot to better describe behaviour;
added ASSERT guarding that this method never returns index properties -
if it ever does, this is unsafe for getPropertySlot.
(JSC::JSObject::getOwnPropertySlot):
- inlineGetOwnPropertySlot -> getOwnNonIndexPropertySlot.
(JSC::JSObject::getPropertySlot):
- In case of object overriding getOwnPropertySlot check if propertyName is an index.
(JSC::JSObject::getNonIndexPropertySlot):
- called by getPropertySlot if we encounter an object that overrides getOwnPropertySlot,
in order to avoid repeated calls to parseIndex.
(JSC::JSObject::inlineGetOwnPropertySlot): Deleted.
- this was renamed to getOwnNonIndexPropertySlot.
(JSC::JSObject::fastGetOwnPropertySlot): Deleted.
- this was folded back in to getPropertySlot.
Source/WebCore:
* testing/Internals.cpp:
(WebCore::Internals::isReadableStreamDisturbed):
- fastGetOwnPropertySlot -> getOwnPropertySlot
(internal method removed; test shouldn't really have been using this anyway)
LayoutTests:
* js/index-property-shadows-overriden-get-own-property-slot-expected.txt: Added.
* js/index-property-shadows-overriden-get-own-property-slot.html: Added.
* js/script-tests/index-property-shadows-overriden-get-own-property-slot.js: Added.
(test):
- added test case.
Modified Paths
- trunk/LayoutTests/ChangeLog
- trunk/Source/_javascript_Core/ChangeLog
- trunk/Source/_javascript_Core/runtime/JSObject.h
- trunk/Source/WebCore/ChangeLog
- trunk/Source/WebCore/testing/Internals.cpp
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (196848 => 196849)
--- trunk/LayoutTests/ChangeLog 2016-02-20 01:37:55 UTC (rev 196848)
+++ trunk/LayoutTests/ChangeLog 2016-02-20 01:51:48 UTC (rev 196849)
@@ -1,3 +1,16 @@
+2016-02-18 Gavin Barraclough <[email protected]>
+
+ JSObject::getPropertySlot - index-as-propertyname, override on prototype, & shadow
+ https://bugs.webkit.org/show_bug.cgi?id=154416
+
+ Reviewed by Geoff Garen.
+
+ * js/index-property-shadows-overriden-get-own-property-slot-expected.txt: Added.
+ * js/index-property-shadows-overriden-get-own-property-slot.html: Added.
+ * js/script-tests/index-property-shadows-overriden-get-own-property-slot.js: Added.
+ (test):
+ - added test case.
+
2016-02-19 Chris Dumez <[email protected]>
HTMLFormElement.autocomplete should only return known values
Added: trunk/LayoutTests/js/index-property-shadows-overriden-get-own-property-slot-expected.txt (0 => 196849)
--- trunk/LayoutTests/js/index-property-shadows-overriden-get-own-property-slot-expected.txt (rev 0)
+++ trunk/LayoutTests/js/index-property-shadows-overriden-get-own-property-slot-expected.txt 2016-02-20 01:51:48 UTC (rev 196849)
@@ -0,0 +1,10 @@
+If an object has an indexed property shadowing a property of the same name on the prototype, the correct shadowing property should be returned - even if teh property from the prototype comes from an overriden implementation of getOwPropertySlot.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS test("0") is "success"
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
Added: trunk/LayoutTests/js/index-property-shadows-overriden-get-own-property-slot.html (0 => 196849)
--- trunk/LayoutTests/js/index-property-shadows-overriden-get-own-property-slot.html (rev 0)
+++ trunk/LayoutTests/js/index-property-shadows-overriden-get-own-property-slot.html 2016-02-20 01:51:48 UTC (rev 196849)
@@ -0,0 +1,10 @@
+<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
+<html>
+<head>
+<script src=""
+</head>
+<body>
+<script src=""
+<script src=""
+</body>
+</html>
Added: trunk/LayoutTests/js/script-tests/index-property-shadows-overriden-get-own-property-slot.js (0 => 196849)
--- trunk/LayoutTests/js/script-tests/index-property-shadows-overriden-get-own-property-slot.js (rev 0)
+++ trunk/LayoutTests/js/script-tests/index-property-shadows-overriden-get-own-property-slot.js 2016-02-20 01:51:48 UTC (rev 196849)
@@ -0,0 +1,13 @@
+description(
+'If an object has an indexed property shadowing a property of the same name on the prototype, the correct shadowing property should be returned - even if teh property from the prototype comes from an overriden implementation of getOwPropertySlot.'
+);
+
+function test(x) {
+ var testObject = {
+ __proto__: new String("X"),
+ "0": "success"
+ };
+ return testObject[x];
+}
+
+shouldBe('test("0")', '"success"');
Modified: trunk/Source/_javascript_Core/ChangeLog (196848 => 196849)
--- trunk/Source/_javascript_Core/ChangeLog 2016-02-20 01:37:55 UTC (rev 196848)
+++ trunk/Source/_javascript_Core/ChangeLog 2016-02-20 01:51:48 UTC (rev 196849)
@@ -1,3 +1,40 @@
+2016-02-18 Gavin Barraclough <[email protected]>
+
+ JSObject::getPropertySlot - index-as-propertyname, override on prototype, & shadow
+ https://bugs.webkit.org/show_bug.cgi?id=154416
+
+ Reviewed by Geoff Garen.
+
+ Here's the bug. Suppose you call JSObject::getOwnProperty and -
+ - PropertyName contains an index,
+ - An object on the prototype chain overrides getOwnPropertySlot, and has that index property,
+ - The base of the access (or another object on the prototype chain) shadows that property.
+
+ JSObject::getPropertySlot is written assuming the common case is that propertyName is not an
+ index, and as such walks up the prototype chain looking for non-index properties before it
+ tries calling parseIndex.
+
+ At the point we reach an object on the prototype chain overriding getOwnPropertySlot (which
+ would potentially return the property) we may have already skipped over non-overriding
+ objects that contain the property in index storage.
+
+ * runtime/JSObject.h:
+ (JSC::JSObject::getOwnNonIndexPropertySlot):
+ - renamed from inlineGetOwnPropertySlot to better describe behaviour;
+ added ASSERT guarding that this method never returns index properties -
+ if it ever does, this is unsafe for getPropertySlot.
+ (JSC::JSObject::getOwnPropertySlot):
+ - inlineGetOwnPropertySlot -> getOwnNonIndexPropertySlot.
+ (JSC::JSObject::getPropertySlot):
+ - In case of object overriding getOwnPropertySlot check if propertyName is an index.
+ (JSC::JSObject::getNonIndexPropertySlot):
+ - called by getPropertySlot if we encounter an object that overrides getOwnPropertySlot,
+ in order to avoid repeated calls to parseIndex.
+ (JSC::JSObject::inlineGetOwnPropertySlot): Deleted.
+ - this was renamed to getOwnNonIndexPropertySlot.
+ (JSC::JSObject::fastGetOwnPropertySlot): Deleted.
+ - this was folded back in to getPropertySlot.
+
2016-02-19 Saam Barati <[email protected]>
[ES6] Implement Proxy.[[Call]]
Modified: trunk/Source/_javascript_Core/runtime/JSObject.h (196848 => 196849)
--- trunk/Source/_javascript_Core/runtime/JSObject.h 2016-02-20 01:37:55 UTC (rev 196848)
+++ trunk/Source/_javascript_Core/runtime/JSObject.h 2016-02-20 01:51:48 UTC (rev 196849)
@@ -113,7 +113,6 @@
JSValue get(ExecState*, PropertyName) const;
JSValue get(ExecState*, unsigned propertyName) const;
- bool fastGetOwnPropertySlot(ExecState*, VM&, Structure&, PropertyName, PropertySlot&);
bool getPropertySlot(ExecState*, PropertyName, PropertySlot&);
bool getPropertySlot(ExecState*, unsigned propertyName, PropertySlot&);
@@ -859,7 +858,8 @@
JS_EXPORT_PRIVATE NEVER_INLINE void putInlineSlow(ExecState*, PropertyName, JSValue, PutPropertySlot&);
- bool inlineGetOwnPropertySlot(VM&, Structure&, PropertyName, PropertySlot&);
+ bool getNonIndexPropertySlot(ExecState*, PropertyName, PropertySlot&);
+ bool getOwnNonIndexPropertySlot(VM&, Structure&, PropertyName, PropertySlot&);
JS_EXPORT_PRIVATE void fillGetterPropertySlot(PropertySlot&, JSValue, unsigned, PropertyOffset);
void fillCustomGetterPropertySlot(PropertySlot&, JSValue, unsigned, Structure&);
@@ -1094,13 +1094,18 @@
return structure()->storedPrototype();
}
-ALWAYS_INLINE bool JSObject::inlineGetOwnPropertySlot(VM& vm, Structure& structure, PropertyName propertyName, PropertySlot& slot)
+// It is safe to call this method with a PropertyName that is actually an index,
+// but if so will always return false (doesn't search index storage).
+ALWAYS_INLINE bool JSObject::getOwnNonIndexPropertySlot(VM& vm, Structure& structure, PropertyName propertyName, PropertySlot& slot)
{
unsigned attributes;
PropertyOffset offset = structure.get(vm, propertyName, attributes);
if (!isValidOffset(offset))
return false;
+ // getPropertySlot relies on this method never returning index properties!
+ ASSERT(!parseIndex(propertyName));
+
JSValue value = getDirect(offset);
if (value.isCell()) {
ASSERT(value);
@@ -1138,20 +1143,13 @@
{
VM& vm = exec->vm();
Structure& structure = *object->structure(vm);
- if (object->inlineGetOwnPropertySlot(vm, structure, propertyName, slot))
+ if (object->getOwnNonIndexPropertySlot(vm, structure, propertyName, slot))
return true;
if (Optional<uint32_t> index = parseIndex(propertyName))
return getOwnPropertySlotByIndex(object, exec, index.value(), slot);
return false;
}
-ALWAYS_INLINE bool JSObject::fastGetOwnPropertySlot(ExecState* exec, VM& vm, Structure& structure, PropertyName propertyName, PropertySlot& slot)
-{
- if (LIKELY(!TypeInfo::overridesGetOwnPropertySlot(inlineTypeFlags())))
- return inlineGetOwnPropertySlot(vm, structure, propertyName, slot);
- return structure.classInfo()->methodTable.getOwnPropertySlot(this, exec, propertyName, slot);
-}
-
// It may seem crazy to inline a function this large but it makes a big difference
// since this is function very hot in variable lookup
ALWAYS_INLINE bool JSObject::getPropertySlot(ExecState* exec, PropertyName propertyName, PropertySlot& slot)
@@ -1160,8 +1158,19 @@
auto& structureIDTable = vm.heap.structureIDTable();
JSObject* object = this;
while (true) {
+ if (UNLIKELY(TypeInfo::overridesGetOwnPropertySlot(object->inlineTypeFlags()))) {
+ // If propertyName is an index then we may have missed it (as this loop is using
+ // getOwnNonIndexPropertySlot), so we cannot safely call the overridden getOwnPropertySlot
+ // (lest we return a property from a prototype that is shadowed). Check now for an index,
+ // if so we need to start afresh from this object.
+ if (Optional<uint32_t> index = parseIndex(propertyName))
+ return getPropertySlot(exec, index.value(), slot);
+ // Safe to continue searching from current position; call getNonIndexPropertySlot to avoid
+ // parsing the int again.
+ return object->getNonIndexPropertySlot(exec, propertyName, slot);
+ }
Structure& structure = *structureIDTable.get(object->structureID());
- if (object->fastGetOwnPropertySlot(exec, vm, structure, propertyName, slot))
+ if (object->getOwnNonIndexPropertySlot(vm, structure, propertyName, slot))
return true;
JSValue prototype = structure.storedPrototype();
if (!prototype.isObject())
@@ -1190,6 +1199,28 @@
}
}
+ALWAYS_INLINE bool JSObject::getNonIndexPropertySlot(ExecState* exec, PropertyName propertyName, PropertySlot& slot)
+{
+ // This method only supports non-index PropertyNames.
+ ASSERT(!parseIndex(propertyName));
+
+ VM& vm = exec->vm();
+ auto& structureIDTable = vm.heap.structureIDTable();
+ JSObject* object = this;
+ while (true) {
+ Structure& structure = *structureIDTable.get(object->structureID());
+ if (LIKELY(!TypeInfo::overridesGetOwnPropertySlot(object->inlineTypeFlags()))) {
+ if (object->getOwnNonIndexPropertySlot(vm, structure, propertyName, slot))
+ return true;
+ } else if (structure.classInfo()->methodTable.getOwnPropertySlot(object, exec, propertyName, slot))
+ return true;
+ JSValue prototype = structure.storedPrototype();
+ if (!prototype.isObject())
+ return false;
+ object = asObject(prototype);
+ }
+}
+
inline JSValue JSObject::get(ExecState* exec, PropertyName propertyName) const
{
PropertySlot slot(this, PropertySlot::InternalMethodType::Get);
Modified: trunk/Source/WebCore/ChangeLog (196848 => 196849)
--- trunk/Source/WebCore/ChangeLog 2016-02-20 01:37:55 UTC (rev 196848)
+++ trunk/Source/WebCore/ChangeLog 2016-02-20 01:51:48 UTC (rev 196849)
@@ -1,3 +1,15 @@
+2016-02-18 Gavin Barraclough <[email protected]>
+
+ JSObject::getPropertySlot - index-as-propertyname, override on prototype, & shadow
+ https://bugs.webkit.org/show_bug.cgi?id=154416
+
+ Reviewed by Geoff Garen.
+
+ * testing/Internals.cpp:
+ (WebCore::Internals::isReadableStreamDisturbed):
+ - fastGetOwnPropertySlot -> getOwnPropertySlot
+ (internal method removed; test shouldn't really have been using this anyway)
+
2016-02-19 Chris Dumez <[email protected]>
HTMLFormElement.autocomplete should only return known values
Modified: trunk/Source/WebCore/testing/Internals.cpp (196848 => 196849)
--- trunk/Source/WebCore/testing/Internals.cpp 2016-02-20 01:37:55 UTC (rev 196848)
+++ trunk/Source/WebCore/testing/Internals.cpp 2016-02-20 01:51:48 UTC (rev 196849)
@@ -3461,7 +3461,7 @@
const Identifier& privateName = clientData->builtinFunctions().readableStreamInternalsBuiltins().isReadableStreamDisturbedPrivateName();
JSValue value;
PropertySlot propertySlot(value, PropertySlot::InternalMethodType::Get);
- globalObject->fastGetOwnPropertySlot(&state, state.vm(), *globalObject->structure(), privateName, propertySlot);
+ globalObject->methodTable()->getOwnPropertySlot(globalObject, &state, privateName, propertySlot);
value = propertySlot.getValue(&state, privateName);
ASSERT(value.isFunction());
_______________________________________________ webkit-changes mailing list [email protected] https://lists.webkit.org/mailman/listinfo/webkit-changes
