Note: There are a few cases of duplicates in user autocompletion which are being worked on.

new Function("let /*") causes "Assertion failure: let || tt == TOK_VAR" [@ Variables]

VERIFIED FIXED in mozilla1.8.1

Status

()

Core
JavaScript Engine
P1
critical
VERIFIED FIXED
11 years ago
6 years ago

People

(Reporter: Jesse Ruderman, Assigned: mrbkap)

Tracking

(Blocks: 1 bug, {crash, testcase, verified1.8.1})

Trunk
mozilla1.8.1
PowerPC
Mac OS X
crash, testcase, verified1.8.1
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [land on trunk], crash signature)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

11 years ago
Steps to reproduce:
  In a debug build,
  javascript:new Function("let /*");

Result:
  Assertion failure: let || tt == TOK_VAR,
  at jsparse.c:3471 [@ Variables]

Expected:
  Throw an error with the message "unterminated comment".

Happens both before and after the fix for bug 350268.
(Assignee)

Updated

11 years ago
Assignee: general → mrbkap
Priority: -- → P1
Target Milestone: --- → mozilla1.8.1
(Assignee)

Updated

11 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 1

11 years ago
Created attachment 235696 [details] [diff] [review]
Minimal patch

This patch lets us avoid doing any work if the current token is an error token. The problem is that js_PeekToken into an error token will leave the current token as the error. This patch avoids changing that behavior. Why does js_UngetToken check if the token stream is in error?
Attachment #235696 - Flags: review?(brendan)
(In reply to comment #1)
> Created an attachment (id=235696) [edit]
> Minimal patch
> 
> This patch lets us avoid doing any work if the current token is an error
> token.

I'd prefer to have the code that peeks check for TSF_ERROR before proceeding, so change the TOK_LET case in Statement, not Variables.

> The problem is that js_PeekToken into an error token will leave the current
> token as the error. This patch avoids changing that behavior. Why does
> js_UngetToken check if the token stream is in error?

So you don't get repeated lexical error reports for a recursive descent parse that tries js_MatchToken at several levels as it unwinds.

Clearly we want errors to cause prompt failure.  No good way to do that without changing a lot of code.  So make the TOK_LET case check for an error after it peeks and does not match TOK_LP.

/be
(In reply to comment #2)
> I'd prefer to have the code that peeks check for TSF_ERROR before proceeding,
> so change the TOK_LET case in Statement, not Variables.

This is so the peek and lexical error test are close to each other.  The attached patch makes it unclear whether other Variables callers peek (they don't, so the lexical error test is wasting cycles in those paths).  So it's anti-modular and ineffecient :-P.

/be
Hmm, maybe the fear of ungetting TOK_ERROR is misplaced.  It tells the parser that the scanner reported a lexical error, so why not unget it?  It should not cause any re-reports.  Try that instead of any jsparse.c patches, prefer it if it cures this bug.

Jesse, are you generating unterminated comments in other contexts?

/be
(Reporter)

Comment 5

11 years ago
> Jesse, are you generating unterminated comments in other contexts?

I am now ;)
(Assignee)

Comment 6

11 years ago
Created attachment 235739 [details] [diff] [review]
Fix jsscan.c instead

This does indeed fix the bug. I don't think that there's any danger of double-reporting errors, given that we can always get at the JSTokenStream, which refuses to double-report errors.
Attachment #235696 - Attachment is obsolete: true
Attachment #235739 - Flags: review?(brendan)
Attachment #235696 - Flags: review?(brendan)
Comment on attachment 235739 [details] [diff] [review]
Fix jsscan.c instead

Great!

/be
Attachment #235739 - Flags: review?(brendan)
Attachment #235739 - Flags: review+
Attachment #235739 - Flags: approval1.8.1?

Updated

11 years ago
Whiteboard: [land on trunk]
Comment on attachment 235739 [details] [diff] [review]
Fix jsscan.c instead

a=beltzner on behalf of 1.8.1drivers
Attachment #235739 - Flags: approval1.8.1? → approval1.8.1+
Fixed on trunk and 1.8 branch.

/be
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED

Comment 10

11 years ago
Checking in regress-350415.js;
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-350415.js,v  <--  regress-350415.js
initial revision: 1.1
Flags: in-testsuite+

Comment 11

11 years ago
verified fixed 1.8, 1.9 20060830 windows/mac*/linux also passes in 1.8.0.7
Status: RESOLVED → VERIFIED
Keywords: fixed1.8.1 → verified1.8.1
Crash Signature: [@ Variables]
You need to log in before you can comment on or make changes to this bug.