The default bug view has changed. See this FAQ.

the decompiler should not depend on JS_C_STRINGS_ARE_UTF8

VERIFIED FIXED

Status

()

Core
JavaScript Engine
VERIFIED FIXED
9 years ago
9 years ago

People

(Reporter: Igor Bukanov, Assigned: Igor Bukanov)

Tracking

({verified1.8.1.12})

Trunk
verified1.8.1.12
Points:
---
Bug Flags:
blocking1.8.1.12 -
in-testsuite +
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

9 years ago
Currently the decompiler includes JS_C_STRINGS_ARE_UTF8 define to avoid using \u escapes in string literals when JS_C_STRINGS_ARE_UTF8 is defined. This is wrong since that makes the decompiler output (which is UCS-2 string) depends on the details of encoding schema. Moreover, it also introduces incompatible change as the specs allows arbitrary UCS-2 chars in JS strings including ones representing invalid UTF-16 surrogate pairs. with JS_C_STRINGS_ARE_UTF8 defined such output would lead to errors.

Thus I suggest to remove that escapes avoidance under JS_C_STRINGS_ARE_UTF8 and always use \u escapes for consistency.
(Assignee)

Comment 1

9 years ago
Created attachment 291193 [details] [diff] [review]
v1

The removal of escape avoidance under JS_C_STRINGS_ARE_UTF8 in the decompiler.
Assignee: general → igor
Status: NEW → ASSIGNED
Attachment #291193 - Flags: review?(brendan)
Note that there are some cases in which it's impossible to avoid outputting UTF-16, e.g. an XML element literal whose name contains a non-ASCII character.  I remember finding one other instance where escapes couldn't be used, but I can't remember what it was right now.  I don't know what the solution is here; I'm kind of a fan of switching the stack to use jschar on the principle that decompilation isn't used enough to worry about the excess memory consumption, but there might be an alternative sometimes.
(Assignee)

Comment 3

9 years ago
(In reply to comment #2)
> Note that there are some cases in which it's impossible to avoid outputting
> UTF-16, e.g. an XML element literal whose name contains a non-ASCII character. 

Right, one can not escape it with &# code points. But this is for another bug if SM can not reproduce it in the decompiler output. This bug relates to the output of string literals that can be escaped. 

Since the output of the decompiler gives a canonical representation of string literals, it should not depend on the details of converting jschar sequences into the char arrays. This is what this bug suggests to fix.
Comment on attachment 291193 [details] [diff] [review]
v1

Ugh, I should have caught this during review of the patch for bug 315974.

/be
Attachment #291193 - Flags: review?(brendan)
Attachment #291193 - Flags: review+
Attachment #291193 - Flags: approval1.9+
Whiteboard: [needs landing]
(Assignee)

Comment 5

9 years ago
I checked in the patch from comment 1 to CVS trunk:

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1196760563&maxdate=1196760660&who=igor%mir2.org
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing]
(Assignee)

Comment 6

9 years ago
I suggest to have the fix on 1.8.1 branch so embeddings that use JS_C_STRINGS_ARE_UTF8 would not be hit by it. This is very safe code change as it removes the code inside "if JS_C_STRINGS_ARE_UTF8" and that JS_C_STRINGS_ARE_UTF8 is 0 in browser builds. 

Flags: blocking1.8.1.12?

Updated

9 years ago
Flags: in-testsuite-
Flags: in-litmus-

Updated

9 years ago
Attachment #291193 - Flags: approval1.8.0.15?

Updated

9 years ago
Attachment #291193 - Flags: approval1.8.1.12?
Comment on attachment 291193 [details] [diff] [review]
v1

approved for 1.8.1.12, a=dveditz for release-drivers
Attachment #291193 - Flags: approval1.8.1.12? → approval1.8.1.12+
approvable, but not blocking
Flags: blocking1.8.1.12? → blocking1.8.1.12-
(Assignee)

Comment 9

9 years ago
Created attachment 296597 [details] [diff] [review]
v1 for 1.8.1 branch

This is a trivial merge of the trunk patch.
Attachment #291193 - Attachment is obsolete: true
Attachment #291193 - Flags: approval1.8.0.15?
(Assignee)

Comment 10

9 years ago
Created attachment 296598 [details] [diff] [review]
trunk and branch diff

This is a diff to confirm a triviality of merge: both patches removes the same code.
(Assignee)

Comment 11

9 years ago
Comment on attachment 296597 [details] [diff] [review]
v1 for 1.8.1 branch

Asking for an approval once again as trunk patch required a trivial but still merging by hands.
Attachment #296597 - Flags: approval1.8.1.12?
(Assignee)

Updated

9 years ago
Attachment #291193 - Flags: approval1.8.1.12+
Comment on attachment 296597 [details] [diff] [review]
v1 for 1.8.1 branch

approved for 1.8.1.12, a=dveditz for release-drivers
Attachment #296597 - Flags: approval1.8.1.12? → approval1.8.1.12+
(Assignee)

Comment 13

9 years ago
I checked in the patch from comment 9 to MOZILLA_1_8_BRANCH:

Checking in jsopcode.c;
/cvsroot/mozilla/js/src/jsopcode.c,v  <--  jsopcode.c
new revision: 3.89.2.74; previous revision: 3.89.2.73
done
Keywords: fixed1.8.1.12
I've verified that this is checked into the 1.8 branch. Is there any other verification needed here for this change?
(Assignee)

Comment 15

9 years ago
(In reply to comment #14)
> I've verified that this is checked into the 1.8 branch. Is there any other
> verification needed here for this change?
> 

To verify this one need to compile the shell with JS_C_STRINGS_ARE_UTF8 defined and then check that the following test case does not throw any exceptions:

var expect = uneval(function() {return "\uD800\uD800";});
var f = eval(expect);
var consistency_check = f();
if (consistency_check !== "\uD800\uD800")
    throw "Unexpected result: "+consistency_check;
var actual = uneval(f);
if (actual !== expect)
    throw "Bad decompilation: "+actual;


Here \uD800\uD800 is an invalid surrogate pair which is without the patch should be rejected during the decompilation. 

Comment 16

9 years ago
verified fixed 1.9.0 and for 1.8.1.12 using test in comment 15. before patch, exception is thrown, after patch and with current branch no exception.
Status: RESOLVED → VERIFIED
Keywords: fixed1.8.1.12 → verified1.8.1.12
(Assignee)

Comment 17

9 years ago
(In reply to comment #16)
> verified fixed 1.9.0 and for 1.8.1.12 using test in comment 15. before patch,
> exception is thrown, after patch and with current branch no exception.

Can you add the test case to the test suite?

Comment 18

9 years ago
Will it be meaningful in the general case since we don't generally test with JS_C_STRINGS_ARE_UTF8? Or would the benefit be just having it in the tree and easy to test with for the times we do run tests with JS_C_STRINGS_ARE_UTF8? But, yes, I'll add it.
Flags: in-testsuite- → in-testsuite?
(Assignee)

Comment 19

9 years ago
(In reply to comment #18)
> Will it be meaningful in the general case since we don't generally test with
> JS_C_STRINGS_ARE_UTF8? 

Since the patch for bug 397215 landed UTF8 stuff is dynamic and can be switched on via calling JS_CStringsAreUTF8(). One day the shell may well do it automatically or at least with a switch.

Comment 20

9 years ago
/cvsroot/mozilla/js/tests/js1_5/decompilation/regress-406555.js,v  <--  regress-406555.js
initial revision: 1.1
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.