Add support for surrogates when serializing and deserializing for native ports

runtime/vm/dart_api_message.cc

 // Copyright (c) 2012, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 #include "vm/dart_api_message.h"
 #include "vm/object.h"
 #include "vm/snapshot_ids.h"
 #include "vm/symbols.h"
 #include "vm/unicode.h"
 
@@ skipping 371 matching lines @@
       ::free(latin1);
       return object;
     }
     case kTwoByteStringCid: {
       intptr_t len = ReadSmiValue();
       intptr_t hash = ReadSmiValue();
       USE(hash);
       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));
       ::free(utf16);
       return object;
     }
     case kUint8ArrayCid: {
       intptr_t len = ReadSmiValue();
       Dart_CObject* object = AllocateDartCObjectUint8Array(len);
       AddBackRef(object_id, object, kIsDeserialized);
@@ skipping 368 matching lines @@
       // Write out the class and tags information.
       WriteIndexedObject(kDoubleCid);
       WriteIntptrValue(0);
       // Write double value.
       Write<double>(object->value.as_double);
       break;
     case Dart_CObject::kString: {
       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;
       }
 
       Utf8::Type type;
       intptr_t len = Utf8::CodePointCount(utf8_str, utf8_len, &type);
 
       // Write out the serialization header value for this object.
       WriteInlinedHeader(object);
       // Write out the class and tags information.
       WriteIndexedObject(type == Utf8::kLatin1 ? kOneByteStringCid
                                                : kTwoByteStringCid);
       WriteIntptrValue(0);
       // Write string length, hash and content
       WriteSmi(len);
       WriteSmi(0);  // TODO(sgjesse): Hash - not written.
       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]);
         }
         ::free(latin1_str);
       } else {
         // 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]);
         }
         ::free(utf16_str);
       }
       break;
     }
     case Dart_CObject::kUint8Array: {
       // Write out the serialization header value for this object.
       WriteInlinedHeader(object);
@@ skipping 52 matching lines @@
     if (!success) {
       UnmarkAllCObjects(object);
       return false;
     }
   }
   UnmarkAllCObjects(object);
   return true;
 }
 
 }  // namespace dart