eval("$\\u0346") Crashes like this: js> eval("$\\u0346") Assertion failure: *userbuf.ptr == c, at ../jsscan.cpp:349 Looks like this assertion was introduced by bug 588648.

Here's a simpler reproducer: a\u0346 = 3; With the old (pre-bug 588648) code, ungetChar() would push back the ungotten jschar into the 'unget' buffer. The next getChar() would read from that buffer, thus obtaining the already-decoded Unicode character. With the new code, ungetChar() just moves back one jschar in the original buffer. Trouble is, with a Unicode sequence we actually need to move back by six jschars in the original buffer to get to the start of the logical character. Doing so is a pain, so the patch instead introduces two variants of getUnicodeEscape() that check the decoded character is of the correct kind before advancing along the buffer. In other words, instead of advancing and then retreating if the check fails, we check before advancing. Nb: prior to the regressing patch, we got this error message: a.js:11: SyntaxError: illegal character: a.js:11: a\u0346 = 3; a.js:11: ......^ With the attached patch, we now get this, which seems better: a.js:11: SyntaxError: illegal character: a.js:11: a\u0346 = 3; a.js:11: .^

Comment on attachment 497195 [details] [diff] [review] patch (against 58545:49f6b73ae373) >+ * We have encountered a '\': check for a Unicode escape sequence after it. >+ * Return 'true' and the character code value (by value) if we found a >+ * Unicode escape sequence. Otherwise, return 'false'. In both cases, do not >+ * advance along the buffer. > */ >+bool >+TokenStream::getUnicodeEscape(int *c) Should this be called peek, then? It doesn't advance as get does in the name scheme from long ago. Yah, int -- but cp would be the canonical name, not c. >+bool >+TokenStream::getUnicodeEscapeIdStart(int32 *c) >+{ >+ getUnicodeEscape(c); >+ return (JS_ISIDSTART(*c)) >+ ? (skipChars(5), true) >+ : false; Nit: a ? b : false -> a || b, plus fit it all on one line. >+bool >+TokenStream::getUnicodeEscapeIdent(int32 *c) >+{ >+ getUnicodeEscape(c); >+ return (JS_ISIDENT(*c)) >+ ? (skipChars(5), true) >+ : false; Ditto, and would it be sweeter to make skipChars return true for more functional-programming-style conciseness? >@@ -990,30 +1007,27 @@ TokenStream::getTokenInternal() > } > > /* > * Look for an identifier. > */ > > hadUnicodeEscape = JS_FALSE; Since you're touching enough, please use bool/true/false and not ye olde JSBool/JS_TRUE/JS_FALSE. >- qc = getUnicodeEscape(); Calling it gc hurts my tiny brain. uc? A bit under the weather here, if you pick these nits I'll read again for substance and stamp quickly. /be

New patch. It includes a test, please let me know if there is a better location for the test, I just guessed where it should go. (In reply to comment #2) > > >+bool > >+TokenStream::getUnicodeEscapeIdStart(int32 *c) > >+{ > >+ getUnicodeEscape(c); > >+ return (JS_ISIDSTART(*c)) > >+ ? (skipChars(5), true) > >+ : false; > > Nit: a ? b : false -> a || b, plus fit it all on one line. > > >+bool > >+TokenStream::getUnicodeEscapeIdent(int32 *c) > >+{ > >+ getUnicodeEscape(c); > >+ return (JS_ISIDENT(*c)) > >+ ? (skipChars(5), true) > >+ : false; > > Ditto, and would it be sweeter to make skipChars return true for more > functional-programming-style conciseness? Let's see: getUnicodeEscape(c); return JS_ISIDENT(*c) || skipChars(5); That's too much for my poor monkey brain. I instead changed it to just use an if -- it's imperative code, skipChars() has side-effects, let's not pretend otherwise. > >- qc = getUnicodeEscape(); > > Calling it gc hurts my tiny brain. uc? That variable already existed with that name, my patch doesn't change that.

Comment on attachment 497372 [details] [diff] [review] patch v2 (against 58571:54b2d6f9948c) >+TokenStream::peekUnicodeEscape(int *cpOut) > { > jschar cp[5]; D'oh, I should have seen that coming! Ok, you want -Out instead of -p for out param pointer types -- but then you don't want both here (-pOut). Just cOut, but that's unsightly. How about int *result and call it a day? >+bool >+TokenStream::getUnicodeEscapeIdStart(int32 *cp) >+{ >+ peekUnicodeEscape(cp); >+ if (JS_ISIDSTART(*cp)) { > skipChars(5); >- return c; >+ return true; > } >- return '\\'; >+ return false; Imperative is better, yeah. But this method is misnamed. It should be matchUnicodeEscapeIdStart by the peek/match/get convention in place. >+bool >+TokenStream::getUnicodeEscapeIdent(int32 *cp) >+{ >+ peekUnicodeEscape(cp); >+ if (JS_ISIDENT(*cp)) { >+ skipChars(5); >+ return true; >+ } >+ return false; > } Ditto. r=me with these fixed. Thanks, /be

Backed out due to orange, sigh: http://hg.mozilla.org/tracemonkey/rev/c86a79eb18b9

The first patch caused problems because it didn't check whether peekUnicodeEscape() succeeded. Paging bug 605349! http://hg.mozilla.org/tracemonkey/rev/dab87d48d3dd