Closed Bug 350415 Opened 18 years ago Closed 18 years ago

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

Categories

(Core :: JavaScript Engine, defect, P1)

PowerPC
macOS
defect

Tracking

()

VERIFIED FIXED
mozilla1.8.1

People

(Reporter: jruderman, Assigned: mrbkap)

Details

(Keywords: crash, testcase, verified1.8.1, Whiteboard: [land on trunk])

Crash Data

Attachments

(1 file, 1 obsolete file)

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: general → mrbkap
Priority: -- → P1
Target Milestone: --- → mozilla1.8.1
Status: NEW → ASSIGNED
Attached patch Minimal patch (obsolete) — Splinter Review
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
> Jesse, are you generating unterminated comments in other contexts?

I am now ;)
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?
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
Closed: 18 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
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+
verified fixed 1.8, 1.9 20060830 windows/mac*/linux also passes in 1.8.0.7
Status: RESOLVED → VERIFIED
Crash Signature: [@ Variables]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: