Do not normalize into NFC the values of form fields

Do not normalize into NFC the values of form fields

This commit renames the TextEncoding::encode(const String&, UnencodableHandling) const
API function to normalizeAndEncode() to make clear that the function
first applies Unicode NFC normalization before encoding the string. All
existing references to encode() except for one were updated to call
normalizeAndEncode() instead.

TextEncoding::encode(const String&, UnencodableHandling) const is added
back as a new API function which only encodes the string, but does not
normalize the string before encoding.

The one call to the old encode() function that was not updated is in
FormDataList::appendString(const String&). This was left as a call to
the new encode() to fix Issue 117128.

Chrome and Safari are unlike other browsers in that they apply Unicode
NFC normalization to form values when submitting a form; in particular,
the following browsers were tested and found not to normalize the form
values:
- Firefox 22.0
- Firefox ESR 17.0.7
- Opera 12.16
- IE 6
- IE 7
- IE 8
- IE 9
- IE 10
- Amaya 11.4.7

NFC normalization actually changes the meaning of text in certain scripts.
Notably, there are certain Biblical Hebrew words for which normalization
causes the word to be erroneously encoded. One example is given on page 9
of the SBL Hebrew Font User Manual version 1.5:
http://www.sbl-site.org/Fonts/SBLHebrewManualv1.5.pdf
This example is added as the new form-data-encoding-3.html layout test.

BUG=117128
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=154881

[dtrebbien, 2013-07-21 21:03:12]

Hello Kent,

Would you please review this CL for
http://code.google.com/p/chromium/issues/detail?id=117128 ?

If someone else should review, let me know and I can change the reviewers list.

[abarth-chromium, 2013-07-22 00:01:36]

Can you add more information about why we should make this change and how other
browsers behave.

[dtrebbien, 2013-07-22 12:13:37]

On 2013/07/22 00:01:36, abarth wrote:
> Can you add more information about why we should make this change and how
other
> browsers behave.

Sure.

I don't know if you saw it, but I reported some findings in comment 19: 
http://code.google.com/p/chromium/issues/detail?id=117128#c19
I can also test Amaya and Safari.

The original WebKit bug, https://bugs.webkit.org/show_bug.cgi?id=8769 , mentions
http://www.w3.org/TR/charmod-norm/ but this standard now has a prominent warning
at the top in a yellow box:  "This version of this document was published to
indicate the Internationalization Core Working Group's intention to
substantially alter or replace the recommendations found here with very
different recommendations in the near future. Other than this note, this Working
Draft is identical to the draft of 2005-10-27."
I will research to see what additional discussion there has been about this
standard, and how they are thinking about modifying it.

Is there any particular information that you would like (which browsers,
standards, etc.)?

[tkent, 2013-07-22 23:36:23]

On 2013/07/22 12:13:37, dtrebbien wrote:
> On 2013/07/22 00:01:36, abarth wrote:
> > Can you add more information about why we should make this change and how
> other
> > browsers behave.
> 
> Sure.
> 
> I don't know if you saw it, but I reported some findings in comment 19: 
> http://code.google.com/p/chromium/issues/detail?id=117128#c19
> I can also test Amaya and Safari.
> 
> The original WebKit bug, https://bugs.webkit.org/show_bug.cgi?id=8769 ,
mentions
> http://www.w3.org/TR/charmod-norm/ but this standard now has a prominent
warning
> at the top in a yellow box:  "This version of this document was published to
> indicate the Internationalization Core Working Group's intention to
> substantially alter or replace the recommendations found here with very
> different recommendations in the near future. Other than this note, this
Working
> Draft is identical to the draft of 2005-10-27."
> I will research to see what additional discussion there has been about this
> standard, and how they are thinking about modifying it.

This information is very useful, and we had better mention it in the change
description. Also, we had better add other browser's behavior to the
description.

[tkent, 2013-07-22 23:38:08]

https://codereview.chromium.org/19845004/diff/1/Source/wtf/text/TextEncoding.h
File Source/wtf/text/TextEncoding.h (right):

https://codereview.chromium.org/19845004/diff/1/Source/wtf/text/TextEncoding....
Source/wtf/text/TextEncoding.h:76: CString encode(const String&,
UnencodableHandling) const;
IMO, we had better rename the existing encode() to normalizeAndEncode() or
encodeWithNormalization(), and not expose NormalizationMode flag.

[dtrebbien, 2013-07-23 11:41:28]

Amaya 11.4.7 sends input1=a%cc%88&input2=%c3%a4

Safari 6.0.5 normalizes the data just like Chrome, sending
input1=%C3%A4&input2=%C3%A4

[dtrebbien, 2013-07-23 13:39:16]

On 2013/07/22 23:38:08, tkent wrote:
> https://codereview.chromium.org/19845004/diff/1/Source/wtf/text/TextEncoding.h
> File Source/wtf/text/TextEncoding.h (right):
> 
>
https://codereview.chromium.org/19845004/diff/1/Source/wtf/text/TextEncoding....
> Source/wtf/text/TextEncoding.h:76: CString encode(const String&,
> UnencodableHandling) const;
> IMO, we had better rename the existing encode() to normalizeAndEncode() or
> encodeWithNormalization(), and not expose NormalizationMode flag.

I like the normalizeAndEncode() idea.  Plus, I think that it better takes care
of the FIXME comment "FIXME: What's the right place to do normalization?" than
simply extending encode().

One possible problem with this idea is that developers might not realize that
the behavior of encode() has changed and that it no longer normalizes the string
first.  However, there are not very many places where encode() is called, so it
is not heavily used, and it seems unlikely that someone will add a new call to
encode().

Patch Set 2 is a revision that implements the normalizeAndEncode() idea.

[tkent, 2013-07-24 00:05:08]

lgtm.  Thank you!

[commit-bot: I haz the power, 2013-07-24 00:06:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dtrebbien@gmail.com/19845004/9001

[commit-bot: I haz the power, 2013-07-24 00:18:57]

Retried try job too often on blink_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=blink_pres...

[tkent, 2013-07-24 00:23:30]

Ah, this needs a weborigin OWNER.

[abarth-chromium, 2013-07-24 18:20:09]

Thank you for the improved description.  That makes this CL much easier to
review.  Unfortunately, the change has a memory safety bug, which I've noted
below.  Once you fix that bug (by restoring the previous declaration location),
this change LGTM.

https://chromiumcodereview.appspot.com/19845004/diff/9001/Source/wtf/text/Tex...
File Source/wtf/text/TextEncoding.cpp (right):

https://chromiumcodereview.appspot.com/19845004/diff/9001/Source/wtf/text/Tex...
Source/wtf/text/TextEncoding.cpp:105: Vector<UChar> normalizedCharacters;
Moving this declaration inside the if statement causes a memory safety problem. 
Notice on line 115 that we store a pointer to this vector's data in |source|,
but after this change |normalizedCharacters| goes out of scope and the memory is
freed.  That means that |source| can point to garbage memory on line 118.

[dtrebbien, 2013-07-24 23:18:37]

On 2013/07/24 18:20:09, abarth wrote:
> Thank you for the improved description.  That makes this CL much easier to
> review.  Unfortunately, the change has a memory safety bug, which I've noted
> below.  Once you fix that bug (by restoring the previous declaration
location),
> this change LGTM.
> 
>
https://chromiumcodereview.appspot.com/19845004/diff/9001/Source/wtf/text/Tex...
> File Source/wtf/text/TextEncoding.cpp (right):
> 
>
https://chromiumcodereview.appspot.com/19845004/diff/9001/Source/wtf/text/Tex...
> Source/wtf/text/TextEncoding.cpp:105: Vector<UChar> normalizedCharacters;
> Moving this declaration inside the if statement causes a memory safety
problem. 
> Notice on line 115 that we store a pointer to this vector's data in |source|,
> but after this change |normalizedCharacters| goes out of scope and the memory
is
> freed.  That means that |source| can point to garbage memory on line 118.

Oh, wow.  Sorry about that!

Patch Set 3 contains this fix.

[commit-bot: I haz the power, 2013-07-24 23:26:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dtrebbien@gmail.com/19845004/22001

[commit-bot: I haz the power, 2013-07-25 02:16:59]

Change committed as 154881