8026858: Array length does not handle defined properties correctly

Reviewed-by: jlaskey

nashorn/src/jdk/nashorn/internal/codegen/Lower.java

@@ -88,12 +88,12 @@ final class Lower extends NodeOperatorVisitor<BlockLexicalContext> {
     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

nashorn/src/jdk/nashorn/internal/runtime/PropertyMap.java

@@ -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 @@ import java.util.WeakHashMap;
 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,27 +95,22 @@ public final class PropertyMap implements Iterable<Object>, PropertyListener {
      * @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++;
         }
     }
 
-    /**
-     * Constructor.
-     *
-     * @param properties A {@link PropertyHashMap} with initial contents.
-     */
-    private PropertyMap(final PropertyHashMap properties) {
-        this(properties, 0, 0, 0);
-    }
-
     /**
      * Cloning constructor.
      *
@@ -152,12 +151,15 @@ public final class PropertyMap implements Iterable<Object>, PropertyListener {
         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 final class PropertyMap implements Iterable<Object>, PropertyListener {
      */
     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 @@ public final class PropertyMap implements Iterable<Object>, PropertyListener {
      * @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 @@ public final class PropertyMap implements Iterable<Object>, PropertyListener {
             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 @@ public final class PropertyMap implements Iterable<Object>, PropertyListener {
 
         final PropertyMap newMap = new PropertyMap(this, newProperties);
         for (final Property property : otherProperties) {
+            if (isValidArrayIndex(getArrayIndex(property.getKey()))) {
+                newMap.setContainsArrayKeys();
+            }
             newMap.spillLength += property.getSpillCount();
         }
 
@@ -699,6 +711,22 @@ public final class PropertyMap implements Iterable<Object>, PropertyListener {
         return new PropertyMapIterator(this);
     }
 
+    /**
+     * 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.
      *

nashorn/src/jdk/nashorn/internal/runtime/ScriptObject.java

@@ -508,7 +508,7 @@ public abstract class ScriptObject extends PropertyListenerManager implements Pr
             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 @@ public abstract class ScriptObject extends PropertyListenerManager implements Pr
         }
     }
 
-    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 @@ public abstract class ScriptObject extends PropertyListenerManager implements Pr
      * 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 @@ public abstract class ScriptObject extends PropertyListenerManager implements Pr
        }
 
        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 @@ public abstract class ScriptObject extends PropertyListenerManager implements Pr
         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);
 

nashorn/test/script/basic/JDK-8026858.js

+/*
+ * 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");
+}
+