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)
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)
682 bytes,
patch
|
brendan
:
review+
beltzner
:
approval1.8.1+
|
Details | Diff | Splinter Review |
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•18 years ago
|
Assignee: general → mrbkap
Priority: -- → P1
Target Milestone: --- → mozilla1.8.1
Assignee | ||
Updated•18 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•18 years ago
|
||
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)
Comment 2•18 years ago
|
||
(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
Comment 3•18 years ago
|
||
(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
Comment 4•18 years ago
|
||
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•18 years ago
|
||
> Jesse, are you generating unterminated comments in other contexts?
I am now ;)
Assignee | ||
Comment 6•18 years ago
|
||
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 7•18 years ago
|
||
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•18 years ago
|
Whiteboard: [land on trunk]
Comment 8•18 years ago
|
||
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+
Comment 9•18 years ago
|
||
Fixed on trunk and 1.8 branch. /be
Comment 10•18 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•18 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
Updated•13 years ago
|
Crash Signature: [@ Variables]
You need to log in
before you can comment on or make changes to this bug.
Description
•