8036127: Prototype filter needs to be applied to getter guard as well, not just getter

Summary: This manifests itself as a bug in optimistic types, as inner functions may access properties of the wrong type, but it is also a bug in tip. Without optimistic types, we have been unable to find a reproducer due to more similar PropertyMaps
Reviewed-by: attila, jlaskey, sundar

nashorn/bin/runoptdualcatch.sh

@@ -4,7 +4,7 @@
 
 FILENAME="./optimistic_dual_catch_$(date|sed "s/ /_/g"|sed "s/:/_/g").jfr"
 
-DIR=/Users/marcus/src/tip/
+DIR=..
 FAST_CATCH_COMBINATOR=$DIR/bin/fastCatchCombinator.jar
 NASHORN_JAR=$DIR/dist/nashorn.jar
 

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

@@ -386,7 +386,7 @@ public class ClassEmitter implements Emitter {
                     return Context.getContext();
                 }
             });
-            TraceClassVisitor tcv = new TraceClassVisitor(null, new NashornTextifier(ctx.getEnv(), cr), pw);
+            final TraceClassVisitor tcv = new TraceClassVisitor(null, new NashornTextifier(ctx.getEnv(), cr), pw);
             cr.accept(tcv, 0);
         }
 

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

@@ -1320,7 +1320,6 @@ final class CodeGenerator extends NodeOperatorVisitor<CodeGeneratorLexicalContex
         lc.nextFreeSlot(block);
 
         final boolean isFunctionBody = lc.isFunctionBody();
-
         final FunctionNode function = lc.getCurrentFunction();
         if (isFunctionBody) {
             if (method.hasScope()) {

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

@@ -101,7 +101,6 @@ public abstract class FieldObjectCreator<T> extends ObjectCreator<T> {
     @Override
     protected void makeObject(final MethodEmitter method) {
         makeMap();
-
         final String className = getClassName();
         try {
             // NOTE: we must load the actual structure class here, because the API operates with Nashorn Type objects,
@@ -166,14 +165,16 @@ public abstract class FieldObjectCreator<T> extends ObjectCreator<T> {
     private void putField(final MethodEmitter method, final String key, final int fieldIndex, final MapTuple<T> tuple) {
         method.dup();
 
-        loadTuple(method, tuple);
-
-        final boolean isPrimitive = tuple.isPrimitive();
-        final Type    fieldType   = isPrimitive ? PRIMITIVE_FIELD_TYPE : Type.OBJECT;
+        final Type    fieldType   = tuple.isPrimitive() ? PRIMITIVE_FIELD_TYPE : Type.OBJECT;
         final String  fieldClass  = getClassName();
         final String  fieldName   = getFieldName(fieldIndex, fieldType);
         final String  fieldDesc   = typeDescriptor(fieldType.getTypeClass());
 
+        assert fieldName.equals(getFieldName(fieldIndex, PRIMITIVE_FIELD_TYPE)) || fieldType.isObject() :    key + " object keys must store to L*-fields";
+        assert fieldName.equals(getFieldName(fieldIndex, Type.OBJECT))          || fieldType.isPrimitive() : key + " primitive keys must store to J*-fields";
+
+        loadTuple(method, tuple);
+
         method.putField(fieldClass, fieldName, fieldDesc);
     }
 

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

@@ -306,16 +306,16 @@ public final class ObjectClassGenerator {
 
         addFields(classEmitter, fieldCount);
 
-        final MethodEmitter init = newInitMethod(classEmitter);
+        final MethodEmitter init = newInitMethod(className, classEmitter);
         init.returnVoid();
         init.end();
 
-        final MethodEmitter initWithSpillArrays = newInitWithSpillArraysMethod(classEmitter, ScriptObject.class);
+        final MethodEmitter initWithSpillArrays = newInitWithSpillArraysMethod(className, classEmitter, ScriptObject.class);
         initWithSpillArrays.returnVoid();
         initWithSpillArrays.end();
 
-        newEmptyInit(classEmitter, className);
-        newAllocate(classEmitter, className);
+        newEmptyInit(className, classEmitter);
+        newAllocate(className, classEmitter);
 
         return toByteArray(classEmitter);
     }
@@ -335,17 +335,17 @@ public final class ObjectClassGenerator {
         final ClassEmitter classEmitter = newClassEmitter(className, superName);
         final List<String> initFields   = addFields(classEmitter, fieldCount);
 
-        final MethodEmitter init = newInitScopeMethod(classEmitter);
+        final MethodEmitter init = newInitScopeMethod(className, classEmitter);
         initializeToUndefined(init, className, initFields);
         init.returnVoid();
         init.end();
 
-        final MethodEmitter initWithSpillArrays = newInitWithSpillArraysMethod(classEmitter, FunctionScope.class);
+        final MethodEmitter initWithSpillArrays = newInitWithSpillArraysMethod(className, classEmitter, FunctionScope.class);
         initializeToUndefined(initWithSpillArrays, className, initFields);
         initWithSpillArrays.returnVoid();
         initWithSpillArrays.end();
 
-        final MethodEmitter initWithArguments = newInitScopeWithArgumentsMethod(classEmitter);
+        final MethodEmitter initWithArguments = newInitScopeWithArgumentsMethod(className, classEmitter);
         initializeToUndefined(initWithArguments, className, initFields);
         initWithArguments.returnVoid();
         initWithArguments.end();
@@ -399,7 +399,7 @@ public final class ObjectClassGenerator {
      *
      * @return Open method emitter.
      */
-    private static MethodEmitter newInitMethod(final ClassEmitter classEmitter) {
+    private static MethodEmitter newInitMethod(final String className, final ClassEmitter classEmitter) {
         final MethodEmitter init = classEmitter.init(PropertyMap.class);
         init.begin();
         init.load(Type.OBJECT, JAVA_THIS.slot());
@@ -409,7 +409,7 @@ public final class ObjectClassGenerator {
         return init;
     }
 
-     private static MethodEmitter newInitWithSpillArraysMethod(final ClassEmitter classEmitter, final Class<?> superClass) {
+     private static MethodEmitter newInitWithSpillArraysMethod(final String className, final ClassEmitter classEmitter, final Class<?> superClass) {
         final MethodEmitter init = classEmitter.init(PropertyMap.class, long[].class, Object[].class);
         init.begin();
         init.load(Type.OBJECT, JAVA_THIS.slot());
@@ -426,7 +426,7 @@ public final class ObjectClassGenerator {
      * @param classEmitter  Open class emitter.
      * @return Open method emitter.
      */
-    private static MethodEmitter newInitScopeMethod(final ClassEmitter classEmitter) {
+    private static MethodEmitter newInitScopeMethod(final String className, final ClassEmitter classEmitter) {
         final MethodEmitter init = classEmitter.init(PropertyMap.class, ScriptObject.class);
         init.begin();
         init.load(Type.OBJECT, JAVA_THIS.slot());
@@ -442,7 +442,7 @@ public final class ObjectClassGenerator {
      * @param classEmitter  Open class emitter.
      * @return Open method emitter.
      */
-    private static MethodEmitter newInitScopeWithArgumentsMethod(final ClassEmitter classEmitter) {
+    private static MethodEmitter newInitScopeWithArgumentsMethod(final String className, final ClassEmitter classEmitter) {
         final MethodEmitter init = classEmitter.init(PropertyMap.class, ScriptObject.class, ScriptObject.class);
         init.begin();
         init.load(Type.OBJECT, JAVA_THIS.slot());
@@ -460,7 +460,7 @@ public final class ObjectClassGenerator {
      * @param classEmitter Open class emitter.
      * @param className    Name of JavaScript class.
      */
-    private static void newEmptyInit(final ClassEmitter classEmitter, final String className) {
+    private static void newEmptyInit(final String className, final ClassEmitter classEmitter) {
         final MethodEmitter emptyInit = classEmitter.init();
         emptyInit.begin();
         emptyInit.load(Type.OBJECT, JAVA_THIS.slot());
@@ -476,7 +476,7 @@ public final class ObjectClassGenerator {
      * @param classEmitter Open class emitter.
      * @param className    Name of JavaScript class.
      */
-    private static void newAllocate(final ClassEmitter classEmitter, final String className) {
+    private static void newAllocate(final String className, final ClassEmitter classEmitter) {
         final MethodEmitter allocate = classEmitter.method(EnumSet.of(Flag.PUBLIC, Flag.STATIC), ALLOCATE.symbolName(), ScriptObject.class, PropertyMap.class);
         allocate.begin();
         allocate._new(className, Type.typeFor(ScriptObject.class));

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

@@ -536,7 +536,6 @@ public class AccessorProperty extends Property {
 
         //all this does is add a return value filter for object fields only
         if (GETTER_CACHE[i] == null) {
-            //System.err.println(this + " needs new getter for " + type);
             GETTER_CACHE[i] = debug(
                 createGetter(
                     getCurrentType(),

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

@@ -105,7 +105,7 @@ public class PropertyListenerManager implements PropertyListener {
      */
     protected synchronized final void notifyPropertyAdded(final ScriptObject object, final Property prop) {
         if (listeners != null) {
-            for (PropertyListener listener : listeners.keySet()) {
+            for (final PropertyListener listener : listeners.keySet()) {
                 listener.propertyAdded(object, prop);
             }
         }
@@ -119,7 +119,7 @@ public class PropertyListenerManager implements PropertyListener {
      */
     protected synchronized final void notifyPropertyDeleted(final ScriptObject object, final Property prop) {
         if (listeners != null) {
-            for (PropertyListener listener : listeners.keySet()) {
+            for (final PropertyListener listener : listeners.keySet()) {
                 listener.propertyDeleted(object, prop);
             }
         }
@@ -134,7 +134,7 @@ public class PropertyListenerManager implements PropertyListener {
      */
     protected synchronized final void notifyPropertyModified(final ScriptObject object, final Property oldProp, final Property newProp) {
         if (listeners != null) {
-            for (PropertyListener listener : listeners.keySet()) {
+            for (final PropertyListener listener : listeners.keySet()) {
                 listener.propertyModified(object, oldProp, newProp);
             }
         }
@@ -149,7 +149,7 @@ public class PropertyListenerManager implements PropertyListener {
      */
     protected synchronized final void notifyProtoChanged(final ScriptObject object, final ScriptObject oldProto, final ScriptObject newProto) {
         if (listeners != null) {
-            for (PropertyListener listener : listeners.keySet()) {
+            for (final PropertyListener listener : listeners.keySet()) {
                 listener.protoChanged(object, oldProto, newProto);
             }
         }

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

@@ -71,7 +71,7 @@ public final class PropertyMap implements Iterable<Object>, PropertyListener {
     private int fieldCount;
 
     /** Number of fields available. */
-    private int fieldMaximum;
+    private final int fieldMaximum;
 
     /** Length of spill in use. */
     private int spillLength;
@@ -167,7 +167,7 @@ public final class PropertyMap implements Iterable<Object>, PropertyListener {
      * @return New {@link PropertyMap}.
      */
     public static PropertyMap newMap(final Collection<Property> properties, final int fieldCount, final int fieldMaximum,  final int spillLength) {
-        PropertyHashMap newProperties = EMPTY_HASHMAP.immutableAdd(properties);
+        final PropertyHashMap newProperties = EMPTY_HASHMAP.immutableAdd(properties);
         return new PropertyMap(newProperties, fieldCount, fieldMaximum, spillLength, false);
     }
 
@@ -181,7 +181,7 @@ public final class PropertyMap implements Iterable<Object>, PropertyListener {
      * @return New {@link PropertyMap}.
      */
     public static PropertyMap newMap(final Collection<Property> properties) {
-        return (properties == null || properties.isEmpty())? newMap() : newMap(properties, 0, 0, 0);
+        return properties == null || properties.isEmpty()? newMap() : newMap(properties, 0, 0, 0);
     }
 
     /**
@@ -356,10 +356,10 @@ public final class PropertyMap implements Iterable<Object>, PropertyListener {
          * the old property is an AccessorProperty and the new one is a UserAccessorProperty property.
          */
 
-        final boolean sameType = (oldProperty.getClass() == newProperty.getClass());
+        final boolean sameType = oldProperty.getClass() == newProperty.getClass();
         assert sameType ||
-                (oldProperty instanceof AccessorProperty &&
-                newProperty instanceof UserAccessorProperty) :
+                oldProperty instanceof AccessorProperty &&
+                newProperty instanceof UserAccessorProperty :
             "arbitrary replaceProperty attempted " + sameType + " oldProperty=" + oldProperty.getClass() + " newProperty=" + newProperty.getClass() + " [" + oldProperty.getCurrentType() + " => " + newProperty.getCurrentType() + "]";
 
         newMap.flags = getClonedFlags();
@@ -468,7 +468,7 @@ public final class PropertyMap implements Iterable<Object>, PropertyListener {
     PropertyMap freeze() {
         PropertyHashMap newProperties = EMPTY_HASHMAP;
 
-        for (Property oldProperty : properties.getProperties()) {
+        for (final Property oldProperty : properties.getProperties()) {
             int propertyFlags = Property.NOT_CONFIGURABLE;
 
             if (!(oldProperty instanceof UserAccessorProperty)) {
@@ -553,7 +553,7 @@ public final class PropertyMap implements Iterable<Object>, PropertyListener {
         final PropertyMap cachedMap;
         if (protoHistory != null) {
             final WeakReference<PropertyMap> weakMap = protoHistory.get(newProto);
-            cachedMap = (weakMap != null ? weakMap.get() : null);
+            cachedMap = weakMap != null ? weakMap.get() : null;
         } else {
             cachedMap = null;
         }
@@ -608,7 +608,7 @@ public final class PropertyMap implements Iterable<Object>, PropertyListener {
      */
     private PropertyMap checkHistory(final Property property) {
         if (history != null) {
-            PropertyMap historicMap = history.get(property);
+            final PropertyMap historicMap = history.get(property);
 
             if (historicMap != null) {
                 if (Context.DEBUG) {

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

@@ -237,12 +237,13 @@ public abstract class ScriptFunction extends ScriptObject {
         if (Context.DEBUG) {
             allocations++;
         }
+
         assert !isBoundFunction(); // allocate never invoked on bound functions
 
         final ScriptObject object = data.allocate();
 
         if (object != null) {
-            Object prototype = getPrototype();
+            final Object prototype = getPrototype();
             if (prototype instanceof ScriptObject) {
                 object.setProto((ScriptObject)prototype);
             }
@@ -378,7 +379,7 @@ public abstract class ScriptFunction extends ScriptObject {
      * @return self's prototype
      */
     public static Object G$prototype(final Object self) {
-        return (self instanceof ScriptFunction) ?
+        return self instanceof ScriptFunction ?
             ((ScriptFunction)self).getPrototype() :
             UNDEFINED;
     }
@@ -610,7 +611,7 @@ public abstract class ScriptFunction extends ScriptObject {
         // Create the same arguments for the delegate linking request that would be passed in an actual apply'd invocation
         final Object[] appliedArgs = new Object[isApply ? 3 : appliedType.parameterCount()];
         appliedArgs[0] = appliedFn;
-        appliedArgs[1] = passesThis ? (appliedFnNeedsWrappedThis ? ScriptFunctionData.wrapThis(args[2]) : args[2]) : ScriptRuntime.UNDEFINED;
+        appliedArgs[1] = passesThis ? appliedFnNeedsWrappedThis ? ScriptFunctionData.wrapThis(args[2]) : args[2] : ScriptRuntime.UNDEFINED;
         if(isApply) {
             appliedArgs[2] = passesArgs ? NativeFunction.toApplyArgs(args[3]) : ScriptRuntime.EMPTY_ARRAY;
         } else {
@@ -757,7 +758,7 @@ public abstract class ScriptFunction extends ScriptObject {
     @SuppressWarnings("unused")
     private static Object[] addZerothElement(final Object[] args, final Object value) {
         // extends input array with by adding new zeroth element
-        final Object[] src = (args == null)? ScriptRuntime.EMPTY_ARRAY : args;
+        final Object[] src = args == null? ScriptRuntime.EMPTY_ARRAY : args;
         final Object[] result = new Object[src.length + 1];
         System.arraycopy(src, 0, result, 1, src.length);
         result[0] = value;

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

@@ -1840,7 +1840,7 @@ public abstract class ScriptObject extends PropertyListenerManager implements Pr
         final int listIndex = depth - 1; // We don't need 0-deep walker
         MethodHandle filter = listIndex < PROTO_FILTERS.size() ? PROTO_FILTERS.get(listIndex) : null;
 
-        if(filter == null) {
+        if (filter == null) {
             filter = addProtoFilter(GETPROTO, depth - 1);
             PROTO_FILTERS.add(null);
             PROTO_FILTERS.set(listIndex, filter);
@@ -1874,7 +1874,7 @@ public abstract class ScriptObject extends PropertyListenerManager implements Pr
                 return new GuardedInvocation(
                         GETPROTO,
                         explicitInstanceOfCheck ?
-                                NashornGuards.getScriptObjectGuard() :
+                                getScriptObjectGuard(desc.getMethodType(), explicitInstanceOfCheck) :
                                 null,
                         null,
                         explicitInstanceOfCheck ?
@@ -1902,9 +1902,9 @@ public abstract class ScriptObject extends PropertyListenerManager implements Pr
 
         mh = find.getGetter(returnType, programPoint);
         //we never need a guard if noGuard is set
-        final boolean noGuard = OBJECT_FIELDS_ONLY && NashornCallSiteDescriptor.isFastScope(desc) && !property.canChangeType();
+        final boolean noGuard =/* OBJECT_FIELDS_ONLY &&*/ NashornCallSiteDescriptor.isFastScope(desc) && !property.canChangeType();
         // getMap() is fine as we have the prototype switchpoint depending on where the property was found
-        final MethodHandle guard;
+        MethodHandle guard;
         final Class<? extends Throwable> exception;
         if (noGuard) {
             guard     = null;
@@ -1925,9 +1925,13 @@ public abstract class ScriptObject extends PropertyListenerManager implements Pr
              }
 
              if (!property.hasGetterFunction(find.getOwner())) {
-                 // If not a scope bind to actual prototype as changing prototype will change the property map.
-                 // For scopes we install a filter that replaces the self object with the prototype owning the property.
-                mh = isScope() ? addProtoFilter(mh,    find.getProtoChainLength()) : bindTo(mh, find.getOwner());
+                 if (isScope()) {
+                     mh = addProtoFilter(mh, find.getProtoChainLength());
+                     guard = NashornGuards.getMapGuard(find.getOwner().getMap(), explicitInstanceOfCheck);
+                     guard = addProtoFilter(guard, find.getProtoChainLength());
+                 } else {
+                     mh = bindTo(mh, find.getOwner());
+                 }
              }
          }
 
@@ -1942,13 +1946,12 @@ public abstract class ScriptObject extends PropertyListenerManager implements Pr
 
     private static GuardedInvocation findMegaMorphicGetMethod(final CallSiteDescriptor desc, final String name, final boolean isMethod) {
         ObjectClassGenerator.LOG.warning("Megamorphic getter: " + desc + " " + name + " " +isMethod);
-        return new GuardedInvocation(MH.insertArguments(MEGAMORPHIC_GET, 1, name, isMethod), null, null, ClassCastException.class);
+        return new GuardedInvocation(MH.insertArguments(MEGAMORPHIC_GET, 1, name, isMethod), getScriptObjectGuard(desc.getMethodType(), true), null, null);
     }
 
     @SuppressWarnings("unused")
     private Object megamorphicGet(final String key, final boolean isMethod) {
         final FindProperty find = findProperty(key, true);
-
         if (find != null) {
             return getObjectValue(find);
         }
@@ -1981,7 +1984,11 @@ public abstract class ScriptObject extends PropertyListenerManager implements Pr
         }
 
         final MethodHandle mh = findGetIndexMethodHandle(returnClass, name, keyClass, desc);
-        return new GuardedInvocation(mh, NashornGuards.getScriptObjectGuard(explicitInstanceOfCheck), null, explicitInstanceOfCheck ? null : ClassCastException.class);
+        return new GuardedInvocation(mh, getScriptObjectGuard(callType, explicitInstanceOfCheck), null, explicitInstanceOfCheck ? null : ClassCastException.class);
+    }
+
+    private static MethodHandle getScriptObjectGuard(final MethodType type, final boolean explicitInstanceOfCheck) {
+        return ScriptObject.class.isAssignableFrom(type.parameterType(0)) ? null : NashornGuards.getScriptObjectGuard(explicitInstanceOfCheck);
     }
 
     /**
@@ -2014,13 +2021,14 @@ public abstract class ScriptObject extends PropertyListenerManager implements Pr
      * @return GuardedInvocation to be invoked at call site.
      */
     protected GuardedInvocation findSetMethod(final CallSiteDescriptor desc, final LinkRequest request) {
-        final String  name = desc.getNameToken(CallSiteDescriptor.NAME_OPERAND);
+        final String name = desc.getNameToken(CallSiteDescriptor.NAME_OPERAND);
+
         if (request.isCallSiteUnstable() || hasWithScope()) {
             return findMegaMorphicSetMethod(desc, name);
         }
 
         final boolean scope                   = isScope();
-           final boolean explicitInstanceOfCheck = explicitInstanceOfCheck(desc, request);
+        final boolean explicitInstanceOfCheck = explicitInstanceOfCheck(desc, request);
 
         /*
          * If doing property set on a scope object, we should stop proto search on the first
@@ -2050,7 +2058,7 @@ public abstract class ScriptObject extends PropertyListenerManager implements Pr
             if (PROTO_PROPERTY_NAME.equals(name)) {
                 return new GuardedInvocation(
                         SETPROTOCHECK,
-                        NashornGuards.getScriptObjectGuard(explicitInstanceOfCheck),
+                        getScriptObjectGuard(desc.getMethodType(), explicitInstanceOfCheck),
                         null,
                         explicitInstanceOfCheck ? null : ClassCastException.class);
             } else if (!isExtensible()) {
@@ -2124,7 +2132,7 @@ public abstract class ScriptObject extends PropertyListenerManager implements Pr
         MethodHandle methodHandle = findOwnMH_V(clazz, "set", void.class, keyClass, valueClass, boolean.class);
         methodHandle = MH.insertArguments(methodHandle, 3, isStrict);
 
-        return new GuardedInvocation(methodHandle, NashornGuards.getScriptObjectGuard(explicitInstanceOfCheck), null, explicitInstanceOfCheck ? null : ClassCastException.class);
+        return new GuardedInvocation(methodHandle, getScriptObjectGuard(callType, explicitInstanceOfCheck), null, explicitInstanceOfCheck ? null : ClassCastException.class);
     }
 
     /**