Change to prevent creation of property descriptors when not required.

mozilla · Dec 2, 2024 · 8681b19 · 8681b19
1 parent 0ba1f30
commit 8681b19
Show file tree

Hide file tree

Showing 4 changed files with 119 additions and 16 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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;
+    }
 }