Closed Bug 42898 Opened 24 years ago Closed 23 years ago

iDNS support

Categories

(Core :: Internationalization, defect, P2)

Product:
Core
Component:
Internationalization
Platform:
All
Other
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla0.9.8

People

(Reporter: bobj, Assigned: darin.moz)

References

(
URL
)

Details

(Keywords: intl)

Attachments

(4 files, 15 obsolete files)

Added comments about locking in nsHttpHandler.cpp and nsDnsService.cpp; s/nsIIDNUtils/nsIIDNService/g
20.08 KB, patch
darin.moz
: review+
brendan
: superreview+
asa
: approval+
Details | Diff | Splinter Review
patch - remove nsStdEscape call
1.05 KB, patch
nhottanscp
: review+
brendan
: superreview+
Details | Diff | Splinter Review
revised patch for latest problem
3.73 KB, patch
nhottanscp
: review+
brendan
: superreview+
Details | Diff | Splinter Review
does not work on linux RH7.1 w/current CVS.
17.48 KB, image/gif
Details 
Putting this on the radar.
INTERNET-DRAFT:
Using the UTF-8 Character Set in the Domain Name System
http://info.broker.isi.edu/0/in-drafts/files/draft-skwan-utf8-dns-03.txt

See also http://www.idns.org/
See also bug 42899: IURI support
what does this bug mean in turn of code ? Any test cases available ?
Status: NEW → ASSIGNED
See bug 43852 "Send URLs as UTF-8" not working.
Probably need to link these either with depend or dup.
momoi, I will mark this bug as future for now.
Can you forward this bug to the Japanese IDNS folks and ask them to put down
more info- especially test cases ?
Target Milestone: --- → Future
Done.
Mark this as nsbeta1, remove the "Future" and reassign to nhotta.
We should spec out a Requirement document for the iDNS support.
Assignee: ftang → nhotta
Status: ASSIGNED → NEW
Keywords: intl, nsbeta1
Target Milestone: Future → ---
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9
Mozilla uses unescaped UTF-8 string for DNS lookup. 
http://lxr.mozilla.org/seamonkey/source/netwerk/dns/src/nsDnsService.cpp#577

At this point, there is no standard for iDNS. Mozilla has no special support for 
iDNS but keeps the current behavior until the standard is settled.

Keep this bug open for future.
Target Milestone: mozilla0.9 → Future
There is ongoing effort to implment following spec.
http://search.ietf.org/internet-drafts/draft-ietf-idn-idna-01.txt

Reset the milestone.
Target Milestone: Future → ---
when do we plan to add the hook. I want to see the propose XPCOM interface first. 

Attached a patch for Mozilla 0.9.

Summary
-------

Files patched:

modules/libpref/src/init/all.js
netwerk/dns/src/Makefile.in
netwerk/dns/src/nsDnsService.cpp
netwerk/dns/public/Makefile.in
netwerk/protocol/http/src/nsHTTPRequest.cpp
netwerk/protocol/http/src/nsHTTPChannel.cpp
netwerk/test/Makefile.in
xpfe/components/prefwindow/resources/content/pref-advanced.xul
xpfe/components/prefwindow/resources/locale/en-US/pref-advanced.dtd


Files added:

netwerk/dns/src/nsIDNConvert.cpp
netwerk/dns/src/nsIDNConvert.h
netwerk/dns/public/nsIIDNConvert.idl
netwerk/test/TestIDN.cpp


Notes
-----
* nsIDNConvert is a normal class used by nsDNSLookup, nsHTTPRequest and 
nsHTTPChannel.  It will be compiled into necko.
* nsIDNConvert checks to see if idnSDK is installed using the ContractID "@i-
dns.net/convert;1".  It will keep the normal behaviour (no nameprep and ACE 
conversion) when idnSDK is not found.
* idnSDK (or any others) needs to conform to the nsIIDNConvert interface.
* Added a checkbox in Advanced preferences dialog to turn on/off IDN capability.
* None of the suggested name/interface changes have been incorporated yet.  
Please feel free to modify them in accordance with your naming convention.
Here are some of my review comments-- 

1. Lookups should stay as efficient as possible-- you should cache the pref for 
enableIDN instead of getting it on each lookup transaction. (Corollary- you need 
to register a pref change function so that you could register changes)

2. once enableIDN becomes mEnableIDN, I'd consider cleaning up the logic to look 
more like this--
   if (isAscii || !mEnableIDN) { 
       // do the ascii stuff here
   }
   else {
       // other stuff
   }
This avoids code duplication and keeps logic clean

3. if (enableIDN == PR_FALSE) should just be "if !enableIDN" but then this will 
go away with #2

4. Do not print stuff to stderr for release builds... and even in debug not for 
everyone... so I'd suggest wrapping all "fprintf(stderr, message)" with #ifdef 
DEBUG_nhotta

5. It seems to me that independent of whether we need to convert of not 
mHostName is always set to the orignial hostName. If that is the case then why 
bother to put it in both the if and the else clause. That way your logic 
simplifies to-- 
   // do ascii stuff
   if (!isAscii && mEnabledIDN) {
       // do other stuff
   }

6. Can the nsIDNConvert be reused? If not then why not? and if it can then we 
should make that a variable of the DNSService so that we don't keep constructing 
and destroying this object. 

7. Seems like mHostName and mHostNameACE could just be the same variable. Is 
there a reason why they have to be different? That will simplify the logic even 
more. 

8. nsHTTPRequest, nsHTTPChannel have changed... please resubmit changes with the 
latest source.

9. modifying the host of mURL in the channel may have some unpleasant side 
affects. Check with darin@netscape.com to confirm if this is the right thing.

10. you should also cache the nsIIDNConverter in nsIDNConvert to reuse it.

11. The UUID explaination in nsIIDNConvert.idl is nice but not needed.

In addition to this. You should really get this reviewed by Gordon as well. 
* Please move the "enableIDN" flag to navigator.properties. So do as below,
pref("network.enableIDN", "chrome://navigator/locale/navigator.properties");
and add a flag to navigator.properties. That allows localization to modify the
flag easily. Related to that, change GetBoolPref to GetLocalizedUnicharPref.
And do not get a pref value everytime but store it (and use observer for pref
change callback) as gagan mentioned.

* Please put comments in nsIIDNConvert.idl to explain about those functions.
I think AutoConvert() is not a good name, please change it to more descriptive name.
What are "src_enc" and "dst_enc"? Are those for specifying charset?

* CreateInstance nsIIDNConvert everytime in AutoConver() is expensive. Please
cache it. Is nsIIDNConvert a thread safe?

* Please attach a screen shot for the UI change of preference dialog.



The additonal checkbox in preference dialog,
"Resolve Internationalized Domain Names (requires idnSDK download)"

How can the user download the sdk?

Set to 0.9.2, not realistic for 0.9.1.
Target Milestone: --- → mozilla0.9.2
To download idnSDK, how about having a button on beside the option, "More
information", that takes the user to the download page?  On unix, my prefs
window is not modal so that is not a problem, but on Windows it is application
modal am I right?
Naoki, I am rather confused why you insisted on using GetLocalizedUnicharPref
instead of GetBoolPref when the option is just a Boolean.  I figured that I may
have overlooked some important points, so I went on to implement what you
suggested.  However, I cannot seem to get the prefwindow to set the preference
automatically for me.  I noticed that the 
intl.menuitems.alwaysappendacceskeys option you suggested me to look at does not
have a corresponding pref dialog UI, appreciate any help on this.

Here's my XUL after moving the resource to navigator.properties.

      <checkbox id="enableIDN" label="&enableIDNCheck.label;"
accesskey="&enableIDNCheck.accesskey;"
                pref="true" preftype="string" prefstring="network.enableIDN"
                prefattribute="checked"/>

Proposed name changes.

nsIIDNConvert => nsIIDNUtils
nsIIDNConvert::autoConvert() => nsIIDNUtils::UTF8ToIDNHostName()
(modified from Frank's suggestion because this seems more descriptive, please 
comment)

class nsIDNConvert => class nsIDNWrapper
nsIDNConvert::AutoConvert() => nsIDNWrapper::UTF8ToIDNHostName()

Willam, you are right. We probably need GetLocalizedBoolPref (filed as bug 82399).
But I am not sure if the UI is really necessary. The pref value can be changed
by JavaScript, so you can do that in the sdk download page.

Cc to jaimejr, adding "download" button in the pref dialog, is that possible? I
remember you mentioned about the modal problem (bug number?).
I have reworked my code for the new nsHttpChannel class and think I found a bug 
where mozilla segfaults on visiting the previous page (back button).  
Apparently it happens somewhere in libnkcache.  Has a bug been filed for this?
The checkbox is so that users may choose to turn it off for whatever reason 
(even if they have the SDK), but you are right that it is not absolutely 
necessary.
The question is how does the user get to know that the idnSDK is needed?  
Ideally, we should insert a check somewhere in the application UI (like 
WebShell?) to detect non-ASCII domain name and prompts users to visit the 
download page.
Thanks to the reviewers (Gagan and Naoki), this one addresses the issues 
raised.  Summary:

* Cached mEnableIDN option in nsHttpHandler and nsDnsService with observer for 
preferences change
* Cached converter (nsIDNConvert) in nsHttpHandler and nsDnsService
* Cached 'nsIIDNConvert *' as member in nsIDNConvert
* Moved 'network.enableIDN' option to navigator.properties
* Tested on Windows 2000 and Linux
* nsDnsLookup has an #ifdef XP_MAC that calls the mac way of doing lookup, 
which I'm not familiar with, so I left it untouched, need help here.
>The question is how does the user get to know that the idnSDK is needed?
We need to find out how to do this, this could be a netscape only issue (Jaime,
any idea?).
And I think the default value of "enableIDN" should be false.
> And I think the default value of "enableIDN" should be false.
Only if there is some other way the user can be made aware of such an 
option/feature, such as the prefwindow.

It also depends on the meaning of enableIDN:
one
---
if (!isAscii && enableIDN) {
  sdkFound = findSDK();
  if (!sdkFound) promptDownloadSDK();
  else UTF8ToACE();
}

two
---
if (!isAscii) {
  sdkFound = findSDK();
  if (!sdkFound) promptDownloadSDK();
  else if (sdkFound && enableIDN) UTF8ToACE();
}

In the first method, enableIDN should be true by default so that SDK will be 
searched for.  This is the method that I have implemented.
I think the issues of sdk download and preference UI want to be separated from
the initial check in. Those can be added later after we decide how to do (either
in mozilla or netscape only). And the backend code still has to be reviewed by
two more people (darin, gordon) too.
So, could you take out the UI changes (.xul, .dtd) from the patch for now and
set the default pref value to false? Also include the remaining suggestions from
gagan and me (or explain if you disagree)? Let's get the backend code reviewed
first.

> 1. Lookups should stay as efficient as possible-- you should cache the pref 
for enableIDN instead of getting it on each lookup transaction. (Corollary- you 
need to register a pref change function so that you could register changes)

Implemented in new patch.

> 2. once enableIDN becomes mEnableIDN, I'd consider cleaning up the logic to 
> look more like this--
>   if (isAscii || !mEnableIDN) { 
>       // do the ascii stuff here
>   }
>   else {
>       // other stuff
>   }
> This avoids code duplication and keeps logic clean

Implemented in new patch.  However mEnableIDN is a member of nsDNSService 
instead of nsDNSLookup.


> 3. if (enableIDN == PR_FALSE) should just be "if !enableIDN" but then this 
> will go away with #2

Yes it has gone away.


> 4. Do not print stuff to stderr for release builds... and even in debug not 
> for everyone... so I'd suggest wrapping all "fprintf(stderr, message)" with 
> #ifdef DEBUG_nhotta

Done.

> 5. It seems to me that independent of whether we need to convert of not 
> mHostName is always set to the orignial hostName. If that is the case then
> why bother to put it in both the if and the else clause. That way your logic 
> simplifies to-- 
>   // do ascii stuff
>   if (!isAscii && mEnabledIDN) {
>       // do other stuff
>   }

You are right, and it has been implemented in the new patch.


> 6. Can the nsIDNConvert be reused? If not then why not? and if it can then we 
> should make that a variable of the DNSService so that we don't keep 
> constructing and destroying this object. 

Implemented in new patch.


> 7. Seems like mHostName and mHostNameACE could just be the same variable. Is 
> there a reason why they have to be different? That will simplify the logic 
> even more. 

In an earlier mail to you (Gagan), I had mentioned the trouble I had with 
this.  If I perform the IDN conversion and store it as mHostName during 
nsDNSLookup::Init(), I'll break lots of things since users of nsDNSLookup would 
be looking for the original UTF8 hostname, but it has been changed to the ACE 
version internally.  So, instead of doing that we store the ACE name in 
mHostNameACE and use it only in gethostbyname (and variations).


> 8. nsHTTPRequest, nsHTTPChannel have changed... please resubmit changes with
> the latest source.

Done.


> 9. modifying the host of mURL in the channel may have some unpleasant side 
> affects. Check with darin@netscape.com to confirm if this is the right thing.

mURL is not modified, but instead I cloned it so that I could change 
the "CONNECT <url>" string for proxy requests.
With the new nsHttp* classes this has been taken out.


> 10. you should also cache the nsIIDNConverter in nsIDNConvert to reuse it.

Implemented.


> 11. The UUID explaination in nsIIDNConvert.idl is nice but not needed.

Deleted.


> * Please move the "enableIDN" flag to navigator.properties.

Done.


> * Please put comments in nsIIDNConvert.idl to explain about those functions.
>   I think AutoConvert() is not a good name, please change it to more
>   descriptive name. What are "src_enc" and "dst_enc"? Are those for 
>   specifying charset?

I have made put in some explanation for the AutoConvert() method, but I'm 
waiting for any comments on the suggestion for the name changes, if there is no 
objection shall I proceed with the name change?
As for the rest of the functions, I would like to remove them for the initial 
checkin (they're not being used directly anyway).  We can then do a review on 
the interface before adding them in.


> * CreateInstance nsIIDNConvert everytime in AutoConver() is expensive. Please
> cache it. Is nsIIDNConvert a thread safe?

Yes it is thread safe.  It is done as suggested.


> * Please attach a screen shot for the UI change of preference dialog.

Done.
I talked to Gordon and DNS code is going to change (bug 30917), put depend on 
that bug for now. The change is supposed to be landed in 0.9.2, there is a 
branch (tag is "DNS_BRANCH"). William, would you update your patch with the 
change?

By the way, is there a way to test the patch? I think the sdk and test server is 
needed.
Depends on: 30917
> William, would you update your patch with the change?

Alright, I shall do that.

> By the way, is there a way to test the patch? I think the sdk and test server 
is needed.

Yes, we have a bunch of test URLs <a href="http://playground.i-
dns.net/~wil/moztest/">here</a>.  I will provide the URL for the SDK download 
later today.
Priority: P3 → P2
mark this as moz0.9.3
Target Milestone: mozilla0.9.2 → mozilla0.9.3
Attached patch Patched for new DNS code (obsolete) — — Splinter Review 

    
    

    
  
I've take a look at the changes to nsDnsService.cpp.  Here are a few comments:

mHostNameACE seems redundant.  Why not have nsDNSService::Lookup convert the
hostname if necessary before calling FindOrCreateLookup?  That avoids the messy
callbacks through nsDNSService::gService, and avoids a strdup().

mEnableIDN seems redundant.  Why not just use mIDNConverter != nsnull?

In nsDNSLookup::Create() the loop checking isAscii should be inside of the if
(nsDNSService::gService->mEnableIDN) bracket. Of course, that code probably
shouldn't be in that function at all, and a check against mIDNConverter should
be used rather than a separate flag that must be kept in sync.

I haven't taken a look at the changes to HTTP.  I strongly suggest having Darin
look at those.



    
    

    
  
too risky for m9.2/nsbranch. Keep it in moz0.9.3. Please check in to trunk if it
pass r= and sr=

    
    

    
  
gorden: can you code review this ?

    
  
Target Milestone: mozilla0.9.3 → ---

    
    

    
  
Reassign to yokoyama.
Assignee: nhotta → yokoyama
Status: ASSIGNED → NEW
Target Milestone: --- → mozilla0.9.4

    
    

    
  
accepting.
Status: NEW → ASSIGNED

    
    

    
  
I'd rather see the code in nsDnsService.cpp look something more like this:

PRBool
IsAsciiString(const char * s)
{
    for (const char *c = s; *c; c++) {
        if (!nsCRT::IsAscii(*c))  return PR_FALSE;
    }
    
    return PR_TRUE
}

----------------------
nsDNSService::Lookup()
----------------------

        nsDNSLookup *lookup = nsnull;
        // IDN handling
        if ((mIDNConverter != nsnull) && (!IsAsciiString(hostName) {

            // XXX should we do any locking to prevent mIDNConverter from being
free'd?
            nsXPIDLCString hostNameACE;
            rv = mIDNConverter->UTF8ToIDNHostName (hostName,
getter_Copies(hostNameACE));
            hostName = hostNameACE.get();
            if (!hostName)  return NS_ERROR_OUT_OF_MEMORY;
        }

        nsDNSLookup * lookup = FindOrCreateLookup(hostName);
        if (!lookup) return NS_ERROR_OUT_OF_MEMORY;
        NS_ADDREF(lookup);  // keep it around for life of this method.

And Darin should really review the changes to HTTP.

    
    

    
  
Where would you suggest IsAsciiString() be put under, nsDnsService?  Since it is
a fairly generic function, would it fit into nsCRT?

    
    

    
  
I'd just put it in nsDnsService.cpp for now.  We can migrate it to a more
general place later.

    
    

    
  
does iDNS support only apply to http:// urls?

    
    

    
  
We hope to start with "http://", the rest would follow.  Getting the IDN
conversion support into mozilla would be the stepping stone for all the other
protocols which needs it.

    
    

    
  
changing to m0.9.5
Target Milestone: mozilla0.9.4 → mozilla0.9.5

    
    

    
  
Wil,
Your latest patch did not reflect the changes that Gordon suggested
for nsDNSService::Lookup()

    
    

    
  
Gagan's suggestion doesn't seem to work:

    .
    .
    .
    hostName = hostNameACE.get();
    if (!hostName) return NS_ERROR_OUT_OF_MEMORY;
}

since hostName would be pointing to non-existent memory outside of the braces. 
If we change it to:

hostName = strdup (hostNameACE.get());

then we'll have to remember to free it later on.
Any suggestion?

    
    

    
  
I see, hostName is passed in as a const char *, so you cannot do what Gordon
suggested.

Another question, if the lookup for the ACE-converted hostname fails, is it
the intention to try to look up the UTF-8 hostname?
+        // if it hasn't been created (no IDN converter / not an IDN hostName)
+        if (lookup == nsnull)
+            lookup = FindOrCreateLookup(hostName);

If so, then that's another reason not to do what Gordon was trying to do.
Because is we only called lookup once, then we could not fallback to the
UTF-8-hostname if the ACE-hostname was not found.




    
    

    
  
I haven't thought about falling back to UTF8, since
1. That does not seem to be the direction that the IETF idn WG would be going
2. It would require quite a twist to the logic of the DNS code

Currently it leaves the original hostname untouched if the external IDNUtil
converter is not instantiated (either not installed, or pref set to false).


    
    

    
  
The debug version of the IDN toolkit for Linux-glibc213 and Windows is
available.  After applying the patch found on this bug, go to this page to
install it.
http://pg.i-dns.net/~wil/xpidn/install.html

After installing, shutdown mozilla.
Append this line to your "prefs.js" file:

user_pref("network.enableIDN", true);

And start mozilla again, go to http://pg.i-dns.net/~wil/moztest/
to test out some some real multilingual domain names.

    
    

    
  
It seems that nsIDNWrapper could be removed in favor of simply making clients
talk directly to nsIIDNUtils.  the nsHttpHandler could for example create an
instance of the IDN utils service at startup, and then the code in nsHttpChannel
could query the handler for the IDN utils service.  if present it would use the
IDN utils server, but otherwise it would simply fallback on the existing
implementation.

    
    

    
  
That would certainly remove a level of indirection.  I did it so that the
various modules that needs IDN service can depend on a stable interface, in this
case, "nsIDNWrapper.h". Also, in the future we might allow different IDN toolkit
to be installed, and configured for use.

    
    

    
  
so, it sounds like you may have need for a nsIIDNToolkit interface or something
that does the backend work, but be it nsIDNWrapper or nsIIDNUtils, you still
need to decide on a stable interface for clients to use.  this should probably
be a XPCOM interface so that it can be used outside of necko.so (for example,
FTP lives in necko2.so).

in your current patch, nsIDNWrapper and nsIIDNUtils have identical interfaces,
and i don't see any advantage to using nsIDNWrapper.

    
    

    
  
Comment on attachment 48688 [details] [diff] [review]
Darin is right, so nsIDNWrapper is removed with this patch.

some comments:

nsIIDNUtils.idl:

1- typo: Proides -> Provides

2- how about adding a contributor line?

3- remove excess whitespace in method declaration

TestIDN.cpp:

4- contributor line?

5- clean up the indentation

all.js:

6- please add a more descriptive comment for network.enableIDN (ie. expand the acronym).

nsDnsService.cpp:

7- remove unused static kIDNUtilsIID.

8- some DEBUG_wil sections can be combined... please do so.
9- if (mIDNConverter != nsnull) can be replaced w/ if (mIDNConverter)
10- remove extra line above XXX comment
11- remove commented out code.

nsDnsService.h:

12- please follow existing convention when declaring member variables... 
remove space between IsAsciiString and (

nsHttpChannel.cpp:
13- remove commented out code.

14- remove extra line above "rv = nsHttpHandler::get()->IDNConverter()"
15- please try to respect a 80 char line length limit.

16- remove space between function name and ()

17- use a COM ptr for tmpURI... no need to call NS_RELEASE early.

18- clean up indentation of "else {" block

19- the connection info's host is the hostname passed to the socket transport.
please resolve the XXX comment.

20- why not overwrite |host| instead of adding a hostACE variable?
you are adding an extra strdup when the IDN converter is not present.
this must be fixed.

21- too bad the definition of IsAsciiString can't be shared.

nsHttpChannel.h:

22- please remove extra space between IsAsciiString and (
nsHttpHandler.cpp:

23- remove unused static kIDNUtilsIID

24- add extra line after declaration of IDNSDK_CONTRACTID 

25- consider moving IDNSDK_CONTRACTID #define into nsNetCID.h

26- do you really need all of the DEBUG_wil fprintf's?

nsHttpHandler.h:

27- IDNConverter() should return |nsIIDNUtils *| and NOT a nsCOMPtr.


ok, so many of the issues i've enumerated are minor details, but they all need to be 
resolved before this patch can be considered for inclusion in mozilla.

    
    

    
  
The above patch addresses all issues presented by Darin on 2001-09-17.
Items not shown is trivially fixed.  Any comments / deviation is listed below:

* All DEBUG_wil have been removed (along with the fprintf calls)

> 12- please follow existing convention when declaring member variables... 
* Done.  Will do so in future efforts.

> 15- please try to respect a 80 char line length limit.
* Done.  Will do so in future patches.

> 16- remove space between function name and ()
* Ditto.

> 18- clean up indentation of "else {" block
* The "else {" block is no longer needed.

> 20- why not overwrite |host| instead of adding a hostACE variable?
> you are adding an extra strdup when the IDN converter is not present.
> this must be fixed.
* Done.  You are right, now we only do extra work only
  if (IDNConverter() && !IsAsciiString())

> 21- too bad the definition of IsAsciiString can't be shared.
* Shouldn't this be in nsCRT or some necko utility library?

> 25- consider moving IDNSDK_CONTRACTID #define into nsNetCID.h
* Done.  Makes sense, since it might be needed elsewhere.

> 27- IDNConverter() should return |nsIIDNUtils *| and NOT a nsCOMPtr.
* Done. Since the object is owned by nsHttpHandler there is no need for ADDREF
and RELEASE right?  Or should I wrap it like this?
nsCOMPtr<nsIIDNUtils> converter = nsHttpHandler::get()->IDNConverter();

    
    

    
  
> 27- IDNConverter() should return |nsIIDNUtils *| and NOT a nsCOMPtr.
* Done. Since the object is owned by nsHttpHandler there is no need for ADDREF
and RELEASE right?  Or should I wrap it like this?
nsCOMPtr<nsIIDNUtils> converter = nsHttpHandler::get()->IDNConverter();

That's necessary if the static nsHttpHandler instance's mIDNConverter member
could change (and it looks like it could, due to pref changes).

(If mIDNConverter could never change, then IDNConverter might rather return an
already_AddRefed<nsIIDNUtils>; FYI only.)

A few more comments:

Prevailing style uses nsnull, not 0 (so mIDNConverter = nsnull).

UTF8ToIDNHostName is a method in a scriptable interface, but it fails to declare
a useful return value.  It could be called the same from C++, and would be both
easier and cheaper to call from JS and other languages, if it were declared as

    string UTF8ToIDNHostName(in string input);

Please be aware that Mozilla interfaces are currently abusing string to mean
both ASCII (its original meaning) and UTF-8 -- this will change before 1.0 (see
bug 84186; again, FYI and for future reference).

/be

    
    

    
  
the latest patch is looking much better.

one final nit:

1- add a section delimiter to nsNetCID.h ... something like:

/******************************************************************************
 * netwerk/dns/ classes
 */

#define NS_IDNUTILS_CONTRACTID "@i-dns.net/IDNUtils;1"

NS_IDNUTILS_CONTRACTID seems like a better name than NS_IDNSDK_CONTRACTID
since it matches the interface nsIIDNUtils, but is there some reason why
you choose NS_IDNSDK_CONTRACTID instead?


    
    

    
  
i think this patch is for the most part ready (r=darin), but let me just ask one
more question:

since the converter method is not exactly finalized, is it correct to use text
like hostACE in the code?

    
    

    
  
ACE is a fairly general term as does not refer to a particular encoding. So
unless the IETF comes out with something radically different we should be fine.

    
    

    
  
sounds good enough to me.

    
    

    
  
Thanks Darin. Would this require a super review? Hopefully we can meet the
target 0.9.5 milestone.

    
    

    
  
please refer to http://mozilla.org/hacking/ for instructions on how to land a
patch.  in summary, you will need a reviewer and a super-reviewer.

    
    

    
  
Brendan, since you have seen the patch and given some comments, would you mind
being the super reviewer? If so, please take a look and raise any concern you
may have. Otherwise I would appreciate any recommendation for a suitable person
to do a super-review.

Thanks.

    
    

    
  
I should add that Precheckin Tests have all passed in addition to the new
features. If anyone is kind enough to apply and patch and want to try out the
converter library, (i.e. be my patch buddy) please contact me. The URL for
downloading the library is

http://playground.i-dns.net/~wil/xpidn/

and a good place to get test cases for internationalized domain names is:

http://www.i-dns.net/samples/

    
    

    
  
Comment on attachment 49862 [details] [diff] [review]
Added section delim to nsNetCID.h, changed identifier to NS_IDNUTILS_CONTRACTID.  The old identifier is a direct consequence of the product name, but you're right that NS_IDNUTILS_CONTRACTID is a more meaningful name.

- Please see http://www.mozilla.org/MPL/boilerplate-1.1/
and use the MPL/LGPL/GPL boilerplate.

- Perhaps cite bug 84186 in the doc comment for nsIIDNUtils?  Just a thought.

- Nit for darin: why isn't PAD allcaps, as the IDN TLA properly is?

 pref("network.enablePad",                   false); // Allow client to do proxy autodiscovery
+pref("network.enableIDN",                   false); // Turn on/off IDN (Internationalized Domain Name) resolution

- Don't initialize nsCOMPtrs, they self-construct to null:

@@ -857,6 +858,7 @@
     , mExpirationInterval(5*60)         // 300 seconds (5 minutes)
     , mMyIPAddress(0)
     , mState(DNS_NOT_INITIALIZED)
+    , mIDNConverter(nsnull)
 #if defined(XP_MAC)

- Why do we need locking beyond that done by nsDnsService.cpp?

+        if (mIDNConverter && !IsAsciiString(hostName)) {
+            // XXX should we do any locking to prevent mIDNConverter from being
+            // free'd before this?  Or should we do another check here?
+            nsXPIDLCString hostNameACE;
+            rv = mIDNConverter->UTF8ToIDNHostName(hostName, getter_Copies(hostNameACE));
+            if (!hostNameACE.get()) return NS_ERROR_OUT_OF_MEMORY;
+            lookup = FindOrCreateLookup(hostNameACE);
+        }

Is the implementation of UTF8ToIDNHostName threaded, or blocking?
If so, we don't want to hold mDNSServiceLock.

- Style nit: the second && operand is over-parenthesized:

+    if ((converter = nsHttpHandler::get()->IDNConverter()) &&
+        (!IsAsciiString(host)))

- Darin, dougt: should we have a bug to consolidate IsAsciiString into nsCRT?

/be

    
    

    
  
wil:

 - how about nsIIDNService instead of nsIIDNUtils?  ...afterall, it is a service.

brendan:
 
 - enablePad looks like something from version 1.1 of all.js... it probably 
   should be enablePAD... come to think of it, i'm not even sure that it is 
   even currently used.

 - we may need to worry about locking if the enableIDN pref were to change, and 
   signaled on the UI thread, while the code for DNS lookups is running on the 
   socket thread.  we need to verify that the DNS pref observer code enters the 
   DNS service lock.

 - if i understand correctly, the IDN conversion is a simple synchronous 
   mapping as yet not standardized.  i talked to bobj about this, and he 
   confirmed that this mapping should not need to block the calling thread 
   waiting on any asynchronous event.  that is to say that all of the 
   approaches under review by the IETF consist of simple computed mappings.
 - see bug 101852 on moving IsAsciiString into nsCRT.

    
    

    
  
- The DNS pref observer does not seem to enter the mDNSServiceLock.
  To avoid calling UTF8ToIDNHostName on a free'd COMPtr, I could 
  make a copy of it before the test, i.e.

  if (mIDNConverter && !IsAsciiString(hostName)) {
      nsCOMPtr<nsIIDNUtils> conv(mIDNConverter);
      if (conv)
          rv = conv->UTF8ToIDNHostName(...);

  Would this solve the problem?
- As Darin said, IDN conversion is a snappy, blocking operation.
  The implementation is re-entrant safe.

- I have no problem renaming nsIIDNUtils => nsIIDNService, but
  is there special requirement for being a service, like being
  a singleton? Incidentally, it also looks awfully similar to
  nsIDNSService.


    
    

    
  
You can't achieve thread-safety by copying an nsCOMPtr, because the reference
could dangle at any instruction (or SMP atomic bus transaction) boundary.  If
you knew the mIDNSConverter was not dangling, you wouldn't need a lock -- but
there's no way to know that when loading the mRawPtr from the nsCOMPtr (we'd
need an atomic "load and addref" operation).

I agree that nsIIDNSService is awfully similar to nsIDNSService (and has too
many adjacent I's and S's), but all things considered, I think nsIIDNSService is
a better name than nsIIDNSUtils.  Is it necessarily not a singleton, or is there
a reason it *should* be a singleton, however?  Maybe we should pin that down
before naming it.

/be

    
    

    
  
Yes, now I see that it was a naive approach. Is it then a valid solution to
acquire the DNS lock for a brief moment in the pref observer just to release
the reference? The same would apply to nsHttpHandler (the pref observer is 
not holding any lock) and nsHttpChannel (has the same risk of calling method
on dangling ptr).

It *should* be a singleton, since the initialization is one-time and the
functions are re-entrant. But users of the service cannot expect it
to be available at all times, since it may not even be installed.

    
    

    
  
> Yes, now I see that it was a naive approach. Is it then a valid solution to
> acquire the DNS lock for a brief moment in the pref observer just to release
> the reference? The same would apply to nsHttpHandler (the pref observer is 
> not holding any lock) and nsHttpChannel (has the same risk of calling method
> on dangling ptr).

If only the main thread ever assigns to mIDNConverter (it looks that way to me),
then we need to lock in the pref observers *only* around assignments to
mIDNConverter.

That means we'll null the mIDNConvert nsCOMPtr and run Release on last deref,
running the destructor for nsIDNUtils (or whatever the concrete class
implementing nsIIDNUtils is).  That dtor must have few instructions, and must
run to completion without blocking on any other lock or resource.

It would be bad to call do_CreateInstance while holding the DNS service lock,
because do_CreateInstance calls the component manager, which may have its own
locks (we should avoid nesting locks in general, esp. among separate modules). 
Instead, optimize by testing !mIDNConverter outside of the DNS service lock, do
the create instance if null, then lock and re-test -- if still null, assign;
else destroy the redundant instance.

> It *should* be a singleton, since the initialization is one-time and the
> functions are re-entrant. But users of the service cannot expect it
> to be available at all times, since it may not even be installed.

If you make it a true singleton, create-instance should never make more than
one, so modify the "destroy the redundant instance" advice above accordingly. 
It's ok for a singleton's code not to be installed -- that's much of the point
of all this XPCOM pain.  We still call such optional singletons "services".  I'm
open to another name, for sure.  Hey, is the iMac-inspired iDNS name one we
should use in types and variables?

/be

    
    

    
  
So putting your idea into code would yield this? This has not taken the
singleton stuff into account.

NS_IMETHODIMP
nsDNSService::Observe(nsISupports *      subject,
                      const PRUnichar *  topic,
                      const PRUnichar *  data)
{
    .
    .
    .
        if (enableIDN && !mIDNConverter) {
            nsCOMPtr<nsIIDNUtils> tmp =
do_CreateInstance(NS_IDNUTILS_CONTRACTID, &rv);
            NS_ASSERTION(NS_SUCCEEDED(rv),"idnSDK not installed");

            /* lock DNSService to ensure atomic construction of mIDNConverter */
            PR_Lock(mDNSServiceLock);
            if (!mIDNConverter)
                mIDNConverter = tmp;
            PR_Unlock(mDNSServiceLock);
        }
        else if (!enableIDN && mIDNConverter) {
            nsAutoLock dnsLock(mDNSServiceLock);
            mIDNConverter = nsnull;
        }

    
    

    
  
If only the main thread ever assigns to mIDNConverter, it's even easier:

        if (enableIDN && !mIDNConverter) {
            nsCOMPtr<nsIIDNUtils> tmp =
do_CreateInstance(NS_IDNUTILS_CONTRACTID, &rv);
            NS_ASSERTION(NS_SUCCEEDED(rv),"idnSDK not installed");

            /* lock DNSService to ensure atomic assignment to mIDNConverter */
            nsAutoLock dnsLock(mDNSServiceLock);
            mIDNConverter = tmp;
        }
        else if (!enableIDN && mIDNConverter) {
            nsAutoLock dnsLock(mDNSServiceLock);
            mIDNConverter = nsnull;
        }

What's more, if pointer stores are atomic, there's no need to lock when setting
mIDNConverter non-null.  That simplifies the above into:

        if (enableIDN && !mIDNConverter) {
            mIDNConverter = do_CreateInstance(NS_IDNUTILS_CONTRACTID, &rv);
            NS_ASSERTION(NS_SUCCEEDED(rv),"idnSDK not installed");
        }
        else if (!enableIDN && mIDNConverter) {
            nsAutoLock dnsLock(mDNSServiceLock);
            mIDNConverter = nsnull;
        }

This needs commenting.

/be
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED

    
    

    
  
Looks good to me, as long as there can be no two concurrent threads executing 
nsDNSService::Observe().

p.s. Why has this been marked RESOLVED?


    
    

    
  
The same problem applies in nsHttpHandler, which has a prefs observer and sets 
mIDNConverter to null when enableIDN==false. But nsHttpHandler does not have
a general lock, do we really want to create a mIDNConverterLock member variable
and use it to guard mIDNConverter? Any suggestion?

    
    

    
  
How'd that happen?!  Sorry, reopening.

/be
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

    
    

    
  
It seems to me this bug ought to be assigned to william.tan@i-dns.net, but I'll
leave it as it was.

/be
Status: REOPENED → ASSIGNED

    
    

    
  
> The same problem applies in nsHttpHandler, which has a prefs observer and sets 
> mIDNConverter to null when enableIDN==false. But nsHttpHandler does not have
> a general lock, do we really want to create a mIDNConverterLock member variable
> and use it to guard mIDNConverter? Any suggestion?

The nsHttpHandler.h mIDNConverer is a different member from the one the patch
adds to nsDnsService.h, so it needs its own lock -- but I don't yet see any uses
of mIDNConverter in nsHttpHandler.cpp outside of the pref change observer yet --
are you going to add some?  If so, to which method in nsHttpHandler.cpp?

We don't need another lock for nsDnsService.cpp uses
nsDNSService::mIDNConverter, nor do we want one (we'd nest locks, or have to do
deadlock avoidance by releasing the DNS service lock undesirably, acquiring an
mIDNConverterLock, doing iDNS, releasing that lock, reacquiring the DNS service
lock, and recovering from all our lost invariants).

So, if nsIDNSUtils is a service interface, we should be using do_GetService, not
do_CreateInstance.

/be


    
    

    
  
There's a miscommunication here. I don't mean another lock for
nsDNSService::mIDNConverter, but one for nsHttpHandler::mIDNConverter,
because it is used by nsHttpChannel like this:

nsCOMPtr<nsIIDNUtils> converter;
if ((converter = nsHttpHandler::get()->IDNConverter()) && !IsAsciiString(host)) {
    ...

As in the case of nsDNSService, we risk deferencing a free'd nsIIDNUtils pointer.
So do you think it is appropriate to add a private member
nsHttpHandler::mIDNConverterLock that gets locked in
nsHttpHandler::IDNConverter() and nsHttpHandler::PrefsChanged()?
Somehow it doesn't look like a good solution, but I don't know the
module well enough to have a solution. Maybe Darin?

Yes, after reading through the XPCOM FAQ, I think we should use do_GetService
and change the name to nsIIDNService.

    
    

    
  
Sorry, typing before caffeinating.  I didn't follow the use of mIDNConverter in
IDNConverter() to the caller(s) of that inline getter!

Does that call in nsHttpChannel.cpp to IDNConverter() run only on the main
thread?  Then no locking is required (but that code might do well to assert that
it's on the main thread -- better yet, put the assertion in IDNConverter's
body). Darin, pls. advise.

/be


    
    

    
  
Don't know much about the thread relationships in mozilla. Is there a remote 
possibility that the pref observer run in a different thread from 
nsHttpChannel::Init()?

And how do I assert that we are running on the main thread?

    
    

    
  
all methods in nsHttpChannel.cpp currently run on the UI thread.
nsHttpHandler::Observe also runs on the UI thread, so no special
locking is needed for HTTP.

many things would break if nsHttpChannel's methods were called
from threads other than the UI thread.

    
    

    
  
Ok then -- no locking needed in nsHttpChannel.cpp or nsHttpHandler.cpp.  The DNS
case is different because the uses of mIDNConverter run on a non-main thread and
could race with the clearing/release of that pointer in the main thread.

I think we are done (to confirm a question I left open earlier, pointer or
pointer-sized stores are atomic on all platforms I know of; wtc@netscape.com
relies on this in NSPR's monitor implementations).

/be

    
    

    
  
The ChangeLog:
* changed to MPL/GPL/LGPL licenses
* removed over parenthesized expression in nsHttpChannel.cpp
* incorporated Brendan's suggestions for locking in nsDnsService.cpp
* s/do_CreateInstance/do_GetService/g
* sync'ed with changes to nsHttpHandler's pref code
Outstanding issues:
* name change (nsIIDNUtils => nsIIDNService) is not in effect yet.
* IsAsciiString is not in nsCRT yet


    
    

    
  
Comment on attachment 51187 [details] [diff] [review]
This patch addresses issues raised by Brendan on 2001-09-26 and after.

+    else if (NS_LITERAL_STRING(NETWORK_ENABLEIDN).Equals(data)) {
+        PRBool enableIDN = PR_FALSE;
+        rv = prefs->GetBoolPref(NETWORK_ENABLEIDN, &enableIDN);
+
+        if (enableIDN && !mIDNConverter) {
+            mIDNConverter = do_GetService(NS_IDNUTILS_CONTRACTID, &rv);
+            NS_ASSERTION(NS_SUCCEEDED(rv),"idnSDK not installed");
+        }
+        else if (!enableIDN && mIDNConverter) {
+            nsAutoLock dnsLock(mDNSServiceLock);
+            mIDNConverter = nsnull;
+        }
+    }

How about that comment talking about why the lock is necessary when clearing,
but not when setting?  The assumptions are (1) this code runs only on the main thread;
(2) uses of mIDNConverter run on other threads inside the DNS service lock;
(3) pointer stores are atomic.

A comment in nsHttpHandler.cpp talking about why similar code there does *not* need
to lock when clearing an analogous mIDNConverter member would be good, too.

I like the name nsIIDNService for the interface -- it differs visibly (to me) from
nsIDNSService -- but maybe I'm just weird :-).

Let's figure out the right name and I'll gladly sr=.

/be

    
    

    
  
Okay, honestly I cannot think of any other name. nsIIDNService seems appropriate
since it really is a service, and suggests that clients use do_GetService to
instantiate it.
If there is no objection, I will go ahead and make the change.
Anyone?

    
    

    
  
i like nsIIDNService, because it is a service.  another name that sort of works
is nsIIDNConverter[Service]... thoughts?

    
    

    
  
That would probably be too long, wouldn't it? Let's stick to nsIIDNService then.

    
    

    
  
Comment on attachment 51265 [details] [diff] [review]
Added comments about locking in nsHttpHandler.cpp and nsDnsService.cpp; s/nsIIDNUtils/nsIIDNService/g

+         * BUT we do not perform lock when setting mIDNConverter because
+         * pointer stores are atomic.

No need for another patch, but could the last sentence say "and the users of mIDNConverter on other threads all test for null before dereferencing", or some such?  Thanks.

sr=brendan@mozilla.org, darin should re-stamp r= (william, you might want to obsolete all the patches before this one).

/be

        Attachment #51265 -
        Flags: superreview+

    
    

    
  
Comment on attachment 51265 [details] [diff] [review]
Added comments about locking in nsHttpHandler.cpp and nsDnsService.cpp; s/nsIIDNUtils/nsIIDNService/g

r=darin w/ brendan's suggested revision to the comments.

        Attachment #51265 -
        Flags: review+

    
    

    
  
Thanks Brendan, Darin!

Let me string them together here then.
+        /* We need to acquire the DNS lock when clearing mIDNConverter because:
+         * 1) this method runs only on the main UI thread
+         * 2) users of mIDNConvert (nsDNSService::Lookup) runs on other
+         *    threads inside the DNS service lock
+         * BUT locking is not required when setting mIDNConverter because
+         * pointer stores are atomic and the users of mIDNConverter on
+         * other threads all test for null before dereferencing.
+         */

Bugzilla says that I have not been authorized to edit attachments (but could
create as much as I like), so I can't mark the rest as obsolete.

Finally, I would need someone to help me commit the code into CVS. Any volunteer?

    
    

    
  
I gave bugzilla user william.tan@i-dns.net both the "canconfim" and "editbugs"
privileges just now.

Wm., either darin or I could do the checkin for you -- darin, do you have an
up-to-date tree ready?

/be

    
  

        Attachment #35318 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 35596 [details]
Proposed enableIDN option in the preference dialog box

This one is no longer in use.

        Attachment #35596 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 35830 [details] [diff] [review]
Revised patch for new nsHttp* classes and caching of converter and preference.

see updated attachment #51265 [details] [diff] [review]

        Attachment #35830 -
        Attachment is obsolete: true

    
  

        Attachment #35935 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 38402 [details] [diff] [review]
Patched for new DNS code

See updated attachment #51265 [details] [diff] [review].

        Attachment #38402 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 40109 [details] [diff] [review]
- applied Gordon's comments, changed pref to bool type.

See updated attachment #51265 [details] [diff] [review].

        Attachment #40109 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 41247 [details] [diff] [review]
The trunk has changed considerably since the last patch submitted, this is updated to ease the review process.

See updated attachment #51265 [details] [diff] [review].

        Attachment #41247 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 47160 [details] [diff] [review]
Added IsAsciiString() as suggested by Gagan

See updated attachment #51265 [details] [diff] [review].

        Attachment #47160 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 48688 [details] [diff] [review]
Darin is right, so nsIDNWrapper is removed with this patch.

See updated attachment #51265 [details] [diff] [review].

        Attachment #48688 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 49733 [details] [diff] [review]
fixed according to Darin's review (explanation in a separate post)

See updated attachment #51265 [details] [diff] [review].

        Attachment #49733 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 49758 [details] [diff] [review]
Thanks Brendan.  Here's the new diff.  Changed |nsIIDNUtils *| to |nsCOMPtr<nsIIDNUtils>|, changed interface for UTF8ToIDNHostName, assignment to 'nsnull' instead of 0.

See updated attachment #51265 [details] [diff] [review].

        Attachment #49758 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 49862 [details] [diff] [review]
Added section delim to nsNetCID.h, changed identifier to NS_IDNUTILS_CONTRACTID.  The old identifier is a direct consequence of the product name, but you're right that NS_IDNUTILS_CONTRACTID is a more meaningful name.

See updated attachment #51265 [details] [diff] [review].

        Attachment #49862 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 51187 [details] [diff] [review]
This patch addresses issues raised by Brendan on 2001-09-26 and after.

See updated attachment #51265 [details] [diff] [review].

        Attachment #51187 -
        Attachment is obsolete: true

    
    

    
  
Is there any chance that we can get this checked in before the freeze,
in order to meet the 0.9.5 target?
Thanks.

    
    

    
  
i can check this in.

    
    

    
  
Great, thanks.

    
    

    
  
adding 102701
Blocks: 102701

    
    

    
  
nhotta is back. assiging back to the original owner.
Assignee: yokoyama → nhotta
Status: ASSIGNED → NEW

    
    

    
  
Do we want this for 0.9.5?
The patch was reviewed, just need check in.
Or can this move to 0.9.6?


    
    

    
  
yes, i'd like to check this in for mozilla 0.9.5

    
    

    
  
Darin, could you get an approval and check in the patch?


    
    

    
  
I think the new file nsIIDNService.idl has to be added to MANIFEST for Macintosh
build. But no MANIFEST_IDL in dns/public. Gordon, do you have any idea?


    
    

    
  
drivers@mozilla.org isn't taking this for mozilla 0.9.5... i'll check it in once 
the tree opens for mozilla 0.9.6.

reassigning to myself for checkin.
Assignee: nhotta → darin
Target Milestone: mozilla0.9.5 → mozilla0.9.6

    
    

    
  
Comment on attachment 51265 [details] [diff] [review]
Added comments about locking in nsHttpHandler.cpp and nsDnsService.cpp; s/nsIIDNUtils/nsIIDNService/g

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

        Attachment #51265 -
        Flags: approval+

    
    

    
  
Yep, the MANIFEST files are out date in the mozilla/netwerk directory.  I've
filed bug 103167 to clean them up.  If you add your IDL files the the
NetworkIDL.mcp project, the headers will get generated in the proper dist
directory.  I can export the IDL files properly when I fix bug 103167.

    
    

    
  
-> ok.. back to 0.9.5
Status: NEW → ASSIGNED
Target Milestone: mozilla0.9.6 → mozilla0.9.5

    
  
Status: ASSIGNED → RESOLVED
Closed: 23 years ago → 23 years ago
Resolution: --- → FIXED

    
    

    
  
fixed-on-trunk

    
    

    
  
I tried today's commercial (not Mozilla) trunk build, the followed the directions
on 
     http://playground.i-dns.net/~wil/xpidn/

to download the component and set the pref.  But the servers are not resolving
from the links on these pages:

     http://www.i-dns.net/samples/

    
    

    
  
The packages at http://pg.i-dns.net/~wil/xpidn/ are not yet updated for the 
new nsIIDNService interface (essentially just the name change). We will
release an updated version soon, I'll post notification here.
What is the difference between mozilla and the netscape commercial build anyway?

    
    

    
  
William: generally, just the addition of extra XPCOM components... so, no
difference as far as this patch is concerned.

    
    

    
  
Wil, Please post a comment when you have updated packages.
We are anxious to try this out!

    
    

    
  
Debug version of the package is available for linux-x86-glibc213 and windows
currently. Get it at http://playground.i-dns.net/mozilla/xpidn/

    
    

    
  
william: it occured to me the other day that we could have probably done this
patch much differently.  wouldn't it have been possible to just do the IDN
conversion while generating the URL objects?  so, given a UTF-8 URL string, we
would convert the hostname of the URL object to IDN format.  had you thought of
doing this?  do you see any problems with a solution like this?

    
    

    
  
I thought about that but was afraid that changing URL could affect other
modules, but now that we are already discussing that option in bug 84032 it
makes good sense.

    
    

    
  
I went to the link and installed it. But I got either site not found or 
redirected to the netscape search.
I used 0.9.5 build with a new user profile on Windows 2000.

I see "IDNService.dll" in componets directory and pref.js has the line as below.
user_pref("network.enableIDN", true);

I quit and restarted tried again but the same result.



    
    

    
  
It looks like a successful installation to me. Few diagnosis:
* Make sure that it is a debug build (I think debug / release builds are 
  binary incompatible right?)
* If you tried the EUC samples, they won't work because I think we (i-DNS)
  recently pulled out of EUC encoding support, but I have left them in the
  test page.
* If you tried the SJIS samples, some would not work because it suffers
  from the lowercasing problem resulting in the wrong RACE. (BTW, do you
  see flashes of "Contacting ra--XXXXX.ra--XXXXXX.aced.net"?
* As a quick test, this page
  http://playground.i-dns.net/mozilla/samples/quicktest.html
  has all the URLs known to work.
Thanks

    
    

    
  
> * Make sure that it is a debug build (I think debug / release builds are 
>   binary incompatible right?)
> 
I used a release build, let me try again.
But could you put a release version? QA only tests release builds.

    
    

    
  
Great, it works with my debug build.

    
    

    
  
Thanks. The modules for release builds will be available shortly, I'll post
it here as soon as I have it.

    
    

    
  
The Windows release build of the library is available at the usual place:
http://playground.i-dns.net/mozilla/

    
    

    
  
I tested this in 2001-11-16 Win32 build with release library.

I tried the sample IDN.  Only UTF-8 - www.yakigashiya.com 洋菓子。会社 works, 
but other IDNs did not work.

    
    

    
  
Yes there is an additional problem with some encodings (SJIS affected) because
the hostname is being lowercased. See bug 102656.
These are the URLs that are not affected by the bug, did you try them?
http://playground.i-dns.net/mozilla/samples/quicktest.html

Thanks!

    
    

    
  
I tested 
http://playground.i-dns.net/mozilla/samples/quicktest.html
with 11-19 trunk build.  In Windows build. this worked fine. 

However,
In LInux, after I installed the Linux library and type
resource:///res/idnconfig.html in url, Netscape crashed.

In termininal, the message says
"creating new nsJSAimChatRendezvous
./mozilla-bin: error in loading shared libraries:
/u/teruko/linux/11-19-install/components/libIDNService.so: undefined symbol:
NS_CurrentThread"

I used Redhat Linux 6.2 JA system.

    
    

    
  
I have encountered this before, when I installed the debug build of the XPIDN
package on a release build. Are you using a release build? I'm sorry, only 
the debug package (for linux) is available at this time. I will to get the 
release build for you soon.

    
    

    
  
William Tan, have you created the release package (for linux), yet.

I reopen this for now.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

    
    

    
  
This seems to be broken on recent builds.
Using 0.9.6, the quick test works.
http://playground.i-dns.net/mozilla/samples/quicktest.html
But it does not work on my debug build which I pulled today, also broken in 12/8
trunk build (win32).

    
    

    
  
Thanks, I will download and verify.

    
    

    
  
Yes it's broken, I tried it on my Linux box.
At this spot:

http://lxr.mozilla.org/seamonkey/source/netwerk/protocol/http/src/nsHttpChannel.cpp#139

the host is ASCII (%-hex encoded UTF8). Previously, mURI->GetHost() gave the 
raw UTF8 string.

It may be due to the fix for bug 11084.

    
    

    
  
I have confirmed that by unescaping the hostname from mURI->GetHost() it works.
But this incurs a performance penalty.

    
    

    
  
william: what build are you testing?  nsStandardURL is not supposed to be
escaping the hostname.  it should be leaving it completely as is (with the
exception of making the hostname lowercase, which shouldn't interfere).  perhaps
someone in another section of the code is forcing the hostname to be escaped??

    
    

    
  
Can we set the target milestone to mozilla0.9.7 or above ?
(Please have a look at www.MozillaQuest.com . I think its not good for the image
of moz.)

    
    

    
  
I'm got the nightly source tarball from 16th December.
You're right, it's got nothing to do with nsStandardURL.We can unescape the
hostname in nsHttpChannel::Init(),
or we can make nsStandardURL::GetHost() unescape it.
What are the performance implications in each of them?

    
    

    
  
no... the hostname must never be unescaped.  because that opens a whole can of
worms.  consider a hostname such as "www.foobar.com%00www.evil.com" ... we've
been careful in necko not to unescape hostnames so as to avoid any wierdness
that could result from such things.

the problem here seems to be that someone is escaping the hostname.  we need to
figure out who is doing that and make them stop ;)

    
    

    
  
I realized that the escaping is likely caused by my change to nsLocation.cpp 
(rev=1.92). The host name should not be escaped there, need a similar change like 
bug 102656.
I will be gone for two week. William, could you take a look at there when you 
have a chance?


    
    

    
  
It doesn't look like nsLocation's fault. I tried reverting to rev 1.91 and the
problem still exists.
Incidentally, I realised that nsStdEscape does escape the hostname. See this line:
http://lxr.mozilla.org/seamonkey/source/content/shared/src/nsHTMLUtils.cpp#240

|spec| contains the escaped hostname after the assignment at line 242.
Does this have anything to do with bug 103916?
Target Milestone: mozilla0.9.5 → mozilla0.9.8

    
    

    
  
Added JP people for reference.

    
    

    
  


  
Darin: The old nsStdURL used to unescape the hostname in GetHost(). If we do
not wish to unescape the hostname, then care has to be taken to avoid escaping
it
which could be difficult when escaping the entire URL.

    
    

    
  
william: unescaping the hostname in GetHost was a nasty security bug (bug
110418) that was only recently fixed.  perhaps it caused this regression, but
undoing the change to GetHost is not possible.

calling nsStdEscape on an entire URL is problematic, especially since mozilla
handles hostnames in a special way.

in the case of NS_MakeAbsoluteURIWithCharset, i suspect that the call to
nsStdEscape is actually unnecessary.  the call to Resolve will take case of
escaping the URL segment-by-segment as necessary.  my assumption of course is
that the input to nsStdEscape is in a format that Resolve would understand (ie.
treating it as an array of ascii chars would have to be OK).

    
    

    
  


  
so, here's a patch that eliminates the call to nsStdEscape.  i believe this is
correct, but i need someone more familar with the charset converters and intl
in general to tell me if i'm correct.

        Attachment #63231 -
        Attachment is obsolete: true

    
    

    
  
andreas: could you take a look at the latest patch to remove the call to
nsStdEscape from NS_MakeAbsoluteWithCharset... am i missing something?? 
shouldn't this just work??

    
    

    
  
I'm really not sure. If I remember correctly waterson inserted this call in the
very first version of this file. It had something to do with charset conversion
and the result violating url syntax or something like that. I guess we should
try, that call really only does very minimal escaping.

    
    

    
  
I think the code in nsHTMLUtils.cpp affects links in a document only (see bug
102656). Currently, the IDN is broken including typing in the URL bar which was
originally enabled by this bug fix.
Daring, do you have any idea why IDN is not working in the URL bar, any solution?

    
    

    
  
no idea... will have to trace through the code to see who is doing the escaping.

if anyone can help with this it would be most appreciated... otherwise, i might
not get to this before 0.9.8
Status: REOPENED → ASSIGNED

    
    

    
  
I put assertions in NS_EscapeURLPart() and nsEscape() to break if the input is
not ASCII.
I typed www.ссс.com to the URL bar. Then NS_EscapeURLPart hit the assertion, the
input was UTF-8 (0xC3A1 for 'с'). Here is the stack.

NS_EscapeURLPart(const char * 0x0012b5e4, int 0x00000004, short 0x0001,
nsACString & {...}) line 377 + 41 bytes
nsStandardURL::EscapeSegment(const char * 0x0012b5e4, const
nsStandardURL::URLSegment & {...}, short 0x0001, nsCString & {...}) line 184 +
31 bytes
nsStandardURL::BuildNormalizedSpec(const char * 0x0012b5e4) line 291 + 34 bytes
nsStandardURL::SetSpec(nsStandardURL * const 0x04617e88, const char *
0x0012b5e4) line 709 + 12 bytes
nsStandardURL::Init(nsStandardURL * const 0x04617e8c, unsigned int 0x00000002,
int 0x00000050, const char * 0x0012b5e4, nsIURI * 0x00000000) line 1689 + 20 bytes
nsHttpHandler::NewURI(nsHttpHandler * const 0x015e21d0, const char * 0x0012b5e4,
nsIURI * 0x00000000, nsIURI * * 0x0012bc64) line 1689 + 35 bytes
nsIOService::NewURI(nsIOService * const 0x011375a8, const char * 0x0012b5e4,
nsIURI * 0x00000000, nsIURI * * 0x0012bc64) line 746 + 35 bytes
NS_NewURI(nsIURI * * 0x0012bc64, const char * 0x0012b5e4, nsIURI * 0x00000000,
nsIIOService * 0x011375a8) line 97 + 24 bytes
NS_NewURI(nsIURI * * 0x0012bc64, const nsAString & {...}, nsIURI * 0x00000000,
nsIIOService * 0x00000000) line 109 + 27 bytes
nsDefaultURIFixup::CreateFixupURI(nsDefaultURIFixup * const 0x02e7efc0, const
unsigned short * 0x0463c818, unsigned int 0x00000001, nsIURI * * 0x0012bc64)
line 158 + 20 bytes
nsDocShell::CreateFixupURI(nsDocShell * const 0x03a78330, const unsigned short *
0x0463c818, nsIURI * * 0x0012bc64) line 4177 + 48 bytes
nsDocShell::LoadURI(nsDocShell * const 0x03a78340, const unsigned short *
0x0463c818, unsigned int 0x00000000, nsIURI * 0x00000000, nsIInputStream *
0x00000000, nsIInputStream * 0x00000000) line 2248 + 50 bytes
XPTC_InvokeByIndex(nsISupports * 0x03a78340, unsigned int 0x00000008, unsigned
int 0x00000005, nsXPTCVariant * 0x0012bde8) line 106
XPCWrappedNative::CallMethod(XPCCallContext & {...}, XPCWrappedNative::CallMode
CALL_METHOD) line 2009 + 42 bytes
XPC_WN_CallMethod(JSContext * 0x02e5f638, JSObject * 0x03d92bd0, unsigned int
0x00000005, long * 0x03d9b970, long * 0x0012c0bc) line 1266 + 14 bytes
js_Invoke(JSContext * 0x02e5f638, unsigned int 0x00000005, unsigned int
0x00000000) line 832 + 23 bytes
js_Interpret(JSContext * 0x02e5f638, long * 0x0012ceac) line 2798 + 15 bytes
js_Invoke(JSContext * 0x02e5f638, unsigned int 0x00000001, unsigned int
0x00000002) line 849 + 13 bytes
fun_apply(JSContext * 0x02e5f638, JSObject * 0x02fd0a28, unsigned int
0x00000001, long * 0x03d9b7a8, long * 0x0012d024) line 1509 + 15 bytes
js_Invoke(JSContext * 0x02e5f638, unsigned int 0x00000002, unsigned int
0x00000000) line 832 + 23 bytes
js_Interpret(JSContext * 0x02e5f638, long * 0x0012de14) line 2798 + 15 bytes
js_Invoke(JSContext * 0x02e5f638, unsigned int 0x00000001, unsigned int
0x00000002) line 849 + 13 bytes
js_InternalInvoke(JSContext * 0x02e5f638, JSObject * 0x02fd0a28, long
0x03d92420, unsigned int 0x00000000, unsigned int 0x00000001, long * 0x0012e084,
long * 0x0012df3c) line 924 + 20 bytes
JS_CallFunctionValue(JSContext * 0x02e5f638, JSObject * 0x02fd0a28, long
0x03d92420, unsigned int 0x00000001, long * 0x0012e084, long * 0x0012df3c) line
3405 + 31 bytes
nsJSContext::CallEventHandler(nsJSContext * const 0x02ebac38, void * 0x02fd0a28,
void * 0x03d92420, unsigned int 0x00000001, void * 0x0012e084, int * 0x0012e088,
int 0x00000000) line 1011 + 33 bytes
nsJSEventListener::HandleEvent(nsJSEventListener * const 0x046e0108, nsIDOMEvent
* 0x03e66dc8) line 180 + 77 bytes
nsXBLPrototypeHandler::ExecuteHandler(nsXBLPrototypeHandler * const 0x0377bd98,
nsIDOMEventReceiver * 0x030ad7b0, nsIDOMEvent * 0x03e66dc8) line 442
DoKey(nsIAtom * 0x0173f908, nsIXBLPrototypeHandler * 0x0377bd98, nsIDOMEvent *
0x03e66dc8, nsIDOMEventReceiver * 0x030ad7b0) line 108
nsXBLKeyHandler::KeyPress(nsXBLKeyHandler * const 0x039f65f8, nsIDOMEvent *
0x03e66dc8) line 123 + 40 bytes
nsEventListenerManager::HandleEvent(nsEventListenerManager * const 0x030ad820,
nsIPresContext * 0x02e49af0, nsEvent * 0x0012f920, nsIDOMEvent * * 0x0012f43c,
nsIDOMEventTarget * 0x030ad7b0, unsigned int 0x00000004, nsEventStatus *
0x0012f88c) line 1636 + 41 bytes
nsXULElement::HandleDOMEvent(nsXULElement * const 0x030ad7a8, nsIPresContext *
0x02e49af0, nsEvent * 0x0012f920, nsIDOMEvent * * 0x0012f43c, unsigned int
0x00000004, nsEventStatus * 0x0012f88c) line 3359
nsXULElement::HandleDOMEvent(nsXULElement * const 0x03ad3c40, nsIPresContext *
0x02e49af0, nsEvent * 0x0012f920, nsIDOMEvent * * 0x0012f43c, unsigned int
0x00000004, nsEventStatus * 0x0012f88c) line 3340
nsGenericElement::HandleDOMEvent(nsGenericElement * const 0x03a49890,
nsIPresContext * 0x02e49af0, nsEvent * 0x0012f920, nsIDOMEvent * * 0x0012f43c,
unsigned int 0x00000001, nsEventStatus * 0x0012f88c) line 1627
nsHTMLInputElement::HandleDOMEvent(nsHTMLInputElement * const 0x03a49890,
nsIPresContext * 0x02e49af0, nsEvent * 0x0012f920, nsIDOMEvent * * 0x00000000,
unsigned int 0x00000001, nsEventStatus * 0x0012f88c) line 1147 + 29 bytes
PresShell::HandleEventInternal(nsEvent * 0x0012f920, nsIView * 0x02eadde0,
unsigned int 0x00000001, nsEventStatus * 0x0012f88c) line 5986 + 44 bytes
PresShell::HandleEvent(PresShell * const 0x017a15ec, nsIView * 0x02eadde0,
nsGUIEvent * 0x0012f920, nsEventStatus * 0x0012f88c, int 0x00000001, int &
0x00000001) line 5909 + 25 bytes
nsView::HandleEvent(nsView * const 0x02eadde0, nsGUIEvent * 0x0012f920, unsigned
int 0x00000000, nsEventStatus * 0x0012f88c, int 0x00000001, int & 0x00000001)
line 387
nsViewManager::DispatchEvent(nsViewManager * const 0x02eadb60, nsGUIEvent *
0x0012f920, nsEventStatus * 0x0012f88c) line 1930
HandleEvent(nsGUIEvent * 0x0012f920) line 83
nsWindow::DispatchEvent(nsWindow * const 0x02eade8c, nsGUIEvent * 0x0012f920,
nsEventStatus & nsEventStatus_eIgnore) line 850 + 10 bytes
nsWindow::DispatchWindowEvent(nsGUIEvent * 0x0012f920) line 871
nsWindow::DispatchKeyEvent(unsigned int 0x00000083, unsigned short 0x0000,
unsigned int 0x0000000d) line 2600 + 15 bytes
nsWindow::OnChar(unsigned int 0x0000000d, unsigned int 0x0000000d, unsigned char
0x01) line 2733
nsWindow::ProcessMessage(unsigned int 0x00000102, unsigned int 0x0000000d, long
0x001c0001, long * 0x0012fd24) line 3282 + 51 bytes
nsWindow::WindowProc(HWND__ * 0x00800226, unsigned int 0x00000102, unsigned int
0x0000000d, long 0x001c0001) line 1115 + 27 bytes
USER32! 77e148dc()
USER32! 77e14aa7()
USER32! 77e266fd()
nsAppShellService::Run(nsAppShellService * const 0x011395f8) line 303
main1(int 0x00000001, char * * 0x003c75e0, nsISupports * 0x00000000) line 1264 +
32 bytes
main(int 0x00000001, char * * 0x003c75e0) line 1594 + 37 bytes
mainCRTStartup() line 338 + 17 bytes
KERNEL32! 77e992a6()


    
    

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

    
    

    
  
nhotta: thanks for the backtrace... that helps.  however, it's unclear just from
that what is going on... i'll have to step through the code to see exactly where
the hostname part of the URL is getting escaped.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch - fix latest problem (obsolete)
        — 
          — Splinter Review
      

    


  
turns out this regression was caused by my change to the impl of ToLowerCase in
nsURLHelper.cpp.  i had changed the impl to call nsCRT::ToLower, but that
function does not handle UTF8 correctly.  the attached patch reverts
ToLowerCase back to its original impl.

    
    

    
  
nhotta: can i get an r= from you on this?

    
    

    
  
Comment on attachment 64946 [details] [diff] [review]
patch - fix latest problem

>Index: nsURLHelper.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/netwerk/base/src/nsURLHelper.cpp,v
>retrieving revision 1.32
>diff -u -r1.32 nsURLHelper.cpp
>--- nsURLHelper.cpp	22 Dec 2001 09:15:48 -0000	1.32
>+++ nsURLHelper.cpp	15 Jan 2002 01:35:49 -0000
>@@ -185,8 +185,10 @@
> void
> ToLowerCase(char *str, PRInt32 length)
> {
>-    for (; length && *str; --length, ++str)
>-        *str = nsCRT::ToLower(*str);
>+    for (; length && *str; --length, ++str) {

Why imitate that loop control, it seems bogus.	Just test length -- or do you
mean to stop on the first NUL embedded before length chars have been iterated?

Optional: a slightly more-likely-to-be-optimized-better loop is:

    for (char *limit = str + length; str < limit; ++str) {

But it's not a big deal.

>+        if (*str >= 'A' && *str <= 'Z')

Avoid a branch, use ((unsigned)(*str - 'A') <= (unsigned)('Z' - 'A')) instead.

>+            *str -= ('A' - 'a');

This looks odd to me, cuz ('a' > 'A') -- why not *str += 'a' - 'A'; instead?

Fix these and sr=brendan@mozilla.org

/be

        Attachment #64946 -
        Flags: superreview+

    
    

    
  
So, was not really related to escaping?
I applied the patch and typed www.á.com in the URL bar then I see lots of dumps
in the console, can you reproduce that with your build?


    
    

    
  
brendan: ToLowerCase is meant to work if length is passed in as -1, meaning that
the length is unspecified.  in which case, ToLowerCase should proceed to the
first null byte.  of course, it looks like all current clients pass in a valid
length.

nhotta: not related to escaping as far as i could tell.  the hostname was being
corrupted after the call to ToLowerCase.  with my patch, ToLowerCase no longer
alters bytes outside the range 'A' - 'Z'.

when i tried loading www.á.com, mozilla said that it was trying to resolve it,
but then reported Document: done without any message about DNS resolution
failing or otherwise (which seems like a bug)... i figure that without the IDN
converter installed i can't really know for sure that everything is fixed.

oh, and now that i test it again, i see that mozilla is consuming vast amounts
of CPU and memory!!  hrm... i didn't notice that before because i'm using a
MOZ_PROFILE build (w/o any debug console spew).  i'll need to spend some more
time on this to determine why we're getting stuck now.



    
    

    
  
i haven't completely figured it out, but it looks like the URL fixup that occurs
when the browser gets a NS_ERROR_UNKNOWN_HOST is continuously modifying the
hostname in the URL such that the browser keeps retrying with a longer and
longer URL.  i'm looking to see why this is happening....

    
    

    
  
darin, do we need to keep that -1 special case?  If so, never mind.  If not (and
why keep it if no one uses it, and if we can simplify?), follow Thoreau (pun,
sorry).

/be

    
    

    
  
Ok, i discovered the source of the problem.  nsWebShell::EndPageLoad is making
the assumption that the URL spec is ASCII, but in fact the URL spec should
actually needs to be treated as UTF8.

this raises the much bigger question of what is the intended format of the
strings used with nsIURI.

with my patch to nsWebShell.cpp, www.ссс.com now generates the host not found
error, but in the status bar, ic something different... something resembling
www.AiAiAi.com (but not exactly).  this seems to be due to the fact that
nsSocketTransport also assumes the hostname will be ASCII, and uses
ConvertASCIItoUCS2 instead of ConvertUTF8toUCS2 when sending status messages.

so, i can attach some patches that fix up these "broken?" ASCII assumptions.

    
    

    
  


  
this patch fixes 3 problems:

1) fixes ToLowerCase as in the previous patch + brendan's suggestions (-1 was
being used in some cases, so i broke ToLowerCase down into 2 functions).

2) fixes nsWebShell::EndPageLoad to allow for UTF8 encoded URL spec (it's of
course only the hostname part that might be UTF8 encoded).

3) fixes nsSocketTransport::OnStatusXXX to use NS_ConvertUTF8toUCS2 instead of
NS_ConvertASCIItoUCS2 on the hostname.

nhotta, brendan: can i get r/sr= from you guys?

        Attachment #64946 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 64962 [details] [diff] [review]
revised patch for latest problem

Nice, sr=brendan@mozilla.org.

This leads right into bug 14186, where darin and I are ready to rumble!

/be

        Attachment #64962 -
        Flags: superreview+

    
    

    
  
Er, I mean "this leads right into bug 84186...."

/be

    
    

    
  
Comment on attachment 64962 [details] [diff] [review]
revised patch for latest problem

r=nhotta

        Attachment #64962 -
        Flags: review+

    
    

    
  
I downloaded the ACE encode dll from i-dns.net.
http://playground.i-dns.net/mozilla/

IDN from the URL bar worked (although some of their example links are broken).
But HREF in a document does not work, the URL was escaped. After applied the
patch #64946, it was not escaped but still did not work. It might be other
problem. Willam, can you reproduce? I am going to reopen bug 102656, so this bug
can close.




    
  
Keywords: nsbeta1

    
    

    
  
fixed-on-trunk
Status: ASSIGNED → RESOLVED
Closed: 23 years ago → 23 years ago
Resolution: --- → FIXED

    
    

    
  
It's not 100% fixed. Loading a URL like:
http://www.døgnnetto.nu/
now works. But the Location bar becomes:
http://www.døgnnetto.nu/
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

    
    

    
  
actually, it works just fine for me... please file a new bug since you are
probably seeing a different problem.
Status: REOPENED → RESOLVED
Closed: 23 years ago → 23 years ago
Resolution: --- → FIXED

    
    

    
  
verified that this works, tested on Redhat Linux 6.2.

    
    

    
  
verified fixed!
I opened bug 120503 on the int letters that doesn't stick in the URL bar in
2002016 on win2k
Status: RESOLVED → VERIFIED

    
    

    
  


  
unable to go to http://www.døgnnetto.nu/ by clicking link or typing in urlbar.
Attached alert is what i get when clicking the link in bugzilla.

    
    

    
  
weird stuff going on.

On 20020116 on Win2k:

1. If I enter "http://www.døgnnetto.nu" directly into the Location and hit enter
URL i correctly loaded but Location after load shows wrong (bug 120503)

2. If I click on a "http://www.døgnnetto.nu" link inside a HTML page Mozilla
goes to http://www.d%c3%b8gnnetto.nu/. Page loads succesfully but it's not the
same page as "http://www.døgnnetto.nu"

3. If I hold the cursor over a "http://www.d%c3%b8gnnetto.nu/" link inside a
HTML page the status bar says "http://www.døgnnetto.nu/"

Status: VERIFIED → REOPENED
Resolution: FIXED → ---

    
    

    
  
actually, i see the same behavior with a recent nightly build.  but, what i was
trying to accomplish with the latest patch was simply to enable the typing of
IDN in the URL bar.  perhaps it is reasonable then to say that this bug is not
fixed.  however, this bug has already grown far too long IMO.  why don't we open
new bugs for the new problems?  perhaps a meta bug could be opened to track all
of the individual bugs.  henrik: you interested in doing this?

    
    

    
  
it's kind of fixed. I've opened bug 120744 to fix the last issues.
Status: REOPENED → RESOLVED
Closed: 23 years ago → 23 years ago
Resolution: --- → FIXED

    
    

    
  
Comment on attachment 64616 [details] [diff] [review]
patch - remove nsStdEscape call

r=nhotta

This is needed by bug 102656.

        Attachment #64616 -
        Flags: review+

    
    

    
  
Comment on attachment 64616 [details] [diff] [review]
patch - remove nsStdEscape call

sr=brendan@mozilla.org

        Attachment #64616 -
        Flags: superreview+

    
    

    
  
attachment 64616 [details] [diff] [review] has landed.



    
    

    
  
I do not know what work in this bug report now.

First of all, the following urls are not working.
http://www.ietf.org/internet-drafts/draft-ietf-idn-idna-03.txt - not found.
http://info.broker.isi.edu/0/in-drafts/files/draft-skwan-utf8-dns-03.txt -not found.
http://search.ietf.org/internet-drafts/draft-ietf-idn-idna-01.txt - not found
.

After I installed idnXPCOM (installed Win32 release version), 
http://playground.i-dns.net/~wil/xpidn/ 

I tried every samples in 
http://playground.i-dns.net/mozilla/samples/

None of the samples worked in 2002-02-01-10 Win32 trunk build.

Could somebody explain how this is supporsed to work?

    
    

    
  
Sorry, URLs in Japanese should be encoded in UTF-8 to work Mozilla well. I don't
know how to send UTF-8 instead of Shift_JIS, so please check by following procedure.

- Copy URL in Japanese
- Paste it at the address bar
- type ENTER


    
    

    
  
I installed iclient from 
http://www.i-dns.net/support_download/downloads/downloads.html
and install it

Then, I tried the Japanese (Shift_JIS) samples in
http://www.i-dns.net/support_download/samples/index.html

everything goes to http://www.nihongo-domain.com/cgi-bin/main.pl

Is this right?  I tried this by using 2-18-06 trunk build.


    
    

    
  
teroku: I am not sure about that. In fact I don't know if iClient integrates
well with Mozilla. It is done by another team. However, if you want to get
XPIDN http://playground.i-dns.net/mozilla/ is where you should go.

nihongo-domain.com is a registrar, I'm not sure why the pages go there. The URL
could be a catchall script.

    
    

    
  
William, I am sorry in my previous comment.
I did what you mentioned in
http://playground.i-dns.net/mozilla/

Just I did the extra things about iclient.

Again, I tried this again with 2-18-03 trunk commercial and mozilla build.
1. Went to http://playground.i-dns.net/mozilla/download.pet
2. Clicked on the link for click here to begin installation in the section 
"Install the XPIDN package (IDN conversion library for Mozilla)"
It went to http://playground.i-dns.net/mozilla/xpidn/install.html
3. Clicked on the Windows 95/98/NT/ME/2000 (RELEASE) radio button
4. Clicked on Install now button in Step 2 to install XPCOM 
5. Entered the text resource:///res/idnconfig.html in URL bar and say "Yes"
6. Went to http://www.i-dns.net/support_download/samples/index.html
7. I tried several different encoding samples

None of the IDN did worked. I will reopen this since this does not work.





Status: RESOLVED → REOPENED
Resolution: FIXED → ---

    
    

    
  
you need to add this line to your prefs.js file:

user_pref("network.enableIDN", true);

IDN is disabled by default.
Status: REOPENED → RESOLVED
Closed: 23 years ago → 23 years ago
Resolution: --- → FIXED

    
    

    
  
Darin Fisher, thank you for information.

After I put the line in pref.js file, it started working.

However, only two samples for Chinese Traditional (BIG5)
www.stee.com.sg, and www.chinese.dns.com worked fine but other
sample for Big 5 did not work. Also, the sample for TAMAL did not work.

Other sample for other encoding worked. Should every sample in the page
work? 

    
    

    
  
The page is automatically generated by script that tries to make sure that it
is up. But there could be other factors beyond our control.
I will do some more testing about the other URLs and also check the Tamil ones.
Thanks!

N.B. The "idnconfig.html" that comes with XPIDN is supposed to automatically 
turn on "network.enableIDN" but apparently the javascript is broken now because
I can no longer call 
Components.manager.autoRegister(1, null);
Thus, manual intervention is necessary. I will fix this.

Thanks for trying, Teroku!

    
    

    
  
William Tan,

I tested this in 4-08 trunk build. Some of the sample did not work.
Do you have list of the sample should work?  Do I still need to put the pref.js
user_pref("network.enableIDN", true); ?

Thanks,
Teruko

    
    

    
  
wil@dready.org = william.tan@i-dns.net

Teruko, the face that some samples worked means that the pref is already 
enabled.
i-DNS samples list is dynamically generated everyday, so there is no guarantee
that they would work.
What is your QA plan?

    
    

    
  
s/face/fact

    
    

    
  
William Tan,

We would like to have some expected result when we go to the sample page.  
Which should work and how should work?

Thanks,
Teruko



    
  
QA Contact: teruko → ruixu

    
    

    
  
Teruko: I am no longer working for i-DNS and the XPIDN web page seems to be down.
I think the samples are real life sites which may be down due to network problems,
and when it happens it's difficult to pinpoint. It is possible to create a 
controlled environment for testing purposes, but that would require some
resources.

    
    

    
  
The IDN support in Mozilla 1.0 broken.
Apparently, nsHttpHandler no longer takes care of IDN hostnames and
nsDnsService gets the %xx version of the hostname.

    
    

    
  
One thing I found was that it is still needed to put "network.enableIDN" as true
in prefs.js.

    
    

    
  
ok, i see.
because nsStandardURL only installs the pref observer, but does not read the
preference initially until the per-user preference is read, am I right?


    
    

    
  
Yes, I think we can change it to try to load the IDN Dll at start up. Is we want
to disabled somehow then the pref can be set.

    
    

    
  
loading http://www.døgnnetto.nu produces the wrong page...!
check with ie and then with mozilla. two different pages are loaded. Mozilla
"talks" to the wrong server.
I've opened bug 164205 on the issue

    
  
QA Contact: ruixu → ylong

    
    

    
  
The URL http://www.brüel.com is not working. The URL is not resolved in the DNS
using Firebird 0.7.

    
    

    
  
Does iDNS support Korean site as well?
Please see  https://bugzilla.mozilla.org/show_bug.cgi?id=297270

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: