196717 - Decode ACE to UTF-8

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[nhottanscp]

Decoding ACE to UTF-8, nsIIDNService::ConvertACEtoUTF8 is not implemented.

darin, what kind of situation we get ACE from server?

Comment 1

[Darin Fisher]

lot's of cases...

 - HTTP redirect
 - cookies
 - HTML content

web content in general can rightfully contain ACE encoded hostnames. 
nsStandardURL simply needs to always check the hostname to see if it matches an
ACE encoding.  seems to me that nsIIDNService should have a method to check if a
string is ACE encoded or not.  because if not, then it is cheaper to avoid the
buffer copy implicit in nsIIDNService::ConvertACEtoUTF8.

Comment 2

[nhottanscp]

yes, we can check the prefix "xn--"

Comment 3

[nhottanscp]

Attachment: Implements ACE to UTF-8 and modified the test program.

Comment 4

[Shanjian Li]

Comment on attachment 117922 [details] [diff] [review]
Implements ACE to UTF-8 and modified the test program.

looks fine.

Comment 5

[Darin Fisher]

Comment on attachment 117922 [details] [diff] [review]
Implements ACE to UTF-8 and modified the test program.

>Index: dns/src/nsIDNService.cpp

>+  if (!IsASCII(input)) {
>+    _retval.Assign(input);
>+    return NS_OK;
>+  }

hmm... i'm not sure we should be checking this here.  it seems like
added overhead that shouldn't be needed.  it should be enough to just
scan the first bytes for the ACE prefix.

remember: this function is going tobe called a lot, so it is worth it
to spend the time to carefully optimize it.


>+  // RFC 3490 - 4.2 ToUnicode
>+  // The ToUnicode output never contains more code points than its input.
>+  PRInt32 prefixLen = strlen(mACEPrefix);

strlen(mACEPrefix) == 4 ... why not just hardcode the value?  or
use a #define at least.  strlen is overkill.


>+                                                PromiseFlatCString(
>+                                                  Substring(
>+                                                            in, 
>+                                                            prefixLen, 
>+                                                            in.Length() - prefixLen)).get(),

isn't this equivalent to:

  in.get() + 4

provided you declare |in| to be |const nsAFlatCString&| or |const nsCString&|.
PromiseFlatCString is expensive here since it will heap allocate and copy the
input string.  avoid if possible.


>+  // UCS4 -> UTF8
>+  output[output_length] = 0;
>+  nsAutoString utf16;
>+  ucs4toUtf16(output, utf16);
>+  delete [] output;
>+  out.Assign(NS_ConvertUCS2toUTF8(utf16));

seems like there should be a common routine for this conversion someplace.
are you duplicating code that could be factored into a common subroutine?


>+  // Validation: encode back to ACE and compare the strings

ouch!  is this strictly required?  i guess so, but it is going to make
the conversion from ACE to UTF-8 much more expensive.  can't we just
assume the input is valid?  can this validation step be DEBUG only?

Comment 6

[nhottanscp]

>+  if (!IsASCII(input)) {
>+    _retval.Assign(input);
>+    return NS_OK;
>+  }
This is based on RFC 3490 4.2.1. The spec seems to be allowing non-Ascii (4.2.2)
which we do not support but we need to check that case.
The prefix check is required after the Ascii check, I forgot to put that but I
need to add the prefix check.
I think the caller may call IsACE() before calling ConvertACEtoUTF8() for
performance.

>+  PRInt32 prefixLen = strlen(mACEPrefix);
This is because the prefix is also specified by pref. But since we only support
"xn--" and "bq--" anyway, we can change it to a constant. I will do the change.

>are you duplicating code that could be factored into a common subroutine?
No, that is specific to that code. There is other code which converts UCS4 to
UTF-16 but not to UTF-8.

>+  // Validation: encode back to ACE and compare the strings
Yes, that is required (RFC 3490 4.2.6-7). I think it is better to follow the
spec at least for now since we may get invalid domain names and it is safter to
do the required check.

Comment 7

[nhottanscp]

>I think the caller may call IsACE() before calling ConvertACEtoUTF8() 
>for performance.
But the case like www.xn--ENCODED.com, IsACE() returns false for that. 
I think IsACE() has to change to search for "xn--" not only for the beginning of
the input string. (but I have no idea how to do that for nsACString).

Comment 8

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

http://lxr.mozilla.org/seamonkey/source/string/public/nsReadableUtils.h#201

Comment 9

[Darin Fisher]

>+  PRInt32 prefixLen = strlen(mACEPrefix);
This is because the prefix is also specified by pref. But since we only support
"xn--" and "bq--" anyway, we can change it to a constant. I will do the change.

remember you made IsACE hardcode 4.  if it is variable, then you should use
strlen.  i didn't realize it was pref controlled.

Comment 10

[nhottanscp]

That was my mistake using 4 there. But we can make it constant now since "xn--"
is official and we only support "xn--" and "bq--".

Comment 11

[nhottanscp]

Attachment: Use constant value for prefix length, changed IsACE to handle entire domain name as an input.

Comment 12

[Darin Fisher]

does our pref checking code ensure that mACEPrefix will always be 4 characters
in length?

Comment 13

[nhottanscp]

yes, the code checks the length
http://lxr.mozilla.org/seamonkey/source/netwerk/dns/src/nsIDNService.cpp#89
 89       rv = prefBranch->GetCharPref("network.IDN_prefix", getter_Copies(prefix));
 90       if (NS_SUCCEEDED(rv) && 
 91         prefix.Length() <= kACEPrefixLen) {
 92         strncpy(mACEPrefix, prefix.get(), kACEPrefixLen);
 93         mACEPrefix[sizeof(mACEPrefix)-1] = '\0';
 94       }

Comment 14

[Darin Fisher]

Comment on attachment 118341 [details] [diff] [review]
Use constant value for prefix length, changed IsACE to handle entire domain name as an input.

>Index: dns/src/nsIDNService.cpp

might be good to move decl of nsDependentCString(mACEPrefix, ...) out:

    nsDependentCString prefix(mACEPrefix, kACEPrefixLen);

then you won't have to instantiate it twice.


>+  // also check for the case like "www.xn--ENCODED.com"
>+  // in case for this is called for an entire domain name

what about "aaa.bbb.xn--bar.com"?


nothing show-stopping...  the code has a lot of buffer copies,
but i don't see a quick way to eliminate those.

Comment 15

[nhottanscp]

>what about "aaa.bbb.xn--bar.com"?
that is also taken care by searching for "." + prefix

Comment 16

[nhottanscp]

Attachment: nsDependentCString change by darin's suggestion.

Comment 17

[Darin Fisher]

Comment on attachment 118442 [details] [diff] [review]
nsDependentCString change by darin's suggestion.

sr=darin (ok)

Comment 18

[nhottanscp]

checked in to the trunk

darin, who should call this decode function, shall I file a separate bug for that?

Comment 19

[Darin Fisher]

nsStandardURL will need to call this decode function.  yes, a separate bug would
be best.

Comment 20

[nhottanscp]

filed bug 199229 for the caller side implementation