[builtins] don't create keys for undefined property descriptors in O.gOPDs

Implements the change proposed at https://github.com/tc39/ecma262/pull/593.

In summary, Object.getOwnPropertyDescriptors can produce results which cause
Object.defineProperties() to throw, by inserting a property with an undefined
descriptor into the result object. This change to the algorithm requires that
the descriptor only be added to the result object if it is not undefined.

BUG=v8:4725
R=littledan@chromium.org, adamk@chromium.org, jwolfe@igalia.com

Review-Url: https://codereview.chromium.org/2118613003
Cr-Commit-Position: refs/heads/master@{#37504}

src/builtins.cc

@@ -2025,7 +2025,6 @@ BUILTIN(ObjectEntries) {
 BUILTIN(ObjectGetOwnPropertyDescriptors) {
   HandleScope scope(isolate);
   Handle<Object> object = args.atOrUndefined(isolate, 1);
-  Handle<Object> undefined = isolate->factory()->undefined_value();
 
   Handle<JSReceiver> receiver;
   ASSIGN_RETURN_FAILURE_ON_EXCEPTION(isolate, receiver,
@@ -2047,9 +2046,8 @@ BUILTIN(ObjectGetOwnPropertyDescriptors) {
         isolate, receiver, key, &descriptor);
     MAYBE_RETURN(did_get_descriptor, isolate->heap()->exception());
 
-    Handle<Object> from_descriptor = did_get_descriptor.FromJust()
-                                         ? descriptor.ToObject(isolate)
-                                         : undefined;
+    if (!did_get_descriptor.FromJust()) continue;
+    Handle<Object> from_descriptor = descriptor.ToObject(isolate);
 
     LookupIterator it = LookupIterator::PropertyOrElement(
         isolate, descriptors, key, descriptors, LookupIterator::OWN);

test/mjsunit/harmony/object-get-own-property-descriptors.js

@@ -195,7 +195,14 @@ function TestDuplicateKeys() {
   });
 
   var result = Object.getOwnPropertyDescriptors(P);
-  assertEquals({ "A": undefined }, result);
+  assertEquals({
+    "A": {
+      "value": "VALUE",
+      "writable": false,
+      "enumerable": false,
+      "configurable": true
+    }
+  }, result);
   assertTrue(result.hasOwnProperty("A"));
   assertEquals([
     "ownKeys()",
@@ -204,3 +211,25 @@ function TestDuplicateKeys() {
   ], log);
 }
 TestDuplicateKeys();
+
+function TestFakeProperty() {
+  var log = [];
+  var P = new Proxy({}, {
+    ownKeys() {
+      log.push(`ownKeys()`);
+      return ["fakeProperty"];
+    },
+    getOwnPropertyDescriptor(target, name) {
+      log.push(`getOwnPropertyDescriptor(${name})`);
+      return;
+    }
+  });
+  var result = Object.getOwnPropertyDescriptors(P);
+  assertEquals({}, result);
+  assertFalse(result.hasOwnProperty("fakeProperty"));
+  assertEquals([
+    "ownKeys()",
+    "getOwnPropertyDescriptor(fakeProperty)"
+  ], log);
+}
+TestFakeProperty();

test/test262/test262.status

@@ -469,6 +469,9 @@
   'annexB/built-ins/Date/prototype/setYear/time-clip': [FAIL],
   'annexB/built-ins/Date/prototype/setYear/year-number-relative': [FAIL],
 
+  # Fixed by https://github.com/tc39/test262/pull/662.
+  'built-ins/Object/getOwnPropertyDescriptors/duplicate-keys': [FAIL],
+
   ######################## NEEDS INVESTIGATION ###########################
 
   # These test failures are specific to the intl402 suite and need investigation