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

CLOSED FIXED in mozilla0.9.4

Status

()

P1
normal
CLOSED FIXED
17 years ago
7 years ago

People

(Reporter: WeirdAl, Assigned: brendan)

Tracking

({js1.5})

Trunk
mozilla0.9.4
js1.5
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Reporter)

Description

17 years ago
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

17 years ago
Confirming; reassigning to khanson.
Assignee: rogerl → khanson
OS: Windows 95 → All
Hardware: PC → All

Updated

17 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 2

17 years ago
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

17 years ago
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 -
(Reporter)

Comment 4

17 years ago
What about (heaven forbid) three backslashes?

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

(Reporter)

Comment 5

17 years ago
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

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

 
(Assignee)

Comment 7

17 years ago
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
Keywords: js1.5
(Assignee)

Comment 8

17 years ago
Created attachment 46691 [details] [diff] [review]
proposed fix (plus part of fix for 75688)

Comment 9

17 years ago
r/sr=jband
(Assignee)

Comment 10

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

/be
Status: NEW → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 11

17 years ago
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\"))"
(Assignee)

Comment 12

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

/be
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 13

17 years ago
Regression I would like to fix for 0.9.4.

/be
Assignee: khanson → brendan
Status: REOPENED → NEW
Priority: -- → P1
Target Milestone: --- → mozilla0.9.4
(Reporter)

Comment 14

17 years ago
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.
(Reporter)

Comment 15

17 years ago
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.
Summary: Error.prototype.toSource() does not escape quotes correctly → uneval(), Error.prototype.toSource() does not escape quotes correctly
(Assignee)

Comment 16

17 years ago
Created attachment 47915 [details] [diff] [review]
proposed fix, looking for fast r= and sr= from same guys as last time!
(Assignee)

Comment 17

17 years ago
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 on attachment 47915 [details] [diff] [review]
proposed fix, looking for fast r= and sr= from same guys as last time!

sr=shaver
Attachment #47915 - Flags: superreview+

Comment 19

17 years ago
Preliminary testing indicates that patch 47915 fixes this bug - 

Comment 20

17 years ago
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

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

Fix that and r=jband

(Assignee)

Comment 22

17 years ago
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
(Assignee)

Comment 23

17 years ago
Created attachment 48083 [details] [diff] [review]
Without C++-style one-line comment, please!

Comment 24

17 years ago
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
Attachment #47915 - Flags: approval+

Comment 25

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

r=jband
Attachment #48083 - Flags: review+
(Assignee)

Comment 26

17 years ago
Fixed, thanks (sorry for the flailing).

/be
Status: NEW → RESOLVED
Last Resolved: 17 years ago17 years ago
Resolution: --- → FIXED

Comment 27

17 years ago
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

17 years ago
VERIFIED FIXED. The testcases pass on WinNT, Linux in both the debug
and optimized JS shells. Built from JS source pulled today -
Status: RESOLVED → VERIFIED
(Reporter)

Comment 29

17 years ago
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
Status: VERIFIED → CLOSED

Updated

14 years ago
Flags: testcase+
You need to log in before you can comment on or make changes to this bug.