[array] Fix prototype chain interaction in sort pre-processing

This CL fixes two bugs. First, when looking for a free spot while
moving elements to the front, the prototype chain was also considered,
even though an object at a specific index might have a hole (free
spot).

Second, when moving an element to the front, we are not allowed to
delete it immediately (to preserve semantics when interacting with
non-extensible objects). Such an element is then a free spot, but
won't be recognised as such. This CL sets that element to undefined
after it was moved, to mark it as a free spot.

R=jgruber@chromium.org

Bug: chromium:897512,v8:8369
Change-Id: I79207215b8b0a3c714f064450d8fe5ca0ea4a096
Reviewed-on: https://chromium-review.googlesource.com/c/1417171
Commit-Queue: Simon Zünd <szuend@chromium.org>
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Cr-Commit-Position: refs/heads/master@{#58878}

src/runtime/runtime-array.cc

@@ -47,7 +47,7 @@ Maybe<uint32_t> FindNextFreePosition(Isolate* isolate,
                                      Handle<JSReceiver> receiver,
                                      uint32_t current_pos) {
   for (uint32_t position = current_pos;; ++position) {
-    Maybe<bool> has_element = JSReceiver::HasElement(receiver, position);
+    Maybe<bool> has_element = JSReceiver::HasOwnProperty(receiver, position);
     MAYBE_RETURN(has_element, Nothing<uint32_t>());
     if (!has_element.FromJust()) return Just(position);
 
@@ -124,9 +124,17 @@ Object RemoveArrayHolesGeneric(Isolate* isolate, Handle<JSReceiver> receiver,
       // If the array contains undefineds, the position at 'key' might later
       // bet set to 'undefined'. If we delete the element now and later set it
       // to undefined, the set operation would throw an exception.
+      // Instead, to mark it up as a free space, we set array[key] to undefined.
+      // As 'key' will be incremented afterward, this undefined value will not
+      // affect 'num_undefined', and the logic afterwards will correctly set
+      // the remaining undefineds or delete the remaining properties.
       RETURN_FAILURE_ON_EXCEPTION(
           isolate, Object::SetElement(isolate, receiver, current_pos, element,
                                       LanguageMode::kStrict));
+      RETURN_FAILURE_ON_EXCEPTION(
+          isolate, Object::SetElement(isolate, receiver, key,
+                                      isolate->factory()->undefined_value(),
+                                      LanguageMode::kStrict));
       ++current_pos;
     }
   }
@@ -154,15 +162,11 @@ Object RemoveArrayHolesGeneric(Isolate* isolate, Handle<JSReceiver> receiver,
     MAYBE_RETURN(delete_result, ReadOnlyRoots(isolate).exception());
   }
 
-  // TODO(jgruber, szuend, chromium:897512): This is a workaround to prevent
-  // returning a number greater than array.length to Array.p.sort, which could
-  // trigger OOB accesses. There is still a correctness bug here though in
-  // how we shift around undefineds and delete elements in the two blocks above.
-  // This needs to be fixed soon.
-  const uint32_t number_of_non_undefined_elements = std::min(limit, result);
+  // The number of non-undefined elements MUST always be smaller then limit.
+  // Violating this may cause OOB reads/writes.
+  CHECK_LE(result, limit);
 
-  return *isolate->factory()->NewNumberFromUint(
-      number_of_non_undefined_elements);
+  return *isolate->factory()->NewNumberFromUint(result);
 }
 
 // Collects all defined (non-hole) and non-undefined (array) elements at the

test/mjsunit/regress/regress-897512.js

@@ -20,5 +20,4 @@ assertEquals(o51.length, 39);
 // Sort triggers the bug.
 o51.sort();
 
-// TODO(chromium:897512): The length should be 39.
-assertEquals(o51.length, 101);
+assertEquals(o51.length, 39);