Fri, 18 Oct 2013 22:42:41 +0200
8026858: Array length does not handle defined properties correctly
Reviewed-by: jlaskey
1.1 --- a/src/jdk/nashorn/internal/codegen/Lower.java Fri Oct 18 12:50:21 2013 +0200 1.2 +++ b/src/jdk/nashorn/internal/codegen/Lower.java Fri Oct 18 22:42:41 2013 +0200 1.3 @@ -88,12 +88,12 @@ 1.4 private static final DebugLogger LOG = new DebugLogger("lower"); 1.5 1.6 // needed only to get unique eval id 1.7 - private final CodeInstaller installer; 1.8 + private final CodeInstaller<?> installer; 1.9 1.10 /** 1.11 * Constructor. 1.12 */ 1.13 - Lower(final CodeInstaller installer) { 1.14 + Lower(final CodeInstaller<?> installer) { 1.15 super(new BlockLexicalContext() { 1.16 1.17 @Override
2.1 --- a/src/jdk/nashorn/internal/runtime/PropertyMap.java Fri Oct 18 12:50:21 2013 +0200 2.2 +++ b/src/jdk/nashorn/internal/runtime/PropertyMap.java Fri Oct 18 22:42:41 2013 +0200 2.3 @@ -26,6 +26,8 @@ 2.4 package jdk.nashorn.internal.runtime; 2.5 2.6 import static jdk.nashorn.internal.runtime.PropertyHashMap.EMPTY_HASHMAP; 2.7 +import static jdk.nashorn.internal.runtime.arrays.ArrayIndex.getArrayIndex; 2.8 +import static jdk.nashorn.internal.runtime.arrays.ArrayIndex.isValidArrayIndex; 2.9 2.10 import java.lang.invoke.SwitchPoint; 2.11 import java.lang.ref.WeakReference; 2.12 @@ -50,6 +52,8 @@ 2.13 public final class PropertyMap implements Iterable<Object>, PropertyListener { 2.14 /** Used for non extensible PropertyMaps, negative logic as the normal case is extensible. See {@link ScriptObject#preventExtensions()} */ 2.15 public static final int NOT_EXTENSIBLE = 0b0000_0001; 2.16 + /** Does this map contain valid array keys? */ 2.17 + public static final int CONTAINS_ARRAY_KEYS = 0b0000_0010; 2.18 /** This mask is used to preserve certain flags when cloning the PropertyMap. Others should not be copied */ 2.19 private static final int CLONEABLE_FLAGS_MASK = 0b0000_1111; 2.20 /** Has a listener been added to this property map. This flag is not copied when cloning a map. See {@link PropertyListener} */ 2.21 @@ -91,12 +95,16 @@ 2.22 * @param fieldCount Number of fields in use. 2.23 * @param fieldMaximum Number of fields available. 2.24 * @param spillLength Number of spill slots used. 2.25 + * @param containsArrayKeys True if properties contain numeric keys 2.26 */ 2.27 - private PropertyMap(final PropertyHashMap properties, final int fieldCount, final int fieldMaximum, final int spillLength) { 2.28 + private PropertyMap(final PropertyHashMap properties, final int fieldCount, final int fieldMaximum, final int spillLength, final boolean containsArrayKeys) { 2.29 this.properties = properties; 2.30 this.fieldCount = fieldCount; 2.31 this.fieldMaximum = fieldMaximum; 2.32 this.spillLength = spillLength; 2.33 + if (containsArrayKeys) { 2.34 + setContainsArrayKeys(); 2.35 + } 2.36 2.37 if (Context.DEBUG) { 2.38 count++; 2.39 @@ -104,15 +112,6 @@ 2.40 } 2.41 2.42 /** 2.43 - * Constructor. 2.44 - * 2.45 - * @param properties A {@link PropertyHashMap} with initial contents. 2.46 - */ 2.47 - private PropertyMap(final PropertyHashMap properties) { 2.48 - this(properties, 0, 0, 0); 2.49 - } 2.50 - 2.51 - /** 2.52 * Cloning constructor. 2.53 * 2.54 * @param propertyMap Existing property map. 2.55 @@ -152,12 +151,15 @@ 2.56 if (Context.DEBUG) { 2.57 duplicatedCount++; 2.58 } 2.59 - return new PropertyMap(this.properties); 2.60 + return new PropertyMap(this.properties, 0, 0, 0, containsArrayKeys()); 2.61 } 2.62 2.63 /** 2.64 * Public property map allocator. 2.65 * 2.66 + * <p>It is the caller's responsibility to make sure that {@code properties} does not contain 2.67 + * properties with keys that are valid array indices.</p> 2.68 + * 2.69 * @param properties Collection of initial properties. 2.70 * @param fieldCount Number of fields in use. 2.71 * @param fieldMaximum Number of fields available. 2.72 @@ -166,11 +168,15 @@ 2.73 */ 2.74 public static PropertyMap newMap(final Collection<Property> properties, final int fieldCount, final int fieldMaximum, final int spillLength) { 2.75 PropertyHashMap newProperties = EMPTY_HASHMAP.immutableAdd(properties); 2.76 - return new PropertyMap(newProperties, fieldCount, fieldMaximum, spillLength); 2.77 + return new PropertyMap(newProperties, fieldCount, fieldMaximum, spillLength, false); 2.78 } 2.79 2.80 /** 2.81 * Public property map allocator. Used by nasgen generated code. 2.82 + * 2.83 + * <p>It is the caller's responsibility to make sure that {@code properties} does not contain 2.84 + * properties with keys that are valid array indices.</p> 2.85 + * 2.86 * @param properties Collection of initial properties. 2.87 * @return New {@link PropertyMap}. 2.88 */ 2.89 @@ -184,7 +190,7 @@ 2.90 * @return New empty {@link PropertyMap}. 2.91 */ 2.92 public static PropertyMap newMap() { 2.93 - return new PropertyMap(EMPTY_HASHMAP); 2.94 + return new PropertyMap(EMPTY_HASHMAP, 0, 0, 0, false); 2.95 } 2.96 2.97 /** 2.98 @@ -294,6 +300,9 @@ 2.99 if(!property.isSpill()) { 2.100 newMap.fieldCount = Math.max(newMap.fieldCount, property.getSlot() + 1); 2.101 } 2.102 + if (isValidArrayIndex(getArrayIndex(property.getKey()))) { 2.103 + newMap.setContainsArrayKeys(); 2.104 + } 2.105 2.106 newMap.spillLength += property.getSpillCount(); 2.107 } 2.108 @@ -408,6 +417,9 @@ 2.109 2.110 final PropertyMap newMap = new PropertyMap(this, newProperties); 2.111 for (final Property property : otherProperties) { 2.112 + if (isValidArrayIndex(getArrayIndex(property.getKey()))) { 2.113 + newMap.setContainsArrayKeys(); 2.114 + } 2.115 newMap.spillLength += property.getSpillCount(); 2.116 } 2.117 2.118 @@ -700,6 +712,22 @@ 2.119 } 2.120 2.121 /** 2.122 + * Check if this map contains properties with valid array keys 2.123 + * 2.124 + * @return {@code true} if this map contains properties with valid array keys 2.125 + */ 2.126 + public final boolean containsArrayKeys() { 2.127 + return (flags & CONTAINS_ARRAY_KEYS) != 0; 2.128 + } 2.129 + 2.130 + /** 2.131 + * Flag this object as having array keys in defined properties 2.132 + */ 2.133 + private void setContainsArrayKeys() { 2.134 + flags |= CONTAINS_ARRAY_KEYS; 2.135 + } 2.136 + 2.137 + /** 2.138 * Check whether a {@link PropertyListener} has been added to this map. 2.139 * 2.140 * @return {@code true} if {@link PropertyListener} exists
3.1 --- a/src/jdk/nashorn/internal/runtime/ScriptObject.java Fri Oct 18 12:50:21 2013 +0200 3.2 +++ b/src/jdk/nashorn/internal/runtime/ScriptObject.java Fri Oct 18 22:42:41 2013 +0200 3.3 @@ -508,7 +508,7 @@ 3.4 if (property == null) { 3.5 // promoting an arrayData value to actual property 3.6 addOwnProperty(key, propFlags, value); 3.7 - removeArraySlot(key); 3.8 + checkIntegerKey(key); 3.9 } else { 3.10 // Now set the new flags 3.11 modifyOwnProperty(property, propFlags); 3.12 @@ -616,15 +616,6 @@ 3.13 } 3.14 } 3.15 3.16 - private void removeArraySlot(final String key) { 3.17 - final int index = getArrayIndex(key); 3.18 - final ArrayData array = getArray(); 3.19 - 3.20 - if (array.has(index)) { 3.21 - setArray(array.delete(index)); 3.22 - } 3.23 - } 3.24 - 3.25 /** 3.26 * Add a new property to the object. 3.27 * 3.28 @@ -1203,21 +1194,10 @@ 3.29 * Check if this ScriptObject has array entries. This means that someone has 3.30 * set values with numeric keys in the object. 3.31 * 3.32 - * Note: this can be O(n) up to the array length 3.33 - * 3.34 * @return true if array entries exists. 3.35 */ 3.36 public boolean hasArrayEntries() { 3.37 - final ArrayData array = getArray(); 3.38 - final long length = array.length(); 3.39 - 3.40 - for (long i = 0; i < length; i++) { 3.41 - if (array.has((int)i)) { 3.42 - return true; 3.43 - } 3.44 - } 3.45 - 3.46 - return false; 3.47 + return getArray().length() > 0 || getMap().containsArrayKeys(); 3.48 } 3.49 3.50 /** 3.51 @@ -2356,8 +2336,29 @@ 3.52 } 3.53 3.54 if (newLength < arrayLength) { 3.55 - setArray(getArray().shrink(newLength)); 3.56 - getArray().setLength(newLength); 3.57 + long actualLength = newLength; 3.58 + 3.59 + // Check for numeric keys in property map and delete them or adjust length, depending on whether 3.60 + // they're defined as configurable. See ES5 #15.4.5.2 3.61 + if (getMap().containsArrayKeys()) { 3.62 + 3.63 + for (long l = arrayLength - 1; l >= newLength; l--) { 3.64 + final FindProperty find = findProperty(JSType.toString(l), false); 3.65 + 3.66 + if (find != null) { 3.67 + 3.68 + if (find.getProperty().isConfigurable()) { 3.69 + deleteOwnProperty(find.getProperty()); 3.70 + } else { 3.71 + actualLength = l + 1; 3.72 + break; 3.73 + } 3.74 + } 3.75 + } 3.76 + } 3.77 + 3.78 + setArray(getArray().shrink(actualLength)); 3.79 + getArray().setLength(actualLength); 3.80 } 3.81 } 3.82 3.83 @@ -2680,7 +2681,7 @@ 3.84 final long oldLength = getArray().length(); 3.85 final long longIndex = index & JSType.MAX_UINT; 3.86 3.87 - if (!getArray().has(index)) { 3.88 + if (getMap().containsArrayKeys()) { 3.89 final String key = JSType.toString(longIndex); 3.90 final FindProperty find = findProperty(key, true); 3.91
4.1 --- /dev/null Thu Jan 01 00:00:00 1970 +0000 4.2 +++ b/test/script/basic/JDK-8026858.js Fri Oct 18 22:42:41 2013 +0200 4.3 @@ -0,0 +1,66 @@ 4.4 +/* 4.5 + * Copyright (c) 2010, 2013, Oracle and/or its affiliates. All rights reserved. 4.6 + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. 4.7 + * 4.8 + * This code is free software; you can redistribute it and/or modify it 4.9 + * under the terms of the GNU General Public License version 2 only, as 4.10 + * published by the Free Software Foundation. 4.11 + * 4.12 + * This code is distributed in the hope that it will be useful, but WITHOUT 4.13 + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or 4.14 + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License 4.15 + * version 2 for more details (a copy is included in the LICENSE file that 4.16 + * accompanied this code). 4.17 + * 4.18 + * You should have received a copy of the GNU General Public License version 4.19 + * 2 along with this work; if not, write to the Free Software Foundation, 4.20 + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. 4.21 + * 4.22 + * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA 4.23 + * or visit www.oracle.com if you need additional information or have any 4.24 + * questions. 4.25 + */ 4.26 + 4.27 +/** 4.28 + * JDK-8026858: Array length does not handle defined properties correctly 4.29 + * 4.30 + * @test 4.31 + * @run 4.32 + */ 4.33 + 4.34 +var arr = []; 4.35 + 4.36 +Object.defineProperty(arr, "3", {value: 1 /* configurable: false */}); 4.37 + 4.38 +if (arr[3] != 1) { 4.39 + throw new Error("arr[3] not defined"); 4.40 +} 4.41 + 4.42 +if (arr.length !== 4) { 4.43 + throw new Error("Array length not updated to 4"); 4.44 +} 4.45 + 4.46 +Object.defineProperty(arr, "5", {value: 1, configurable: true}); 4.47 + 4.48 +if (arr[5] != 1) { 4.49 + throw new Error("arr[5] not defined"); 4.50 +} 4.51 + 4.52 +if (arr.length !== 6) { 4.53 + throw new Error("Array length not updated to 4"); 4.54 +} 4.55 + 4.56 +arr.length = 0; 4.57 + 4.58 +if (5 in arr) { 4.59 + throw new Error("configurable element was not deleted"); 4.60 +} 4.61 + 4.62 +if (arr[3] != 1) { 4.63 + throw new Error("non-configurable element was deleted"); 4.64 +} 4.65 + 4.66 +if (arr.length !== 4) { 4.67 + throw new Error("Array length not set"); 4.68 +} 4.69 +