The current IDN code has two shortcomings which are preventing me from cleanly integrating the new script-mixing filter and UseSTDASCII enforcement code. The problems are: * isOnlySafeChars() is being called within core IDN code, when, since it relates to display issues only, it should instead _only_ be called in one place, within nsStandardURL::UTF8toDisplayIDN -- however, the result of this change will need to be checked carefully * the DNS resolver code, both sync and async, does not appear to correctly handle the failure of ConvertUTF8ToACE, the failure of which should abort DNS lookups as per RFC, rather than (apparently) letting the lookup continue with the unconverted result * it is unclear what happens in nsStandardURL::UTF8toDisplayIDN in the case where ConvertUTF8ToACE fails, as Normalise will therefore fail, causing the code to try to return the result of ConvertUTF8ToACE, which fails, causing nsStandardURL::UTF8toDisplayIDN itself to fail: what happens next? * in general, error handling in all of the calling points of ConvertUTF8ToACE needs to be examined * the logging of failures of ConvertUTF8ToACE should be removed: this should never be a "cannot happen" condition... (consider, for example, overlength IDN labels) Needless to say, these changes should be made with as little alteration to the rest of the IDN code as possible.

Notes: * isOnlySafeChars is called from exactly two places; in nsIDNService::Normalize() in nsIDNService::decodeACE(). It probably shouldn't be in either. On the other hand, it probably _should_ be called from nsStandardURL::UTF8toDisplayIDN(). * ConvertUTF8ToACE() is called in 7 places; in nsStandardURL::UTF8toDisplayIDN() [2 occurrences] in nsIDNService::Normalize() in nsStandardURL::UTF8toDisplayIDN() in nsIDNService::decodeACE() in nsDNSService::AsyncResolve() in nsDNSService::Resolve() and also within the test program in TestIDN.cpp All of these calls need to be reviewed for error handling behaviour.

Further up the call tree: nsStandardURL::UTF8toDisplayIDN() is called in only one place, in nsStandardURL::NormalizeIDN(), which is in turn called: in nsStandardURL::BuildNormalizedSpec() in nsStandardURL::SetHost() Comments: * If NormalizeIDN() fails in either call case, the host string falls back to remaining as the raw input string. This is... suboptimal.

Comment on attachment 8667751 [details] [diff] [review] Patch v.1 Review of attachment 8667751 [details] [diff] [review]: ----------------------------------------------------------------- rs=mcmanus