8136544: Call site switching to megamorphic causes incorrect property read

Reviewed-by: attila, mhaupt

nashorn/src/jdk.scripting.nashorn/share/classes/jdk/nashorn/internal/objects/NativeJavaImporter.java

@@ -134,7 +134,7 @@ public final class NativeJavaImporter extends ScriptObject {
     }
 
     @Override
-    protected Object invokeNoSuchProperty(final String name, final int programPoint) {
+    protected Object invokeNoSuchProperty(final String name, final boolean isScope, final int programPoint) {
         final Object retval = createProperty(name);
         if (isValid(programPoint)) {
             throw new UnwarrantedOptimismException(retval, programPoint);

nashorn/src/jdk.scripting.nashorn/share/classes/jdk/nashorn/internal/runtime/NativeJavaPackage.java

@@ -206,7 +206,7 @@ public final class NativeJavaPackage extends ScriptObject {
     }
 
     @Override
-    protected Object invokeNoSuchProperty(final String key, final int programPoint) {
+    protected Object invokeNoSuchProperty(final String key, final boolean isScope, final int programPoint) {
         final Object retval = createProperty(key);
         if (isValid(programPoint)) {
             throw new UnwarrantedOptimismException(retval, programPoint);

nashorn/src/jdk.scripting.nashorn/share/classes/jdk/nashorn/internal/runtime/ScriptObject.java

@@ -149,7 +149,7 @@ public abstract class ScriptObject implements PropertyAccess, Cloneable {
     /** Method handle to retrieve prototype of this object */
     public static final MethodHandle GETPROTO      = findOwnMH_V("getProto", ScriptObject.class);
 
-    static final MethodHandle MEGAMORPHIC_GET    = findOwnMH_V("megamorphicGet", Object.class, String.class, boolean.class);
+    static final MethodHandle MEGAMORPHIC_GET    = findOwnMH_V("megamorphicGet", Object.class, String.class, boolean.class, boolean.class);
     static final MethodHandle GLOBALFILTER       = findOwnMH_S("globalFilter", Object.class, Object.class);
     static final MethodHandle DECLARE_AND_SET    = findOwnMH_V("declareAndSet", void.class, String.class, Object.class);
 
@@ -2035,19 +2035,19 @@ public abstract class ScriptObject implements PropertyAccess, Cloneable {
 
     private static GuardedInvocation findMegaMorphicGetMethod(final CallSiteDescriptor desc, final String name, final boolean isMethod) {
         Context.getContextTrusted().getLogger(ObjectClassGenerator.class).warning("Megamorphic getter: " + desc + " " + name + " " +isMethod);
-        final MethodHandle invoker = MH.insertArguments(MEGAMORPHIC_GET, 1, name, isMethod);
+        final MethodHandle invoker = MH.insertArguments(MEGAMORPHIC_GET, 1, name, isMethod, NashornCallSiteDescriptor.isScope(desc));
         final MethodHandle guard   = getScriptObjectGuard(desc.getMethodType(), true);
         return new GuardedInvocation(invoker, guard);
     }
 
     @SuppressWarnings("unused")
-    private Object megamorphicGet(final String key, final boolean isMethod) {
+    private Object megamorphicGet(final String key, final boolean isMethod, final boolean isScope) {
         final FindProperty find = findProperty(key, true);
         if (find != null) {
             return find.getObjectValue();
         }
 
-        return isMethod ? getNoSuchMethod(key, INVALID_PROGRAM_POINT) : invokeNoSuchProperty(key, INVALID_PROGRAM_POINT);
+        return isMethod ? getNoSuchMethod(key, isScope, INVALID_PROGRAM_POINT) : invokeNoSuchProperty(key, isScope, INVALID_PROGRAM_POINT);
     }
 
     // Marks a property as declared and sets its value. Used as slow path for block-scoped LET and CONST
@@ -2382,10 +2382,11 @@ public abstract class ScriptObject implements PropertyAccess, Cloneable {
     /**
      * Invoke fall back if a property is not found.
      * @param name Name of property.
+     * @param isScope is this a scope access?
      * @param programPoint program point
      * @return Result from call.
      */
-    protected Object invokeNoSuchProperty(final String name, final int programPoint) {
+    protected Object invokeNoSuchProperty(final String name, final boolean isScope, final int programPoint) {
         final FindProperty find = findProperty(NO_SUCH_PROPERTY_NAME, true);
 
         Object ret = UNDEFINED;
@@ -2394,7 +2395,9 @@ public abstract class ScriptObject implements PropertyAccess, Cloneable {
             final Object func = find.getObjectValue();
 
             if (func instanceof ScriptFunction) {
-                ret = ScriptRuntime.apply((ScriptFunction)func, this, name);
+                final ScriptFunction sfunc = (ScriptFunction)func;
+                final Object self = isScope && sfunc.isStrict()? UNDEFINED : this;
+                ret = ScriptRuntime.apply(sfunc, self, name);
             }
         }
 
@@ -2409,13 +2412,14 @@ public abstract class ScriptObject implements PropertyAccess, Cloneable {
     /**
      * Get __noSuchMethod__ as a function bound to this object and {@code name} if it is defined.
      * @param name the method name
+     * @param isScope is this a scope access?
      * @return the bound function, or undefined
      */
-    private Object getNoSuchMethod(final String name, final int programPoint) {
+    private Object getNoSuchMethod(final String name, final boolean isScope, final int programPoint) {
         final FindProperty find = findProperty(NO_SUCH_METHOD_NAME, true);
 
         if (find == null) {
-            return invokeNoSuchProperty(name, programPoint);
+            return invokeNoSuchProperty(name, isScope, programPoint);
         }
 
         final Object value = find.getObjectValue();
@@ -2423,7 +2427,9 @@ public abstract class ScriptObject implements PropertyAccess, Cloneable {
             return UNDEFINED;
         }
 
-        return ((ScriptFunction)value).createBound(this, new Object[] {name});
+        final ScriptFunction func = (ScriptFunction)value;
+        final Object self = isScope && func.isStrict()? UNDEFINED : this;
+        return func.createBound(self, new Object[] {name});
     }
 
     private GuardedInvocation createEmptyGetter(final CallSiteDescriptor desc, final boolean explicitInstanceOfCheck, final String name) {
@@ -2738,7 +2744,7 @@ public abstract class ScriptObject implements PropertyAccess, Cloneable {
             }
         }
 
-        return JSType.toInt32(invokeNoSuchProperty(key, programPoint));
+        return JSType.toInt32(invokeNoSuchProperty(key, false, programPoint));
     }
 
     @Override
@@ -2820,7 +2826,7 @@ public abstract class ScriptObject implements PropertyAccess, Cloneable {
             }
         }
 
-        return JSType.toLong(invokeNoSuchProperty(key, programPoint));
+        return JSType.toLong(invokeNoSuchProperty(key, false, programPoint));
     }
 
     @Override
@@ -2902,7 +2908,7 @@ public abstract class ScriptObject implements PropertyAccess, Cloneable {
             }
         }
 
-        return JSType.toNumber(invokeNoSuchProperty(key, INVALID_PROGRAM_POINT));
+        return JSType.toNumber(invokeNoSuchProperty(key, false, INVALID_PROGRAM_POINT));
     }
 
     @Override
@@ -2983,7 +2989,7 @@ public abstract class ScriptObject implements PropertyAccess, Cloneable {
             }
         }
 
-        return invokeNoSuchProperty(key, INVALID_PROGRAM_POINT);
+        return invokeNoSuchProperty(key, false, INVALID_PROGRAM_POINT);
     }
 
     @Override

nashorn/src/jdk.scripting.nashorn/share/classes/jdk/nashorn/internal/runtime/WithObject.java

@@ -26,6 +26,7 @@
 package jdk.nashorn.internal.runtime;
 
 import static jdk.nashorn.internal.lookup.Lookup.MH;
+import static jdk.nashorn.internal.runtime.ScriptRuntime.UNDEFINED;
 
 import java.lang.invoke.MethodHandle;
 import java.lang.invoke.MethodHandles;
@@ -209,16 +210,18 @@ public final class WithObject extends Scope {
     }
 
     @Override
-    protected Object invokeNoSuchProperty(final String name, final int programPoint) {
+    protected Object invokeNoSuchProperty(final String name, final boolean isScope, final int programPoint) {
         FindProperty find = expression.findProperty(NO_SUCH_PROPERTY_NAME, true);
         if (find != null) {
             final Object func = find.getObjectValue();
             if (func instanceof ScriptFunction) {
-                return ScriptRuntime.apply((ScriptFunction)func, expression, name);
+                final ScriptFunction sfunc = (ScriptFunction)func;
+                final Object self = isScope && sfunc.isStrict()? UNDEFINED : expression;
+                return ScriptRuntime.apply(sfunc, self, name);
             }
         }
 
-        return getProto().invokeNoSuchProperty(name, programPoint);
+        return getProto().invokeNoSuchProperty(name, isScope, programPoint);
     }
 
     @Override

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

+/*
+ * Copyright (c) 2015, 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-8136544: Call site switching to megamorphic causes incorrect property read
+ *
+ * @test
+ * @fork
+ * @option -Dnashorn.unstable.relink.threshold=8
+ * @run
+ */
+
+var ScriptContext = Java.type("javax.script.ScriptContext");
+var ScriptEngineManager = Java.type("javax.script.ScriptEngineManager");
+var m = new ScriptEngineManager();
+var e = m.getEngineByName("nashorn");
+
+var scope = e.getBindings(ScriptContext.ENGINE_SCOPE);
+var MYVAR = "myvar";
+
+function loopupVar() {
+    try {
+        e.eval(MYVAR);
+        return true;
+    } catch (e) {
+        return false;
+    }
+}
+
+// make sure we exercise callsite beyond megamorphic threshold we set
+// in this test via nashorn.unstable.relink.threshold property
+// In each iteration, callsite is exercised twice (two evals)
+// So, LIMIT should be more than 4 to exercise megamorphic callsites.
+
+var LIMIT = 5; // This LIMIT should be more than 4
+
+for (var i = 0; i < LIMIT; i++) {
+    // remove the variable and lookup
+    delete scope[MYVAR];
+    Assert.assertFalse(loopupVar(), "Expected true in iteration " + i);
+
+    // set that variable and check again
+    scope[MYVAR] = "foo";
+    Assert.assertTrue(loopupVar(), "Expected false in iteration " + i);
+}

nashorn/test/src/jdk/nashorn/api/scripting/test/ScopeTest.java

@@ -26,6 +26,7 @@ package jdk.nashorn.api.scripting.test;
 
 import static org.testng.Assert.assertEquals;
 import static org.testng.Assert.assertNotNull;
+import static org.testng.Assert.assertFalse;
 import static org.testng.Assert.assertTrue;
 import static org.testng.Assert.fail;
 import javax.script.Bindings;
@@ -820,4 +821,38 @@ public class ScopeTest {
     public void recursiveEvalCallScriptContextTest() throws ScriptException {
         new RecursiveEval().program();
     }
+
+    private static final String VAR_NAME = "myvar";
+
+    private static boolean lookupVar(final ScriptEngine engine, final String varName) {
+        try {
+            engine.eval(varName);
+            return true;
+        } catch (final ScriptException se) {
+            return false;
+        }
+    }
+
+    // @bug 8136544: Call site switching to megamorphic causes incorrect property read
+    @Test
+    public void megamorphicPropertyReadTest() throws ScriptException {
+        final NashornScriptEngineFactory factory = new NashornScriptEngineFactory();
+        final ScriptEngine engine = factory.getScriptEngine();
+        final Bindings scope = engine.getBindings(ScriptContext.ENGINE_SCOPE);
+        boolean ret;
+
+        // Why 16 is the upper limit of this loop? The default nashorn dynalink megamorphic threshold is 16.
+        // See jdk.nashorn.internal.runtime.linker.Bootstrap.NASHORN_DEFAULT_UNSTABLE_RELINK_THRESHOLD
+        // We do, 'eval' of the same in this loop twice. So, 16*2 = 32 times that callsite in the script
+        // is exercised - much beyond the default megamorphic threshold.
+
+        for (int i = 0; i < 16; i++) {
+            scope.remove(VAR_NAME);
+            ret = lookupVar(engine, VAR_NAME);
+            assertFalse(ret, "Expected false in iteration " + i);
+            scope.put(VAR_NAME, "foo");
+            ret = lookupVar(engine, VAR_NAME);
+            assertTrue(ret, "Expected true in iteration " + i);
+        }
+    }
 }