Bug 172005 - This crashes object rest parameter
Summary: This crashes object rest parameter
Status: RESOLVED DUPLICATE of bug 172147
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-05-11 18:08 PDT by Saam Barati
Modified: 2017-05-16 10:50 PDT (History)
12 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2017-05-11 18:08:48 PDT
>>> let a = "foo";
undefined
>>> let {[a]:b, ...rest} = {foo:20, baz:40}
Segmentation fault: 11
Comment 1 Radar WebKit Bug Importer 2017-05-11 18:09:16 PDT
<rdar://problem/32147443>
Comment 2 Caio Lima 2017-05-12 10:18:09 PDT
If it's ok to you, I can look this bug tonight.
Comment 3 Mark Lam 2017-05-12 10:23:55 PDT
(In reply to Caio Lima from comment #2)
> If it's ok to you, I can look this bug tonight.

Hi Caio, I'm already looking into this.  But feel free to look and contribute if you like.
Comment 4 Caio Lima 2017-05-12 20:47:52 PDT
(In reply to Mark Lam from comment #3)
> (In reply to Caio Lima from comment #2)
> > If it's ok to you, I can look this bug tonight.
> 
> Hi Caio, I'm already looking into this.  But feel free to look and
> contribute if you like.

I just looked at it and figured out the problem. I changed ObjectPatternNode::bindValue to add target.propertyName into excluded set and this code consider's that always we have a propertyName, which isn't true and I didn't think in this edge case.

FYI, the excludedSet in https://github.com/caiolima/webkit/blob/master/Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp#L4070 is used to collect statically the identifiers that should be excluded from the rest destructuring. We then use this set to create a JSSet in constant pool at link time in https://github.com/caiolima/webkit/blob/master/Source/JavaScriptCore/bytecode/CodeBlock.cpp#L870

This approach isn't valid for this case because the property will just be evaluated in runtime and we can't populate excludedSet in compile time here. Does it make sense?

One solution I have in mind to handle this specific case is to emit code to populate the identifier/value into excludedSet dynamically, like the first patch version of https://bugs.webkit.org/show_bug.cgi?id=167962
Comment 5 Mark Lam 2017-05-16 10:50:13 PDT
The offending patch came from r213697 due to https://bugs.webkit.org/show_bug.cgi?id=167962,

... and was rolled out in r216891: <http://trac.webkit.org/r216891> due to https://bugs.webkit.org/show_bug.cgi?id=172147.

Hence, this specific issue is no more.  Let's fix the rest destructuring implementation in https://bugs.webkit.org/show_bug.cgi?id=167962.  Closing this one as a dupe of 172147.

*** This bug has been marked as a duplicate of bug 172147 ***