225695 - Problem with non-ASCII characters in network error pages

Status: RESOLVED INACTIVE

(Core :: Internationalization, defect)

Description

[u60234]

Mozilla/5.0 (Windows; U; Windows NT 5.1; sv-SE; rv:1.6b) Gecko/20031112
Firebird/0.7+

The error pages that are displayed when a network error has occurred do no
longer show the right non-ASCII characters. This is a regression between the
20031111 and 20031112 Firebird nightlies.


Steps to reproduce:

1. Set the pref "user_pref("browser.xul.error_pages.enabled", true);"
2. In the en-US.jar file, add a non-ASCII character to the
locale/en-US/global/appstrings.properties file. For example, add a "ö" (o with
two dots) by changing the line:

"dnsNotFound=%S could not be found. Please check the name and try again."

to

"dnsNotFound=%S could not be found. Please check the name and tr\u00F6 again."

3. Repack the en-US.jar file.
4. Open the browser and try open a bogus address, for example, www.yyyyyyyy.org.

Actual Results:
Error page tells me that,
"www.yyyyyyyy.org could not be found. Please check the name and trö again."

Expected Results:
See a error page with the message:
"www.yyyyyyyy.org could not be found. Please check the name and trö again."

Comment 1

[Jungshik Shin]

seems to be a regression of my fix for bug 44272. Somewhere, xul handling code
is treating UTF-8 as the charset of the current locale (in this case iso-8859-1).
Possibly related with bug 91190, but not sure.

Comment 2

[Boris Zbarsky [:bzbarsky]]

The escaping is done at
http://lxr.mozilla.org/seamonkey/source/docshell/base/nsDocShell.cpp#2674
(nsDocShell::LoadErrorPage)

The unescaping is done at
http://lxr.mozilla.org/seamonkey/source/docshell/resources/content/netError.js#9
and company.

This is another impedance mismatch between nsEscape and JS escape(), this time
in the opposite direction (nsEscape and JS unescape() doing different things --
nsEscape produces %XX%XX for the UTF-8 encoding of non-ASCII chars, while JS
treats that as being two Unicode chars).

The real solution is that we need a JS-compatible mode for nsEscape/nsUnescape,
I think... Depending on server support for %u escapes, we may want to make that
the default mode.

Comment 3

[Jungshik Shin]

Thanks for pinpointing where the problem lies.

> The real solution is that we need a JS-compatible 
> mode for nsEscape/nsUnescape,

 How about the other way around? I know doing that way is more work, but if we
pretend that Mozilla-specific URIs are comformant to IRIs (see below), we have
to do that way. That is, we have to add unescapeURI (or something) (to DOM to(
use in url-unescaping. This is to resurrect the code I just removed in bug 44272
with a different name....

To fix view-selected-source, we also need escapeURI (or sth. like that). Hmm..
this is a full cycle...

> Depending on server support for %u escapes, we may want 
> to make that the default mode.

No, this is out of question both practically and in terms of the standard
compliance. It'd have been great if URL-escaping had been defined that way (or
URL had been made to be always in UTF-8, IRI) from the very beginning.
Unfortunately, that didn't happen and the best thing we can ask for is to make
all http servers understand url-escaped UTF-8 (IRI). See
http://www.w3.org/2003/Talks/0904-IUC-IRI/

Comment 4

[Boris Zbarsky [:bzbarsky]]

Wait.  So JS escape() does NOT do "url-escaping"? In other words, escaping a
string with JS escape() and constructing a URI out of it is likely to fail when
someone actually tries to send it to a server?

I find it somewhat hard to believe the the escape() in IE has such behavior...

Comment 5

[Boris Zbarsky [:bzbarsky]]

Or do you mean that what escape() produces is correct per IRI spec but not
understood by servers in practice?

Comment 6

[Jungshik Shin]

What JS escape() does is to escape per ECMA 262 (ECMAscript standard edition 3).
However, JS escape() in that standard is DIFFERENT from IRI escaping. In IRI
escaping, all characters have to be represented in UTF-8 and escaped with their
byte values (%HH%HH for characters bet. U+0080 and U+07FF,  %HH%HH%HH for chars
bet. U+0800 and U+0FFFF and so forth). In JS escape(), characters below U+0100
is escaped with '%HH' but those above U+0100 are escaped with %uHHHH (and
presumably, non-BMP characters are escaped with '%uHHHH%uLLLL')  

That's why I wrote 'if ... had been defined ...'. 

We've had enough hard time making servers understand IRI escaping so that it's
all but impossible to make them understand ECMAscript escape() as well (there's
no justification because ECMAscript escape() is different from IRI spec.)

Comment 7

[Boris Zbarsky [:bzbarsky]]

Oh.  So in other words, as I said, using escape() in JS does not produce a
usable URI?  I still find it hard to believe that that's what IE does, but if
it's so it's so... 

In any case, sounds like we just nead IRI-escaping and IRI-unescaping in JS.
Luckily for us, see sections 15.1.3.1 and 15.1.3.3 (decodeURI and encodeURI) of
http://www.ecma-international.org/publications/files/ECMA-ST/Ecma-262.pdf

Oh, and we happen to implement those.  ;)

Note that those methods will in fact just convert the native UTF-16
representation of JS to UTF-8 and escape (or conversely, unescape and do a
UTF8-to-UTF16 conversion).  So they're not quite like what we used to have with
its native encoding stuff... but for chrome, which is all UTF-8 anyway, it's all
the same.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: like this

tested both parts of that patch, even....  (well, testes that a URI of 'ö')
gives the right error page; if someone wants to test the error message that
would be nice too.

We should probably do a sweep of all escape() and unescape() calls in lxr and
change them as needed...

Comment 9

[Jungshik Shin]

I've just made exactly the same patch but you went there first :-) Yes, I agree
that we have to do tree-wide sweep.
Do you want to do it here or just fill two holes (already found) first and do
that later? 


As for IE, I think either most JS script authors don't bother to escape URLs in
their scripts or are more familiar with ECMAscript standard than Mozilla
developers have been so that they use encodeURI(Component). Note that MS IE
turns on 'Always send URLs in UTF-8' by default despite the fact that IRI is not
so well supported. It's one of rare occasions that IE shows the willingness to
support the standard breaking the backward compatibility. Or they may have
already fixed their IIS to support IRI well, in which case using
encodeURI(Component) would just work for IIS served pages. 

Comment 10

[Boris Zbarsky [:bzbarsky]]

Either way for the sweep.  We have a goodly number of consumers of this stuff in
the tree (72 JS callers of escape(), and 25 of unescape()).  Some of these could
continue using escape/unescape, I bet, and some others probably really want to
be using nsITextToSubURI.....  If it looks like the patch will need
non-rubberstamp reviews, then multiple smaller patches are better, of course.

Comment 11

[Jungshik Shin]

In addition to those 97 cases in JS files (which agrees with my tally), there
are one escape and one unescape in xul files. 

You're right that some of them are not trivial replacement. So, as the first
installment of smaller patches, why don't you go ahead with attachment 1355438? 

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 135543 [details] [diff] [review]
like this

Alright, then.	;)

Comment 13

[Jungshik Shin]

Comment on attachment 135543 [details] [diff] [review]
like this

r=jshin

Comment 14

[Jungshik Shin]

Comment on attachment 135543 [details] [diff] [review]
like this

btw, don't forget to fix

toolkit/components/viewsource/content/viewPartialSource.js

for firebird.

Comment 15

[Jungshik Shin]

Attachment: treewide sweep (draft)

Just to give some 'feel' for what has to be done.
Most cases are pretty obvious, but in some cases I'm not sure (especially in
mailnews)
 
It turned out that there are three *xml files with escape/unescape(). 
Is there any other file type than js, xul and xml?

Comment 16

[Boris Zbarsky [:bzbarsky]]

Quick comments from a quick glance:

1)  There is no such thing as "unencodeURIComponent".  You want
    "decodeURIComponent".  Same for "unencodeURI".
2)  contentAreaUtils.js -- escaping is done by nsStandardURL or somesuch;
    decodeURIComponent is correct there.
3)  Did line endings change in pref-manager.js or something?
4)  In irc/xul/content/commands.js, would it be better to concatenate it all
    together and then simply encodeURI?  I'm really not sure which is better...
5)  I'd assume ecmaUnescape would get replaced with decodeURIComponent?

That's about it, though ecmaUnescape could likely be removed altogether....
Unless we have no window object in that code?

Comment 17

[Jungshik Shin]

Attachment: update

Something got wrong with me to use 'unencode' in place of 'decode' :-). I
removed ecmaEscape/ecmaUnescape completely. I fixed the line ending problem in
pref-manager.js (OT: my source tree is exported via NFS/samba from another
computer to my build machine - Linux/Win2k dual boot - and I hadn't had this
problem before, but somehow it's happening now. perhaps because I changed my
line-ending convention in cygwin to Unix from DOS.....)

So far, history search, bookmark search, search side bar, url bar, and mail
search have been verified to work fine with non-ASCII characters. I still hava
to check mail attachment, irc, mailto:	and other things.

Comment 18

[Simon Paquet [:sipaq]]

Comment on attachment 135644 [details] [diff] [review]
update

Boris, rbs can you please review.

Comment 19

[Simon Paquet [:sipaq]]

*** Bug 225874 has been marked as a duplicate of this bug. ***

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 135644 [details] [diff] [review]
update

Not by freeze.	And some of this (eg the venkman changes) needs actual module
owner review.

I will try to review as much of this as I can on Thursday, but I'm not likely
to have time before then.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 135543 [details] [diff] [review]
like this

And this is no good; the view-source change should really use
encodeURIFragment.

Comment 22

[Boris Zbarsky [:bzbarsky]]

I do think that if we can roust up reviews and testing for this quickly this is
worth holding beta for.... :(  If we can't, then we can't.

Comment 23

[Jungshik Shin]

bz, please, note that I did not ask for r/sr because I hadn't tested 
it sufficiently. (it was not much more than a global search and 
replacement.) I was thinking about splitting the patch into smaller 
pieces (mailnews/mail, irc, verkman, navigator, etc) to expedite 
testing and reviewing. 
Anyway, I'd be glad to see it go in 1.6b, but as you
wrote, if we can, we can, but we can't if we can't :-)

Comment 24

[rbs]

This is a sneaky bug due the uncertainties of those numerous places in the tree
with escape/unescape. One cannot tell what is going to happen. I cannot help
feeling unconfident about the product.

Because bug 44272 was made very late in the cycle, it should perhaps be backed
out and be re-checked in at the next cylce, together with the fixups of this
bug. There isn't (and hasn't been) any particular urgency with bug 44272. What
do you think?

Comment 25

[Boris Zbarsky [:bzbarsky]]

> There isn't (and hasn't been) any particular urgency with bug 44272

Actually, it fixed a number of outstanding problems we had on various sites...

Comment 26

[rbs]

These are/were known, and it seems to me that if they have waited that long,
they can wait just another cycle. Compared with the unknown that has been created.

What I am saying is that the fix for bug 44272 is good, but was perhaps poorly
timed.

Comment 27

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

How do you want to move forward for 1.6b and 1.6?  One approach is to back out
bug 44272.  Another is to get this patch in soon so that it gets testing for
beta and we can try to fix problems in time for final.  Which do you prefer?

Comment 28

[Boris Zbarsky [:bzbarsky]]

I don't have cycles to spend on fixing this in the near future, though I could
review (but not till after thanksgiving weekend, at this point)....  If jshin
doesn't have time to fix it either, then we should probably back out.

Comment 29

[Jungshik Shin]

I have a little cycle today so that I want to give it a shot before backing out
(I have to admit that the timing was  not great for bug 44272 fix). Let me try
to split the latest patch into three parts, mailnews part, extension part (irc,
verkman) and the rest and see if we can make it.  The part I'm least sure of is
mailnews. 

I'm adding people working on irc, verkman and mailnews to cc. Some of them are
outside the US so probably they can help this weekend. 

Comment 30

[[e-mail]]

IIRC in msgHdrViewOverlay.js you need to use encodeURIComponent - displayName
eventually becomes a &filename= URI parameter.

Comment 31

[[e-mail]]

>@@ -1274,15 +1274,15 @@ function ComposeStartup(recycled, aParam
>         if (params.bodyIsLink)
>         {
>           var body = msgCompFields.body;
>           if (gMsgCompose.composeHTML)
>           {
>             var cleanBody;
>             try {
>-              cleanBody = unescape(body);
>+              cleanBody = decodeURIComponent(body);
>             } catch(e) { cleanBody = body;}
> 
>             msgCompFields.body = "<BR><A HREF=\"" + body + "\">" + cleanBody +
"</A><BR>";
>           }
>           else
>             msgCompFields.body = "\n<" + body + ">\n";
>         }
body is a URI, we're cleaning it so we can put unicode into the message, but
IMHO decodeURI would do it right where decodeURIComponent might not.
(Of course, there'a a side issue of a URI that contains an HTML entity...)

Comment 32

[Jungshik Shin]

Attachment: main browser part (including firebird)

This patch include everything other than mailnews, mail and extension.
I'm pretty sure that this works fine.

Comment 33

[Jungshik Shin]

Attachment: mail/mailnews patch (thunderbird included)

Thanks, neil, for catching bugs. In one of cases, I was inconsistent (in mail I
used on and in mailnews, I used the other).

Comment 34

[Jungshik Shin]

Attachment: extension patch : IRC, Verkman,

I fixed several typos ('URiComponent' in place of 'URIComponent')

Comment 35

[Jungshik Shin]

neil and rbs (both not in the US :-))  do you care to review?  

re. comment #21 and attachment 135443 [details] [diff] [review], I forgot to include it in my patch, but
replacing escape with encodeURIComponent (instead of using encodeURI for the
whole thing) should work, as you wrote. 

Comment 36

[Boris Zbarsky [:bzbarsky]]

Note that the first patch (to viewpartialsource and neterror) needs to be
updated too... it's not included in jshin's patches.  :(

Comment 37

[Jungshik Shin]

Attachment: update for browser part (include view source patch)

I added bz's patch with the change he mentioned. It also includes the
corresponding change for firebird (int toolkit directory)

Comment 38

[Jesse Ruderman]

*** Bug 226765 has been marked as a duplicate of this bug. ***

Comment 39

[Jungshik Shin]

Comment on attachment 136481 [details] [diff] [review]
update for browser part (include view source patch)

asking for r/sr.
Given a lot of bugs have been filed for a short period (since JS
escape/unescape fix), I guess we'll be able to catch any  new bug (that is not
caught in r/sr process) pretty soon. Let's be brave :-) 

I've been using an optimized build with this patch for the last 24hours(?) and
haven't yet come across a problem (in keyword search, bookmark, mail search,
url bar) although it doesn't tell much.

Comment 40

[Jungshik Shin]

Comment on attachment 136363 [details] [diff] [review]
mail/mailnews patch (thunderbird included)

asking for r/sr.

a couple of places I have some doubt about:

In mail/components/prefwindow/content/pref-themes.js:


> -  file = 'file:///' + escape(filePath.replace(/\\/g,'/'));
> +  file = 'file:///' + encodeURIComponent(filePath.replace(/\\/g,'/'));
   > InstallTrigger.installChrome(InstallTrigger.SKIN, file, getName(file));

We might be better off using encodeURI over the whole thing(including
'file:///')

Comment 41

[Jungshik Shin]

Comment on attachment 136364 [details] [diff] [review]
extension patch : IRC, Verkman, 

asking for r/sr.

Comment 42

[Scott MacGregor]

*** Bug 226982 has been marked as a duplicate of this bug. ***

Comment 43

[[e-mail]]

Comment on attachment 136363 [details] [diff] [review]
mail/mailnews patch (thunderbird included)

>-  dump("SaveAsFile from XUL\n");
>-  var subject = document.getElementById('msgSubject').value; 
>-  GetCurrentEditor().setDocumentTitle(subject); 
>+  dump("SaveAsFile from XUL\n");
>+  var subject = document.getElementById('msgSubject').value; 
>+  GetCurrentEditor().setDocumentTitle(subject); 
Note sure what this is doing here...

>     var file = '';
>-    file = 'file:///' + escape(fp.file.path.replace(/\\/g,'/'));
>+    file = 'file:///' + encodeURIComponent(fp.file.path.replace(/\\/g,'/'));
>     doXPIInstall(file, getName(file));
I'm not surprised you were confused by this, it makes certain possibly
unjustified assumptions about files and URLs. Here's some code which makes no
assumptions:

const nsIIOService = Components.interfaces.nsIIOService;
const nsIFileProtocolHandler = Components.interfaces.nsIFileProtocolHandler;
const nsIURL = Components.interfaces.nsIURL;
var ioService =
Components.classes['@mozilla.org/network/io-service;1'].getService(nsIIOService
);
var fileProtocolHandler =
ioService.getProtocolHandler("file").QueryInterface(nsIFileProtocolHandler);
var url = fileProtocolHandler.newFileURI(fp.file).QueryInterface(nsIURL);
// extensions
var xpi = {};
xpi[decodeURIComponent(url.fileBaseName)] = url.spec;
InstallTrigger.install(xpi);
// themes
InstallTrigger.installChrome(InstallTrigger.SKIN, url.spec,
decodeURIComponent(url.fileBaseName));

In the mean time I suggest you encode the whole file:/// URI. Other than that,
what sort of testing (if any) were you looking for? So far I haven't tested
anything, I've only read the differences.

Comment 44

[Scott MacGregor]

*** Bug 226999 has been marked as a duplicate of this bug. ***

Comment 45

[HARUNAGA Hirotoshi]

I can see this also on Mac OS 10.2.8.
Changing Hardware to All.

Comment 46

[Jason Barnabe (np)]

*** Bug 227270 has been marked as a duplicate of this bug. ***

Comment 47

[Jungshik Shin]

Thanks, neil. The first chunk you're not sure of is spurrious and should haven't 
been there. As for the second, I'll call encodeURI() over the whole thing as you 
suggested. 

In mailnews, I tested search with non-ASCII strings and saving an attachment and 
attachments with non-ASCII filenames. Testing addressbook would be nice, but I 
guess it'd be all right if you don't find anything obviously wrong in the patch.

neil, do you want me to upload a new file? Otherwise, please stamp 'r' on 
attachement 136363 with the understanding that I'll make changes outlined above. 
mscott, it'd be nice if you can review attachment 136363 [details] [diff] [review] as you see fit. There 
are a lot of dupes and I guess we all want to get this and other two patches in.
 

Comment 48

[[e-mail]]

Comment on attachment 136363 [details] [diff] [review]
mail/mailnews patch (thunderbird included)

OK, so based on comment #47, I'm convinced. Somehow I don't expect anybody to
fix that faulty file to url conversion this year ;-)

Comment 49

[David :Bienvenu]

Comment on attachment 136363 [details] [diff] [review]
mail/mailnews patch (thunderbird included)

can you fix the missing newlines while you're at it?

Comment 50

[Jungshik Shin]

Comment on attachment 136363 [details] [diff] [review]
mail/mailnews patch (thunderbird included)

thanks for r/sr.
asking for a1.6
hope it's not too late.

it'd be nice get r/sr for other two patches, soon

Comment 51

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 136481 [details] [diff] [review]
update for browser part (include view source patch)

>Index: ./xpfe/components/related/resources/related-panel.js
>@@ -65,17 +65,17 @@ function (oSubject, sMessage, sContextUr
>-	return escape(sCDT);
>+	return encodeURI(sCDT);

This should be encodeURIComponent, imo.

With that, r=bzbarsky

Comment 52

[rbs]

Comment on attachment 136481 [details] [diff] [review]
update for browser part (include view source patch)

sr=rbs

Useless XXX comment:
+    try {  // XXX_JSHIN
+      item.setAttribute("tooltiptext", decodeURI(attachment.url));
     } catch(e) {
       item.setAttribute("tooltiptext", attachment.url);
     }

Care to also post on n.p.m.seamonkey that people shouldn't use escape/unescape
anymore to tweak URIs, as they are now in parity with IE:

http://msdn.microsoft.com/library/default.asp?url=/library/en-us/script56/html/
js56jsmthencodeuricomponent.asp

Comment 53

[Adrian Pavlykevych]

*** Bug 226997 has been marked as a duplicate of this bug. ***

Comment 54

[Jungshik Shin]

Comment on attachment 136481 [details] [diff] [review]
update for browser part (include view source patch)

thanks for r/sr.
I'll fix two issues raised in the review comment. I'll post to the newsgroup
about escape/unescape vs en/decodeURI(Component).BTW, it's not so much about IE
parity as about the standard compliance :-).

asking for 1.6b.There's some risk, but we have to either back out JS
escape/unescape fix (, which would keep Mozilla NOT compliant to the ECMAscript
standard) or take the risk. Given that a number of bugs have been filed over a
short time-span (since JS escape/unescape was fixed), I'm pretty sure that
whatever bug not fixed/introduced with this fix will be caught during the beta
period.

Comment 55

[Jungshik Shin]

Comment on attachment 136364 [details] [diff] [review]
extension patch : IRC, Verkman, 

>Index: ./extensions/irc/xul/content/static.js

>@@ -1201,17 +1201,17 @@ function parseIRCURL (url)
>         ary = rest.match (/^\/([^\,\?\s\/]*)?\/?(,[^\?]*)?(\?.*)?$/);
>         if (!ary)
>         {
>             dd ("parseIRCURL: rest split failed ``" + rest + "''");
>             return null;
>         }
>         
>         rv.target = arrayHasElementAt(ary, 1) ? 
>-            ecmaUnescape(ary[1]).replace("\n", "\\n") : "";
>+            encodeURIComponent(ary[1]).replace("\n", "\\n") : "";

  I'll do : 
    s/encodeURIComponent/decodeURIComponent/




>Index: extensions/irc/xul/content/static.js

>@@ -1201,17 +1201,17 @@ function parseIRCURL (url)

>         
>         rv.target = arrayHasElementAt(ary, 1) ? 
>-            ecmaUnescape(ary[1]).replace("\n", "\\n") : "";
>+            encodeURIComponent(ary[1]).replace("\n", "\\n") : "";

  The same here.

Comment 56

[Robert Ginda]

Comment on attachment 136364 [details] [diff] [review]
extension patch : IRC, Verkman, 

Am I correct in assuming that encode/decodeURI deal with unicode characters by
converting them to UTF-8 and then escaping with %XX?

If so, then ecmaEscape and ecmaUnescape are still useful.  They work according
to the ECMA spec, which is to say:

  For those characters being replaced whose code point value is 0xFF or
  less, a two-digit escape sequence of the form %xx is used. For those
  characters being replaced whose code point value is greater than 0xFF,
  a four-digit escape sequence of the form %uxxxx is used.

While Mozilla's escape/unescape may have finally been fixed to comply, the
latest versions of chatzilla and venkman can be installed on older, broken
versions of mozilla.  We try to make them work there, wherever possible.


@@ -207,27 +207,27 @@ function pm_arrayupdate(prefName)
 PrefManager.prototype.stringToArray =
 function pm_s2a(string)
 {
     if (string.search(/\S/) == -1)
	 return [];

     var ary = string.split(/\s*;\s*/);
     for (var i = 0; i < ary.length; ++i)
-	 ary[i] = toUnicode(unescape(ary[i]), PREF_CHARSET);
+	 ary[i] = decodeURI(ary[i]);

     return ary;
 }

 PrefManager.prototype.arrayToString =
 function pm_a2s(ary)
 {
     var escapedAry = new Array()
     for (var i = 0; i < ary.length; ++i)
-	 escapedAry[i] = escape(fromUnicode(ary[i], PREF_CHARSET));
+	 escapedAry[i] = encodeURI(ary[i]);

This code deals with seralizing javascript arrays into a string pref in a
general way.  It is not URI specific.  This should probably become ecmaUnescape
and ecmaEscape to be safe.  Also remember that if you change the escape
mechanism for prefs, you're going to break everyone's profile, which is a Bad
Thing.

-	 display(unescape(MSG_TEST_CTLCHR), "PRIVMSG", sampleUser, me);
-	 display(unescape(MSG_TEST_COLOR), "PRIVMSG", sampleUser, me);
+	 // XXX unescape() may just work as well here?
+	 display(decodeURIComponent(MSG_TEST_CTLCHR), "PRIVMSG", sampleUser,
me);
+	 display(decodeURIComponent(MSG_TEST_COLOR), "PRIVMSG", sampleUser,
me);

This is not a URI either.  It should be left as is, or changed to ecmaUnescape.

     var encodedMatchText = fromUnicode(matchText, eventData.sourceObject);
     var anchor = document.createElementNS("http://www.w3.org/1999/xhtml",
					   "html:a");
     anchor.setAttribute ("href", eventData.network.getURL() +
-			  ecmaEscape(encodedMatchText));
+			  encodeURIComponent(encodedMatchText));

This has already been encoded, as indicated by the name and the call to
fromUnicode.  If you use encodeURIComponent here, you're going to double encode
the URL.  The correct fix is probably to call encodeURIComponent on matchText,
and remove the fromUnicode call.

	 rv.target = arrayHasElementAt(ary, 1) ? 
-	     ecmaUnescape(ary[1]).replace("\n", "\\n") : "";
+	     encodeURIComponent(ary[1]).replace("\n", "\\n") : "";

You've replaced an unescape with an encode.  Sounds backwards to me.

+++ extensions/irc/xul/content/prefs.js 26 Nov 2003 15:17:15 -0000
@@ -139,17 +139,17 @@ function initPrefs()
     client.charset			 = client.prefs["charset"];

     initAliases();
 }

 function pref_mungeName(name)
 {
     var safeName = name.replace(/\./g, "-").replace(/:/g, "_").toLowerCase();
-    return ecmaEscape(safeName);
+    return encodeURIComponent(safeName);
 }

That's not a URI, it's the name of a pref.  Some folks might lose their network
prefs if you did that.

I don't want to see this land without some real testing.  Please fix these
issues and we'll spin an XPI for hacksrus.com.

Comment 57

[Jungshik Shin]

Thanks for review and I'll make changes. There are some issues to resolve, 
though.

> Am I correct in assuming that encode/decodeURI deal with unicode characters by
> converting them to UTF-8 and then escaping with %XX?

Yes, that's what ECMA 262 15.1.3 says and what Mozilla JS engine implemented. 
Note that en/decodeURI(Component) haven't been touched. They've been always 
compliant to the ECMA 262. en/decodeURIComponent() are not much URI-specific and 
work similarly to the old broken escape/unescape() when the 'document' encoding 
is UTF-8, which is the case in IRC and Venkman. Unfortunately, 
en/decodeURIComponent() are not a drop-in replacement for the old 
escape/unescape because they don't agree on which characters to escape and which 
not to.

> - escapedAry[i] = escape(fromUnicode(ary[i], PREF_CHARSET));
> + escapedAry[i] = encodeURI(ary[i]);

> This code deals with seralizing javascript arrays into a string pref in a
> general way.  It is not URI specific.  This should probably become 
ecmaUnescape
> and ecmaEscape to be safe.  

> Also remember that if you change the escape mechanism for prefs, you're going 
> to break everyone's profile, which is a Bad Thing.

  As long as PREF_CHARSET is UTF-8 (which is the case as far as I can tell), my 
change(assuming I use en/decodeURIComponent() instead of en/decodeURI) wouldn't 
make a great difference. Still, there's a difference as noted above.  Using 
ecmaEscape() instead of encodeURIComponent(which would for sure a good idea if 
we began from the scratch) would introduce a even more extensive backward 
compatibility problem you're afraid of, wouldn't it? I don't know how to 
preserve old profiles at the same time making a new version work with older 
Mozilla here(if ecmaEscape()/ecmaUnescape() had been used, it'd be possible as 
in the following case).

> function pref_mungeName(name)
> {
>     var safeName = name.replace(/\./g, "-").replace(/:/g, "_").toLowerCase();
> -    return ecmaEscape(safeName);
> +    return encodeURIComponent(safeName);
> }

> That's not a URI, it's the name of a pref. Some folks might lose their 
> network prefs if you did that.


  OK.  To keep old profiles work with a new version and to accomodate older 
Mozilla, we have to keep ecmaEscape() around.   Where is the corresponding 
'unmunge' code, btw?

Comment 58

[Scott MacGregor]

FYI, the mail patch breaks the ability to install extensions and themes and
thunderbird when they are local files. That's the primary way we install themes
and extensions too. We probably need to fix that before checking this in. I'm
looking into it. 

Comment 59

[Scott MacGregor]

Ignore that last comment. When I applied the patch, I did not read Neil's comment:

"In the mean time I suggest you encode the whole file:/// URI. Other than that,
what sort of testing (if any) were you looking for? So far I haven't tested
anything, I've only read the differences."

for pref-themes.js and pref-extensions.js. I made that change and
themes/extensions work again.

Comment 60

[Jungshik Shin]

Attachment: a new patch for IRC/Venkman

I addressed rgina's concerns. As I wrote before, for some of them, we need to
decide what to do, in terms of backward compatibility (with old profiles and
with old versions of Mozilla) so that this patch still has some rough edges to
smooth out.

Anyway, as bz wrote to me in an email, we may not have to block 1.6b for IRC
and Venkman. rgina also thinks that it needs to be tested more thoroughly
before landing. So, I guess we'll go with two other patches(pending 1.6b
approval) for 1.6b and bake this patch (for IRC and Venkman) for a while. 

BTW, mscott, thanks for testing the mail(news) patch.

Comment 61

[Asa Dotzler [:asa]]

Comment on attachment 136363 [details] [diff] [review]
mail/mailnews patch (thunderbird included)

a=asa (on behalf of drivers) for checkin to 1.6beta

Comment 62

[Asa Dotzler [:asa]]

Comment on attachment 136481 [details] [diff] [review]
update for browser part (include view source patch)

a=asa (on behalf of drivers) for checkin to 1.6beta

Comment 63

[Jungshik Shin]

browser and mailnews/mail patches checked in.
thanks all.

btw, the seemingly spurrious diff. (comment #43) turned out due to that the old
file had spurrious CR at the end of three lines affected. (no CR is used
elsewhere in the file). As for the missing newlines, I guess they were missing
in the original file so that my check-in added them, which is harmless.

Comment 64

[Jungshik Shin]

Now that two patches (for components more frequently used than those fixed by
the third) have been landed, what should I do? The blocking flag was removed,
but it's still listed in the blocking bug list(I realized that the URL in the
tinder box was made manually). Should I mark this as fixed and work on the rest
in a separate bug or just keep this open with the understanding that it doesn't
block 1.6b? 

rgina thinks that Venkman patch is rather risk-free compared with IRC patch.
Besides, Venkman seems to get more exposures than chatzilla.  Therefore, we
might want to get that part in. 

Comment 65

[Jungshik Shin]

Attachment: Venkman only patch

This is rather straightforward replacing escape/unescape with
encodeURIComponent/decodeURIComponent. I replaced two instances of encodeURI()
in attachment 136740 [details] [diff] [review] with encodeURIComponent()

Comment 66

[Scott MacGregor]

I think you checked in a file with a conflict. I just fixed it but be careful
next time :). It's a dangerous world out there...

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/mail/components/compose/content&command=DIFF_FRAMESET&file=MsgComposeCommands.js&rev1=1.26&rev2=1.27&root=/cvsroot

Comment 67

[Jungshik Shin]

Ooops. sorry and thanks for fixing it. I removed the identical conflict in the
corresponding file in mailnews tree, but apparently missed it in TB tree. I just
'grepped' all ohter files I checked in to make sure there's no more conflict
sneaked in and there is no more.

Comment 68

[[e-mail]]

*** Bug 128716 has been marked as a duplicate of this bug. ***

Comment 69

[Robert Ginda]

Comment on attachment 136770 [details] [diff] [review]
Venkman only patch

r=rginda

Comment 70

[Jungshik Shin]

Comment on attachment 136770 [details] [diff] [review]
Venkman only patch

Asking for sr.

In a recent poll (conducted at builders.com??), Mozilla composer was ranked the
third as a 'web developer' platform thanks largely to Venkman. Given that, I
guess we don't want to break it in 1.6(b). If possible, let's get this in as
well.

Comment 71

[Jungshik Shin]

Comment on attachment 136364 [details] [diff] [review]
extension patch : IRC, Verkman, 

sorry for spamming.

Comment 72

[Brendan Eich [:brendan]]

Comment on attachment 136770 [details] [diff] [review]
Venkman only patch

sr=me, and please get it in tonight -- thanks!

/be

Comment 73

[Jungshik Shin]

Comment on attachment 136770 [details] [diff] [review]
Venkman only patch

brendan, thansk for quick sr. while you're at it, can you put a1.6b stamp as
well. I guess you effectively did, but...

Comment 74

[Jungshik Shin]

Thanks. Fix just got landed. If necessary (for 1.6b release tracking
convenience), I'll mark this as fixed and open a separate bug for chatzilla. 

Comment 75

[[e-mail]]

BTW, for
var fileProtocolHandler =
ioService.getProtocolHandler("file").QueryInterface(nsIFileProtocolHandler);
var url = fileProtocolHandler.newFileURI(fp.file).QueryInterface(nsIURL);

there's a handy shortcut:
var url = ioService.newFileURI(fp.file).QueryInterface(nsIURL);

Comment 76

[Oleg Sidletskiy]

*** Bug 227025 has been marked as a duplicate of this bug. ***

Comment 77

[Oleg Sidletskiy]

*** Bug 227509 has been marked as a duplicate of this bug. ***

Comment 78

[Jungshik Shin]

Attachment: extra patch for thunderbird theme/extension installer

I just did what Neil suggested in comment #43 to show  his hunch wrong :-)

Comment 79

[Jungshik Shin]

Comment on attachment 137750 [details] [diff] [review]
extra patch for thunderbird theme/extension installer 

asking for r/sr

Comment 80

[[e-mail]]

Comment on attachment 137750 [details] [diff] [review]
extra patch for thunderbird theme/extension installer 

Any review I did of this would probably break three rules of review...

Comment 81

[Jungshik Shin]

Comment on attachment 137750 [details] [diff] [review]
extra patch for thunderbird theme/extension installer 

Thanks for sr.

Because it's neil's patch, asking him for review broke rules (as he wrote).  I
should have incorporated it in the first round. 

BTW, I'm changing the function signature of 'installTheme' to accept  a file
picker instead of a file path. No other caller was found in lxr on mozilla so I
think it's safe. I considered merging installTheme() with installSkin() (the
only caller), but just left it alone.

Comment 82

[[e-mail]]

jshin, you might have missed a few (un)escapes? LXR shows these:
/editor/ui/dialogs/content/EditorPublish.js, line 160
 -- gDialog.FilenameInput.value = unescape(filename);
/xpfe/components/prefwindow/resources/content/pref-applications.js, line 111
 -- type = unescape(neverAskSave[i]);
/xpfe/components/prefwindow/resources/content/pref-applications.js, line 120
 -- type = unescape(neverAskOpen[i]);
/mailnews/base/resources/content/subscribe.js, line 349
 -- SetState(escape(name), state);

Comment 83

[Jungshik Shin]

Thanks. All of them were in my original list, but somehow I didn't fix them. I
ran 'find ... xargs grep ' on my tree and found three more (two in
browser/components/bookmarks/src/nsBookmarkProtocolHandler.js that was checked
on December 4th - i.e. after I made patches - and one in
layout/html/tests/block/bugs/58652.js).


-    bookmark.url = unescape(rv[1]);
+    bookmark.url = decodeURI(rv[1]);

       var propertyData = propertyPairs[i].split("=");
-      bookmark[propertyData[0]] = unescape(propertyData[1]);
+      bookmark[propertyData[0]] = decodeURIComponent(propertyData[1]);

I made a preliminary patch, but am not sure if that's right because I can't find
the corresponding escape/unescape for all cases. I'm gonna uploade it soon

Comment 84

[Jungshik Shin]

Attachment: additional patch for cases found by neil (and a new file)

thank you, neil, for catching them.

Comment 85

[[e-mail]]

I see what you mean - subscribe.js weirdly only escapes some of the time...

Comment 86

[Jungshik Shin]

Yeah, that's what I'm least sure of. (see bug 126453 as well)

As for 'pref-applications.js', I guess we can just do what I did in the latest
patch (because MIME type would be just ASCII with '/' and '-' if I'm right about
what 'AskForSave' is). The most conspicuous one would be
'nsBookmarkProtocolHandler.js' (the new one). We probably have to fix it before
firebird 0.8 ships.

 'Publish' (composer) part I have to try it myself. 

Comment 87

[Jungshik Shin]

Comment on attachment 137829 [details] [diff] [review]
additional patch for cases found by neil (and a new file)

asking for r/sr.

1. EditorPublish.js : I went through the file and am pretty sure that we have
to use decodeURIComponent().

2. subscribe.js : It should be all right. 

3. nsBookmarkProtocol.js : I should have pushed a little harder and a little
earlier. I think this should go in for firebird 0.8release

Comment 88

[Jungshik Shin]

Comment on attachment 137750 [details] [diff] [review]
extra patch for thunderbird theme/extension installer 

mscott landed almost identical patch in bug 228300 a couple of weeks ago.

Comment 89

[Pascal Chevrel:pascalc]

*** Bug 241596 has been marked as a duplicate of this bug. ***

Comment 90

[Henrik Skupin [:whimboo][⌚️UTC+2]]

*** Bug 242174 has been marked as a duplicate of this bug. ***

Comment 91

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

What is keeping this bug from being resolved?

Comment 92

[Brendan Eich [:brendan]]

jshin, will you check in that last patch, or does it need more work not reported
here?

/be

Comment 93

[Jungshik Shin]

Attachment 137829 [details] [diff] was checked in (I'm sorry I haven't noted it here) in Feburary
last year. What keep this from being resolved is that IRC patch hasn't been
applied yet.  I didn't ask for review of IRC/Vekman patch (attachment 136740 [details] [diff] [review])
and uploaded Vekman only patch(that was landed) perhaps because IRC patch was
regarded as a bit too dangerous. (see comment #64, comment #60). 

Comment 94

[Jonathan Nieder]

Perhaps it would be worthwhile to update the summary?

Comment 95

[Zibi Braniecki [:zbraniecki][:gandalf]]

I'm going to assume that this is inactive. Reopen if needed.