378991 - Enable IDN link support in messages for Thunderbird and don't pass only punycode values to external apps

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect, P2)

Description

[u217243]

Attachment: the fix

JPRS, the .JP domain registry asked us about IDN link support of Thunderbird.

When clicking on a IDN link in messages, the default browser opens and fails to load the %-escaped URI. Although Firefox will support such URIs in the future (bug 309671), current version and some other browsers don't support for now.

Can you implement a workaround, to send a link in the Punycode URI? Browsers will open it with the Unicode URI (if that TLD is on the browser's whitelist.)

JPRS hopes the fix will be included in Thunderbird 2.0.0.x.

Comment 1

[Scott MacGregor]

Kohei, do you know who the best reviewers are for this change? I don't know anything about this pref myself to understand the ramifications.

Also, any sense of the risk? We might try to participate in the 2.0.0.4 release but the cut off for that is Monday so it would be good to have a sense for how risky this is.

Thanks for tracking this down!

Comment 2

[u217243]

(In reply to comment #1)
> Kohei, do you know who the best reviewers are for this change? I don't know
> anything about this pref myself to understand the ramifications.

CCing Darin and Masayuki. They might understand about this.

> Also, any sense of the risk? We might try to participate in the 2.0.0.4 release
> but the cut off for that is Monday so it would be good to have a sense for how
> risky this is.

My understanding is that the risk for email features should be low because there aren't IDN email addresses yet. Anyway, much more work must be done to support such addresses.

So think about the browser or message view feature. With the fix, IDN URIs can be loaded in the message view through feeds or start page. Link URIs are shown as Punycode on the status bar. Your default browser will open that link and display the URI as Unicode or Punycode according to its policy (black/whitelist).

Masayuki: any concerns?

Comment 3

[u217243]

(In reply to comment #2)
> So think about the browser or message view feature. With the fix, IDN URIs can
> be loaded in the message view through feeds or start page. 

Umm... IDN URIs cannot be added to the feed reader of Thunderbird ;-)
I'll file a bug about that.

Comment 5

[Scott MacGregor]

Comment on attachment 262993 [details] [diff] [review]
the fix

Is there any reason not to start trying this out on the trunk? I don't think so...

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 262993 [details] [diff] [review]
the fix

Yoshino-san:

why is network.IDN_show_punycode true?

By this, IDN is always shown as punycode.

Comment 7

[u217243]

(In reply to comment #6)
> why is network.IDN_show_punycode true?
> 
> By this, IDN is always shown as punycode.

Yes. That's for browsers compatibility.
See my comment #1 and <http://日本語.jp/support/mail_guide/mailer.html>.
Only Opera can resolve %-escaped URIs for now.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 262993 [details] [diff] [review]
the fix

fm... if this patch is used for branch, I can agree the approach, but on trunk, we should not use this approach.

I agree to use punycode for other applications. Because decoded IDN is not realistic for non-unicode applications.

We should only use punycode when we are sending it to other applications.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #7)
> Only Opera can resolve %-escaped URIs for now.

Oops, I may misread your comment.

Opera cannot load punycode URI from command line? If so, we should not support opera (Opera should fix the bug, not our duty, and we should not include the quirks).

I checked the code for this, the external applications should get punycode URI.
http://lxr.mozilla.org/mozilla/source/uriloader/exthandler/win/nsOSHelperAppService.cpp#163
> 163 nsresult nsOSHelperAppService::LoadUriInternal(nsIURI * aURL)
> 164 {
> 165   nsresult rv = NS_OK;
> 166 
> 167   // 1. Find the default app for this protocol
> 168   // 2. Set up the command line
> 169   // 3. Launch the app.
> 170 
> 171   // For now, we'll just cheat essentially, check for the command line
> 172   // then just call ShellExecute()!
> 173 
> 174   if (aURL)
> 175   {
> 176     // extract the url spec from the url
> 177     nsCAutoString urlSpec;
> 178     aURL->GetAsciiSpec(urlSpec);

It uses GetAsciiSpec which should return the punycode URI.
http://lxr.mozilla.org/mozilla/source/netwerk/base/src/nsStandardURL.cpp#1031

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

taking this.

I think that we should use ideally way on trunk, i.e.,

1. IDN should be enabled.
2. The safety IDN URIs should not shown as punycode.
3. We should use ASCII spec in nsOSHelperAppService::LoadUriInternal on Mac. (Win32 and Linux are using ASCII spec in it.)

Comment 11

[Mark Banner (:standard8)]

This is one of the bugs that is blocking getting core xpcshell unit tests passing on Thunderbird. Seeing as a resolution doesn't look to be too far off, I'd like to push it on a bit.

Masayuki, 

> I think that we should use ideally way on trunk, i.e.,
> 
> 1. IDN should be enabled.
> 2. The safety IDN URIs should not shown as punycode.

I understand these.

> 3. We should use ASCII spec in nsOSHelperAppService::LoadUriInternal on Mac.
> (Win32 and Linux are using ASCII spec in it.)

Whilst I understand what you are saying, I don't see how that relates to this bug.

I can supply a patch for 1 (2 doesn't need a change), probably 3 as well, but I'd like to understand why 3 is needed as well.

Could you give us some pointers?

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #11)
> > 3. We should use ASCII spec in nsOSHelperAppService::LoadUriInternal on Mac.
> > (Win32 and Linux are using ASCII spec in it.)
> 
> Whilst I understand what you are saying, I don't see how that relates to this
> bug.

We should not send non-punycode IDN to another applications. Because the another application may not support IDN.

Comment 13

[Mark Banner (:standard8)]

Attachment: Enable IDN and change mac to send punycode

Masayuki, I think this does what you want. I tested it in Thunderbird with Safari as the external browser using the url on this bug and it seemed to work fine.

I've got no idea if it is possible to do unit tests for this, I expect not (or it may be difficult) as it is loading an external app. Suggestions welcome though.

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 318153 [details] [diff] [review]
Enable IDN and change mac to send punycode

Thanks. The statusbar display looks, ok for me. (2.)

Comment 15

[Mark Banner (:standard8)]

Comment on attachment 318153 [details] [diff] [review]
Enable IDN and change mac to send punycode

Dan, please confirm we're happy with the Thunderbird prefs change.

Benjamin, please could you sr these changes for the uriloader?

Comment 16

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 318153 [details] [diff] [review]
Enable IDN and change mac to send punycode

can you please use more context next time?

it looks like there are also a few more places that should be updated for this (nsLocalHandlerAppMac and nsLocalHandlerApp come to mind, probably also the OS/2 and BeOS MIME infos)

Comment 17

[Mark Banner (:standard8)]

(In reply to comment #16)
> (From update of attachment 318153 [details] [diff] [review])
> can you please use more context next time?

Yes, sorry.

> it looks like there are also a few more places that should be updated for this
> (nsLocalHandlerAppMac and nsLocalHandlerApp come to mind, probably also the
> OS/2 and BeOS MIME infos)

I have raised bug 431516 to cover this.

Comment 18

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 318153 [details] [diff] [review]
Enable IDN and change mac to send punycode

Looks good, but I don't understand why it makes sense to split the other cases that biesi mentioned into another bug, given that they're just as trivial to fix.  r- unless there's some reason I don't know about.

As far as testing, the localHandler app stuff could be tested from handlerApps.js (called by test_handlerApps.xhtml).  I suspect the best way to do testing for this stuff would be to invoke an application ("perl -e", perhaps) that takes whatever is on the command-line and writes it to a file.  You could then use nsILocalFile and friends to check that the file contains what you think it should.

Comment 19

[Dan Mosedale (:dmosedale, :dmose)]

It might be harder to find a Mac application bundle that does what I described, but the approach should work on Windows & Linux at least, I'd think.

Comment 20

[Mark Banner (:standard8)]

Moving to core (given where the fixes are) so once the new patch is reviewed, I'll be able to request approval and get it noticed.

Comment 21

[Mark Banner (:standard8)]

Attachment: Enable IDN and change handlers to pass punycode and provide mochitest

This patch fixes all the instances that I could find in uriloader/exthandler where we are loading URIs for external applications - they should now all be in punycode.

Also provides a mochitest for testing this. I have had problems with the mac mochitest - I think it doesn't work mainly because its not an application bundle, I'm not sure how to set that up either (and I may be wrong). Given where we are, I'd just like to get this into the tree and if anyone can help fix the mac case then we can do that after.

Note that the WriteApplication purposely doesn't use any xpcom items due to the way it is being called (the shared libraries wouldn't load). Hence the reason for the pre-processing.

I've tested the patch on Mac and Linux and it works as expected (FF & SM & TB - TB w/o mochitest), unable to test on Windows as I don't have a build environment (and the try servers don't do mochitests :-( ).

Comment 22

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 319969 [details] [diff] [review]
Enable IDN and change handlers to pass punycode and provide mochitest

Looks great; r=dmose.

Comment 23

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 319969 [details] [diff] [review]
Enable IDN and change handlers to pass punycode and provide mochitest

+++ uriloader/exthandler/tests/mochitest/WriteArgument.cpp	8 May 2008 10:18:33 -0000
+    return -1;
..
+    return -2;

You'd normally return positive numbers from main...

+  fprintf(outfile, "%s", argv[1]);

fputs?

+++ uriloader/exthandler/tests/mochitest/punycodeURIs.js.in	8 May 2008 10:18:33 -0000

Have you considered (for this an WriteArgument) using an environment variable for the destdir? That way you can avoid the preprocessing.


Also, why's this a mochitest instead of an xpcshell test?

+  // This currently fails on Mac with an argument like -psn_0_nnnnnn
+  // This seems to be to do with how the executable is called, but I couldn't
+  // find a way around it.

Would writing the last argument to the file work?

+  tempFile.remove(false);

shouldn't you remove the file even if the test fails? (I'm assuming that is() throws an exception in the failure case, I might be wrong there)

Comment 24

[Mark Banner (:standard8)]

(In reply to comment #23)
> (From update of attachment 319969 [details] [diff] [review])
> +++ uriloader/exthandler/tests/mochitest/punycodeURIs.js.in     8 May 2008
> 10:18:33 -0000
> 
> Have you considered (for this an WriteArgument) using an environment variable
> for the destdir? That way you can avoid the preprocessing.

I've now changed this to pass an environment variable to WriteArgument. The test has been reworked in such a way that it doesn't need preprocessing either.

> Also, why's this a mochitest instead of an xpcshell test?

I was copying and pasting, however its now an xpcshell test.

> +  // This currently fails on Mac with an argument like -psn_0_nnnnnn
> +  // This seems to be to do with how the executable is called, but I couldn't
> +  // find a way around it.
> 
> Would writing the last argument to the file work?

Unfortunately no, -psn_0_nnnnnn is the only argument in this case.

> +  tempFile.remove(false);
> 
> shouldn't you remove the file even if the test fails? (I'm assuming that is()
> throws an exception in the failure case, I might be wrong there)

Yes, I've moved this removal to be earlier in the file.

Comment 25

[Mark Banner (:standard8)]

Attachment: Enable IDN and change handlers to pass punycode and provide mochitest v2

Updated patch per biesi's comments (only changed unit test). Requesting approval for checking in on 1.9. This patch changes the way we call external applications with URIs so that we don't pass them IDN values, but pass them punycode which is a safer form. I expect this will have most effect on Thunderbird (which will be an improvement). Obviously there will be an effect on other applications where they call external applications.

Patch has been tested on Mac and Unix. I don't expect any problems with windows (there's no unit test try server :-( ) but the unit test should pick it up if there is.

Comment 26

[Mike Schroepfer]

Mark/Dmose can this wait until 1.9.0.x?   I'm really nervous about touching this since URL/IDN handling is notoriously tricky.  That plus we are locked down for showstoppers for RC1 only.

Comment 27

[Mike Schroepfer]

Comment on attachment 320163 [details] [diff] [review]
Enable IDN and change handlers to pass punycode and provide mochitest v2

only taking zero risk (e.g. css) or showstopper issues at this point.  If this is one of those please re-nom and let me know.

Comment 28

[Mark Banner (:standard8)]

(In reply to comment #26)
> Mark/Dmose can this wait until 1.9.0.x?   I'm really nervous about touching
> this since URL/IDN handling is notoriously tricky.  That plus we are locked
> down for showstoppers for RC1 only.
> 
This can wait, as long as we can get it into the 1.9.0.x builds.

Comment 29

[Mark Banner (:standard8)]

Attachment: Enable IDN and change handlers to pass punycode and provide mochitest v2a

Updated patch to include checking for WriteArgument.exe on Windows within the unit test.

The core changes in this patch have been checked in mozilla-central. I am waiting nominating it for 1.9 pending 1.9.0.x looking open and/or a decision on where Thunderbird is heading to in terms of branches.

Comment 30

[Mark Banner (:standard8)]

(In reply to comment #29)
> The core changes in this patch have been checked in mozilla-central. I am
> waiting nominating it for 1.9 pending 1.9.0.x looking open and/or a decision on
> where Thunderbird is heading to in terms of branches.

The current plan seems to be heading towards Thunderbird picking up the 1.9.1 development. Therefore I'm not going to nominate this for the 1.9.0.x branches for the time being, to eliminate any risk of regressions on those branches.

Note: can't resolve as fixed until Thunderbird picks up the 1.9.1 branch and the change to all-thunderbird.js is checked in.

Comment 31

[Mark Banner (:standard8)]

This patch is now completely checked in. Marking as FIXED.

Note the test fails on SeaMonkey Mac OS X 10.4, but I think that's more likely a problem with launchFromURI than from enabling punycode in this bug.

Comment 32

[Mark Banner (:standard8)]

(In reply to comment #31)
> Note the test fails on SeaMonkey Mac OS X 10.4, but I think that's more likely
> a problem with launchFromURI than from enabling punycode in this bug.

Which is bug 447999

Comment 33

[Shawn Wilsher :sdwilsh]

I filed bug 488597 about a possible random failure of a test added with this bug.