From 1e4a0003e1c79dc14c1cf2ce0a0a2b05217d4f92 Mon Sep 17 00:00:00 2001 From: Paul Bakker Date: Mon, 7 Jun 2021 11:52:33 +0200 Subject: [PATCH] Code Review comments, fix for Arrays, better docs --- .../AbstractEcmaObjectOperations.java | 57 ++++++++++++------- src/org/mozilla/javascript/NativeArray.java | 20 +++++-- src/org/mozilla/javascript/NativeObject.java | 8 +-- 3 files changed, 54 insertions(+), 31 deletions(-) diff --git a/src/org/mozilla/javascript/AbstractEcmaObjectOperations.java b/src/org/mozilla/javascript/AbstractEcmaObjectOperations.java index 5ec1a76193..0e7261481e 100644 --- a/src/org/mozilla/javascript/AbstractEcmaObjectOperations.java +++ b/src/org/mozilla/javascript/AbstractEcmaObjectOperations.java @@ -5,6 +5,18 @@ * * @see Abstract * Operations - Operations on Objects + *

Notes + *

*/ class AbstractEcmaObjectOperations { static enum INTEGRITY_LEVEL { @@ -15,40 +27,41 @@ static enum INTEGRITY_LEVEL { /** * Implementation of Abstract Object operation testIntegrityLevel as defined by EcmaScript * + * @param cx * @param o * @param level * @return boolean * @see TestIntegrityLevel */ - static boolean testIntegrityLevel(Object o, INTEGRITY_LEVEL level) { + static boolean testIntegrityLevel(Context cx, Object o, INTEGRITY_LEVEL level) { ScriptableObject obj = ScriptableObject.ensureScriptableObject(o); - Context cx = Context.getCurrentContext(); - if (obj.isExtensible()) return Boolean.FALSE; + if (obj.isExtensible()) return false; for (Object name : obj.getIds(true, true)) { ScriptableObject desc = obj.getOwnPropertyDescriptor(cx, name); - if (Boolean.TRUE.equals(desc.get("configurable"))) return Boolean.FALSE; + if (Boolean.TRUE.equals(desc.get("configurable"))) return false; if (level == INTEGRITY_LEVEL.FROZEN && desc.isDataDescriptor(desc) - && Boolean.TRUE.equals(desc.get("writable"))) return Boolean.FALSE; + && Boolean.TRUE.equals(desc.get("writable"))) return false; } - return Boolean.TRUE; + return true; } /** * Implementation of Abstract Object operation setIntegrityLevel as defined by EcmaScript * + * @param cx * @param o * @param level * @return boolean * @see SetIntegrityLevel */ - static boolean setIntegrityLevel(Object o, INTEGRITY_LEVEL level) { + static boolean setIntegrityLevel(Context cx, Object o, INTEGRITY_LEVEL level) { /* 1. Assert: Type(O) is Object. 2. Assert: level is either sealed or frozen. @@ -71,41 +84,43 @@ static boolean setIntegrityLevel(Object o, INTEGRITY_LEVEL level) { 8. Return true. NOTES - - Step 3 calls for the .preventExtensions() before updating the propertyDescriptors, - In Rhino however .preventExtensions() never returns false - and calling it before will block updating the propertyDescriptors afterwards - While steps 6.a.i and 7.b.ii.3 call for the Abstract DefinePropertyOrThrow operation, - the conditions under which a throw would occur aren't applicable when freezing or sealing an object + the conditions under which a throw would occur aren't applicable when freezing or sealing an object, + see https://262.ecma-international.org/11.0/#sec-validateandapplypropertydescriptor: + 1. n/a + 2. current cannot be undefined, because the logic operates only on existing properties + 3. n/a + 4. this code doesn't ever set configurable to true or modifies the enumerable property + 5. n/a + 6. as current and desc start out the same and the writable property is set only after checking if isDataDescriptor == true, this condition cannot occur + 7. both conditions under which false would be returned cannot occur here + 8. both conditions under which false would be returned cannot occur here */ ScriptableObject obj = ScriptableObject.ensureScriptableObject(o); - Context cx = Context.getCurrentContext(); + + // TODO check .preventExtensions() return value once implemented and act accordingly to spec + obj.preventExtensions(); for (Object key : obj.getIds(true, true)) { ScriptableObject desc = obj.getOwnPropertyDescriptor(cx, key); if (level == INTEGRITY_LEVEL.SEALED) { if (Boolean.TRUE.equals(desc.get("configurable"))) { - desc.put("configurable", desc, Boolean.FALSE); + desc.put("configurable", desc, false); obj.defineOwnProperty(cx, key, desc, false); } } else { if (obj.isDataDescriptor(desc) && Boolean.TRUE.equals(desc.get("writable"))) { - desc.put("writable", desc, Boolean.FALSE); + desc.put("writable", desc, false); } if (Boolean.TRUE.equals(desc.get("configurable"))) { - desc.put("configurable", desc, Boolean.FALSE); + desc.put("configurable", desc, false); } obj.defineOwnProperty(cx, key, desc, false); } } - obj.preventExtensions(); - - return true; - } - - static boolean definePropertyOrThrow(Object o, Object p, Scriptable desc) { return true; } } diff --git a/src/org/mozilla/javascript/NativeArray.java b/src/org/mozilla/javascript/NativeArray.java index e309b4d261..bd35bcbff6 100644 --- a/src/org/mozilla/javascript/NativeArray.java +++ b/src/org/mozilla/javascript/NativeArray.java @@ -682,21 +682,29 @@ protected ScriptableObject getOwnPropertyDescriptor(Context cx, Object id) { @Override protected void defineOwnProperty( Context cx, Object id, ScriptableObject desc, boolean checkValid) { - if (dense != null) { + long index = toArrayIndex(id); + if (index >= length) { + length = index + 1; + modCount++; + } + + if (index != -1 && dense != null) { Object[] values = dense; dense = null; denseOnly = false; for (int i = 0; i < values.length; i++) { if (values[i] != NOT_FOUND) { + if (!isExtensible()) { + // Force creating a slot, before calling .put(...) on the next line, which + // would otherwise fail on a array on which preventExtensions() has been + // called + setAttributes(i, 0); + } put(i, this, values[i]); } } } - long index = toArrayIndex(id); - if (index >= length) { - length = index + 1; - modCount++; - } + super.defineOwnProperty(cx, id, desc, checkValid); if (id instanceof String && ((String) id).equals("length")) { diff --git a/src/org/mozilla/javascript/NativeObject.java b/src/org/mozilla/javascript/NativeObject.java index 9f7fe79996..4f9b846212 100644 --- a/src/org/mozilla/javascript/NativeObject.java +++ b/src/org/mozilla/javascript/NativeObject.java @@ -574,7 +574,7 @@ public Object execIdCall( } return AbstractEcmaObjectOperations.testIntegrityLevel( - arg, AbstractEcmaObjectOperations.INTEGRITY_LEVEL.SEALED); + cx, arg, AbstractEcmaObjectOperations.INTEGRITY_LEVEL.SEALED); } case ConstructorId_isFrozen: { @@ -585,7 +585,7 @@ public Object execIdCall( } return AbstractEcmaObjectOperations.testIntegrityLevel( - arg, AbstractEcmaObjectOperations.INTEGRITY_LEVEL.FROZEN); + cx, arg, AbstractEcmaObjectOperations.INTEGRITY_LEVEL.FROZEN); } case ConstructorId_seal: { @@ -596,7 +596,7 @@ public Object execIdCall( } AbstractEcmaObjectOperations.setIntegrityLevel( - arg, AbstractEcmaObjectOperations.INTEGRITY_LEVEL.SEALED); + cx, arg, AbstractEcmaObjectOperations.INTEGRITY_LEVEL.SEALED); return arg; } @@ -609,7 +609,7 @@ public Object execIdCall( } AbstractEcmaObjectOperations.setIntegrityLevel( - arg, AbstractEcmaObjectOperations.INTEGRITY_LEVEL.FROZEN); + cx, arg, AbstractEcmaObjectOperations.INTEGRITY_LEVEL.FROZEN); return arg; }