Last Comment Bug 763313 - Braceless functions have their destructuring-args bytecode duplicated
: Braceless functions have their destructuring-args bytecode duplicated
Status: RESOLVED FIXED
: regression, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86_64 Mac OS X
: -- normal (vote)
: mozilla16
Assigned To: :Benjamin Peterson
:
Mentors:
Depends on:
Blocks: jsfunfuzz 759498
  Show dependency treegraph
 
Reported: 2012-06-10 10:04 PDT by Jesse Ruderman
Modified: 2012-06-12 18:34 PDT (History)
3 users (show)
dzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix (1.42 KB, patch)
2012-06-10 13:47 PDT, :Benjamin Peterson
jorendorff: review+
Details | Diff | Splinter Review

Description Jesse Ruderman 2012-06-10 10:04:23 PDT
js> (function ([]) 1)
(function ([]) let [] = arguments[0];1)

jsfunfuzz noticed this bug because it makes decompilation introduce a syntax error in some contexts:

js> (function () {for (var x in (function ([]) 1)) {}})
(function () {for (var x in (function ([]) let [] = arguments[0];1)) {}})
Comment 1 Jesse Ruderman 2012-06-10 10:04:37 PDT
js> dis(function ([]) 1)   
flags: LAMBDA EXPR_CLOSURE NULL_CLOSURE
loc     op
-----   --
main:
00000:  getarg 0
00003:  dup
00004:  pop
00005:  pop
00006:  getarg 0
00009:  dup
00010:  pop
00011:  pop
00012:  one
00013:  return
00014:  stop

Source notes:
 ofs  line    pc  delta desc     args
---- ---- ----- ------ -------- ------
  0:   14     3 [   3] decl     offset 2
  2:   14     9 [   6] decl     offset 2
Comment 2 Jesse Ruderman 2012-06-10 10:05:14 PDT
Regression from bug 759498.
Comment 3 :Benjamin Peterson 2012-06-10 13:47:12 PDT
Created attachment 631769 [details] [diff] [review]
fix
Comment 4 Jason Orendorff [:jorendorff] 2012-06-11 11:02:42 PDT
Comment on attachment 631769 [details] [diff] [review]
fix

Nice work. I dig the test.

Feel free to rename PNK_SEQ to something else. (If you want to clean this up more I'd be happy to review, but that's totally optional.)

Is PNX_DESTRUCT always set in this case? Can you assert it instead of checking?
Comment 5 :Benjamin Peterson 2012-06-11 11:26:44 PDT
(In reply to Jason Orendorff [:jorendorff] from comment #4)
> Comment on attachment 631769 [details] [diff] [review]
> fix
> 
> Nice work. I dig the test.
> 
> Feel free to rename PNK_SEQ to something else. (If you want to clean this up
> more I'd be happy to review, but that's totally optional.)

What would I rename it to? I think PNK_SEQ is supposed to be a generic sequence of statements. It's also used somewhere in for loops, too.

> 
> Is PNX_DESTRUCT always set in this case? Can you assert it instead of
> checking?

No, PNK_SEQ appears in for loops, too.
Comment 6 David Zbarsky (:dzbarsky) 2012-06-11 22:53:10 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bcd13a8efb3
Comment 7 Matt Brubeck (:mbrubeck) 2012-06-12 18:34:12 PDT
https://hg.mozilla.org/mozilla-central/rev/2bcd13a8efb3

Note You need to log in before you can comment on or make changes to this bug.