From d69afa8aa8d03d70e945d9fd07d218c7d790b521 Mon Sep 17 00:00:00 2001 From: nabacg Date: Mon, 2 Dec 2024 12:48:36 +0000 Subject: [PATCH 01/10] adding PropertyTest redefinePropertyWithThreadSafeSlotMap as a repro case of Dead Lock that can occure when new SlotMap.compute method is used with ThreadSafeSlotMapContainer --- .../javascript/tests/es6/PropertyTest.java | 60 +++++++++++++++++++ 1 file changed, 60 insertions(+) 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..c10a44e33f 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 @@ -1,12 +1,19 @@ package org.mozilla.javascript.tests.es6; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.mozilla.javascript.tests.Utils.DEFAULT_OPT_LEVELS; +import java.io.FileReader; +import java.io.IOException; import java.lang.reflect.Method; import org.junit.Test; import org.mozilla.javascript.Context; +import org.mozilla.javascript.ContextFactory; +import org.mozilla.javascript.Scriptable; import org.mozilla.javascript.ScriptableObject; import org.mozilla.javascript.tests.Utils; +import org.mozilla.javascript.tools.shell.Global; public class PropertyTest { @@ -118,6 +125,59 @@ public void redefineSetterProperty() throws Exception { return null; }); + + + + } + + @Test + public void redefinePropertyWithThreadSafeSlotMap() { + + final ContextFactory factory = + new ContextFactory() { + @Override + protected boolean hasFeature(Context cx, int featureIndex) { + switch (featureIndex) { + case Context.FEATURE_THREAD_SAFE_OBJECTS: + return true; + default: + 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 { From 466c6e8e97c90bea15a5457ec0caf7c42c4d2fcc Mon Sep 17 00:00:00 2001 From: nabacg Date: Mon, 2 Dec 2024 13:35:47 +0000 Subject: [PATCH 02/10] adding minimal Deadlock repro case --- .../tests/es6/DeadlockReproTest.java | 43 +++++++++++++++++++ .../javascript/tests/es6/PropertyTest.java | 8 ++-- 2 files changed, 46 insertions(+), 5 deletions(-) create mode 100644 tests/src/test/java/org/mozilla/javascript/tests/es6/DeadlockReproTest.java 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..099f4739c5 --- /dev/null +++ b/tests/src/test/java/org/mozilla/javascript/tests/es6/DeadlockReproTest.java @@ -0,0 +1,43 @@ +package org.mozilla.javascript.tests.es6; + +import org.junit.Test; +import org.mozilla.javascript.Context; +import org.mozilla.javascript.ContextFactory; +import org.mozilla.javascript.NativeObject; +import org.mozilla.javascript.ScriptableObject; + +import static org.junit.Assert.assertEquals; + +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 c10a44e33f..0ed3413757 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 @@ -137,12 +137,10 @@ public void redefinePropertyWithThreadSafeSlotMap() { new ContextFactory() { @Override protected boolean hasFeature(Context cx, int featureIndex) { - switch (featureIndex) { - case Context.FEATURE_THREAD_SAFE_OBJECTS: - return true; - default: - return super.hasFeature(cx, featureIndex); + if (featureIndex == Context.FEATURE_THREAD_SAFE_OBJECTS) { + return true; } + return super.hasFeature(cx, featureIndex); } }; From 0ba1f300ba65be31778230d2b84ca5ac9945b267 Mon Sep 17 00:00:00 2001 From: nabacg Date: Mon, 2 Dec 2024 17:28:15 +0000 Subject: [PATCH 03/10] spotlessApply --- .../javascript/tests/es6/DeadlockReproTest.java | 17 +++++++---------- .../javascript/tests/es6/PropertyTest.java | 17 ++--------------- 2 files changed, 9 insertions(+), 25 deletions(-) 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 index 099f4739c5..7d6b8a6e72 100644 --- a/tests/src/test/java/org/mozilla/javascript/tests/es6/DeadlockReproTest.java +++ b/tests/src/test/java/org/mozilla/javascript/tests/es6/DeadlockReproTest.java @@ -1,13 +1,13 @@ 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; -import static org.junit.Assert.assertEquals; - public class DeadlockReproTest { @Test public void redefinePropertyWithThreadSafeSlotMap() { @@ -27,17 +27,14 @@ protected boolean hasFeature(Context cx, int featureIndex) { 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"; + 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); + 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 0ed3413757..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 @@ -1,19 +1,13 @@ package org.mozilla.javascript.tests.es6; import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertFalse; -import static org.mozilla.javascript.tests.Utils.DEFAULT_OPT_LEVELS; -import java.io.FileReader; -import java.io.IOException; import java.lang.reflect.Method; import org.junit.Test; import org.mozilla.javascript.Context; import org.mozilla.javascript.ContextFactory; -import org.mozilla.javascript.Scriptable; import org.mozilla.javascript.ScriptableObject; import org.mozilla.javascript.tests.Utils; -import org.mozilla.javascript.tools.shell.Global; public class PropertyTest { @@ -125,9 +119,6 @@ public void redefineSetterProperty() throws Exception { return null; }); - - - } @Test @@ -163,19 +154,15 @@ protected boolean hasFeature(Context cx, int featureIndex) { // 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); + 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); + final String result = (String) cx.evaluateString(scope, script, "myScript", 1, null); assertEquals(expected, result); - } - } public static class MyHostObject extends ScriptableObject { From 8681b1961ed9051e803a646583a3f8384edc6873 Mon Sep 17 00:00:00 2001 From: "duncan.macgregor" Date: Mon, 2 Dec 2024 17:16:38 +0000 Subject: [PATCH 04/10] Change to prevent creation of property descriptors when not required. --- .../org/mozilla/javascript/AccessorSlot.java | 48 ++++++++++++++ .../org/mozilla/javascript/MemberBox.java | 10 +++ .../mozilla/javascript/ScriptableObject.java | 62 ++++++++++++++----- .../java/org/mozilla/javascript/Slot.java | 15 +++++ 4 files changed, 119 insertions(+), 16 deletions(-) diff --git a/rhino/src/main/java/org/mozilla/javascript/AccessorSlot.java b/rhino/src/main/java/org/mozilla/javascript/AccessorSlot.java index a12841b8f6..690c4245c3 100644 --- a/rhino/src/main/java/org/mozilla/javascript/AccessorSlot.java +++ b/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); + } } } 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 8fc9cd5eca..b91ea7654e 100644 --- a/rhino/src/main/java/org/mozilla/javascript/ScriptableObject.java +++ b/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; @@ -1818,6 +1816,49 @@ protected void checkPropertyChange(Object id, ScriptableObject current, Scriptab } } + protected void checkPropertyChangeForSlot(Object id, Slot current, ScriptableObject desc) { + if (current == null) { // new property + if (!isExtensible()) throw ScriptRuntime.typeErrorById("msg.not.extensible"); + } else { + if ((current.getAttributes() & PERMANENT) != 0) { + if (isTrue(getProperty(desc, "configurable"))) + throw ScriptRuntime.typeErrorById("msg.change.configurable.false.to.true", id); + 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) { + 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.value)) + throw ScriptRuntime.typeErrorById( + "msg.change.value.with.writable.false", id); + } + } 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 (!accessor.isSameGetterFunction(getProperty(desc, "get"))) + throw ScriptRuntime.typeErrorById( + "msg.change.getter.with.configurable.false", id); + } else { + throw ScriptRuntime.typeErrorById( + "msg.change.property.data.to.accessor.with.configurable.false", id); + } + } + } + } + protected static boolean isTrue(Object value) { return (value != NOT_FOUND) && ScriptRuntime.toBoolean(value); } @@ -2776,19 +2817,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 { diff --git a/rhino/src/main/java/org/mozilla/javascript/Slot.java b/rhino/src/main/java/org/mozilla/javascript/Slot.java index 5f29c04146..1f741b6c52 100644 --- a/rhino/src/main/java/org/mozilla/javascript/Slot.java +++ b/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; + } } From 0f4e8562ba651e10f042a8ec4761efb3e76063e5 Mon Sep 17 00:00:00 2001 From: "duncan.macgregor" Date: Mon, 2 Dec 2024 20:33:01 +0000 Subject: [PATCH 05/10] Refactor `IdScriptableObject` to avoid building property descriptors. --- .../javascript/IdScriptableObject.java | 60 +++++++++++++++---- .../mozilla/javascript/ScriptableObject.java | 45 -------------- 2 files changed, 47 insertions(+), 58 deletions(-) diff --git a/rhino/src/main/java/org/mozilla/javascript/IdScriptableObject.java b/rhino/src/main/java/org/mozilla/javascript/IdScriptableObject.java index b21484dda3..651ddd7be2 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)) { @@ -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; diff --git a/rhino/src/main/java/org/mozilla/javascript/ScriptableObject.java b/rhino/src/main/java/org/mozilla/javascript/ScriptableObject.java index b91ea7654e..931ca8b0ce 100644 --- a/rhino/src/main/java/org/mozilla/javascript/ScriptableObject.java +++ b/rhino/src/main/java/org/mozilla/javascript/ScriptableObject.java @@ -1771,51 +1771,6 @@ protected void checkPropertyDefinition(ScriptableObject desc) { } } - protected void checkPropertyChange(Object id, ScriptableObject current, ScriptableObject desc) { - if (current == null) { // new property - if (!isExtensible()) throw ScriptRuntime.typeErrorById("msg.not.extensible"); - } else { - if (isFalse(current.get("configurable", current))) { - if (isTrue(getProperty(desc, "configurable"))) - throw ScriptRuntime.typeErrorById("msg.change.configurable.false.to.true", id); - if (isTrue(current.get("enumerable", current)) - != 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))) { - 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))) - throw ScriptRuntime.typeErrorById( - "msg.change.value.with.writable.false", id); - } - } else if (isAccessor && isAccessorDescriptor(current)) { - if (!sameValue(getProperty(desc, "set"), current.get("set", current))) - throw ScriptRuntime.typeErrorById( - "msg.change.setter.with.configurable.false", id); - - if (!sameValue(getProperty(desc, "get"), current.get("get", current))) - 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); - } - } - } - } - protected void checkPropertyChangeForSlot(Object id, Slot current, ScriptableObject desc) { if (current == null) { // new property if (!isExtensible()) throw ScriptRuntime.typeErrorById("msg.not.extensible"); From c14ec1c2953f4ca808f4cbbff9f00fe14bb50aab Mon Sep 17 00:00:00 2001 From: "duncan.macgregor" Date: Tue, 3 Dec 2024 12:36:09 +0000 Subject: [PATCH 06/10] Minor style change round if...else cases with `return`. --- .../java/org/mozilla/javascript/AccessorSlot.java | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/rhino/src/main/java/org/mozilla/javascript/AccessorSlot.java b/rhino/src/main/java/org/mozilla/javascript/AccessorSlot.java index 690c4245c3..5cbb465904 100644 --- a/rhino/src/main/java/org/mozilla/javascript/AccessorSlot.java +++ b/rhino/src/main/java/org/mozilla/javascript/AccessorSlot.java @@ -117,22 +117,22 @@ Function getGetterFunction(String name, Scriptable scope) { boolean isSameGetterFunction(Object function) { if (function == Scriptable.NOT_FOUND) { return true; - } else if (getter == null) { + } + if (getter == null) { return ScriptRuntime.shallowEq(Undefined.instance, function); - } else { - return getter.isSameGetterFunction(function); } + return getter.isSameGetterFunction(function); } @Override boolean isSameSetterFunction(Object function) { if (function == Scriptable.NOT_FOUND) { return true; - } else if (setter == null) { + } + if (setter == null) { return ScriptRuntime.shallowEq(Undefined.instance, function); - } else { - return setter.isSameSetterFunction(function); } + return setter.isSameSetterFunction(function); } interface Getter { From ad7d0a6cc00543dd76f8b2024f76893b0ebbc670 Mon Sep 17 00:00:00 2001 From: "duncan.macgregor" Date: Tue, 3 Dec 2024 12:45:09 +0000 Subject: [PATCH 07/10] Change `getBuiltInDescriptor` to `getBuiltInDataDescriptor` for clarity. --- .../org/mozilla/javascript/IdScriptableObject.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/rhino/src/main/java/org/mozilla/javascript/IdScriptableObject.java b/rhino/src/main/java/org/mozilla/javascript/IdScriptableObject.java index 651ddd7be2..56098d8563 100644 --- a/rhino/src/main/java/org/mozilla/javascript/IdScriptableObject.java +++ b/rhino/src/main/java/org/mozilla/javascript/IdScriptableObject.java @@ -922,15 +922,15 @@ 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; @@ -954,7 +954,7 @@ private Slot queryOrFakeSlot(Context cx, Object id) { return slot; } - private ScriptableObject getBuiltInDescriptor(String name) { + private ScriptableObject getBuiltInDataDescriptor(String name) { Scriptable scope = getParentScope(); if (scope == null) { scope = this; @@ -990,7 +990,7 @@ private Slot getBuiltInSlot(String name) { return null; } - private ScriptableObject getBuiltInDescriptor(Symbol key) { + private ScriptableObject getBuiltInDataDescriptor(Symbol key) { Scriptable scope = getParentScope(); if (scope == null) { scope = this; From e20ecb059baac35e24541fb78daf5fd0aa31d312 Mon Sep 17 00:00:00 2001 From: nabacg Date: Wed, 4 Dec 2024 13:15:48 +0000 Subject: [PATCH 08/10] moving all getProperty out of slotMap.compute lambda inside ScriptableObject::defineOwnProperty method --- .../javascript/IdScriptableObject.java | 14 ++++++-- .../mozilla/javascript/ScriptableObject.java | 35 +++++++++++++------ 2 files changed, 37 insertions(+), 12 deletions(-) diff --git a/rhino/src/main/java/org/mozilla/javascript/IdScriptableObject.java b/rhino/src/main/java/org/mozilla/javascript/IdScriptableObject.java index 56098d8563..6de2f80532 100644 --- a/rhino/src/main/java/org/mozilla/javascript/IdScriptableObject.java +++ b/rhino/src/main/java/org/mozilla/javascript/IdScriptableObject.java @@ -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; } @@ -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 diff --git a/rhino/src/main/java/org/mozilla/javascript/ScriptableObject.java b/rhino/src/main/java/org/mozilla/javascript/ScriptableObject.java index 931ca8b0ce..c8dcabd86d 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( @@ -1634,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; @@ -1649,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); } @@ -1663,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) { @@ -1851,8 +1868,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) @@ -1860,7 +1877,6 @@ protected int applyDescriptorToAttributeBitset(int attributes, ScriptableObject : attributes | DONTENUM; } - Object writable = getProperty(desc, "writable"); if (writable != NOT_FOUND) { attributes = ScriptRuntime.toBoolean(writable) @@ -1868,7 +1884,6 @@ protected int applyDescriptorToAttributeBitset(int attributes, ScriptableObject : attributes | READONLY; } - Object configurable = getProperty(desc, "configurable"); if (configurable != NOT_FOUND) { attributes = ScriptRuntime.toBoolean(configurable) From d350e81e5fb71ad83b5f19564d8aaa8e5e143b9e Mon Sep 17 00:00:00 2001 From: nabacg Date: Thu, 5 Dec 2024 16:45:54 +0000 Subject: [PATCH 09/10] refactoring LambdaAccessorSlot and corresponding ScriptableObject::defineProperty to avoid deadlocks, that happen when current instance of ScriptableObject is used as scope inside slotMap.compute lambda. --- .../javascript/LambdaAccessorSlot.java | 20 ++++++- .../mozilla/javascript/ScriptableObject.java | 59 ++++++++++--------- 2 files changed, 49 insertions(+), 30 deletions(-) diff --git a/rhino/src/main/java/org/mozilla/javascript/LambdaAccessorSlot.java b/rhino/src/main/java/org/mozilla/javascript/LambdaAccessorSlot.java index 18538473c5..fefbbf28e2 100644 --- a/rhino/src/main/java/org/mozilla/javascript/LambdaAccessorSlot.java +++ b/rhino/src/main/java/org/mozilla/javascript/LambdaAccessorSlot.java @@ -40,7 +40,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; @@ -126,4 +136,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/ScriptableObject.java b/rhino/src/main/java/org/mozilla/javascript/ScriptableObject.java index c8dcabd86d..5a61bb317d 100644 --- a/rhino/src/main/java/org/mozilla/javascript/ScriptableObject.java +++ b/rhino/src/main/java/org/mozilla/javascript/ScriptableObject.java @@ -1744,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); @@ -2775,22 +2792,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); - - checkPropertyChangeForSlot(name, existing, newDesc); - return newSlot; - } - private void writeObject(ObjectOutputStream out) throws IOException { out.defaultWriteObject(); final long stamp = slotMap.readLock(); From 474c3ba6ad89d2067dba48ec98d0b98b2e959a3d Mon Sep 17 00:00:00 2001 From: nabacg Date: Mon, 9 Dec 2024 11:33:54 +0000 Subject: [PATCH 10/10] adding redefine existing property test to LambdaAccessorSlotTests to cover that code path --- .../tests/LambdaAccessorSlotTest.java | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) 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(