200984 - [FIX]Arabic text in Javascript "unescape" function returns the wrong output

Status: VERIFIED FIXED

(Core :: Internationalization, defect, P1)

Description

[Thamer Mahmoud]

Gecko/20030328

TO REPRODUCE:
1. view testcase

ACTUAL: 
the unescape function produces Latin characters instead of Arabic

EXPECTED: 
only characters in their %xx.. hexadecimal form should be affected? (see IE6)

Comment 1

[Thamer Mahmoud]

Attachment: unescape testcase

Comment 2

[Boris Zbarsky [:bzbarsky]]

Intl -- not arabic specific.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Oops, that had an extra chunk in it...

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 119647 [details] [diff] [review]
Oops, that had an extra chunk in it...

Would you review?  The key here is that ToNewCString does lossy "conversion" to
a CString.... This patch makes the conversion use the proper charset encoder,
does a bit of cleanup, and removes the unnecessary call to Reset() (new
decoders already come blank).

Comment 6

[Boris Zbarsky [:bzbarsky]]

mine.

Comment 7

[Johnny Stenback (:jst)]

Comment on attachment 119647 [details] [diff] [review]
Oops, that had an extra chunk in it...

   // Allocate a buffer of the maximum length
   PRUnichar *dest = (PRUnichar*)nsMemory::Alloc(sizeof(PRUnichar) *
maxLength);
+  NS_ENSURE_TRUE(dest, NS_ERROR_OUT_OF_MEMORY);
+  
...
+  rv = decoder->Convert(encodedData, &unescapedByteCount, dest, &destLen);
+  if (NS_FAILED(rv)) {
     nsMemory::Free(dest);
-    return result;
+    return rv;
   }

   aReturn.Assign(dest, destLen);
   nsMemory::Free(dest);

Could you make dest an nsAutoPtr<PRUnichar *> here? If not, you may want to
flip the code around so that you need only one call to nsMemory::Free(dest)...

sr=jst

Comment 8

[Boris Zbarsky [:bzbarsky]]

nsAutoPtr will call "delete" on the pointer, so that's no good here (unless I
actually do a "new PRUnichar[]" and cast or something like that....

I'll flip the test around; good catch.

Comment 9

[Simon Montagu :smontagu]

Before I review, can you quiet my suspicions that this is a non-problem?
Specifically, is non-ASCII text legal input to unescape(), and does calling
unescape() with such text have defined output?

Comment 10

[Simon Montagu :smontagu]

... and another question (possibly for another bug) : is %DA%D1%C8%ED the
correct escaping of the string in the testcase? Why not %0639%0631%0628%064A? Or
possibly %D8%B9%D8%B1%D8%A8%D9%89?

Comment 11

[Simon Montagu :smontagu]

Comment on attachment 119647 [details] [diff] [review]
Oops, that had an extra chunk in it...

OK, I accept that we need this for compatibility, although we need a new bug to
consider what the correct behaviour is, especially of escape().

Opera, IE, and Konqueror all render the second line of the testcase as:

escaped: %u0639%u0631%u0628%u064A%20%u0639%u0631%u0628%u064A

>+  // To gracefully deal with encoding issues, we have to do the following:

Nit: don't split infinitives

r=smontagu

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 119647 [details] [diff] [review]
Oops, that had an extra chunk in it...

Waldemar, Simon asked me to make sure you were OK with this patch.. could you
take a look, please?

Comment 13

[Simon Montagu :smontagu]

The "new bug" I requested in comment 11 would be a dupe of bug 44272 :-)

Comment 14

[Waldemar Horwat]

I'll take a look at what the patch does.  I'm currently building Mozilla so I
can try it.

Comment 15

[Waldemar Horwat]

Attachment: UTF-16 test case

I'm not familiar with the character set used in the first test case, so I used
a UTF-16 test case.  encodeURI/decodeURI work as expected, but escape and
unescape don't produce visible output with the patch applied.  Hmmmm.....

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 120504 [details]
UTF-16 test case

With this testcase, "escape" produces no output even without my patch...

The problem is the use of flat char* buffers to handle UTF-16 text, of course. 
Things die at the first null, which is the first byte if your endianness is
right.

I suppose I could go ahead and rewrite all that stuff in nsGlobalWindow to not
be broken like that....

Comment 17

[Boris Zbarsky [:bzbarsky]]

ccing some intl people for opinions

Comment 18

[Waldemar Horwat]

Asking the more general question: why do the JavaScript 'escape' and 'unescape'
functions care what the document character set is?  They are always passed
UTF-16 from the JS engine and should send UTF-16 back to it.  'encodeURI' and
'decodeURI' don't care and work properly.

Comment 19

[Boris Zbarsky [:bzbarsky]]

> why do the JavaScript 'escape' and 'unescape' functions care what the document
> character set is?

Because that's the behavior real web sites expect.  In particular, one will
often have a site escape() a string, then on the server unescape it and expect
it to be in the document encoding....  Also, some sites will escape a string
server-side, converting a string in document encoding to %-escapes. When
unescaped, we get a byte sequence that's in the document encoding; to give it
back to the JS engine we have to convert it to UTF-16.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Waldemar, please see comment 19?

Comment 21

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to tip

Comment 22

[Boris Zbarsky [:bzbarsky]]

Checked in.

Comment 23

[Yuying Long]

Verified fixed in 06-17 trunk build on WinXP except the problem in bug 44272.