[PR: 5556] Proxy : ownKeys trap.

Returning duplicates from ownKeys trap will lead to type error.

Author: akroshg

lib/Common/DataStructures/BaseDictionary.h

@@ -250,6 +250,7 @@ namespace JsUtil
             return Insert<Insert_Add>(key, value);
         }
 
+        // Returns -1 if the key is already in the dictionary
         int AddNew(const TKey& key, const TValue& value)
         {
             return Insert<Insert_AddNew>(key, value);

lib/Parser/rterrors.h

@@ -367,6 +367,7 @@ RT_ERROR_MSG(JSERR_RegExpInvalidEscape, 5674, "", "Invalid regular expression: i
 RT_ERROR_MSG(JSERR_RegExpTooManyCapturingGroups, 5675, "", "Regular expression cannot have more than 32,767 capturing groups", kjstRangeError, 0)
 RT_ERROR_MSG(JSERR_ProxyHandlerReturnedFalse, 5676, "Proxy %s handler returned false", "Proxy handler returned false", kjstTypeError, 0)
 RT_ERROR_MSG(JSERR_UnicodeRegExpRangeContainsCharClass, 5677, "%s", "Character classes not allowed in a RegExp class range.", kjstSyntaxError, 0)
+RT_ERROR_MSG(JSERR_DuplicateKeysFromOwnPropertyKeys, 5678, "%s", "Proxy's ownKeys trap returned duplicate keys", kjstTypeError, 0)
 
 //Host errors
 RT_ERROR_MSG(JSERR_HostMaybeMissingPromiseContinuationCallback, 5700, "", "Host may not have set any promise continuation callback. Promises may not be executed.", kjstTypeError, 0)

lib/Runtime/Library/JavascriptProxy.cpp

@@ -2342,10 +2342,6 @@ namespace Js
         }
         RecyclableObject* trapResultArray = RecyclableObject::FromVar(ownKeysResult);
 
-        BOOL isTargetExtensible = targetObj->IsExtensible();
-
-        targetKeys = JavascriptOperators::GetOwnPropertyKeys(targetObj, requestContext);
-
         //15. Assert: targetKeys is a List containing only String and Symbol values.
         //16. Let targetConfigurableKeys be an empty List.
         //17. Let targetNonconfigurableKeys be an empty List.
@@ -2427,6 +2423,7 @@ namespace Js
         Var element;
         PropertyId propertyId;
         const PropertyRecord* propertyRecord = nullptr;
+        BOOL isTargetExtensible = FALSE;
 
         BEGIN_TEMP_ALLOCATOR(tempAllocator, requestContext, _u("Runtime"))
         {
@@ -2465,6 +2462,9 @@ namespace Js
                 break;
             }
 
+            isTargetExtensible = targetObj->IsExtensible();
+            targetKeys = JavascriptOperators::GetOwnPropertyKeys(targetObj, requestContext);
+
             for (uint32 i = 0; i < targetKeys->GetLength(); i++)
             {
                 element = targetKeys->DirectGetItem(i);

lib/Runtime/Library/JavascriptProxy.h

@@ -176,16 +176,14 @@ namespace Js
                 JavascriptConversion::ToPropertyKey(element, scriptContext, &propertyRecord, nullptr);
                 propertyId = propertyRecord->GetPropertyId();
 
-                if (!targetToTrapResultMap.ContainsKey(propertyId))
+                if (propertyId != Constants::NoProperty)
                 {
-                    if (propertyId != Constants::NoProperty)
+                    if (targetToTrapResultMap.AddNew(propertyId, true) == -1)
                     {
-                        targetToTrapResultMap.Add(propertyId, true);
+                        JavascriptError::ThrowTypeError(scriptContext, JSERR_DuplicateKeysFromOwnPropertyKeys);
                     }
                 }
 
-                // We explicitly allow duplicates in the results. A map is sufficient since the spec steps that remove entries
-                // remove ALL of them at the same time.
                 if (fn(propertyRecord))
                 {
                     trapResult->DirectSetItemAt(trapResultIndex++, element);

test/es6/proxybugs.js

@@ -484,6 +484,17 @@ var tests = [
             var obj2 = Reflect.construct(proxy2, [20]);
             assert.areEqual(20, obj2.x);
         }
+    },
+    {
+        name: "Proxy's ownKeys is returning duplicate keys should throw",
+        body() {
+            var proxy = new Proxy({}, {
+                ownKeys: function (t) {
+                    return ["a", "a"];
+                }
+            });
+            assert.throws(()=> { Object.keys(proxy);}, TypeError, "proxy's ownKeys is returning duplicate keys", "Proxy's ownKeys trap returned duplicate keys");
+        }
     }
 ];
 

test/es6/proxyenumremoval.js

@@ -86,18 +86,16 @@ passed &= keys==="a";
 
 // check property descriptor trap
 var keys=""
-var already_non_enmerable = false;
 var getPrototypeOfCalled = 0;
 var proxy = new Proxy({}, {
   ownKeys: function() {
-    return ['a','b','a']; // make first a non-enumerable, and second a enumerable, second a won't show up in for-in
+    return ['a','b']; // make a non-enumerable and b enumerable
   },
   getOwnPropertyDescriptor: function(target, key){
     var enumerable = true;
-    if(key === "a" && !already_non_enmerable)
+    if(key === "a")
     {
       enumerable=false;
-      already_non_enmerable = true;
     }
     return {
       configurable: true,