8026858: Array length does not handle defined properties correctly
Reviewed-by: jlaskey
--- a/nashorn/src/jdk/nashorn/internal/codegen/Lower.java Fri Oct 18 12:50:21 2013 +0200
+++ b/nashorn/src/jdk/nashorn/internal/codegen/Lower.java Fri Oct 18 22:42:41 2013 +0200
@@ -88,12 +88,12 @@
private static final DebugLogger LOG = new DebugLogger("lower");
// needed only to get unique eval id
- private final CodeInstaller installer;
+ private final CodeInstaller<?> installer;
/**
* Constructor.
*/
- Lower(final CodeInstaller installer) {
+ Lower(final CodeInstaller<?> installer) {
super(new BlockLexicalContext() {
@Override
--- a/nashorn/src/jdk/nashorn/internal/runtime/PropertyMap.java Fri Oct 18 12:50:21 2013 +0200
+++ b/nashorn/src/jdk/nashorn/internal/runtime/PropertyMap.java Fri Oct 18 22:42:41 2013 +0200
@@ -26,6 +26,8 @@
package jdk.nashorn.internal.runtime;
import static jdk.nashorn.internal.runtime.PropertyHashMap.EMPTY_HASHMAP;
+import static jdk.nashorn.internal.runtime.arrays.ArrayIndex.getArrayIndex;
+import static jdk.nashorn.internal.runtime.arrays.ArrayIndex.isValidArrayIndex;
import java.lang.invoke.SwitchPoint;
import java.lang.ref.WeakReference;
@@ -50,6 +52,8 @@
public final class PropertyMap implements Iterable<Object>, PropertyListener {
/** Used for non extensible PropertyMaps, negative logic as the normal case is extensible. See {@link ScriptObject#preventExtensions()} */
public static final int NOT_EXTENSIBLE = 0b0000_0001;
+ /** Does this map contain valid array keys? */
+ public static final int CONTAINS_ARRAY_KEYS = 0b0000_0010;
/** This mask is used to preserve certain flags when cloning the PropertyMap. Others should not be copied */
private static final int CLONEABLE_FLAGS_MASK = 0b0000_1111;
/** Has a listener been added to this property map. This flag is not copied when cloning a map. See {@link PropertyListener} */
@@ -91,12 +95,16 @@
* @param fieldCount Number of fields in use.
* @param fieldMaximum Number of fields available.
* @param spillLength Number of spill slots used.
+ * @param containsArrayKeys True if properties contain numeric keys
*/
- private PropertyMap(final PropertyHashMap properties, final int fieldCount, final int fieldMaximum, final int spillLength) {
+ private PropertyMap(final PropertyHashMap properties, final int fieldCount, final int fieldMaximum, final int spillLength, final boolean containsArrayKeys) {
this.properties = properties;
this.fieldCount = fieldCount;
this.fieldMaximum = fieldMaximum;
this.spillLength = spillLength;
+ if (containsArrayKeys) {
+ setContainsArrayKeys();
+ }
if (Context.DEBUG) {
count++;
@@ -104,15 +112,6 @@
}
/**
- * Constructor.
- *
- * @param properties A {@link PropertyHashMap} with initial contents.
- */
- private PropertyMap(final PropertyHashMap properties) {
- this(properties, 0, 0, 0);
- }
-
- /**
* Cloning constructor.
*
* @param propertyMap Existing property map.
@@ -152,12 +151,15 @@
if (Context.DEBUG) {
duplicatedCount++;
}
- return new PropertyMap(this.properties);
+ return new PropertyMap(this.properties, 0, 0, 0, containsArrayKeys());
}
/**
* Public property map allocator.
*
+ * <p>It is the caller's responsibility to make sure that {@code properties} does not contain
+ * properties with keys that are valid array indices.</p>
+ *
* @param properties Collection of initial properties.
* @param fieldCount Number of fields in use.
* @param fieldMaximum Number of fields available.
@@ -166,11 +168,15 @@
*/
public static PropertyMap newMap(final Collection<Property> properties, final int fieldCount, final int fieldMaximum, final int spillLength) {
PropertyHashMap newProperties = EMPTY_HASHMAP.immutableAdd(properties);
- return new PropertyMap(newProperties, fieldCount, fieldMaximum, spillLength);
+ return new PropertyMap(newProperties, fieldCount, fieldMaximum, spillLength, false);
}
/**
* Public property map allocator. Used by nasgen generated code.
+ *
+ * <p>It is the caller's responsibility to make sure that {@code properties} does not contain
+ * properties with keys that are valid array indices.</p>
+ *
* @param properties Collection of initial properties.
* @return New {@link PropertyMap}.
*/
@@ -184,7 +190,7 @@
* @return New empty {@link PropertyMap}.
*/
public static PropertyMap newMap() {
- return new PropertyMap(EMPTY_HASHMAP);
+ return new PropertyMap(EMPTY_HASHMAP, 0, 0, 0, false);
}
/**
@@ -294,6 +300,9 @@
if(!property.isSpill()) {
newMap.fieldCount = Math.max(newMap.fieldCount, property.getSlot() + 1);
}
+ if (isValidArrayIndex(getArrayIndex(property.getKey()))) {
+ newMap.setContainsArrayKeys();
+ }
newMap.spillLength += property.getSpillCount();
}
@@ -408,6 +417,9 @@
final PropertyMap newMap = new PropertyMap(this, newProperties);
for (final Property property : otherProperties) {
+ if (isValidArrayIndex(getArrayIndex(property.getKey()))) {
+ newMap.setContainsArrayKeys();
+ }
newMap.spillLength += property.getSpillCount();
}
@@ -700,6 +712,22 @@
}
/**
+ * Check if this map contains properties with valid array keys
+ *
+ * @return {@code true} if this map contains properties with valid array keys
+ */
+ public final boolean containsArrayKeys() {
+ return (flags & CONTAINS_ARRAY_KEYS) != 0;
+ }
+
+ /**
+ * Flag this object as having array keys in defined properties
+ */
+ private void setContainsArrayKeys() {
+ flags |= CONTAINS_ARRAY_KEYS;
+ }
+
+ /**
* Check whether a {@link PropertyListener} has been added to this map.
*
* @return {@code true} if {@link PropertyListener} exists
--- a/nashorn/src/jdk/nashorn/internal/runtime/ScriptObject.java Fri Oct 18 12:50:21 2013 +0200
+++ b/nashorn/src/jdk/nashorn/internal/runtime/ScriptObject.java Fri Oct 18 22:42:41 2013 +0200
@@ -508,7 +508,7 @@
if (property == null) {
// promoting an arrayData value to actual property
addOwnProperty(key, propFlags, value);
- removeArraySlot(key);
+ checkIntegerKey(key);
} else {
// Now set the new flags
modifyOwnProperty(property, propFlags);
@@ -616,15 +616,6 @@
}
}
- private void removeArraySlot(final String key) {
- final int index = getArrayIndex(key);
- final ArrayData array = getArray();
-
- if (array.has(index)) {
- setArray(array.delete(index));
- }
- }
-
/**
* Add a new property to the object.
*
@@ -1203,21 +1194,10 @@
* Check if this ScriptObject has array entries. This means that someone has
* set values with numeric keys in the object.
*
- * Note: this can be O(n) up to the array length
- *
* @return true if array entries exists.
*/
public boolean hasArrayEntries() {
- final ArrayData array = getArray();
- final long length = array.length();
-
- for (long i = 0; i < length; i++) {
- if (array.has((int)i)) {
- return true;
- }
- }
-
- return false;
+ return getArray().length() > 0 || getMap().containsArrayKeys();
}
/**
@@ -2356,8 +2336,29 @@
}
if (newLength < arrayLength) {
- setArray(getArray().shrink(newLength));
- getArray().setLength(newLength);
+ long actualLength = newLength;
+
+ // Check for numeric keys in property map and delete them or adjust length, depending on whether
+ // they're defined as configurable. See ES5 #15.4.5.2
+ if (getMap().containsArrayKeys()) {
+
+ for (long l = arrayLength - 1; l >= newLength; l--) {
+ final FindProperty find = findProperty(JSType.toString(l), false);
+
+ if (find != null) {
+
+ if (find.getProperty().isConfigurable()) {
+ deleteOwnProperty(find.getProperty());
+ } else {
+ actualLength = l + 1;
+ break;
+ }
+ }
+ }
+ }
+
+ setArray(getArray().shrink(actualLength));
+ getArray().setLength(actualLength);
}
}
@@ -2680,7 +2681,7 @@
final long oldLength = getArray().length();
final long longIndex = index & JSType.MAX_UINT;
- if (!getArray().has(index)) {
+ if (getMap().containsArrayKeys()) {
final String key = JSType.toString(longIndex);
final FindProperty find = findProperty(key, true);
--- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/nashorn/test/script/basic/JDK-8026858.js Fri Oct 18 22:42:41 2013 +0200
@@ -0,0 +1,66 @@
+/*
+ * Copyright (c) 2010, 2013, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/**
+ * JDK-8026858: Array length does not handle defined properties correctly
+ *
+ * @test
+ * @run
+ */
+
+var arr = [];
+
+Object.defineProperty(arr, "3", {value: 1 /* configurable: false */});
+
+if (arr[3] != 1) {
+ throw new Error("arr[3] not defined");
+}
+
+if (arr.length !== 4) {
+ throw new Error("Array length not updated to 4");
+}
+
+Object.defineProperty(arr, "5", {value: 1, configurable: true});
+
+if (arr[5] != 1) {
+ throw new Error("arr[5] not defined");
+}
+
+if (arr.length !== 6) {
+ throw new Error("Array length not updated to 4");
+}
+
+arr.length = 0;
+
+if (5 in arr) {
+ throw new Error("configurable element was not deleted");
+}
+
+if (arr[3] != 1) {
+ throw new Error("non-configurable element was deleted");
+}
+
+if (arr.length !== 4) {
+ throw new Error("Array length not set");
+}
+