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: