8044851: nashorn properties leak memory

Reviewed-by: attila, jlaskey, lagergren

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

@@ -712,7 +712,7 @@ public class AccessorProperty extends Property {
 
 
     private MethodHandle debug(final MethodHandle mh, final Class<?> forType, final Class<?> type, final String tag) {
-        if (!Global.hasInstance()) {
+        if (!Context.DEBUG || !Global.hasInstance()) {
             return mh;
         }
 
@@ -734,7 +734,7 @@ public class AccessorProperty extends Property {
     }
 
     private MethodHandle debugReplace(final Class<?> oldType, final Class<?> newType, final PropertyMap oldMap, final PropertyMap newMap) {
-        if (!Global.hasInstance()) {
+        if (!Context.DEBUG || !Global.hasInstance()) {
             return REPLACE_MAP;
         }
 
@@ -767,7 +767,7 @@ public class AccessorProperty extends Property {
     }
 
     private static MethodHandle debugInvalidate(final String key, final SwitchPoint sp) {
-        if (!Global.hasInstance()) {
+        if (!Context.DEBUG || !Global.hasInstance()) {
             return INVALIDATE_SP;
         }
 

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

@@ -286,17 +286,6 @@ public abstract class Property implements Serializable {
         return (flags & IS_BOUND) == IS_BOUND;
     }
 
-    /**
-     * Does this property use any slots in the spill array described in
-     * {@link Property#isSpill}? In that case how many. Currently a property
-     * only uses max one spill slot, but this may change in future representations
-     *
-     * @return number of spill slots a property is using
-     */
-    public int getSpillCount() {
-        return 0;
-    }
-
     /**
      * Add more property flags to the property. Properties are immutable here,
      * so any property change that results in a larger flag set results in the

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

@@ -36,6 +36,7 @@ import java.io.Serializable;
 import java.lang.invoke.SwitchPoint;
 import java.lang.ref.SoftReference;
 import java.util.Arrays;
+import java.util.BitSet;
 import java.util.Collection;
 import java.util.HashMap;
 import java.util.Iterator;
@@ -88,6 +89,8 @@ public final class PropertyMap implements Iterable<Object>, Serializable {
     /** property listeners */
     private transient PropertyListeners listeners;
 
+    private transient BitSet freeSlots;
+
     private static final long serialVersionUID = -7041836752008732533L;
 
     /**
@@ -129,6 +132,7 @@ public final class PropertyMap implements Iterable<Object>, Serializable {
         this.fieldMaximum = propertyMap.fieldMaximum;
         // We inherit the parent property listeners instance. It will be cloned when a new listener is added.
         this.listeners    = propertyMap.listeners;
+        this.freeSlots    = propertyMap.freeSlots;
 
         if (Context.DEBUG) {
             count++;
@@ -350,6 +354,51 @@ public final class PropertyMap implements Iterable<Object>, Serializable {
         return addPropertyNoHistory(new AccessorProperty(property, bindTo));
     }
 
+    // Get a logical slot index for a property, with spill slot 0 starting at fieldMaximum.
+    private int logicalSlotIndex(final Property property) {
+        final int slot = property.getSlot();
+        if (slot < 0) {
+            return -1;
+        }
+        return property.isSpill() ? slot + fieldMaximum : slot;
+    }
+
+    // Update boundaries and flags after a property has been added
+    private void updateFlagsAndBoundaries(final Property newProperty) {
+        if(newProperty.isSpill()) {
+            spillLength = Math.max(spillLength, newProperty.getSlot() + 1);
+        } else {
+            fieldCount = Math.max(fieldCount, newProperty.getSlot() + 1);
+        }
+        if (isValidArrayIndex(getArrayIndex(newProperty.getKey()))) {
+            setContainsArrayKeys();
+        }
+    }
+
+    // Update the free slots bitmap for a property that has been deleted and/or added.
+    private void updateFreeSlots(final Property oldProperty, final Property newProperty) {
+        // Free slots bitset is possibly shared with parent map, so we must clone it before making modifications.
+        boolean freeSlotsCloned = false;
+        if (oldProperty != null) {
+            final int slotIndex = logicalSlotIndex(oldProperty);
+            if (slotIndex >= 0) {
+                final BitSet newFreeSlots = freeSlots == null ? new BitSet() : (BitSet)freeSlots.clone();
+                assert !newFreeSlots.get(slotIndex);
+                newFreeSlots.set(slotIndex);
+                freeSlots = newFreeSlots;
+                freeSlotsCloned = true;
+            }
+        }
+        if (freeSlots != null && newProperty != null) {
+            final int slotIndex = logicalSlotIndex(newProperty);
+            if (slotIndex > -1 && freeSlots.get(slotIndex)) {
+                final BitSet newFreeSlots = freeSlotsCloned ? freeSlots : ((BitSet)freeSlots.clone());
+                newFreeSlots.clear(slotIndex);
+                freeSlots = newFreeSlots.isEmpty() ? null : newFreeSlots;
+            }
+        }
+    }
+
     /**
      * Add a property to the map without adding it to the history. This should be used for properties that
      * can't be shared such as bound properties, or properties that are expected to be added only once.
@@ -363,15 +412,9 @@ public final class PropertyMap implements Iterable<Object>, Serializable {
         }
         final PropertyHashMap newProperties = properties.immutableAdd(property);
         final PropertyMap newMap = new PropertyMap(this, newProperties);
+        newMap.updateFlagsAndBoundaries(property);
+        newMap.updateFreeSlots(null, property);
 
-        if(!property.isSpill()) {
-            newMap.fieldCount = Math.max(newMap.fieldCount, property.getSlot() + 1);
-        }
-        if (isValidArrayIndex(getArrayIndex(property.getKey()))) {
-            newMap.setContainsArrayKeys();
-        }
-
-        newMap.spillLength += property.getSpillCount();
         return newMap;
     }
 
@@ -392,15 +435,8 @@ public final class PropertyMap implements Iterable<Object>, Serializable {
             final PropertyHashMap newProperties = properties.immutableAdd(property);
             newMap = new PropertyMap(this, newProperties);
             addToHistory(property, newMap);
-
-            if (!property.isSpill()) {
-                newMap.fieldCount = Math.max(newMap.fieldCount, property.getSlot() + 1);
-            }
-            if (isValidArrayIndex(getArrayIndex(property.getKey()))) {
-                newMap.setContainsArrayKeys();
-            }
-
-            newMap.spillLength += property.getSpillCount();
+            newMap.updateFlagsAndBoundaries(property);
+            newMap.updateFreeSlots(null, property);
         }
 
         return newMap;
@@ -422,7 +458,20 @@ public final class PropertyMap implements Iterable<Object>, Serializable {
 
         if (newMap == null && properties.containsKey(key)) {
             final PropertyHashMap newProperties = properties.immutableRemove(key);
-            newMap = new PropertyMap(this, newProperties);
+            final boolean isSpill = property.isSpill();
+            final int slot = property.getSlot();
+            // If deleted property was last field or spill slot we can make it reusable by reducing field/slot count.
+            // Otherwise mark it as free in free slots bitset.
+            if (isSpill && slot >= 0 && slot == spillLength - 1) {
+                newMap = new PropertyMap(newProperties, className, fieldCount, fieldMaximum, spillLength - 1, containsArrayKeys());
+                newMap.freeSlots = freeSlots;
+            } else if (!isSpill && slot >= 0 && slot == fieldCount - 1) {
+                newMap = new PropertyMap(newProperties, className, fieldCount - 1, fieldMaximum, spillLength, containsArrayKeys());
+                newMap.freeSlots = freeSlots;
+            } else {
+                newMap = new PropertyMap(this, newProperties);
+                newMap.updateFreeSlots(property, null);
+            }
             addToHistory(property, newMap);
         }
 
@@ -471,7 +520,10 @@ public final class PropertyMap implements Iterable<Object>, Serializable {
          * spillLength remains same in case (1) and (2) because of slot reuse. Only for case (3), we need
          * to add spill count of the newly added UserAccessorProperty property.
          */
-        newMap.spillLength = spillLength;
+        if (!sameType) {
+            newMap.spillLength = Math.max(spillLength, newProperty.getSlot() + 1);
+            newMap.updateFreeSlots(oldProperty, newProperty);
+        }
         return newMap;
     }
 
@@ -486,7 +538,7 @@ public final class PropertyMap implements Iterable<Object>, Serializable {
      * @return the newly created UserAccessorProperty
      */
     public UserAccessorProperty newUserAccessors(final String key, final int propertyFlags) {
-        return new UserAccessorProperty(key, propertyFlags, spillLength);
+        return new UserAccessorProperty(key, propertyFlags, getFreeSpillSlot());
     }
 
     /**
@@ -514,10 +566,11 @@ public final class PropertyMap implements Iterable<Object>, Serializable {
 
         final PropertyMap newMap = new PropertyMap(this, newProperties);
         for (final Property property : otherProperties) {
+            // This method is only safe to use with non-slotted, native getter/setter properties
+            assert property.getSlot() == -1;
             if (isValidArrayIndex(getArrayIndex(property.getKey()))) {
                 newMap.setContainsArrayKeys();
             }
-            newMap.spillLength += property.getSpillCount();
         }
 
         return newMap;
@@ -790,29 +843,37 @@ public final class PropertyMap implements Iterable<Object>, Serializable {
     boolean isFrozen() {
         return !isExtensible() && allFrozen();
     }
+
     /**
-     * Get the number of fields allocated for this {@link PropertyMap}.
-     *
-     * @return Number of fields allocated.
-     */
-    int getFieldCount() {
-        return fieldCount;
-    }
-    /**
-     * Get maximum number of fields available for this {@link PropertyMap}.
+     * Return a free field slot for this map, or {@code -1} if none is available.
      *
-     * @return Number of fields available.
+     * @return free field slot or -1
      */
-    int getFieldMaximum() {
-        return fieldMaximum;
+    int getFreeFieldSlot() {
+        if (freeSlots != null) {
+            final int freeSlot = freeSlots.nextSetBit(0);
+            if (freeSlot > -1 && freeSlot < fieldMaximum) {
+                return freeSlot;
+            }
+        }
+        if (fieldCount < fieldMaximum) {
+            return fieldCount;
+        }
+        return -1;
     }
 
     /**
-     * Get length of spill area associated with this {@link PropertyMap}.
+     * Get a free spill slot for this map.
      *
-     * @return Length of spill area.
+     * @return free spill slot
      */
-    int getSpillLength() {
+    int getFreeSpillSlot() {
+        if (freeSlots != null) {
+            final int freeSlot = freeSlots.nextSetBit(fieldMaximum);
+            if (freeSlot > -1) {
+                return freeSlot - fieldMaximum;
+            }
+        }
         return spillLength;
     }
 

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

@@ -2457,20 +2457,19 @@ public abstract class ScriptObject implements PropertyAccess {
      */
     private Property addSpillProperty(final String key, final int propertyFlags, final Object value, final boolean hasInitialValue) {
         final PropertyMap propertyMap = getMap();
-        final int         fieldCount  = propertyMap.getFieldCount();
-        final int         fieldMax    = propertyMap.getFieldMaximum();
+        final int fieldSlot  = propertyMap.getFreeFieldSlot();
 
         Property property;
-        if (fieldCount < fieldMax) {
+        if (fieldSlot > -1) {
             property = hasInitialValue ?
-                new AccessorProperty(key, propertyFlags, fieldCount, this, value) :
-                new AccessorProperty(key, propertyFlags, getClass(), fieldCount);
+                new AccessorProperty(key, propertyFlags, fieldSlot, this, value) :
+                new AccessorProperty(key, propertyFlags, getClass(), fieldSlot);
             property = addOwnProperty(property);
         } else {
-            final int spillCount = propertyMap.getSpillLength();
+            final int spillSlot = propertyMap.getFreeSpillSlot();
             property = hasInitialValue ?
-                new SpillProperty(key, propertyFlags, spillCount, this, value) :
-                new SpillProperty(key, propertyFlags, spillCount);
+                new SpillProperty(key, propertyFlags, spillSlot, this, value) :
+                new SpillProperty(key, propertyFlags, spillSlot);
             property = addOwnProperty(property);
             ensureSpillSize(property.getSlot());
         }

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

@@ -160,7 +160,7 @@ final class SetMethodCreator {
     }
 
     private SetMethod createNewPropertySetter() {
-        final SetMethod sm = map.getFieldCount() < map.getFieldMaximum() ? createNewFieldSetter() : createNewSpillPropertySetter();
+        final SetMethod sm = map.getFreeFieldSlot() > -1 ? createNewFieldSetter() : createNewSpillPropertySetter();
         final PropertyListeners listeners = map.getListeners();
         if (listeners != null) {
             listeners.propertyAdded(sm.property);
@@ -205,11 +205,11 @@ final class SetMethodCreator {
     }
 
     private SetMethod createNewFieldSetter() {
-        return createNewSetter(new AccessorProperty(getName(), 0, sobj.getClass(), getMap().getFieldCount(), type));
+        return createNewSetter(new AccessorProperty(getName(), 0, sobj.getClass(), getMap().getFreeFieldSlot(), type));
     }
 
     private SetMethod createNewSpillPropertySetter() {
-        return createNewSetter(new SpillProperty(getName(), 0, getMap().getSpillLength(), type));
+        return createNewSetter(new SpillProperty(getName(), 0, getMap().getFreeSpillSlot(), type));
     }
 
     private PropertyMap getNewMap(final Property property) {

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

@@ -206,11 +206,6 @@ public class SpillProperty extends AccessorProperty {
         return true;
     }
 
-    @Override
-    public int getSpillCount() {
-        return 1;
-    }
-
     @Override
     void initMethodHandles(Class<?> structure) {
         final int slot  = getSlot();

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

@@ -43,16 +43,6 @@ import jdk.nashorn.internal.runtime.linker.Bootstrap;
  * Property with user defined getters/setters. Actual getter and setter
  * functions are stored in underlying ScriptObject. Only the 'slot' info is
  * stored in the property.
- *
- * The slots here denote either ScriptObject embed field number or spill
- * array index. For spill array index, we use slot value of
- * (index + ScriptObject.embedSize). See also ScriptObject.getEmbedOrSpill
- * method. Negative slot value means that the corresponding getter or setter
- * is null. Note that always two slots are allocated in ScriptObject - but
- * negative (less by 1) slot number is stored for null getter or setter.
- * This is done so that when the property is redefined with a different
- * getter and setter (say, both non-null), we'll have spill slots to store
- * those. When a slot is negative, (-slot - 1) is the embed/spill index.
  */
 public final class UserAccessorProperty extends SpillProperty {
 
@@ -117,10 +107,9 @@ public final class UserAccessorProperty extends SpillProperty {
     /**
      * Constructor
      *
-     * @param key        property key
-     * @param flags      property flags
-     * @param getterSlot getter slot, starting at first embed
-     * @param setterSlot setter slot, starting at first embed
+     * @param key   property key
+     * @param flags property flags
+     * @param slot  spill slot
      */
     UserAccessorProperty(final String key, final int flags, final int slot) {
         super(key, flags, slot);
@@ -206,17 +195,17 @@ public final class UserAccessorProperty extends SpillProperty {
 
     @Override
     public void setValue(final ScriptObject self, final ScriptObject owner, final int value, final boolean strict) {
-        setValue(self, owner, value, strict);
+        setValue(self, owner, (Object) value, strict);
     }
 
     @Override
     public void setValue(final ScriptObject self, final ScriptObject owner, final long value, final boolean strict) {
-        setValue(self, owner, value, strict);
+        setValue(self, owner, (Object) value, strict);
     }
 
     @Override
     public void setValue(final ScriptObject self, final ScriptObject owner, final double value, final boolean strict) {
-        setValue(self, owner, value, strict);
+        setValue(self, owner, (Object) value, strict);
     }
 
     @Override

nashorn/test/script/nosecurity/JDK-8044851.js

+/*
+ * Copyright (c) 2010, 2014, 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-8044851: nashorn properties leak memory
+ *
+ * @test
+ * @run
+ * @option -Dnashorn.debug=true
+ * @fork
+ */
+
+function printProperty(value, property) {
+    print(value, property.getKey(), property.isSpill() ? "spill" : "field", property.getSlot());
+}
+
+var obj = {}, i, name;
+
+for (i = 0; i < 8; ++i) {
+    name = 'property' + i;
+    obj[name] = 'a' + i;
+    printProperty(obj[name], Debug.map(obj).findProperty(name));
+}
+print();
+
+for (i = 0; i < 8; ++i) {
+    name = 'property' + i;
+    delete obj[name];
+}
+
+for (i = 0; i < 8; ++i) {
+    name = 'property' + i;
+    obj[name] = 'b' + i;
+    printProperty(obj[name], Debug.map(obj).findProperty(name));
+}
+print();
+
+for (i = 0; i < 8; ++i) {
+    name = 'property' + i;
+    Object.defineProperty(obj, name, {get: function() {return i;}, set: function(v) {}, configurable: true});
+    printProperty(obj[name], Debug.map(obj).findProperty(name));
+}
+print();
+
+for (i = 0; i < 8; ++i) {
+    name = 'property' + i;
+    delete obj[name];
+}
+
+for (i = 0; i < 8; ++i) {
+    name = 'property' + i;
+    obj[name] = 'c' + i;
+    printProperty(obj[name], Debug.map(obj).findProperty(name));
+}
+print();
+
+for (i = 7; i > -1; --i) {
+    name = 'property' + i;
+    delete obj[name];
+}
+
+for (i = 0; i < 8; ++i) {
+    name = 'property' + i;
+    obj[name] = 'd' + i;
+    printProperty(obj[name], Debug.map(obj).findProperty(name));
+}
+print();
+
+for (i = 0; i < 8; ++i) {
+    name = 'property' + i;
+    Object.defineProperty(obj, name, {get: function() {return i;}, set: function(v) {}});
+    printProperty(obj[name], Debug.map(obj).findProperty(name));
+}

nashorn/test/script/nosecurity/JDK-8044851.js.EXPECTED

+a0 property0 field 0
+a1 property1 field 1
+a2 property2 field 2
+a3 property3 field 3
+a4 property4 spill 0
+a5 property5 spill 1
+a6 property6 spill 2
+a7 property7 spill 3
+
+b0 property0 field 0
+b1 property1 field 1
+b2 property2 field 2
+b3 property3 field 3
+b4 property4 spill 0
+b5 property5 spill 1
+b6 property6 spill 2
+b7 property7 spill 3
+
+0 property0 spill 4
+1 property1 spill 5
+2 property2 spill 6
+3 property3 spill 7
+4 property4 spill 8
+5 property5 spill 0
+6 property6 spill 1
+7 property7 spill 2
+
+c0 property0 field 0
+c1 property1 field 1
+c2 property2 field 2
+c3 property3 field 3
+c4 property4 spill 0
+c5 property5 spill 1
+c6 property6 spill 2
+c7 property7 spill 3
+
+d0 property0 field 0
+d1 property1 field 1
+d2 property2 field 2
+d3 property3 field 3
+d4 property4 spill 0
+d5 property5 spill 1
+d6 property6 spill 2
+d7 property7 spill 3
+
+0 property0 spill 4
+1 property1 spill 5
+2 property2 spill 6
+3 property3 spill 7
+4 property4 spill 8
+5 property5 spill 0
+6 property6 spill 1
+7 property7 spill 2