96284 - uneval(), Error.prototype.toSource() does not escape quotes correctly

Status: CLOSED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Alex Vincent [:WeirdAl]]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Win95; en-US; rv:0.9.3) Gecko/20010801
BuildID:    Mozilla/5.0 (Windows; U; Win95; en-US; rv:0.9.3) Gecko/20010801

The toSource() method of Error.prototype incorrectly handles the quotes for a
literal string.  An attempt to eval() code it outputs clearly will produce an error.

I've been seeing this since Netscape 6.0, although I hadn't recognized it as a bug.

Reproducible: Always
Steps to Reproduce:
1.  Run the following JavaScript code.

try {
   var x = "A".toNumber() // lineNumber == 6
   alert("Hello")
   }
catch (exception) {
   document.write(exception.toSource())
   throw exception
   }


Actual Results:  The page writes:
(new TypeError(""A".toNumber is not a function",
"file:///C:/WIN95/DESKTOP/Book/2013/Lab.htm", 9))

Expected Results:  The page should write:
(new TypeError("\"A\".toNumber is not a function",
"file:///C:/WIN95/DESKTOP/Book/2013/Lab.htm", 9))

Mr. Martin Honnen deserves credit for this one; although my code detected the
bug, I did not recognize it as a bug.  He recognized the faulty output from
Netscape 6.00.  After reviewing his comments, I re-ran the code in Mozilla 0.9.3
milestone build and saw the same results.

Comment 1

[Phil Schwartau]

Confirming; reassigning to khanson.

Comment 2

[Phil Schwartau]

Not sure if satyisfying this request will have any practicality.
Here is the result of eval() on the current toSource output:

js> eval('new TypeError(""A".toNumber is not a function")');
25: SyntaxError: missing ) after argument list:
25: new TypeError(""A".toNumber is not a function"
25: ................^


Presumably this is why the current bug was filed. But even if we add the 
requested backslashes before the double quotes, we get the same error: 

js> eval('new TypeError("\"A\".toNumber is not a function")')
23: SyntaxError: missing ) after argument list:
23: new TypeError(""A".toNumber is not a function")
23: ................^



The only thing that works is ALTERNATION of single and double quotes:

js> eval("new TypeError('\"A\".toNumber is not a function')")
TypeError: "A".toNumber is not a function



The trouble is, the string returned by Error.prototype.toSource
does NOT currently use the single-quote character to quote the
message string inside new Error(). That is, it returns 

                   new TypeError("etc.")
NOT                new TypeError('etc.')



So we can't get the single vs. double quote alternation we need.
Even if we were to change Error.prototype.toSource to employ the
single-quote character, we'd have the analogous problem for it...

Comment 3

[Phil Schwartau]

One other option would be using concatenation to "trick" eval(). 

But before this gets any more complicated, let me cc Brendan on whether 
this bug is valid. This same issue may have come up with other objects.

Note, Object.prototype.toSource() was never standardized by ECMA 
Therefore we cannot consult any ECMA spec on this -

Comment 4

[Alex Vincent [:WeirdAl]]

What about (heaven forbid) three backslashes?

\\\" becomes \" which then on eval() becomes "

Comment 5

[Alex Vincent [:WeirdAl]]

Ugh, this starts to get ugly, because then we have to escape backslashes as 
well as quote marks.  I'm not sure if it's a triple backslash we need or a 
double.

Comment 6

[Phil Schwartau]

Also note Brendan's comments regarding uneval() and toSource() in bug 64111,
"XPConnect vs. Object.prototype.toSource woes",  at 2001-01-09 19:32

 

Comment 7

[Brendan Eich [:brendan]]

This is a bug.  The jsexn.c exn_toSource function fails to make appropriate use
of js_QuoteString.  Patch coming up, as I'm hacking jsexn.c coincidentally, for
bug 75688.

/be

Comment 8

[Brendan Eich [:brendan]]

Attachment: proposed fix (plus part of fix for 75688)

Comment 9

[John Bandhauer]

r/sr=jband

Comment 10

[Brendan Eich [:brendan]]

I'll take a post-checkin r=, as I checked in the fix along with bug 75688's fix.

/be

Comment 11

[Phil Schwartau]

Using JS source pulled today at 9AM. I don't understand my current results.
This is what I'm getting now:

js> obj1 = function(x) {return x+1;};
function (x) {
    return x + 1;
}
js> obj1.toSource()
(function (x) {return x + 1;})


js> obj1 = {color:'red', texture:'smooth', hasOwnProperty:42};
[object Object]
js> obj1.toSource()
({color:"red", texture:"smooth"}) <---(hasOwnProperty missing; see bug 90596)


js> obj1 = new String('Hello');
Hello
js> obj1.toSource()
(new String("Hello"))


js> obj1 = new Error('Hello');
Error: Hello
js> obj1.toSource()
"(new Error(\"Hello\"))"


Notice the extra set of double quotes for the Error object. This means 
that if I do eval('obj2 = obj1.toSource()'), then obj2 ends up being 
a String instead of an Error object. 

With all the other objects, obj2 ends up having the same type as obj1;
furthermore, obj2.toSource() == obj1.toSource(). But with the Error objects, 
this is not working ...

I also don't see why the double quotes around 'Hello' are getting 
escaped for the Error object, but not in the String object:

js> obj1.toSource()
(new String("Hello"))

js> obj1.toSource()
"(new Error(\"Hello\"))"

Comment 12

[Brendan Eich [:brendan]]

Well, that was just the wrong patch, wasn't it?   Sorry.  Reopening and taking.
 This toSource stuff is my mess.

/be

Comment 13

[Brendan Eich [:brendan]]

Regression I would like to fix for 0.9.4.

/be

Comment 14

[Alex Vincent [:WeirdAl]]

Gentlemen, based on P. Schwartau's analysis, I think the summary for this bug 
needs to be changed.  His results (if confirmable -- I don't have access to the 
JS engine directly) indicate the problem is not with escaping quotes, but with 
what toSource outputs in the first place.

Comment 15

[Alex Vincent [:WeirdAl]]

http://bugzilla.mozilla.org/showattachment.cgi?attach_id=46982

This is an XHTML 1.0 Transitional document comparing x.toSource() against
uneval(x).  Apparently, they have the same bug on the original error case above.

Comment 16

[Brendan Eich [:brendan]]

Attachment: proposed fix, looking for fast r= and sr= from same guys as last time!

Comment 17

[Brendan Eich [:brendan]]

Asa, this one (once it is reviewed; it passes tests for me) is good for 0.9.4,
with low risk especially, because the affected code is not reached by Mozilla JS
or C/C++.  IOW, only those who call .toSource as the testcase does will care.
I'd like to avoid shipping a Mozilla milestone with the regression I added the
other week.  Thanks,

/be

Comment 18

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 47915 [details] [diff] [review]
proposed fix, looking for fast r= and sr= from same guys as last time!

sr=shaver

Comment 19

[Phil Schwartau]

Preliminary testing indicates that patch 47915 fixes this bug - 

Comment 20

[Phil Schwartau]

I now see the mistake I made in entry 2001-08-21 17:27 above.
I forgot that to provide the literal character "\", one must
escape it! So when I was testing this string:

js> eval('new TypeError("\"A\".toNumber is not a function")')
23: SyntaxError: missing ) after argument list:
23: new TypeError(""A".toNumber is not a function")
23: ................^

I needed to have extra backslashes, as in:

js> eval('new TypeError("\\"A\\".toNumber is not a function")')
TypeError: "A".toNumber is not a function

This ensures the string inside eval() is the desired one:
'new TypeError("\"A\".toNumber is not a function")'

Comment 21

[John Bandhauer]

!!!C++ comment alert!!!
+    // Magic 8, for the characters in ``(new ())''. */

Fix that and r=jband

Comment 22

[Brendan Eich [:brendan]]

Argh!  I've been infected!  Oh well, JS uses // comments too.  Thanks, going for
approval.  (jband, can you check the has-review box on the patch manager for the
patch I'm about to attach?)

/be

Comment 23

[Brendan Eich [:brendan]]

Attachment: Without C++-style one-line comment, please!

Comment 24

[Asa Dotzler [:asa]]

Comment on attachment 47915 [details] [diff] [review]
proposed fix, looking for fast r= and sr= from same guys as last time!

a=asa on behalf of drivers

Comment 25

[John Bandhauer]

Comment on attachment 48083 [details] [diff] [review]
Without C++-style one-line comment, please!

r=jband

Comment 26

[Brendan Eich [:brendan]]

Fixed, thanks (sorry for the flailing).

/be

Comment 27

[Phil Schwartau]

Testcases added to JS test suite:

        mozilla/js/tests/js1_5/Object/regress-96284-001.js
        mozilla/js/tests/js1_5/Object/regress-96284-002.js

The first one tests toSource(); the second one tests uneval().

Comment 28

[Phil Schwartau]

VERIFIED FIXED. The testcases pass on WinNT, Linux in both the debug
and optimized JS shells. Built from JS source pulled today -

Comment 29

[Alex Vincent [:WeirdAl]]

The Lizard has put its foot down and stomped this bug dead.  Many, many thanks
to Mr. Eich and Mr. Schwartau on this one!  8)x