#274 Remove [[Enumerate]] and associated reflective capabilities #976

leirocks · 2016-05-12T17:30:13Z

#274 Remove [[Enumerate]] and associated reflective capabilities

Fixes #274

leirocks · 2016-05-12T17:32:37Z

@Yongqu can you review?

Yongqu · 2016-05-16T15:41:44Z

test/es6/proxyenumremoval.js

+
+var proxy = new Proxy({x:1}, {
+  ownKeys: function() {
+    return ['a','b'];


add a test case with returning symbol?

Yongqu · 2016-05-16T15:46:21Z

BOOL JavascriptProxy::GetEnumerator(BOOL enumNonEnumerable, Var* enumerator, ScriptContext * requestContext, bool preferSnapshotSemantics, bool enumSymbols)

we need to cleanup all the comments, and basically implements EnumerateObjectProperties, going through prototype etc. Please also cleanup the comments. hmm we shouldn't go through the prototype here as the forin enumerator will go through the prototype chain. but we need to enumerate only enumerable properties. #Closed

Refers to: lib/Runtime/Library/JavascriptProxy.cpp:855 in b74d582. [](commit_id = b74d582, deletion_comment = False)

Yongqu · 2016-05-16T15:46:57Z

test/es6/proxyenumremoval.js

+});
+
+var keys=""
+for(var key in proxy){ keys += key;}


test case with proxy on the prototype

Yongqu · 2016-05-19T21:20:34Z

lib/Runtime/Library/JavascriptProxy.cpp

-        *enumerator = IteratorObjectEnumerator::Create(scriptContext, trapResult);
+
+        JavascriptArray* arrTrapResult = JavascriptArray::FromVar(trapResult);        
+        uint32 len = arrTrapResult->GetLength();


check the return is JavascriptArray

checked above: JavascriptOperators::IsArray
do you mean we need to check with JavascriptArray::Is()? #Resolved

hmm interesting. JavascriptOperators::IsArray() is true for proxy over array. you can't cast it to JavascriptArray if the trapResult is a proxy.

In reply to: 63961681 [](ancestors = 63961681)

need a generic method to iterate the array (or array like) elements? do we have such method?

In reply to: 63966947 [](ancestors = 63966947,63961681)

Yongqu · 2016-05-25T21:42:51Z

    // 1. Assert: Either Type(V) is Object or Type(V) is Null.

I think we should just call JavascriptOperators::GetOwnPropertyNames(this, scriptContext) to get the propertyNames.

Refers to: lib/Runtime/Library/JavascriptProxy.cpp:886 in ceae1ba. [](commit_id = ceae1ba, deletion_comment = False)

Yongqu · 2016-05-25T21:43:40Z

lib/Runtime/Library/JavascriptProxy.cpp

+
+        Var arrayIterator = JavascriptOperators::GetIterator(RecyclableObject::FromVar(trapResult), scriptContext);
+        *enumerator = IteratorObjectEnumerator::Create(scriptContext, arrayIterator);
+
        return TRUE;


instead of iterators, it's easier to use enumerator. You can see the code in JavascriptObject::CreateKeysHelper, in similar pattern.

leirocks · 2016-06-06T21:46:13Z

    // 1. Assert: Either Type(V) is Object or Type(V) is Null.

done

In reply to: 221716949 [](ancestors = 221716949)

Refers to: lib/Runtime/Library/JavascriptProxy.cpp:886 in c542fd8. [](commit_id = c542fd8, deletion_comment = False)

Yongqu · 2016-06-06T22:39:49Z

lib/Runtime/Library/JavascriptProxy.cpp

+        // 13.7.5.15EnumerateObjectProperties(O)#
+        //  for (let key of Reflect.ownKeys(obj)) {    
+        Var trapResult = JavascriptOperators::GetOwnPropertyNames(this, scriptContext);
+        Assert(JavascriptArray::Is(trapResult));


I would prefer a check in runtime then just Assert, at least a defense in depth.

Yongqu · 2016-06-06T22:45:35Z

dilijev · 2016-07-19T00:51:18Z

@leirocks any update on this?

…lities

check the property descriptor, only enumerable properties should be shown up

add test case for getPrototypeOf, and non-string values in ownKeys trap returning array

…ests

…ma262#594 for detail

…ctive capabilities Merge pull request #976 from leirocks:proxyenumerator #274 Remove [[Enumerate]] and associated reflective capabilities Fixes #274

msftclas added the cla-already-signed label May 12, 2016

Yongqu reviewed May 16, 2016
View reviewed changes

test/es6/proxyenumremoval.js

});

var keys=""

for(var key in proxy){ keys += key;}

Copy link

Contributor

Yongqu May 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test case with proxy on the prototype

leirocks force-pushed the proxyenumerator branch from 63666bd to d283070 Compare May 16, 2016 21:50

Yongqu reviewed May 19, 2016
View reviewed changes

Yongqu reviewed May 25, 2016
View reviewed changes

leirocks force-pushed the proxyenumerator branch from 4d34e36 to c542fd8 Compare June 6, 2016 21:44

Yongqu reviewed Jun 6, 2016
View reviewed changes

leirocks added 4 commits August 24, 2016 16:03

chakra-core#274 Remove [[Enumerate]] and associated reflective capabi…

0052c5c

…lities

address review comments.

5584598

check the property descriptor, only enumerable properties should be shown up

address review comment

e87184c

add test case for getPrototypeOf, and non-string values in ownKeys trap returning array

fix for the ownKeys trap returning a proxy over array scenario, add t…

3ddd2bb

…ests

leirocks added 5 commits August 24, 2016 16:03

fix test

b812eb9

address review comments

1c74103

fix issue on duplicated ownKeys trap results. see spec change tc39/ec…

bb59c0f

…ma262#594 for detail

address review comment

9ed60d6

incorperate recent enumerator change

48db1aa

leirocks force-pushed the proxyenumerator branch from c542fd8 to 48db1aa Compare August 25, 2016 18:23

leirocks added 2 commits August 25, 2016 15:26

Merge branch 'master' into proxyenumerator

5a373d5

Merge branch 'master' into proxyenumerator

98557fd

chakrabot merged commit 98557fd into chakra-core:master Aug 26, 2016

snyk-bot mentioned this pull request Mar 28, 2021

[Snyk] Security upgrade yargs from 4.8.1 to 10.0.0 saurabharch/ChakraCore#4

Open

This was referenced Aug 21, 2023

[Snyk] Upgrade fs-extra from 9.1.0 to 11.1.1 Woodpile37/ChakraCore#2

Merged

[Snyk] Upgrade fs-extra from 9.1.0 to 11.1.1 Woodpile37/ChakraCore#7

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#274 Remove [[Enumerate]] and associated reflective capabilities #976

#274 Remove [[Enumerate]] and associated reflective capabilities #976

leirocks commented May 12, 2016 •

edited by dilijev

Loading

leirocks commented May 12, 2016

Yongqu May 16, 2016

Yongqu commented May 16, 2016 •

edited

Loading

Yongqu May 16, 2016

Yongqu May 19, 2016

leirocks May 19, 2016 •

edited

Loading

Yongqu May 19, 2016

leirocks May 19, 2016

Yongqu commented May 25, 2016

Yongqu May 25, 2016

leirocks commented Jun 6, 2016

Yongqu Jun 6, 2016

leirocks Jun 6, 2016

Yongqu commented Jun 6, 2016

dilijev commented Jul 19, 2016

#274 Remove [[Enumerate]] and associated reflective capabilities #976

#274 Remove [[Enumerate]] and associated reflective capabilities #976

Conversation

leirocks commented May 12, 2016 • edited by dilijev Loading

leirocks commented May 12, 2016

Choose a reason for hiding this comment

Yongqu commented May 16, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leirocks May 19, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yongqu commented May 25, 2016

Choose a reason for hiding this comment

leirocks commented Jun 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yongqu commented Jun 6, 2016

dilijev commented Jul 19, 2016

leirocks commented May 12, 2016 •

edited by dilijev

Loading

Yongqu commented May 16, 2016 •

edited

Loading

leirocks May 19, 2016 •

edited

Loading