From 80933c51bfc5186a31161a4435a2f5f9320bd9c2 Mon Sep 17 00:00:00 2001 From: Grzegorz Caban Date: Wed, 11 Dec 2024 01:19:05 +0000 Subject: [PATCH] SlotMap.compute can lead to deadlock in ThreadSafeSlotMapContainer (#1746) Refactor code to prevent a deadlock that can arise when Rhino is run with the "thread safe objects" feature turned on. Co-authored-by: duncan.macgregor --- .../org/mozilla/javascript/AccessorSlot.java | 48 +++++++ .../javascript/IdScriptableObject.java | 82 ++++++++--- .../javascript/LambdaAccessorSlot.java | 20 ++- .../org/mozilla/javascript/MemberBox.java | 10 ++ .../mozilla/javascript/ScriptableObject.java | 133 +++++++++--------- .../java/org/mozilla/javascript/Slot.java | 15 ++ .../tests/LambdaAccessorSlotTest.java | 29 ++++ .../tests/es6/DeadlockReproTest.java | 40 ++++++ .../javascript/tests/es6/PropertyTest.java | 45 ++++++ 9 files changed, 336 insertions(+), 86 deletions(-) create mode 100644 tests/src/test/java/org/mozilla/javascript/tests/es6/DeadlockReproTest.java diff --git a/rhino/src/main/java/org/mozilla/javascript/AccessorSlot.java b/rhino/src/main/java/org/mozilla/javascript/AccessorSlot.java index a9a3800bf0..02f26b2f6a 100644 --- a/rhino/src/main/java/org/mozilla/javascript/AccessorSlot.java +++ b/rhino/src/main/java/org/mozilla/javascript/AccessorSlot.java @@ -124,10 +124,34 @@ Function getGetterFunction(String name, Scriptable scope) { return getter.asGetterFunction(name, scope); } + @Override + boolean isSameGetterFunction(Object function) { + if (function == Scriptable.NOT_FOUND) { + return true; + } + if (getter == null) { + return ScriptRuntime.shallowEq(Undefined.instance, function); + } + return getter.isSameGetterFunction(function); + } + + @Override + boolean isSameSetterFunction(Object function) { + if (function == Scriptable.NOT_FOUND) { + return true; + } + if (setter == null) { + return ScriptRuntime.shallowEq(Undefined.instance, function); + } + 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. */ @@ -150,6 +174,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. */ @@ -175,12 +204,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. */ @@ -213,6 +250,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); + } } /** @@ -239,5 +281,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); + } } } diff --git a/rhino/src/main/java/org/mozilla/javascript/IdScriptableObject.java b/rhino/src/main/java/org/mozilla/javascript/IdScriptableObject.java index b21484dda3..6de2f80532 100644 --- a/rhino/src/main/java/org/mozilla/javascript/IdScriptableObject.java +++ b/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)) { @@ -877,7 +877,12 @@ protected void defineOwnProperty( setInstanceIdValue(id, value); } } - attr = applyDescriptorToAttributeBitset(attr, desc); + attr = + applyDescriptorToAttributeBitset( + attr, + getProperty(desc, "enumerable"), + getProperty(desc, "writable"), + getProperty(desc, "configurable")); setAttributes(name, attr); return; } @@ -889,8 +894,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) { @@ -900,7 +905,12 @@ protected void defineOwnProperty( } } prototypeValues.setAttributes( - id, applyDescriptorToAttributeBitset(attr, desc)); + id, + applyDescriptorToAttributeBitset( + attr, + getProperty(desc, "enumerable"), + getProperty(desc, "writable"), + getProperty(desc, "configurable"))); // Handle the regular slot that was created if this property was previously // replaced @@ -922,62 +932,96 @@ protected ScriptableObject getOwnPropertyDescriptor(Context cx, Object id) { ScriptableObject desc = super.getOwnPropertyDescriptor(cx, id); if (desc == null) { if (id instanceof String) { - return getBuiltInDescriptor((String) id); + return getBuiltInDataDescriptor((String) id); } if (ScriptRuntime.isSymbol(id)) { if (id instanceof SymbolKey) { - return getBuiltInDescriptor((SymbolKey) id); + return getBuiltInDataDescriptor((SymbolKey) id); } - return getBuiltInDescriptor(((NativeSymbol) id).getKey()); + return getBuiltInDataDescriptor(((NativeSymbol) id).getKey()); } } 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 getBuiltInDataDescriptor(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; - + private ScriptableObject getBuiltInDataDescriptor(Symbol key) { 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; diff --git a/rhino/src/main/java/org/mozilla/javascript/LambdaAccessorSlot.java b/rhino/src/main/java/org/mozilla/javascript/LambdaAccessorSlot.java index f9209fffa2..3802bcc146 100644 --- a/rhino/src/main/java/org/mozilla/javascript/LambdaAccessorSlot.java +++ b/rhino/src/main/java/org/mozilla/javascript/LambdaAccessorSlot.java @@ -53,7 +53,17 @@ boolean isSetterSlot() { @Override ScriptableObject getPropertyDescriptor(Context cx, Scriptable scope) { - ScriptableObject desc = (ScriptableObject) cx.newObject(scope); + return buildPropertyDescriptor(cx); + } + + /** + * The method exists avoid changing the getPropertyDescriptor signature and at the same time to + * make it explicit that we don't use Scriptable scope parameter of getPropertyDescriptor, since + * it can be problematic when called from inside ThreadSafeSlotMapContainer::compute lambda + * which can lead to deadlocks. + */ + public ScriptableObject buildPropertyDescriptor(Context cx) { + ScriptableObject desc = new NativeObject(); int attr = getAttributes(); boolean es6 = cx.getLanguageVersion() >= Context.VERSION_ES6; @@ -139,4 +149,12 @@ public void setSetter(Scriptable scope, BiConsumer setter) { }); } } + + public void replaceWith(LambdaAccessorSlot slot) { + this.getterFunction = slot.getterFunction; + this.getter = slot.getter; + this.setterFunction = slot.setterFunction; + this.setter = slot.setter; + setAttributes(slot.getAttributes()); + } } diff --git a/rhino/src/main/java/org/mozilla/javascript/MemberBox.java b/rhino/src/main/java/org/mozilla/javascript/MemberBox.java index 1f6ab40c14..3035950a2b 100644 --- a/rhino/src/main/java/org/mozilla/javascript/MemberBox.java +++ b/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 diff --git a/rhino/src/main/java/org/mozilla/javascript/ScriptableObject.java b/rhino/src/main/java/org/mozilla/javascript/ScriptableObject.java index 3b630dd273..439af07d0d 100644 --- a/rhino/src/main/java/org/mozilla/javascript/ScriptableObject.java +++ b/rhino/src/main/java/org/mozilla/javascript/ScriptableObject.java @@ -1617,6 +1617,17 @@ protected void defineOwnProperty( } } + // this property lookup cannot happen from inside slotMap.compute lambda + // as it risks causing a deadlock if ThreadSafeSlotMapContainer is used + // and `this` is in prototype chain of `desc` + Object enumerable = getProperty(desc, "enumerable"); + Object writable = getProperty(desc, "writable"); + Object configurable = getProperty(desc, "configurable"); + Object getter = getProperty(desc, "get"); + Object setter = getProperty(desc, "set"); + Object value = getProperty(desc, "value"); + boolean accessorDescriptor = isAccessorDescriptor(desc); + // Do some complex stuff depending on whether or not the key // already exists in a single hash map operation slotMap.compute( @@ -1624,9 +1635,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; @@ -1636,14 +1645,21 @@ protected void defineOwnProperty( slot = new Slot(k, ix, 0); attributes = applyDescriptorToAttributeBitset( - DONTENUM | READONLY | PERMANENT, desc); + DONTENUM | READONLY | PERMANENT, + enumerable, + writable, + configurable); } else { slot = existing; attributes = - applyDescriptorToAttributeBitset(existing.getAttributes(), desc); + applyDescriptorToAttributeBitset( + existing.getAttributes(), + enumerable, + writable, + configurable); } - if (isAccessorDescriptor(desc)) { + if (accessorDescriptor) { AccessorSlot fslot; if (slot instanceof AccessorSlot) { fslot = (AccessorSlot) slot; @@ -1651,11 +1667,10 @@ protected void defineOwnProperty( fslot = new AccessorSlot(slot); slot = fslot; } - Object getter = getProperty(desc, "get"); if (getter != NOT_FOUND) { fslot.getter = new AccessorSlot.FunctionGetter(getter); } - Object setter = getProperty(desc, "set"); + if (setter != NOT_FOUND) { fslot.setter = new AccessorSlot.FunctionSetter(setter); } @@ -1665,7 +1680,7 @@ protected void defineOwnProperty( // Replace a non-base slot with a regular slot slot = new Slot(slot); } - Object value = getProperty(desc, "value"); + if (value != NOT_FOUND) { slot.value = value; } else if (existing == null) { @@ -1729,30 +1744,47 @@ public void defineProperty( if (getter == null && setter == null) throw ScriptRuntime.typeError("at least one of {getter, setter} is required"); + LambdaAccessorSlot newSlot = createLambdaAccessorSlot(name, 0, getter, setter, attributes); + ScriptableObject newDesc = newSlot.buildPropertyDescriptor(cx); + checkPropertyDefinition(newDesc); slotMap.compute( name, 0, - (id, index, existing) -> - ensureLambdaAccessorSlot( - cx, id, index, existing, getter, setter, attributes)); + (id, index, existing) -> { + if (existing != null) { + // it's dangerous to use `this` as scope inside slotMap.compute. + // It can cause deadlock when ThreadSafeSlotMapContainer is used + + return replaceExistingLambdaSlot(cx, name, existing, newSlot); + } + checkPropertyChangeForSlot(name, null, newDesc); + return newSlot; + }); + } + + private LambdaAccessorSlot replaceExistingLambdaSlot( + Context cx, String name, Slot existing, LambdaAccessorSlot newSlot) { + LambdaAccessorSlot replacedSlot; + if (existing instanceof LambdaAccessorSlot) { + replacedSlot = (LambdaAccessorSlot) existing; + } else { + replacedSlot = new LambdaAccessorSlot(existing); + } + + replacedSlot.replaceWith(newSlot); + var replacedDesc = replacedSlot.buildPropertyDescriptor(cx); + + checkPropertyChangeForSlot(name, existing, replacedDesc); + return replacedSlot; } private LambdaAccessorSlot createLambdaAccessorSlot( Object name, int index, - Slot existing, java.util.function.Function getter, BiConsumer setter, int attributes) { - LambdaAccessorSlot slot; - if (existing == null) { - slot = new LambdaAccessorSlot(name, index); - } else if (existing instanceof LambdaAccessorSlot) { - slot = (LambdaAccessorSlot) existing; - } else { - slot = new LambdaAccessorSlot(existing); - } - + LambdaAccessorSlot slot = new LambdaAccessorSlot(name, index); slot.setGetter(this, getter); slot.setSetter(this, setter); slot.setAttributes(attributes); @@ -1773,14 +1805,14 @@ 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); @@ -1788,31 +1820,29 @@ protected void checkPropertyChange(Object id, ScriptableObject current, Scriptab 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); } } } @@ -1855,8 +1885,8 @@ protected boolean sameValue(Object newValue, Object currentValue) { return ScriptRuntime.shallowEq(currentValue, newValue); } - protected int applyDescriptorToAttributeBitset(int attributes, ScriptableObject desc) { - Object enumerable = getProperty(desc, "enumerable"); + protected int applyDescriptorToAttributeBitset( + int attributes, Object enumerable, Object writable, Object configurable) { if (enumerable != NOT_FOUND) { attributes = ScriptRuntime.toBoolean(enumerable) @@ -1864,7 +1894,6 @@ protected int applyDescriptorToAttributeBitset(int attributes, ScriptableObject : attributes | DONTENUM; } - Object writable = getProperty(desc, "writable"); if (writable != NOT_FOUND) { attributes = ScriptRuntime.toBoolean(writable) @@ -1872,7 +1901,6 @@ protected int applyDescriptorToAttributeBitset(int attributes, ScriptableObject : attributes | READONLY; } - Object configurable = getProperty(desc, "configurable"); if (configurable != NOT_FOUND) { attributes = ScriptRuntime.toBoolean(configurable) @@ -2815,33 +2843,6 @@ private static LambdaSlot ensureLambdaSlot(Object name, int index, Slot existing } } - private LambdaAccessorSlot ensureLambdaAccessorSlot( - Context cx, - Object name, - int index, - Slot existing, - java.util.function.Function getter, - BiConsumer setter, - int attributes) { - var newSlot = createLambdaAccessorSlot(name, index, existing, getter, setter, attributes); - 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; - } - } - private void writeObject(ObjectOutputStream out) throws IOException { out.defaultWriteObject(); final long stamp = slotMap.readLock(); diff --git a/rhino/src/main/java/org/mozilla/javascript/Slot.java b/rhino/src/main/java/org/mozilla/javascript/Slot.java index feab49eac8..a065b96123 100644 --- a/rhino/src/main/java/org/mozilla/javascript/Slot.java +++ b/rhino/src/main/java/org/mozilla/javascript/Slot.java @@ -127,4 +127,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; + } } diff --git a/tests/src/test/java/org/mozilla/javascript/tests/LambdaAccessorSlotTest.java b/tests/src/test/java/org/mozilla/javascript/tests/LambdaAccessorSlotTest.java index 92b696a04a..51097fe5e8 100644 --- a/tests/src/test/java/org/mozilla/javascript/tests/LambdaAccessorSlotTest.java +++ b/tests/src/test/java/org/mozilla/javascript/tests/LambdaAccessorSlotTest.java @@ -117,6 +117,35 @@ public void testOnlyGetterCanBeAccessed() { }); } + @Test + public void testRedefineExistingProperty() { + Utils.runWithAllOptimizationLevels( + cx -> { + Scriptable scope = cx.initStandardObjects(); + var sh = new StatusHolder("PENDING"); + + sh.defineProperty("value", "oldValueOfValue", DONTENUM); + + sh.defineProperty(cx, "value", (thisObj) -> "valueOfValue", null, DONTENUM); + + sh.defineProperty(cx, "status", (thisObj) -> 42, null, DONTENUM); + + sh.defineProperty( + cx, + "status", + (thisObj) -> self(thisObj).getStatus(), + (thisObj, value) -> self(thisObj).setStatus(value), + DONTENUM); + + var status = sh.get("status", sh); + assertEquals("PENDING", status); + + var value = sh.get("value", sh); + assertEquals("valueOfValue", value); + return null; + }); + } + @Test public void testWhenNoSetterDefined_InStrictMode_WillThrowException() { Utils.runWithAllOptimizationLevels( diff --git a/tests/src/test/java/org/mozilla/javascript/tests/es6/DeadlockReproTest.java b/tests/src/test/java/org/mozilla/javascript/tests/es6/DeadlockReproTest.java new file mode 100644 index 0000000000..7d6b8a6e72 --- /dev/null +++ b/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); + } + } +} diff --git a/tests/src/test/java/org/mozilla/javascript/tests/es6/PropertyTest.java b/tests/src/test/java/org/mozilla/javascript/tests/es6/PropertyTest.java index 8ea34f50c8..16d5810474 100644 --- a/tests/src/test/java/org/mozilla/javascript/tests/es6/PropertyTest.java +++ b/tests/src/test/java/org/mozilla/javascript/tests/es6/PropertyTest.java @@ -5,6 +5,7 @@ import java.lang.reflect.Method; import org.junit.Test; import org.mozilla.javascript.Context; +import org.mozilla.javascript.ContextFactory; import org.mozilla.javascript.ScriptableObject; import org.mozilla.javascript.tests.Utils; @@ -120,6 +121,50 @@ public void redefineSetterProperty() throws Exception { }); } + @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(); + + final String expected = "undefined - true - true | function - function"; + + final String script = + "Object.defineProperty(MyHostObject, 'foo', { enumerable: !0, configurable: !0, set: function() { return !0 }});\n" + + "var desc = Object.getOwnPropertyDescriptor(MyHostObject, 'foo');" + + "var result = '' + desc.writable + ' - ' + desc.configurable + ' - ' + desc.enumerable;" + + "result = result + ' | ' + typeof desc.get + ' - ' + typeof desc.set;" + + "result;"; + + try { + final MyHostObject myHostObject = new MyHostObject(); + + // define custom getter method + final Method getter = MyHostObject.class.getMethod("getFoo"); + final Method setter = MyHostObject.class.getMethod("setFoo", String.class); + myHostObject.defineProperty("foo", null, getter, setter, ScriptableObject.EMPTY); + scope.put("MyHostObject", scope, myHostObject); + } catch (Exception e) { + } + + final String result = (String) cx.evaluateString(scope, script, "myScript", 1, null); + + assertEquals(expected, result); + } + } + public static class MyHostObject extends ScriptableObject { private String foo;