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
Binary file nashorn/bin/fastCatchCombinator.jar has changed
--- a/nashorn/bin/runoptdualcatch.sh Mon Mar 03 11:24:44 2014 +0100
+++ b/nashorn/bin/runoptdualcatch.sh Wed Mar 05 09:51:00 2014 +0100
@@ -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
--- a/nashorn/src/jdk/nashorn/internal/codegen/ClassEmitter.java Mon Mar 03 11:24:44 2014 +0100
+++ b/nashorn/src/jdk/nashorn/internal/codegen/ClassEmitter.java Wed Mar 05 09:51:00 2014 +0100
@@ -386,7 +386,7 @@
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);
}
--- a/nashorn/src/jdk/nashorn/internal/codegen/CodeGenerator.java Mon Mar 03 11:24:44 2014 +0100
+++ b/nashorn/src/jdk/nashorn/internal/codegen/CodeGenerator.java Wed Mar 05 09:51:00 2014 +0100
@@ -1320,7 +1320,6 @@
lc.nextFreeSlot(block);
final boolean isFunctionBody = lc.isFunctionBody();
-
final FunctionNode function = lc.getCurrentFunction();
if (isFunctionBody) {
if (method.hasScope()) {
--- a/nashorn/src/jdk/nashorn/internal/codegen/FieldObjectCreator.java Mon Mar 03 11:24:44 2014 +0100
+++ b/nashorn/src/jdk/nashorn/internal/codegen/FieldObjectCreator.java Wed Mar 05 09:51:00 2014 +0100
@@ -101,7 +101,6 @@
@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 @@
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);
}
--- a/nashorn/src/jdk/nashorn/internal/codegen/ObjectClassGenerator.java Mon Mar 03 11:24:44 2014 +0100
+++ b/nashorn/src/jdk/nashorn/internal/codegen/ObjectClassGenerator.java Wed Mar 05 09:51:00 2014 +0100
@@ -306,16 +306,16 @@
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 @@
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 @@
*
* @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 @@
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 @@
* @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 @@
* @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 @@
* @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 @@
* @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));
--- a/nashorn/src/jdk/nashorn/internal/runtime/AccessorProperty.java Mon Mar 03 11:24:44 2014 +0100
+++ b/nashorn/src/jdk/nashorn/internal/runtime/AccessorProperty.java Wed Mar 05 09:51:00 2014 +0100
@@ -536,7 +536,6 @@
//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(),
--- a/nashorn/src/jdk/nashorn/internal/runtime/PropertyListenerManager.java Mon Mar 03 11:24:44 2014 +0100
+++ b/nashorn/src/jdk/nashorn/internal/runtime/PropertyListenerManager.java Wed Mar 05 09:51:00 2014 +0100
@@ -105,7 +105,7 @@
*/
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 @@
*/
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 @@
*/
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 @@
*/
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);
}
}
--- a/nashorn/src/jdk/nashorn/internal/runtime/PropertyMap.java Mon Mar 03 11:24:44 2014 +0100
+++ b/nashorn/src/jdk/nashorn/internal/runtime/PropertyMap.java Wed Mar 05 09:51:00 2014 +0100
@@ -71,7 +71,7 @@
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 @@
* @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 @@
* @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 @@
* 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 @@
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 @@
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 @@
*/
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) {
--- a/nashorn/src/jdk/nashorn/internal/runtime/ScriptFunction.java Mon Mar 03 11:24:44 2014 +0100
+++ b/nashorn/src/jdk/nashorn/internal/runtime/ScriptFunction.java Wed Mar 05 09:51:00 2014 +0100
@@ -237,12 +237,13 @@
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 @@
* @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 @@
// 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 @@
@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;
--- a/nashorn/src/jdk/nashorn/internal/runtime/ScriptObject.java Mon Mar 03 11:24:44 2014 +0100
+++ b/nashorn/src/jdk/nashorn/internal/runtime/ScriptObject.java Wed Mar 05 09:51:00 2014 +0100
@@ -1840,7 +1840,7 @@
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 @@
return new GuardedInvocation(
GETPROTO,
explicitInstanceOfCheck ?
- NashornGuards.getScriptObjectGuard() :
+ getScriptObjectGuard(desc.getMethodType(), explicitInstanceOfCheck) :
null,
null,
explicitInstanceOfCheck ?
@@ -1902,9 +1902,9 @@
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 @@
}
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 @@
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 @@
}
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 @@
* @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 @@
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 @@
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);
}
/**