Closed Bug 418051 Opened 16 years ago Closed 16 years ago

"({x:[]}={x}" causes assertion failure in jsparse.c


(Core :: JavaScript Engine, defect)

Not set





(Reporter: jruderman, Assigned: mrbkap)



(Keywords: assertion, testcase, verified1.9.0.6)


(2 files, 5 obsolete files)

js> eval("({x:[]}={x}")
Assertion failure: (pnkey)->pn_arity == PN_NULLARY && ((pnkey)->pn_type == TOK_NUMBER || (pnkey)->pn_type == TOK_STRING || (pnkey)->pn_type == TOK_NAME), at jsparse.c:1974

Opt doesn't blow up too badly.
Asking for dot-release scheduling; if nothing else, masks fuzzing of things like 433279, which would have been nice to catch before RC1 :/
Flags: wanted1.9.0.x?
It doesn't really mask that bug.  I can avoid this bug most of the time with a regexp and still have the fuzzer hit bug 433279.
Brian, who can own this?
Flags: wanted1.9.0.x? → wanted1.9.0.x+
The usual suspects would be Brendan, mrbkap, or perhaps Igor.  I'd look at it if I had time, but I'm largely occupied at the moment, and I've never spent much time in the parser.
Blake, can you take a look at this for
Assignee: general → mrbkap
Attached patch Proposed fix (obsolete) — Splinter Review
Brendan pointed me on the right track here. Without destructuring assignment, we catch this in the TOK_RC case in the emitter.
Attachment #334589 - Flags: review?(brendan)
Comment on attachment 334589 [details] [diff] [review]
Proposed fix

Yay! Let's get this in before the freeze tonight. Thanks,

Attachment #334589 - Flags: review?(brendan) → review+
Closed: 16 years ago
Resolution: --- → FIXED
This patch is causing startup crashes:

1998	    if (right->pn_extra & PNX_SHORTHAND) {
(gdb) print right
$1 = (JSParseNode *) 0x0

Backed out:
Resolution: FIXED → ---
Depends on: 451340
Attached patch Better proposed fix (obsolete) — Splinter Review
If I'd bothered reading any more of the rest of CheckDestructuring, I would have noticed that |right| is null checked before ever other use :-/
Attachment #334589 - Attachment is obsolete: true
Attachment #334701 - Flags: review?(brendan)
Comment on attachment 334701 [details] [diff] [review]
Better proposed fix

Oops, yeah -- sorry.

Nit: We traditionally parenthesize bitwise ops against almost all ops (not against assignment).

Attachment #334701 - Flags: review?(brendan) → review+
Pushed to m-c as
Closed: 16 years ago16 years ago
Resolution: --- → FIXED
this caused js1_7/regress/regress-389605.js 

let ([] = function() { }) { try { throw 1; } catch(x) { } }

to throw a syntax error.

Depends on: 453345
/cvsroot/mozilla/js/tests/js1_7/expressions/regress-418051.js,v  <--  regress-418051.js
initial revision: 1.1
Flags: in-testsuite+
Flags: in-litmus-
Attached patch backported patch to 1.9.0.x (obsolete) — Splinter Review
The assert occurs pretty frequently when jsfunfuzz-ing 1.9.0.x. This patch backport fixes the assert.
Attachment #334701 - Attachment is obsolete: true
Attachment #348414 - Flags: review?(mrbkap)
Attachment #348414 - Flags: approval1.9.0.6?
Comment on attachment 348414 [details] [diff] [review]
backported patch to 1.9.0.x

This is missing the followup fix from bug 453345.
Attachment #348414 - Flags: review?(mrbkap)
Attachment #348414 - Flags: review-
Attachment #348414 - Flags: approval1.9.0.6?
Attached patch patch v2 for 1.9.0.x (obsolete) — Splinter Review
Second pass at a backport.

Console output with this patch applied:

$ ./js
js> let ([] = function() { }) { try { throw 1; } catch(x) { } }

$ ./js
js> eval("({x:[]}={x}")
typein:1: SyntaxError: invalid object initializer:
typein:1: ({x:[]}={x}
typein:1: ........^
Attachment #348414 - Attachment is obsolete: true
Attachment #348785 - Flags: review?(mrbkap)
Comment on attachment 348785 [details] [diff] [review]
patch v2 for 1.9.0.x

>+    if (right && right->pn_arity == PN_LIST && 
>+        (right->pn_extra & PNX_SHORTHAND)) {

Nit: SpiderMonkey style for the 1.9 branch wants:

if (right &&
    right->pn_arity == PN_LIST &&
    (right->pn_extra & PNX_SHORTHAND)) {

r=mrbkap with that.
Attachment #348785 - Flags: review?(mrbkap) → review+
Attached patch third passSplinter Review
this should do it. bringing over r+.
Attachment #348785 - Attachment is obsolete: true
Attachment #348893 - Flags: review+
Attachment #348893 - Flags: approval1.9.0.6?
diff between both v2 and v3 1.9.0.x patches just to be (very) safe.
Comment on attachment 348893 [details] [diff] [review]
third pass

Approved for, a=dveditz for release-drivers.
Attachment #348893 - Flags: approval1.9.0.6? → approval1.9.0.6+
Gary, are you going to land this patch?
(In reply to comment #23)
> Gary, are you going to land this patch?

Nope, I don't have checkin privs.
checked in for

Checking in js/src/jsparse.c;
/cvsroot/mozilla/js/src/jsparse.c,v  <--  jsparse.c
new revision: 3.350; previous revision: 3.349
Keywords: fixed1.9.0.6
v 1.9.0, 1.9.1, 1.9.2
You need to log in before you can comment on or make changes to this bug.