618572 - Assertion failure: *userbuf.ptr == c, at ../jsscan.cpp:349

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

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.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: patch (against 58545:49f6b73ae373)

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 2

[Brendan Eich [:brendan]]

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

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: patch v2 (against 58571:54b2d6f9948c)

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 4

[Brendan Eich [:brendan]]

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

Comment 5

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/tracemonkey/rev/2892db39b022

Comment 6

[Nicholas Nethercote [inactive]]

Backed out due to orange, sigh:

http://hg.mozilla.org/tracemonkey/rev/c86a79eb18b9

Comment 7

[Nicholas Nethercote [inactive]]

The first patch caused problems because it didn't check whether peekUnicodeEscape() succeeded.  Paging bug 605349!

http://hg.mozilla.org/tracemonkey/rev/dab87d48d3dd

Comment 8

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/mozilla-central/rev/dab87d48d3dd