moving all getProperty out of slotMap.compute lambda inside Scriptabl…

…eObject::defineOwnProperty method
mozilla · Dec 4, 2024 · e20ecb0 · e20ecb0
1 parent ad7d0a6
commit e20ecb0
Show file tree

Hide file tree

Showing 2 changed files with 37 additions and 12 deletions.
diff --git 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
@@ -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,26 +1645,32 @@ 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;
                         } else {
                             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,24 +1868,22 @@ 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)
                             ? attributes & ~DONTENUM
                             : attributes | DONTENUM;
         }
 
-        Object writable = getProperty(desc, "writable");
         if (writable != NOT_FOUND) {
             attributes =
                     ScriptRuntime.toBoolean(writable)
                             ? attributes & ~READONLY
                             : attributes | READONLY;
         }
 
-        Object configurable = getProperty(desc, "configurable");
         if (configurable != NOT_FOUND) {
             attributes =
                     ScriptRuntime.toBoolean(configurable)