352372 - More ways to hit the assertion in js_PeekTokenSameLine from CheckGetterOrSetter

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Jesse Ruderman]

These all cause an assertion:

eval("setter/*\n*/;")
eval("setter/*\n*/g")
eval("setter/*\n*/ ;")
eval("setter/*\n*/ g")

Assertion failure: ts->lookahead == 0 || (ts->flags & TSF_ERROR) || ON_CURRENT_LINE(ts, CURRENT_TOKEN(ts).pos) || (tt = ts->tokens[(ts->cursor+ts->lookahead) & NTOKENS_MASK].type, tt == TOK_EOL || tt == TOK_EOF), at jsscan.c:1018

I guess the fixes in bug 352208 weren't enough.

Comment 1

[Brendan Eich [:brendan]]

Attachment: fix

You're right, that was a bogus assertion.  js_PeekTokenSameLine must return TOK_EOL if the stream's lineno is ahead of the current token's ending lineno. The "Same" in the function name means that the token we are peeking at must be on the same line as the current token.

The recent variations that botched the assertion all involved commented newlines.  Since commented newlines are not tokenized, but ts->lineno advances as the commented newline is scanned, we have to return a token type other than the wanted one.  We must not buffer the type in ts->tokens, but we need to return something that can't be on the same line.  TOK_EOL suffices.

/be

Comment 2

[Brendan Eich [:brendan]]

Fixed on trunk.

/be

Comment 3

[Brendan Eich [:brendan]]

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

This is a safe patch to replace a bogus assertion with a test for whether the scanner has moved on to a higher line number from the one in the current token.  This helps Jesse's fuzzer avoid asserting.  It avoids peeking at another line's token when the caller wants only tokens from the current line (this happens when a comment hides a newline).

For the 1.8 branch, it should be safe to simply remove the assertion.  The fact that the scanner is on a successor line to the line containing the current token means that someone already peeked, so the js_PeekToken should not scan further ahead, but simply find the buffered token.  However, js_PeekTokenSameLine will then return as if that token were on the same line as the current token, when it's not.

So I think it's simpler and safer to take the whole patch for 1.8.1.

/be

Comment 4

[Mike Schroepfer]

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

a=schrep for js crash fix.

Comment 5

[Brendan Eich [:brendan]]

Fixed on the 1.8 branch.

/be

Comment 6

[Bob Clary [:bc] (inactive)]

Checking in regress-352372.js;
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-352372.js,v  <--  regress-352372.js
initial revision: 1.1
done

Comment 7

[Bob Clary [:bc] (inactive)]

verified fixed 1.8 20060914 windows/linux 1.9 20060914 windows/mac*/linux