463360 - Uneval then eval E4X with { gives error

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Biju]

xml =
<x>
  <y><![CDATA[ { ]]></y>
</x>;

eval(uneval(xml));

gives error

Comment 1

[Jesse Ruderman]

Similarly:

js> uneval(<x>&#x7B;</x>)
<x>{</x>

Comment 2

[Rasmus Jensen]

Attachment: patch that should solve the bug, plus regression test

Comment 3

[Jeff Walden [:Waldo]]

Comment on attachment 440727 [details] [diff] [review]
patch that should solve the bug, plus regression test

Looks good, thanks for the patch and test!

In the future, rather than requesting review "from the wind", as Bugzilla in some circumstances calls it, you should try to request review from a specific person involved in the code in question.  You can find out who such people are through hg annotate of relevant files plus changeset descriptions, which will link to bugs and thereby reviewer email addresses.  Alternately (and for the JS engine this works particularly well, due to how the Mozilla source is partitioned), the Mozilla modules list will point you at a good list of possibilities:

http://www.mozilla.org/about/owners.html#javascript

I'll add this to my queue and push it to the main repository where JS work happens, and it'll filter its way to the main Mozilla repository sometime soon after.

Comment 4

[Jeff Walden [:Waldo]]

Comment on attachment 440727 [details] [diff] [review]
patch that should solve the bug, plus regression test

Actually, a couple comments:

>diff -r 4a9b09ae01a7 js/src/jsxml.cpp

>           case '&':
>             if (!js_AppendLiteral(cb, js_amp_entity_str))
>                 return NULL;
>+            break;
>+          case '{':
>+            /* If EscapeElementValue is called by toSource/uneval
>+             * we also need to escape '{'. See bug#463360.
>+             */
>+            if(toSourceFlag) {
>+                if (!js_AppendLiteral(cb, js_leftcurly_entity_str))
>+                    return NULL;
>+            } else {
>+                if (!cb.append(c))
>+                    return NULL;
>+            }
>             break;
>           default:
>             if (!cb.append(c))
>                 return NULL;
>         }

SpiderMonkey style inserts a space between "if" and "(".  Also, it seems better to share the character-append in the '{' case with the default beneath it.

I'll fix both these in the patch before pushing it.

Comment 5

[Jeff Walden [:Waldo]]

Comment on attachment 440727 [details] [diff] [review]
patch that should solve the bug, plus regression test

>diff -r 4a9b09ae01a7 js/src/jsxml.cpp
>+            /* If EscapeElementValue is called by toSource/uneval
>+             * we also need to escape '{'. See bug#463360.
>+             */

And one more: style for multi-line comments (those which, if typed out, intrude upon the 80th column) is like so:

/*
 * Blah blah blah
 * blah blah blah
 */

...so the first line of such should be "/*" with nothing after it.

Comment 6

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/tracemonkey/rev/350ed77bf399

...and this typo-fix to note patch authorship in the changelog, since I forgot to adjust the author appropriately before pushing:

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

Comment 7

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/350ed77bf399