Code Review comments, fix for Arrays, better docs

src/org/mozilla/javascript/AbstractEcmaObjectOperations.java

@@ -5,6 +5,18 @@
  *
  * @see <a href="https://262.ecma-international.org/11.0/#sec-operations-on-objects">Abstract
  *     Operations - Operations on Objects</a>
+ *     <p>Notes
+ *     <ul>
+ *       <li>all methods are to deviate from the method signature defined in the EcmaScript
+ *           specification, by taking an additional 1st parameter of type Context: (downstream)
+ *           methods may need the Context object to read flags and we want to avoid having to look
+ *           up the current context (for performance reasons)
+ *       <li>all methods that implement an Abstract Operation as defined by EcmaScript are to be
+ *           package-scopes methods, to prevent them from being used directly by 3rd party code,
+ *           which would hamper evolving them over time to adept to newer EcmaScript specifications
+ *       <li>a link to the method specification of the specific (EcmaScript) version implemented
+ *           will be put in the JavaDoc of each method that implements an Abstract Operations
+ *     </ul>
  */
 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 <a
      *     href="https://262.ecma-international.org/11.0/#sec-testintegritylevel">TestIntegrityLevel</a>
      */
-    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 <a
      *     href="https://262.ecma-international.org/11.0/#sec-setintegritylevel">SetIntegrityLevel</a>
      */
-    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;
     }
 }

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")) {

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;
                 }