[runtime] Prevent performing GetOwnPropertyDescriptor on excluded keys

Excluded keys should not be performed with GetOwnPropertyDescriptor on
source object in CopyDataProperties.

The key values fetch in CopyDataProperties might be arbitrary kind. It
may be smi, string, and symbol. Yet the proxy keys collected by
KeyAccumulator are not expected types for numeric keys. Those keys
should be converted to expected types.

Also updates a typo in comments of
BytecodeGenerator::BuildDestructuringObjectAssignment. The elements in
rest_runtime_callargs should be [value, ...excluded_properties].

Refs: https://tc39.es/ecma262/#sec-copydataproperties
Bug: v8:11532
Change-Id: If71bfedf8272ce8405e8566a016fae66b3007dd9
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3060275Reviewed-by: Leszek Swirski <leszeks@chromium.org>
Commit-Queue: Leszek Swirski <leszeks@chromium.org>
Cr-Commit-Position: refs/heads/master@{#76171}

src/interpreter/bytecode-generator.cc

@@ -4197,7 +4197,7 @@ void BytecodeGenerator::BuildDestructuringArrayAssignment(
 // var rest_runtime_callargs = new Array(3);
 // rest_runtime_callargs[0] = value;
 //
-// rest_runtime_callargs[1] = value;
+// rest_runtime_callargs[1] = "y";
 // y = value.y;
 //
 // var temp1 = %ToName(x++);

src/objects/js-objects.cc

@@ -303,6 +303,8 @@ V8_WARN_UNUSED_RESULT Maybe<bool> FastAssign(
           descriptors.PatchValue(map->instance_descriptors(isolate));
         }
       } else {
+        // No element indexes should get here or the exclusion check may
+        // yield false negatives for type mismatch.
         if (excluded_properties != nullptr &&
             HasExcludedProperty(excluded_properties, next_key)) {
           continue;
@@ -381,6 +383,11 @@ Maybe<bool> JSReceiver::SetOrCopyDataProperties(
   // 4. Repeat for each element nextKey of keys in List order,
   for (int i = 0; i < keys->length(); ++i) {
     Handle<Object> next_key(keys->get(i), isolate);
+    if (excluded_properties != nullptr &&
+        HasExcludedProperty(excluded_properties, next_key)) {
+      continue;
+    }
+
     // 4a i. Let desc be ? from.[[GetOwnProperty]](nextKey).
     PropertyDescriptor desc;
     Maybe<bool> found =
@@ -404,11 +411,6 @@ Maybe<bool> JSReceiver::SetOrCopyDataProperties(
                                        Just(ShouldThrow::kThrowOnError)),
             Nothing<bool>());
       } else {
-        if (excluded_properties != nullptr &&
-            HasExcludedProperty(excluded_properties, next_key)) {
-          continue;
-        }
-
         // 4a ii 2. Perform ! CreateDataProperty(target, nextKey, propValue).
         PropertyKey key(isolate, next_key);
         LookupIterator it(isolate, target, key, LookupIterator::OWN);

src/objects/keys.cc

@@ -100,10 +100,6 @@ Handle<FixedArray> KeyAccumulator::GetKeys(GetKeysConversion convert) {
   if (keys_.is_null()) {
     return isolate_->factory()->empty_fixed_array();
   }
-  if (mode_ == KeyCollectionMode::kOwnOnly &&
-      keys_->map() == ReadOnlyRoots(isolate_).fixed_array_map()) {
-    return Handle<FixedArray>::cast(keys_);
-  }
   USE(ContainsOnlyValidKeys);
   Handle<FixedArray> result =
       OrderedHashSet::ConvertToKeysArray(isolate(), keys(), convert);
@@ -224,14 +220,12 @@ Maybe<bool> KeyAccumulator::AddKeysFromJSProxy(Handle<JSProxy> proxy,
     ASSIGN_RETURN_ON_EXCEPTION_VALUE(
         isolate_, keys, FilterProxyKeys(this, proxy, keys, filter_),
         Nothing<bool>());
-    if (mode_ == KeyCollectionMode::kOwnOnly) {
-      // If we collect only the keys from a JSProxy do not sort or deduplicate.
-      keys_ = keys;
-      return Just(true);
-    }
   }
-  RETURN_NOTHING_IF_NOT_SUCCESSFUL(
-      AddKeys(keys, is_for_in_ ? CONVERT_TO_ARRAY_INDEX : DO_NOT_CONVERT));
+  // https://tc39.es/ecma262/#sec-proxy-object-internal-methods-and-internal-slots-ownpropertykeys
+  // As of 10.5.11.9 says, the keys collected from Proxy should not contain
+  // any duplicates. And the order of the keys is preserved by the
+  // OrderedHashTable.
+  RETURN_NOTHING_IF_NOT_SUCCESSFUL(AddKeys(keys, CONVERT_TO_ARRAY_INDEX));
   return Just(true);
 }
 

src/objects/keys.h

@@ -141,10 +141,7 @@ class KeyAccumulator final {
   void set_may_have_elements(bool value) { may_have_elements_ = value; }
 
   Isolate* isolate_;
-  // keys_ is either an Handle<OrderedHashSet> or in the case of own JSProxy
-  // keys a Handle<FixedArray>. The OrderedHashSet is in-place converted to the
-  // result list, a FixedArray containing all collected keys.
-  Handle<FixedArray> keys_;
+  Handle<OrderedHashSet> keys_;
   Handle<Map> first_prototype_map_;
   Handle<JSReceiver> receiver_;
   Handle<JSReceiver> last_non_empty_prototype_;

test/mjsunit/es9/object-rest-basic.js

@@ -89,7 +89,7 @@ var p = new Proxy({}, {
 });
 assertThrows(() => { var { ...y } = p });
 
-var z = { b: 1}
+var z = { b: 1};
 var p = new Proxy(z, {
   ownKeys() { return Object.keys(z); },
   get(_, prop) { return z[prop]; },
@@ -97,9 +97,20 @@ var p = new Proxy(z, {
     return Object.getOwnPropertyDescriptor(z, prop);
   },
 });
-var { ...y } = p ;
+var { ...y } = p;
 assertEquals(z, y);
 
+var z = { 1: 1, 2: 2, 3: 3 };
+var p = new Proxy(z, {
+  ownKeys() { return ['1', '2']; },
+  getOwnPropertyDescriptor(_, prop) {
+    return Object.getOwnPropertyDescriptor(z, prop);
+  },
+});
+var { 1: x, ...y } = p;
+assertEquals(1, x);
+assertEquals({ 2: 2 }, y);
+
 var z = { b: 1}
 var { ...y } =  { ...z} ;
 assertEquals(z, y);

test/test262/test262.status

@@ -546,9 +546,6 @@
   # http://crbug/v8/11531
   'built-ins/RegExp/prototype/flags/get-order': [FAIL],
 
-  # http://crbug/v8/11532
-  'language/expressions/object/dstr/object-rest-proxy-gopd-not-called-on-excluded-keys': [FAIL],
-
   # http://crbug/v8/11533
   'language/statements/class/subclass/default-constructor-spread-override': [FAIL],