Closed Bug 1049884 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: fitzgen, Assigned: tromey)

Details

Attachments

(2 files, 1 obsolete file)

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.
Assignee: nobody → ttromey
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
Attachment #8488677 - Flags: review?(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.
Attachment #8488677 - Flags: review?(jimb)
> 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.
Oh crud, I forgot to remove the "import re" I added while hacking around.
Sorry about that, will remove here and resend momentarily.
Attachment #8489514 - Attachment is obsolete: true
Attachment #8489514 - Flags: review?(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!
Attachment #8489519 - Flags: review?(jimb) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/138c8ff89fe5
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: