350279 - "Assertion failure: left->pn_type == TOK_RC" [@ CheckDestructuring]

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Jesse Ruderman]

javascript:let [2 for (x in [])] = 4;

Assertion failure: left->pn_type == TOK_RC, at jsparse.c:2121

Comment 1

[Brendan Eich [:brendan]]

Attachment: fix

Comment 2

[Brian Crowder]

Maybe the nearby "goto no_var_name" thing nearby should handle jumping out more like this does?

Comment 3

[Brendan Eich [:brendan]]

(In reply to comment #2)
> Maybe the nearby "goto no_var_name" thing nearby should handle jumping out more
> like this does?

That case shares code at no_var_name with other downward gotos on error.  This new case is unique.  Therefore there is nothing more to share than via label bad.

/be

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

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

>Index: jsparse.c
>     fpvd.table.ops = NULL;
>     lhs = left->pn_head;
>+    if (left->pn_type == TOK_ARRAYCOMP) {
>+        js_ReportCompileErrorNumber(cx, left, JSREPORT_PN | JSREPORT_ERROR,
>+                                    JSMSG_ARRAY_COMP_LEFTSIDE);
>+        goto bad;
>+    }
> ...
> out:
>     if (fpvd.table.ops)
>         JS_DHashTableFinish(&fpvd.table);

I don't really see why we need to goto out: here. return JS_FALSE seems like it would be just as correct and avoid the additional label and double-goto.

r=mrbkap either way.

Comment 5

[Brendan Eich [:brendan]]

Attachment: fix, v2

Good point, we can check for this error right away.  No need to partake of the out: code that other paths through no_var_name: need for cleanup.

/be

Comment 6

[Brendan Eich [:brendan]]

Fixed on trunk.

/be

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 237059 [details] [diff] [review]
fix, v2

This assertion catches a missing error check, which becomes a crash in release builds.  Safe fix, no point in risking anything by not taking it even if we didn't care about the crash instead of an error message.

/be

Comment 8

[Bob Clary [:bc] (inactive)]

Checking in regress-350279.js;
/cvsroot/mozilla/js/tests/js1_7/block/regress-350279.js,v  <--  regress-350279.js
initial revision: 1.1

Comment 9

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 237059 [details] [diff] [review]
fix, v2

a=beltzner on behalf of 181drivers

Comment 10

[Brendan Eich [:brendan]]

Fixed on the 1.8 branch.

/be

Comment 11

[Bob Clary [:bc] (inactive)]

verified fixed 1.9a1_2006090707 windows/mac*/linux

Comment 12

[Bob Clary [:bc] (inactive)]

verified fixed 1.8 1.9 20060909 windows/mac*/linux