433279 - Object destructuring shorthand can cause "Assertion failure: pn != tc->parseContext->nodeList"

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[nanto_vi (TOYAMA Nao)]

js> var { sin, PI } = Math; sin(PI / 2);
Assertion failure: pn != tc->parseContext->nodeList, at jsparse.c:249

In a browser, it causes a "too much recursion" error.

This bug doesn't occur if you split line in two in JavaScript shell or introduce a temporary variable.

js> var { sin, PI } = Math;
js> sin(PI / 2);
1

js> var { sin, PI } = Math, halfPI = PI / 2; sin(halfPI);
1

Comment 1

[Jesse Ruderman]

Here's an example that doesn't use Math:

js> var {a} = b; c(d + 1);
Assertion failure: pn != tc->parseContext->nodeList, at jsparse.c:249

This seems like it could affect a lot of the people who try to take advantage of the shorthand introduced in bug 404734 :(  I think I forgot to add this destructuring shorthand to jsfunfuzz :(

Comment 2

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Key JS 1.8 feature busted, not sure we can get a fix that has low-enough risk profile that we'd take it, but this is the first thing I've seen go by recently that I thought was worth putting on drivers' radar. :/

Comment 3

[Jesse Ruderman]

The examples above seem to work fine in opt, but here's one jsfunfuzz found that crashes opt with a null deref [@ Variables]:

({a}) = b; with({}) { for(let y in z) { } }

Comment 4

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

mrbkap found the bug, but I note that he didn't say anything in here; can you help us out, Blake?

If we have a clean patch and we respin 3, it'd be good to take it.  If we don't, we can put it in a quickish 3.0.1, since there are no web-compat issues for new features.

Igor: do you have an ETA for a fix?

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

This is caused by calling NewBinary with left == right. This happens in the object destructuring shorthand case:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/js/src/jsparse.c&rev=3.347&root=/cvsroot&mark=5573,5587#5557

The simplest fix for this bug is to clone the name node that we have in the object destructuring case instead of using the exact same node. I think a better fix is to let the list of destructuring names be either binary or unary nodes and deal with the special case in the emitter, but that seems riskier if we want this for Firefox 3.

Comment 6

[Brendan Eich [:brendan]]

Attachment: fix

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 320776 [details] [diff] [review]
fix

This is the minimal fix. It's zero-risk and does the job. Please help get it in ASAP. Thanks,

/be

Comment 8

[Brendan Eich [:brendan]]

How do you nominate in case there's an RC2?

/be

Comment 9

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #8)
> How do you nominate in case there's an RC2?

[RC2?] in the whiteboard.

Comment 10

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 320776 [details] [diff] [review]
fix

r=shaver

Comment 11

[Jesse Ruderman]

I did some fuzzing with this patch.  I didn't see any new assertions, and this assertion went away.

Comment 12

[Mike Schroepfer]

Ok this is marked with RC2? and Wanted1.9.0.x so it will go in whichever one of those is the next release but won't force an RC2 on it's own.  So I'm removing the nom flag.

Comment 13

[Mike Schroepfer]

Comment on attachment 320776 [details] [diff] [review]
fix

a+ schrep for 3.0.1 or RC2 please land on cvs trunk.

Comment 14

[Brendan Eich [:brendan]]

Fixed on cvs.mozilla.org trunk:

js/src/jsparse.c 3.348

/be

Comment 15

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/js1_8/regress/regress-433279-01.js,v  <--  regress-433279-01.js
initial revision: 1.1

/cvsroot/mozilla/js/tests/js1_8/regress/regress-433279-02.js,v  <--  regress-433279-02.js
initial revision: 1.1
/cvsroot/mozilla/js/tests/js1_8/regress/regress-433279-03.js,v  <--  regress-433279-03.js
initial revision: 1.1

mozilla-central: changeset:   16439:c19194ae08ea