Add support for surrogates when serializing and deserializing for native ports

runtime/vm/dart_api_message.cc

Index: runtime/vm/dart_api_message.cc
diff --git a/runtime/vm/dart_api_message.cc b/runtime/vm/dart_api_message.cc
index 4cee392ee75564f19ef8bdd909d8116b9ed76e27..bd111ba91207b5e38afad3e4c9d6264d10b8d08c 100644
--- a/runtime/vm/dart_api_message.cc
+++ b/runtime/vm/dart_api_message.cc
@@ -389,17 +389,21 @@ Dart_CObject* ApiMessageReader::ReadInternalVMObject(intptr_t class_id,
       uint16_t *utf16 =
           reinterpret_cast<uint16_t*>(::malloc(len * sizeof(uint16_t)));
       intptr_t utf8_len = 0;
+      // Read all the UTF-16 code units.
       for (intptr_t i = 0; i < len; i++) {
         utf16[i] = Read<uint16_t>();
-        // TODO(sgjesse): Check for surrogate pairs.
-        utf8_len += Utf8::Length(utf16[i]);
+      }
+      // Calculate the UTF-8 length.
+      Utf16::CodePointIterator it(utf16, len);

[siva, 2012/11/27 03:00:25]

If you get invalid characters here it.Next() could
potentially return false right at the first character
and we would end up with utf8_len being 0.

Should that be reported as an error as just silently
dropped and an empty string returned like it is being
done now?

[Søren Gjesse, 2012/11/27 11:35:54]

There are no invalid characters in an UTF-16 sequence. So that cannot happen.
Added test to unicode_test.cc.

+      while (it.Next()) {
+        utf8_len += Utf8::Length(it.Current());
       }
       Dart_CObject* object = AllocateDartCObjectString(utf8_len);
       AddBackRef(object_id, object, kIsDeserialized);
       char* p = object->value.as_string;
-      for (intptr_t i = 0; i < len; i++) {
-        // TODO(sgjesse): Check for surrogate pairs.
-        p += Utf8::Encode(utf16[i], p);
+      Utf16::CodePointIterator it2(utf16, len);

[siva, 2012/11/27 03:00:25]

Would it make sense to have a reset method on the iterator
instead so that you can start iterating again on the same iterator?

[Søren Gjesse, 2012/11/27 11:35:54]

Good point added Reset() here and for String::CodePointIterator as well.

+      while (it2.Next()) {
+        p += Utf8::Encode(it2.Current(), p);
       }
       *p = '\0';
       ASSERT(p == (object->value.as_string + utf8_len));
@@ -788,7 +792,7 @@ bool ApiMessageWriter::WriteCObjectInlined(Dart_CObject* object,
       const uint8_t* utf8_str =
           reinterpret_cast<const uint8_t*>(object->value.as_string);
       intptr_t utf8_len = strlen(object->value.as_string);
-      if (!Utf8::IsValid(utf8_str, utf8_len)) {
+      if (!Utf8::IsValidAllowSurrogates(utf8_str, utf8_len)) {

[siva, 2012/11/27 03:00:25]

I am not sure I understand the need for this to be
IsValidAllowSurrogates and not just IsValid?

Is it because you might have read a partial utf8 string and are expecting more?

[Søren Gjesse, 2012/11/27 11:35:54]

The current Utf8::IsValid does not allow for Utf8 encoded Utf16 surrogate code
points. That is the 3-byte Utf8 encodings of the code point range d800 - dbff
and dc00 - dfff are not allowed. However as the Utf16 two byte strings posted
can contain these code points the Utf8 strings in the Dart_CObject structures
can contain 3-byte Utf8 encodings of Utf16 surrogate code points. We need to
allow sending the same data as can be received.

Maybe we should just make IsValid allow surrogate code points in all cases.
Currently you cannot create all the strings using the Dart API
Dart_NewStringFromUTF8 that you can using String.fromCharCodes inside Dart (e.g.
String.fromCharCodes([0xd800])).

[siva, 2012/11/28 03:28:23]

I was under the impression that we allow for Utf8 encoded Utf16 surrogate code
points, at least that was the intention.

This might be a bug, I should probably write a test case
to verify this. I would prefer if we did not have to distinguish between IsValid
and IsValidAllowSurrogates.
Similarly Utf8::DecodeToUTF16 should allow surrogates and
not have to distinguish between the two.

For instance if you look at  Utf8::DecodeToUTF16 it tries
to deal with supplementary characters. The fact that
Utf8::Decode doesn't deal with it correctly is a bug.

I think we should remove this distinction, fix Utf8::ISValid and Utf8::Decode,
then this CL would be good to go. What do you think?

On 2012/11/27 11:35:54, Søren Gjesse wrote:

         return false;
       }
 
@@ -807,7 +811,11 @@ bool ApiMessageWriter::WriteCObjectInlined(Dart_CObject* object,
       if (type == Utf8::kLatin1) {
         uint8_t* latin1_str =
             reinterpret_cast<uint8_t*>(::malloc(len * sizeof(uint8_t)));
-        Utf8::DecodeToLatin1(utf8_str, utf8_len, latin1_str, len);
+        bool success = Utf8::DecodeToLatin1(utf8_str,
+                                            utf8_len,
+                                            latin1_str,
+                                            len);
+        ASSERT(success);
         for (intptr_t i = 0; i < len; i++) {
           Write<uint8_t>(latin1_str[i]);
         }
@@ -816,7 +824,11 @@ bool ApiMessageWriter::WriteCObjectInlined(Dart_CObject* object,
         // TODO(sgjesse): Make sure surrogate pairs are handled.
         uint16_t* utf16_str =
             reinterpret_cast<uint16_t*>(::malloc(len * sizeof(uint16_t)));
-        Utf8::DecodeToUTF16(utf8_str, utf8_len, utf16_str, len);
+        bool success = Utf8::DecodeToUTF16AllowSurrogates(utf8_str,
+                                                          utf8_len,
+                                                          utf16_str,
+                                                          len);
+        ASSERT(success);
         for (intptr_t i = 0; i < len; i++) {
           Write<uint16_t>(utf16_str[i]);
         }