8062308: Incorrect constant linkage with multiple Globals in a Context

Reviewed-by: lagergren, sundar

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

@@ -29,6 +29,7 @@ import static jdk.nashorn.internal.lookup.Lookup.MH;
 import static jdk.nashorn.internal.runtime.ECMAErrors.referenceError;
 import static jdk.nashorn.internal.runtime.ECMAErrors.typeError;
 import static jdk.nashorn.internal.runtime.ScriptRuntime.UNDEFINED;
+
 import java.io.IOException;
 import java.io.PrintWriter;
 import java.lang.invoke.MethodHandle;
@@ -41,7 +42,6 @@ import java.util.List;
 import java.util.Map;
 import java.util.concurrent.Callable;
 import java.util.concurrent.ConcurrentHashMap;
-import java.util.concurrent.atomic.AtomicReference;
 import javax.script.ScriptContext;
 import javax.script.ScriptEngine;
 import jdk.internal.dynalink.linker.GuardedInvocation;
@@ -54,7 +54,6 @@ import jdk.nashorn.internal.objects.annotations.Property;
 import jdk.nashorn.internal.objects.annotations.ScriptClass;
 import jdk.nashorn.internal.runtime.ConsString;
 import jdk.nashorn.internal.runtime.Context;
-import jdk.nashorn.internal.runtime.GlobalConstants;
 import jdk.nashorn.internal.runtime.GlobalFunctions;
 import jdk.nashorn.internal.runtime.JSType;
 import jdk.nashorn.internal.runtime.NativeJavaPackage;
@@ -438,9 +437,6 @@ public final class Global extends ScriptObject implements Scope {
         this.scontext = scontext;
     }
 
-    // global constants for this global - they can be replaced with MethodHandle.constant until invalidated
-    private static AtomicReference<GlobalConstants> gcsInstance = new AtomicReference<>();
-
     @Override
     protected Context getContext() {
         return context;
@@ -470,11 +466,6 @@ public final class Global extends ScriptObject implements Scope {
         super(checkAndGetMap(context));
         this.context = context;
         this.setIsScope();
-        //we can only share one instance of Global constants between globals, or we consume way too much
-        //memory - this is good enough for most programs
-        while (gcsInstance.get() == null) {
-            gcsInstance.compareAndSet(null, new GlobalConstants(context.getLogger(GlobalConstants.class)));
-        }
     }
 
     /**
@@ -492,15 +483,6 @@ public final class Global extends ScriptObject implements Scope {
         return self instanceof Global? (Global)self : instance();
     }
 
-    /**
-     * Return the global constants map for fields that
-     * can be accessed as MethodHandle.constant
-     * @return constant map
-     */
-    public static GlobalConstants getConstants() {
-        return gcsInstance.get();
-    }
-
     /**
      * Check if we have a Global instance
      * @return true if one exists

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

@@ -33,6 +33,7 @@ import static jdk.nashorn.internal.runtime.CodeStore.newCodeStore;
 import static jdk.nashorn.internal.runtime.ECMAErrors.typeError;
 import static jdk.nashorn.internal.runtime.ScriptRuntime.UNDEFINED;
 import static jdk.nashorn.internal.runtime.Source.sourceFor;
+
 import java.io.File;
 import java.io.IOException;
 import java.io.PrintWriter;
@@ -60,6 +61,7 @@ import java.util.HashMap;
 import java.util.LinkedHashMap;
 import java.util.Map;
 import java.util.concurrent.atomic.AtomicLong;
+import java.util.concurrent.atomic.AtomicReference;
 import java.util.function.Consumer;
 import java.util.function.Supplier;
 import java.util.logging.Level;
@@ -262,6 +264,10 @@ public final class Context {
     // persistent code store
     private CodeStore codeStore;
 
+    // A factory for linking global properties as constant method handles. It is created when the first Global
+    // is created, and invalidated forever once the second global is created.
+    private final AtomicReference<GlobalConstants> globalConstantsRef = new AtomicReference<>();
+
     /**
      * Get the current global scope
      * @return the current global scope
@@ -293,7 +299,10 @@ public final class Context {
         assert getGlobal() != global;
         //same code can be cached between globals, then we need to invalidate method handle constants
         if (global != null) {
-            Global.getConstants().invalidateAll();
+            final GlobalConstants globalConstants = getContext(global).getGlobalConstants();
+            if (globalConstants != null) {
+                globalConstants.invalidateAll();
+            }
         }
         currentGlobal.set(global);
     }
@@ -528,6 +537,15 @@ public final class Context {
         return classFilter;
     }
 
+    /**
+     * Returns the factory for constant method handles for global properties. The returned factory can be
+     * invalidated if this Context has more than one Global.
+     * @return the factory for constant method handles for global properties.
+     */
+    GlobalConstants getGlobalConstants() {
+        return globalConstantsRef.get();
+    }
+
     /**
      * Get the error manager for this context
      * @return error manger
@@ -1016,9 +1034,32 @@ public final class Context {
      * @return the global script object
      */
     public Global newGlobal() {
+        createOrInvalidateGlobalConstants();
         return new Global(this);
     }
 
+    private void createOrInvalidateGlobalConstants() {
+        for (;;) {
+            final GlobalConstants currentGlobalConstants = getGlobalConstants();
+            if (currentGlobalConstants != null) {
+                // Subsequent invocation; we're creating our second or later Global. GlobalConstants is not safe to use
+                // with more than one Global, as the constant method handle linkages it creates create a coupling
+                // between the Global and the call sites in the compiled code.
+                currentGlobalConstants.invalidateForever();
+                return;
+            }
+            final GlobalConstants newGlobalConstants = new GlobalConstants(getLogger(GlobalConstants.class));
+            if (globalConstantsRef.compareAndSet(null, newGlobalConstants)) {
+                // First invocation; we're creating the first Global in this Context. Create the GlobalConstants object
+                // for this Context.
+                return;
+            }
+
+            // If we reach here, then we started out as the first invocation, but another concurrent invocation won the
+            // CAS race. We'll just let the loop repeat and invalidate the CAS race winner.
+        }
+    }
+
     /**
      * Initialize given global scope object.
      *
@@ -1057,12 +1098,19 @@ public final class Context {
      * @return current global's context
      */
     static Context getContextTrusted() {
-        return ((ScriptObject)Context.getGlobal()).getContext();
+        return getContext(getGlobal());
     }
 
     static Context getContextTrustedOrNull() {
         final Global global = Context.getGlobal();
-        return global == null ? null : ((ScriptObject)global).getContext();
+        return global == null ? null : getContext(global);
+    }
+
+    private static Context getContext(final Global global) {
+        // We can't invoke Global.getContext() directly, as it's a protected override, and Global isn't in our package.
+        // In order to access the method, we must cast it to ScriptObject first (which is in our package) and then let
+        // virtual invocation do its thing.
+        return ((ScriptObject)global).getContext();
     }
 
     /**

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

@@ -31,12 +31,14 @@ import static jdk.nashorn.internal.lookup.Lookup.MH;
 import static jdk.nashorn.internal.runtime.UnwarrantedOptimismException.INVALID_PROGRAM_POINT;
 import static jdk.nashorn.internal.runtime.linker.NashornCallSiteDescriptor.getProgramPoint;
 import static jdk.nashorn.internal.runtime.logging.DebugLogger.quote;
+
 import java.lang.invoke.MethodHandle;
 import java.lang.invoke.MethodHandles;
 import java.lang.invoke.SwitchPoint;
 import java.util.Arrays;
 import java.util.HashMap;
 import java.util.Map;
+import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.logging.Level;
 import jdk.internal.dynalink.CallSiteDescriptor;
 import jdk.internal.dynalink.DynamicLinker;
@@ -50,7 +52,7 @@ import jdk.nashorn.internal.runtime.logging.Loggable;
 import jdk.nashorn.internal.runtime.logging.Logger;
 
 /**
- * Each global owns one of these. This is basically table of accessors
+ * Each context owns one of these. This is basically table of accessors
  * for global properties. A global constant is evaluated to a MethodHandle.constant
  * for faster access and to avoid walking to proto chain looking for it.
  *
@@ -67,12 +69,19 @@ import jdk.nashorn.internal.runtime.logging.Logger;
  * reregister the switchpoint. Set twice or more - don't try again forever, or we'd
  * just end up relinking our way into megamorphisism.
  *
+ * Also it has to be noted that this kind of linking creates a coupling between a Global
+ * and the call sites in compiled code belonging to the Context. For this reason, the
+ * linkage becomes incorrect as soon as the Context has more than one Global. The
+ * {@link #invalidateForever()} is invoked by the Context to invalidate all linkages and
+ * turn off the functionality of this object as soon as the Context's {@link Context#newGlobal()} is invoked
+ * for second time.
+ *
  * We can extend this to ScriptObjects in general (GLOBAL_ONLY=false), which requires
  * a receiver guard on the constant getter, but it currently leaks memory and its benefits
  * have not yet been investigated property.
  *
- * As long as all Globals share the same constant instance, we need synchronization
+ * As long as all Globals in a Context share the same GlobalConstants instance, we need synchronization
- * whenever we access the instance.
+ * whenever we access it.
  */
 @Logger(name="const")
 public final class GlobalConstants implements Loggable {
@@ -82,7 +91,7 @@ public final class GlobalConstants implements Loggable {
      * Script objects require a receiver guard, which is memory intensive, so this is currently
      * disabled. We might implement a weak reference based approach to this later.
      */
-    private static final boolean GLOBAL_ONLY = true;
+    public static final boolean GLOBAL_ONLY = true;
 
     private static final MethodHandles.Lookup LOOKUP = MethodHandles.lookup();
 
@@ -98,6 +107,8 @@ public final class GlobalConstants implements Loggable {
      */
     private final Map<String, Access> map = new HashMap<>();
 
+    private final AtomicBoolean invalidatedForever = new AtomicBoolean(false);
+
     /**
      * Constructor - used only by global
      * @param log logger, or null if none
@@ -216,12 +227,36 @@ public final class GlobalConstants implements Loggable {
      * the same class for a new global, but the builtins and global scoped variables
      * will have changed.
      */
-    public synchronized void invalidateAll() {
+    public void invalidateAll() {
+        if (!invalidatedForever.get()) {
             log.info("New global created - invalidating all constant callsites without increasing invocation count.");
+            synchronized (this) {
                 for (final Access acc : map.values()) {
                     acc.invalidateUncounted();
                 }
             }
+        }
+    }
+
+    /**
+     * To avoid an expensive global guard "is this the same global", similar to the
+     * receiver guard on the ScriptObject level, we invalidate all getters when the
+     * second Global is created by the Context owning this instance. After this
+     * method is invoked, this GlobalConstants instance will both invalidate all the
+     * switch points it produced, and it will stop handing out new method handles
+     * altogether.
+     */
+    public void invalidateForever() {
+        if (invalidatedForever.compareAndSet(false, true)) {
+            log.info("New global created - invalidating all constant callsites.");
+            synchronized (this) {
+                for (final Access acc : map.values()) {
+                    acc.invalidateForever();
+                }
+                map.clear();
+            }
+        }
+    }
 
     /**
      * Invalidate the switchpoint of an access - we have written to
@@ -251,7 +286,7 @@ public final class GlobalConstants implements Loggable {
         return obj;
     }
 
-    private synchronized Access getOrCreateSwitchPoint(final String name) {
+    private Access getOrCreateSwitchPoint(final String name) {
         Access acc = map.get(name);
         if (acc != null) {
             return acc;
@@ -267,11 +302,15 @@ public final class GlobalConstants implements Loggable {
      * @param name name of property
      */
     void delete(final String name) {
+        if (!invalidatedForever.get()) {
+            synchronized (this) {
                 final Access acc = map.get(name);
                 if (acc != null) {
                     acc.invalidateForever();
                 }
             }
+        }
+    }
 
     /**
      * Receiver guard is used if we extend the global constants to script objects in general.
@@ -313,28 +352,27 @@ public final class GlobalConstants implements Loggable {
      *
      * @return null if failed to set up constant linkage
      */
-    synchronized GuardedInvocation findSetMethod(final FindProperty find, final ScriptObject receiver, final GuardedInvocation inv, final CallSiteDescriptor desc, final LinkRequest request) {
+    GuardedInvocation findSetMethod(final FindProperty find, final ScriptObject receiver, final GuardedInvocation inv, final CallSiteDescriptor desc, final LinkRequest request) {
-        if (GLOBAL_ONLY && !isGlobalSetter(receiver, find)) {
+        if (invalidatedForever.get() || (GLOBAL_ONLY && !isGlobalSetter(receiver, find))) {
             return null;
         }
 
         final String name = desc.getNameToken(CallSiteDescriptor.NAME_OPERAND);
 
+        synchronized (this) {
             final Access acc  = getOrCreateSwitchPoint(name);
 
             if (log.isEnabled()) {
                 log.fine("Trying to link constant SETTER ", acc);
             }
 
-        if (!acc.mayRetry()) {
+            if (!acc.mayRetry() || invalidatedForever.get()) {
                 if (log.isEnabled()) {
                     log.fine("*** SET: Giving up on " + quote(name) + " - retry count has exceeded " + DynamicLinker.getLinkedCallSiteLocation());
                 }
                 return null;
             }
 
-        assert acc.mayRetry();
-
             if (acc.hasBeenInvalidated()) {
                 log.info("New chance for " + acc);
                 acc.newSwitchPoint();
@@ -353,6 +391,7 @@ public final class GlobalConstants implements Loggable {
             log.info("Linked setter " + quote(name) + " " + acc.getSwitchPoint());
             return new GuardedInvocation(mh, inv.getGuard(), acc.getSwitchPoint(), inv.getException());
         }
+    }
 
     /**
      * Try to reuse constant method handles for getters
@@ -380,11 +419,11 @@ public final class GlobalConstants implements Loggable {
      *
      * @return resulting getter, or null if failed to create constant
      */
-    synchronized GuardedInvocation findGetMethod(final FindProperty find, final ScriptObject receiver, final CallSiteDescriptor desc) {
+    GuardedInvocation findGetMethod(final FindProperty find, final ScriptObject receiver, final CallSiteDescriptor desc) {
         // Only use constant getter for fast scope access, because the receiver may change between invocations
         // for slow-scope and non-scope callsites.
         // Also return null for user accessor properties as they may have side effects.
-        if (!NashornCallSiteDescriptor.isFastScope(desc)
+        if (invalidatedForever.get() || !NashornCallSiteDescriptor.isFastScope(desc)
                 || (GLOBAL_ONLY && !find.getOwner().isGlobal())
                 || find.getProperty() instanceof UserAccessorProperty) {
             return null;
@@ -395,6 +434,7 @@ public final class GlobalConstants implements Loggable {
         final Class<?> retType      = desc.getMethodType().returnType();
         final String   name         = desc.getNameToken(CallSiteDescriptor.NAME_OPERAND);
 
+        synchronized (this) {
             final Access acc = getOrCreateSwitchPoint(name);
 
             log.fine("Starting to look up object value " + name);
@@ -404,7 +444,7 @@ public final class GlobalConstants implements Loggable {
                 log.fine("Trying to link constant GETTER " + acc + " value = " + c);
             }
 
-        if (acc.hasBeenInvalidated() || acc.guardFailed()) {
+            if (acc.hasBeenInvalidated() || acc.guardFailed() || invalidatedForever.get()) {
                 if (log.isEnabled()) {
                     log.info("*** GET: Giving up on " + quote(name) + " - retry count has exceeded " + DynamicLinker.getLinkedCallSiteLocation());
                 }
@@ -442,4 +482,5 @@ public final class GlobalConstants implements Loggable {
 
             return new GuardedInvocation(mh, guard, acc.getSwitchPoint(), null);
         }
+    }
 }

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

@@ -47,6 +47,7 @@ import static jdk.nashorn.internal.runtime.UnwarrantedOptimismException.isValid;
 import static jdk.nashorn.internal.runtime.arrays.ArrayIndex.getArrayIndex;
 import static jdk.nashorn.internal.runtime.arrays.ArrayIndex.isValidArrayIndex;
 import static jdk.nashorn.internal.runtime.linker.NashornGuards.explicitInstanceOfCheck;
+
 import java.lang.invoke.MethodHandle;
 import java.lang.invoke.MethodHandles;
 import java.lang.invoke.MethodType;
@@ -922,7 +923,10 @@ public abstract class ScriptObject implements PropertyAccess {
                 if (property instanceof UserAccessorProperty) {
                     ((UserAccessorProperty)property).setAccessors(this, getMap(), null);
                 }
-                Global.getConstants().delete(property.getKey());
+                final GlobalConstants globalConstants = getGlobalConstants();
+                if (globalConstants != null) {
+                    globalConstants.delete(property.getKey());
+                }
                 return true;
             }
         }
@@ -1983,10 +1987,13 @@ public abstract class ScriptObject implements PropertyAccess {
             }
         }
 
-        final GuardedInvocation cinv = Global.getConstants().findGetMethod(find, this, desc);
+        final GlobalConstants globalConstants = getGlobalConstants();
+        if (globalConstants != null) {
+            final GuardedInvocation cinv = globalConstants.findGetMethod(find, this, desc);
             if (cinv != null) {
                 return cinv;
             }
+        }
 
         final Class<?> returnType = desc.getMethodType().returnType();
         final Property property   = find.getProperty();
@@ -2183,14 +2190,22 @@ public abstract class ScriptObject implements PropertyAccess {
 
         final GuardedInvocation inv = new SetMethodCreator(this, find, desc, request).createGuardedInvocation(findBuiltinSwitchPoint(name));
 
-        final GuardedInvocation cinv = Global.getConstants().findSetMethod(find, this, inv, desc, request);
+        final GlobalConstants globalConstants = getGlobalConstants();
+        if (globalConstants != null) {
+            final GuardedInvocation cinv = globalConstants.findSetMethod(find, this, inv, desc, request);
             if (cinv != null) {
                 return cinv;
             }
+        }
 
         return inv;
     }
 
+    private GlobalConstants getGlobalConstants() {
+        // Avoid hitting getContext() which might be costly for a non-Global unless needed.
+        return GlobalConstants.GLOBAL_ONLY && !isGlobal() ? null : getContext().getGlobalConstants();
+    }
+
     private GuardedInvocation createEmptySetMethod(final CallSiteDescriptor desc, final boolean explicitInstanceOfCheck, final String strictErrorMessage, final boolean canBeFastScope) {
         final String  name = desc.getNameToken(CallSiteDescriptor.NAME_OPERAND);
         if (NashornCallSiteDescriptor.isStrict(desc)) {