SlotMap.compute can lead to deadlock in ThreadSafeSlotMapContainer

rhino/src/main/java/org/mozilla/javascript/AccessorSlot.java

@@ -113,10 +113,34 @@ Function getGetterFunction(String name, Scriptable scope) {
         return getter.asGetterFunction(name, scope);
     }
 
+    @Override
+    boolean isSameGetterFunction(Object function) {
+        if (function == Scriptable.NOT_FOUND) {
+            return true;
+        } else if (getter == null) {
+            return ScriptRuntime.shallowEq(Undefined.instance, function);
+        } else {
+            return getter.isSameGetterFunction(function);
+        }
+    }
+
+    @Override
+    boolean isSameSetterFunction(Object function) {
+        if (function == Scriptable.NOT_FOUND) {
+            return true;
+        } else if (setter == null) {
+            return ScriptRuntime.shallowEq(Undefined.instance, function);
+        } else {
+            return setter.isSameSetterFunction(function);
+        }
+    }
+
     interface Getter {
         Object getValue(Scriptable start);
 
         Function asGetterFunction(final String name, final Scriptable scope);
+
+        boolean isSameGetterFunction(Object getter);
     }
 
     /** This is a Getter that delegates to a Java function via a MemberBox. */
@@ -139,6 +163,11 @@ public Object getValue(Scriptable start) {
         public Function asGetterFunction(String name, Scriptable scope) {
             return member.asGetterFunction(name, scope);
         }
+
+        @Override
+        public boolean isSameGetterFunction(Object function) {
+            return member.isSameGetterFunction(function);
+        }
     }
 
     /** This is a getter that delegates to a JavaScript function. */
@@ -164,12 +193,20 @@ public Object getValue(Scriptable start) {
         public Function asGetterFunction(String name, Scriptable scope) {
             return target instanceof Function ? (Function) target : null;
         }
+
+        @Override
+        public boolean isSameGetterFunction(Object function) {
+            return ScriptRuntime.shallowEq(
+                    target instanceof Function ? (Function) target : Undefined.instance, function);
+        }
     }
 
     interface Setter {
         boolean setValue(Object value, Scriptable owner, Scriptable start);
 
         Function asSetterFunction(final String name, final Scriptable scope);
+
+        boolean isSameSetterFunction(Object getter);
     }
 
     /** Invoke the setter on this slot via reflection using MemberBox. */
@@ -202,6 +239,11 @@ public boolean setValue(Object value, Scriptable owner, Scriptable start) {
         public Function asSetterFunction(String name, Scriptable scope) {
             return member.asSetterFunction(name, scope);
         }
+
+        @Override
+        public boolean isSameSetterFunction(Object function) {
+            return member.isSameSetterFunction(function);
+        }
     }
 
     /**
@@ -228,5 +270,11 @@ public boolean setValue(Object value, Scriptable owner, Scriptable start) {
         public Function asSetterFunction(String name, Scriptable scope) {
             return target instanceof Function ? (Function) target : null;
         }
+
+        @Override
+        public boolean isSameSetterFunction(Object function) {
+            return ScriptRuntime.shallowEq(
+                    target instanceof Function ? (Function) target : Undefined.instance, function);
+        }
     }
 }

rhino/src/main/java/org/mozilla/javascript/IdScriptableObject.java

@@ -867,8 +867,8 @@ protected void defineOwnProperty(
                     delete(id); // it will be replaced with a slot
                 } else {
                     checkPropertyDefinition(desc);
-                    ScriptableObject current = getOwnPropertyDescriptor(cx, key);
-                    checkPropertyChange(name, current, desc);
+                    var slot = queryOrFakeSlot(cx, key);
+                    checkPropertyChangeForSlot(name, slot, desc);
                     int attr = (info >>> 16);
                     Object value = getProperty(desc, "value");
                     if (value != NOT_FOUND && ((attr & READONLY) == 0 || (attr & PERMANENT) == 0)) {
@@ -889,8 +889,8 @@ protected void defineOwnProperty(
                         prototypeValues.delete(id); // it will be replaced with a slot
                     } else {
                         checkPropertyDefinition(desc);
-                        ScriptableObject current = getOwnPropertyDescriptor(cx, key);
-                        checkPropertyChange(name, current, desc);
+                        var slot = queryOrFakeSlot(cx, key);
+                        checkPropertyChangeForSlot(name, slot, desc);
                         int attr = prototypeValues.getAttributes(id);
                         Object value = getProperty(desc, "value");
                         if (value != NOT_FOUND && (attr & READONLY) == 0) {
@@ -936,48 +936,82 @@ protected ScriptableObject getOwnPropertyDescriptor(Context cx, Object id) {
         return desc;
     }
 
-    private ScriptableObject getBuiltInDescriptor(String name) {
-        Object value = null;
-        int attr = EMPTY;
+    private Slot queryOrFakeSlot(Context cx, Object id) {
+        var slot = querySlot(cx, id);
+        if (slot == null) {
+            if (id instanceof String) {
+                return getBuiltInSlot((String) id);
+            }
+
+            if (ScriptRuntime.isSymbol(id)) {
+                if (id instanceof SymbolKey) {
+                    return getBuiltInSlot((SymbolKey) id);
+                }
 
+                return getBuiltInSlot(((NativeSymbol) id).getKey());
+            }
+        }
+        return slot;
+    }
+
+    private ScriptableObject getBuiltInDescriptor(String name) {
         Scriptable scope = getParentScope();
         if (scope == null) {
             scope = this;
         }
 
+        var slot = getBuiltInSlot(name);
+        return slot == null ? null : buildDataDescriptor(scope, slot.value, slot.getAttributes());
+    }
+
+    private Slot getBuiltInSlot(String name) {
+        Object value = null;
+        int attr = EMPTY;
+
         int info = findInstanceIdInfo(name);
         if (info != 0) {
             int id = (info & 0xFFFF);
             value = getInstanceIdValue(id);
             attr = (info >>> 16);
-            return buildDataDescriptor(scope, value, attr);
+            var slot = new Slot(name, 0, attr);
+            slot.value = value;
+            return slot;
         }
         if (prototypeValues != null) {
             int id = prototypeValues.findId(name);
             if (id != 0) {
                 value = prototypeValues.get(id);
                 attr = prototypeValues.getAttributes(id);
-                return buildDataDescriptor(scope, value, attr);
+                var slot = new Slot(name, 0, attr);
+                slot.value = value;
+                return slot;
             }
         }
         return null;
     }
 
     private ScriptableObject getBuiltInDescriptor(Symbol key) {
-        Object value = null;
-        int attr = EMPTY;
-
         Scriptable scope = getParentScope();
         if (scope == null) {
             scope = this;
         }
 
+        var slot = getBuiltInSlot(key);
+        return slot == null ? null : buildDataDescriptor(scope, slot.value, slot.getAttributes());
+    }
+
+    private Slot getBuiltInSlot(Symbol key) {
+        Object value = null;
+        int attr = EMPTY;
+
         if (prototypeValues != null) {
             int id = prototypeValues.findId(key);
             if (id != 0) {
                 value = prototypeValues.get(id);
                 attr = prototypeValues.getAttributes(id);
-                return buildDataDescriptor(scope, value, attr);
+                var slot = new Slot(key, 0, attr);
+                slot.value = value;
+                return slot;
             }
         }
         return null;

rhino/src/main/java/org/mozilla/javascript/MemberBox.java

@@ -114,6 +114,16 @@ public String toString() {
         return memberObject.toString();
     }
 
+    boolean isSameGetterFunction(Object function) {
+        var f = asGetterFunction == null ? Undefined.instance : asGetterFunction;
+        return ScriptRuntime.shallowEq(function, f);
+    }
+
+    boolean isSameSetterFunction(Object function) {
+        var f = asSetterFunction == null ? Undefined.instance : asSetterFunction;
+        return ScriptRuntime.shallowEq(function, f);
+    }
+
     /** Function returned by calls to __lookupGetter__ */
     Function asGetterFunction(final String name, final Scriptable scope) {
         // Note: scope is the scriptable this function is related to; therefore this function

rhino/src/main/java/org/mozilla/javascript/ScriptableObject.java

@@ -1624,9 +1624,7 @@ protected void defineOwnProperty(
                 index,
                 (k, ix, existing) -> {
                     if (checkValid) {
-                        ScriptableObject current =
-                                existing == null ? null : existing.getPropertyDescriptor(cx, this);
-                        checkPropertyChange(id, current, desc);
+                        checkPropertyChangeForSlot(id, existing, desc);
                     }
 
                     Slot slot;
@@ -1773,46 +1771,44 @@ protected void checkPropertyDefinition(ScriptableObject desc) {
         }
     }
 
-    protected void checkPropertyChange(Object id, ScriptableObject current, ScriptableObject desc) {
+    protected void checkPropertyChangeForSlot(Object id, Slot current, ScriptableObject desc) {
         if (current == null) { // new property
             if (!isExtensible()) throw ScriptRuntime.typeErrorById("msg.not.extensible");
         } else {
-            if (isFalse(current.get("configurable", current))) {
+            if ((current.getAttributes() & PERMANENT) != 0) {
                 if (isTrue(getProperty(desc, "configurable")))
                     throw ScriptRuntime.typeErrorById("msg.change.configurable.false.to.true", id);
-                if (isTrue(current.get("enumerable", current))
+                if (((current.getAttributes() & DONTENUM) == 0)
                         != isTrue(getProperty(desc, "enumerable")))
                     throw ScriptRuntime.typeErrorById(
                             "msg.change.enumerable.with.configurable.false", id);
                 boolean isData = isDataDescriptor(desc);
                 boolean isAccessor = isAccessorDescriptor(desc);
                 if (!isData && !isAccessor) {
                     // no further validation required for generic descriptor
-                } else if (isData && isDataDescriptor(current)) {
-                    if (isFalse(current.get("writable", current))) {
+                } else if (isData) {
+                    if ((current.getAttributes() & READONLY) != 0) {
                         if (isTrue(getProperty(desc, "writable")))
                             throw ScriptRuntime.typeErrorById(
                                     "msg.change.writable.false.to.true.with.configurable.false",
                                     id);
 
-                        if (!sameValue(getProperty(desc, "value"), current.get("value", current)))
+                        if (!sameValue(getProperty(desc, "value"), current.value))
                             throw ScriptRuntime.typeErrorById(
                                     "msg.change.value.with.writable.false", id);
                     }
-                } else if (isAccessor && isAccessorDescriptor(current)) {
-                    if (!sameValue(getProperty(desc, "set"), current.get("set", current)))
+                } else if (isAccessor && current instanceof AccessorSlot) {
+                    AccessorSlot accessor = (AccessorSlot) current;
+                    if (!accessor.isSameSetterFunction(getProperty(desc, "set")))
                         throw ScriptRuntime.typeErrorById(
                                 "msg.change.setter.with.configurable.false", id);
 
-                    if (!sameValue(getProperty(desc, "get"), current.get("get", current)))
+                    if (!accessor.isSameGetterFunction(getProperty(desc, "get")))
                         throw ScriptRuntime.typeErrorById(
                                 "msg.change.getter.with.configurable.false", id);
                 } else {
-                    if (isDataDescriptor(current))
-                        throw ScriptRuntime.typeErrorById(
-                                "msg.change.property.data.to.accessor.with.configurable.false", id);
                     throw ScriptRuntime.typeErrorById(
-                            "msg.change.property.accessor.to.data.with.configurable.false", id);
+                            "msg.change.property.data.to.accessor.with.configurable.false", id);
                 }
             }
         }
@@ -2776,19 +2772,8 @@ private LambdaAccessorSlot ensureLambdaAccessorSlot(
         var newDesc = newSlot.getPropertyDescriptor(cx, this);
         checkPropertyDefinition(newDesc);
 
-        if (existing == null) {
-            checkPropertyChange(name, null, newDesc);
-            return newSlot;
-        } else if (existing instanceof LambdaAccessorSlot) {
-            var slot = (LambdaAccessorSlot) existing;
-            var existingDesc = slot.getPropertyDescriptor(cx, this);
-            checkPropertyChange(name, existingDesc, newDesc);
-            return newSlot;
-        } else {
-            var existingDesc = existing.getPropertyDescriptor(cx, this);
-            checkPropertyChange(name, existingDesc, newDesc);
-            return newSlot;
-        }
+        checkPropertyChangeForSlot(name, existing, newDesc);
+        return newSlot;
     }
 
     private void writeObject(ObjectOutputStream out) throws IOException {

rhino/src/main/java/org/mozilla/javascript/Slot.java

@@ -120,4 +120,19 @@ Function getSetterFunction(String name, Scriptable scope) {
     Function getGetterFunction(String name, Scriptable scope) {
         return null;
     }
+
+    /**
+     * Compare the JavaScript function that represents the "setter" to the provided Object. We do
+     * this to avoid generating a new function object when it might not be required. Specifically,
+     * if we have a cached funciion object that has not yet been generated then we don't have to
+     * generate it because it cannot be the same as the provided function.
+     */
+    boolean isSameSetterFunction(Object function) {
+        return false;
+    }
+
+    /** Same for the "getter" function. */
+    boolean isSameGetterFunction(Object function) {
+        return false;
+    }
 }

tests/src/test/java/org/mozilla/javascript/tests/es6/DeadlockReproTest.java

@@ -0,0 +1,40 @@
+package org.mozilla.javascript.tests.es6;
+
+import static org.junit.Assert.assertEquals;
+
+import org.junit.Test;
+import org.mozilla.javascript.Context;
+import org.mozilla.javascript.ContextFactory;
+import org.mozilla.javascript.NativeObject;
+import org.mozilla.javascript.ScriptableObject;
+
+public class DeadlockReproTest {
+    @Test
+    public void redefinePropertyWithThreadSafeSlotMap() {
+        final ContextFactory factory =
+                new ContextFactory() {
+                    @Override
+                    protected boolean hasFeature(Context cx, int featureIndex) {
+                        if (featureIndex == Context.FEATURE_THREAD_SAFE_OBJECTS) {
+                            return true;
+                        }
+                        return super.hasFeature(cx, featureIndex);
+                    }
+                };
+
+        try (Context cx = factory.enterContext()) {
+            cx.setLanguageVersion(Context.VERSION_ES6);
+            ScriptableObject scope = cx.initStandardObjects();
+
+            scope.put("o", scope, new NativeObject());
+            final String script =
+                    "Object.defineProperty(o, 'test', {value: '1', configurable: !0});"
+                            + "Object.defineProperty(o, 'test', {value: 2});"
+                            + "o.test";
+
+            var result = cx.evaluateString(scope, script, "myScript", 1, null);
+
+            assertEquals(2, result);
+        }
+    }
+}