Bug 154385

Summary: Proxy's don't properly handle Symbols as PropertyKeys.
Product: WebKit Reporter: Saam Barati <saam>
Component: JavaScriptCoreAssignee: Saam Barati <saam>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, commit-queue, fpizlo, ggaren, gskachkov, keith_miller, mark.lam, msaboff, oliver, sukolsak, ysuzuki
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
none
patch
mark.lam: review+
patch ysuzuki: review+

Saam Barati
Reported 2016-02-17 23:02:03 PST
...
Attachments
patch (4.94 KB, patch)
2016-02-17 23:14 PST, Saam Barati 		no flags
DetailsFormatted DiffDiff
patch (5.77 KB, patch)
2016-02-17 23:42 PST, Saam Barati 		mark.lam: review+
DetailsFormatted DiffDiff
patch (6.23 KB, patch)
2016-02-18 11:55 PST, Saam Barati 		ysuzuki: review+
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Saam Barati
Comment 1 2016-02-17 23:14:39 PST
Created attachment 271638 [details] patch
Saam Barati
Comment 2 2016-02-17 23:42:12 PST
Created attachment 271639 [details] patch
Mark Lam
Comment 3 2016-02-18 08:50:08 PST
Comment on attachment 271639 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=271639&action=review r=me > Source/JavaScriptCore/runtime/ProxyObject.cpp:30 > +#include "Identifier.h" Because you #include "IdentifierInlines.h", you don't need this. #include "IdentifierInlines.h" implies that you that #include'd "Identifier.h".
Yusuke Suzuki
Comment 4 2016-02-18 08:53:11 PST
Comment on attachment 271639 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=271639&action=review > Source/JavaScriptCore/runtime/ProxyObject.cpp:113 > + arguments.append(identifierToJSValue(vm, Identifier::fromUid(&vm, propertyName.uid()))); We should avoid leaking private symbols (used for JSC runtime) to users (e.g. @arrayIteratorNextIndex). It offers an accessability to the private (unsafe) data structures in JSObject. So if the given properyName is a private symbol, it should not be trapped by Proxies.
Saam Barati
Comment 5 2016-02-18 11:55:32 PST
Created attachment 271678 [details] patch updated with Yusuke's comments.
Yusuke Suzuki
Comment 6 2016-02-18 12:24:28 PST
Comment on attachment 271678 [details] patch Adding test for this would be nice :D (I think ArrayIterator.prototype.next can be used).
Saam Barati
Comment 7 2016-02-18 12:30:26 PST
(In reply to comment #6) > Comment on attachment 271678 [details] > patch > > Adding test for this would be nice :D (I think ArrayIterator.prototype.next > can be used). Will add. Thanks
Saam Barati
Comment 8 2016-02-18 16:28:02 PST
landed in: http://trac.webkit.org/changeset/196785
Note You need to log in before you can comment on or make changes to this bug.