1049884 - jittests.py assumes that %r produces a valid JS string literal, but it doesn't always

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

https://hg.mozilla.org/mozilla-central/file/50127aca27dc/js/src/tests/lib/jittests.py#l185

I'm getting SyntaxErrors because of u"..."

Comment 1

[Tom Tromey :tromey]

This worked for me:

diff -r d74f59b2ee85 js/src/tests/lib/jittests.py
--- a/js/src/tests/lib/jittests.py	Mon Sep 08 15:07:03 2014 -0700
+++ b/js/src/tests/lib/jittests.py	Mon Sep 08 17:05:40 2014 -0700
@@ -185,7 +185,9 @@
         fmt = 'const platform=%r; const libdir=%r; const scriptdir=%r'
         if remote_prefix:
             fmt = 'const platform="%s"; const libdir="%s"; const scriptdir="%s"'
-        expr = fmt % (sys.platform, libdir, scriptdir_var)
+        # Use |str| to make sure that unicode objects are printed as
+        # strings.
+        expr = fmt % (str(sys.platform), str(libdir), str(scriptdir_var))
 
         # We may have specified '-a' or '-d' twice: once via --jitflags, once
         # via the "|jit-test|" line.  Remove dups because they are toggles.


But it might be cleaner to always use %s and use explicit single quotes in the
to-be-formerly-%r case.

Comment 2

[Tom Tromey :tromey]

To reproduce, make a file listing a test to run:

pokyo. cat /tmp/w
debug/Debugger-ctor-01.js

Then re-run just that test:

pokyo. ./jit-test/jit_test.py -r /tmp/w ~/firefox-hg/js-build/dist/bin/js -o
-e:1:111 SyntaxError: missing ; before statement:
-e:1:111 /firefox-hg/firefox/js/src/jit-test/lib/'; const scriptdir=u'/home/tromey/firefox-hg/firefox/js/src/jit-test/tests/debug
-e:1:111 ............................................................^
Exit code: 3

Comment 3

[Tom Tromey :tromey]

Attachment: always use %s to avoid u''-style output with %r

Comment 4

[Jim Blandy :jimb]

Comment on attachment 8488677 [details] [diff] [review]
always use %s to avoid u''-style output with %r

Review of attachment 8488677 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/tests/lib/jittests.py
@@ +181,4 @@
>          if remote_prefix:
>              fmt = 'const platform="%s"; const libdir="%s"; const scriptdir="%s"'
> +        else:
> +            fmt = "const platform='%s'; const libdir='%s'; const scriptdir='%s'"

One thing that %r takes care of is escaping embedded backslashes and quote characters. Do we know for sure that those don't arise in the paths we see?

The bug here is that we're not taking generating JavaScript code seriously. I think if we're going to really fix this, we need a 'js_string_literal' function that takes a string, and returns JavaScript source text for a string literal whose characters are identical with the given string. I guess it has to take a flag saying which quote character to use.

Comment 5

[Tom Tromey :tromey]

> One thing that %r takes care of is escaping embedded backslashes and quote
> characters. Do we know for sure that those don't arise in the paths we see?

I don't know, but the comment gives me pause:

        # Platforms where subprocess immediately invokes exec do not care
        # whether we use double or single quotes. On windows and when using
        # a remote device, however, we have to be careful to use the quote
        # style that is the opposite of what the exec wrapper uses.

If the code has to pick some different quoting regime for remote tests, then
it sounds like some second level of not-fully-general quoting and/or dequoting
is happening.  I don't know how to find out what this might be...

Meanwhile it's easy to do the right thing for the js layer of quoting, patch to follow.

Comment 6

[Tom Tromey :tromey]

Attachment: implement javascript string quoting for command line arguments

Comment 7

[Tom Tromey :tromey]

Oh crud, I forgot to remove the "import re" I added while hacking around.
Sorry about that, will remove here and resend momentarily.

Comment 8

[Tom Tromey :tromey]

Attachment: implement javascript string quoting for command line arguments

Comment 9

[Jim Blandy :jimb]

Comment on attachment 8489519 [details] [diff] [review]
implement javascript string quoting for command line arguments

Review of attachment 8489519 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks very much!

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/138c8ff89fe5

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/138c8ff89fe5