Closed
Bug 39141
Opened 24 years ago
Closed 24 years ago
Omitting values or putting comments in array Literals cause crash; i.e. [1,,2] or [1,/*comment*/,2]
Categories
(Core :: JavaScript Engine, defect, P3)
Tracking
()
RESOLVED
FIXED
People
(Reporter: eXv, Assigned: rogerl)
References
()
Details
(Keywords: crash, Whiteboard: [nsbeta3+])
Attachments
(1 file)
Don't know quite how I stumbled upon this, but: If you have this: [1, /* comment */, 2] The interpreter crashes. Note the extra comma there. Without the comma, it does not crash. This does not affect Communicator (4.61) I'm using build 2000051120. I have also tested it under: JavaScript-C 1.5 pre-release 1 1999 10 31 And that crashes as well. The following do not crash: [1, /*ccc*/ 2] [2 /* ewrew */, 1] print( ({prop: /* value */ "real value"}).prop); ["one",,,,/**/,2] <-- strangely, does not crash The following do crash: [ ,1,2,3] <-- i guess this should be just a syntax error maybe? But, in 4.61 it's just a null first elem. ["one",/**/,,2] [/* comment */, 1,2,3] The URL provided has the same info, but it has a link that eval()'s the code so you can see whether it crases you.
Comment 2•24 years ago
|
||
Confirming. Reproduced on PC/Linux with build 2000050811. Stack trace from a 4/28 build (clicking on the link in the attachment): #0 js_Interpret () #1 js_Execute () #2 obj_eval () #3 js_Invoke () #4 js_Interpret () #5 js_Invoke () #6 js_Interpret () #7 js_Execute () #8 JS_EvaluateUCScriptForPrincipals () #9 nsJSContext::EvaluateString () #10 nsEvaluateStringProxy::EvaluateString () #11 XPTC_InvokeByIndex () #12 EventHandler ()
Status: UNCONFIRMED → NEW
Ever confirmed: true
Updated•24 years ago
|
Status: NEW → ASSIGNED
Comment 4•24 years ago
|
||
Taking bugs off Roger's plate
Assignee: rogerl → mccabe
Status: ASSIGNED → NEW
Updated•24 years ago
|
Status: NEW → ASSIGNED
Comment 5•24 years ago
|
||
Please see bug #47390. I guess problem is not about embedded comments : null (or omitted) values in array literals always cause crash. Simply try and enjoy : var myArray= ["Laurel",,"and","Hardy"]; A major bug, as this has always been considered as correct syntax.
Comment 7•24 years ago
|
||
Updating summary
Summary: Some Comments in array Literals cause crash [1, /* comment */ ,2] → Omitting values or putting comments in array Literals cause crash; i.e. [1,,2] or [1,/*comment*/,2]
Comment 8•24 years ago
|
||
All these crashing examples involving comments involve omitted values too. Embedded comments without omitted values don't crash. Omitted values without embedded comments always crash.
Comment 9•24 years ago
|
||
Regression from supported JS behavior, nominating nsbeta3.
Keywords: nsbeta3
Assignee | ||
Comment 11•24 years ago
|
||
The code for building the element initializer list gets confused whenever the initial comma token has been unget'd. The current token is comma, but passing &CURRENT_TOKEN(ts) to NewParseNode gets the preceding token since CURRENT_TOKEN doesn't allow for any lookahead tokens. This can be fixed by changing CURRENT_TOKEN to handle lookahead, but that seems like quite a significant change so I'm leaving it to the parser gods to decide... Here's the patch: diff -u -r3.13 jsscan.h --- jsscan.h 2000/07/10 20:23:54 3.13 +++ jsscan.h 2000/08/15 22:34:44 @@ -168,7 +168,7 @@ void *listenerTSData;/* listener data for this TokenStream */ }; -#define CURRENT_TOKEN(ts) ((ts)->tokens[(ts)->cursor]) +#define CURRENT_TOKEN(ts) ((ts)->tokens[(ts->cursor + ts->lookahead) & NTOKENS_MASK]) /* JSTokenStream flags */ #define TSF_ERROR 0x01 /* fatal error while compiling */
Comment 12•24 years ago
|
||
Fix looks good to me. How did CURRENT_TOKEN ever work, given that it was missing '& NTOKENS_MASK' ? Some brendan-nits - You could parenthesize (ts) in the macro, to guard against unintended interactions with the macro arguments. Also, most if JS is 80-column-clean. You could use #define CURRENT_TOKEN(ts) ((ts)->tokens[((ts)->cursor + (ts)->lookahead)\ & NTOKENS_MASK]) instead. r=mccabe, assigning to rogerl to check in fix.
Assignee: mccabe → rogerl
Status: ASSIGNED → NEW
Assignee | ||
Comment 13•24 years ago
|
||
With nits out, checked in.
Status: NEW → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 14•24 years ago
|
||
I backed out the fix because it was breaking the Linux & Sparc builds. Turns out that adding lookahead in CURRENT_TOKEN is not the right change (I think mccabe was on the right track when he asked 'how did this ever work'?). After staring at the source a whole bunch more I convinced myself that the interaction between CURRENT_TOKEN & lookahead is correct. The original problem can be fixed more locally by: diff -u -r3.38 jsparse.c --- jsparse.c 2000/05/15 03:54:50 3.38 +++ jsparse.c 2000/08/18 21:27:07 @@ -2439,8 +2439,11 @@ break; } - if (tt == TOK_COMMA) + if (tt == TOK_COMMA) { pn2 = NewParseNode(cx, &CURRENT_TOKEN(ts), PN_NULLARY); + /* Current token is actually the token behind the comma */ + pn2->pn_type = TOK_COMMA; + } else pn2 = AssignExpr(cx, ts, tc); if (!pn2) Excepting that this seems kind of 'hacky' it does fix the bug. Mike, can you r= this (and even better suggest a less hacky alternative?)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 15•24 years ago
|
||
Comment 16•24 years ago
|
||
I think the above will work. r=? I'd be happy to check it in.
Comment 17•24 years ago
|
||
Fix checked in.
Status: REOPENED → RESOLVED
Closed: 24 years ago → 24 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•