Assign external sizes to external strings and to external typed data created by the VM.

runtime/vm/object.cc

Index: runtime/vm/object.cc
diff --git a/runtime/vm/object.cc b/runtime/vm/object.cc
index 3ad9de1e8beb6c646a19597c1de7316e0e7cf5af..ab2f4b2cecbc654b8d25e2009293d1ab0278c4bd 100644
--- a/runtime/vm/object.cc
+++ b/runtime/vm/object.cc
@@ -8313,7 +8313,7 @@ RawTokenStream* TokenStream::New(intptr_t len) {
       zone,
       ExternalTypedData::New(kExternalTypedDataUint8ArrayCid,
                              data, len, Heap::kOld));
-  stream.AddFinalizer(data, DataFinalizer);
+  stream.AddFinalizer(data, DataFinalizer, len);
   const TokenStream& result = TokenStream::Handle(zone, TokenStream::New());
   result.SetStream(stream);
   return result.raw();
@@ -8506,7 +8506,8 @@ RawTokenStream* TokenStream::New(const String& source,
       zone,
       ExternalTypedData::New(kExternalTypedDataUint8ArrayCid,
                              data.GetStream(), data.Length(), Heap::kOld));
-  stream.AddFinalizer(data.GetStream(), DataFinalizer);
+  intptr_t external_size = data.Length();
+  stream.AddFinalizer(data.GetStream(), DataFinalizer, external_size);

[siva, 2016/08/29 23:03:45]

In this case we are not allocating a new buffer but instead just pointing to the
stream in the snapshot, shouldn't external_size be 0 in this case.

[rmacnak, 2016/08/30 20:49:17]

DataFinalizer here does a free, so this can't be pointing into the snapshot.

This stream was allocated by CompressedTokenStreamData::realloc.

The one that points into the snapshot is from clustered_snapshot.cc

   const TokenStream& result = TokenStream::Handle(zone, New());
   result.SetPrivateKey(private_key);
   {
@@ -20437,11 +20438,10 @@ void String::ToUTF8(uint8_t* utf8_array, intptr_t array_len) const {
 static FinalizablePersistentHandle* AddFinalizer(
     const Object& referent,
     void* peer,
-    Dart_WeakPersistentHandleFinalizer callback) {
+    Dart_WeakPersistentHandleFinalizer callback,
+    intptr_t external_size) {
   ASSERT((callback != NULL && peer != NULL) ||
          (callback == NULL && peer == NULL));
-  // TODO(19482): Make API consistent for external size of strings/typed data.
-  const intptr_t external_size = 0;
   return FinalizablePersistentHandle::New(Isolate::Current(),
                                           referent,
                                           peer,
@@ -20457,6 +20457,7 @@ RawString* String::MakeExternal(void* array,
   ASSERT(FLAG_support_externalizable_strings);
   String& result = String::Handle();
   void* external_data;
+  intptr_t external_size;
   Dart_WeakPersistentHandleFinalizer finalizer;
   {
     NoSafepointScope no_safepoint;
@@ -20500,6 +20501,7 @@ RawString* String::MakeExternal(void* array,
              (result.Hash() == String::Hash(ext_array, str_length)));
       ExternalOneByteString::SetExternalData(result, ext_data);
       external_data = ext_data;
+      external_size = str_length;

[siva, 2016/08/29 23:03:45]

Shouldn't external_size just be 'length' as that is the size passed in from the
external call and it can potentially be larger than str_length.

[rmacnak, 2016/08/30 20:49:17]

Yes. Renamed external_size here and external_allocation_size in the
Dart_MakeStringExternal for consistency with Dart_NewWeakPersistentHandle

       finalizer = ExternalOneByteString::Finalize;
     } else {
       ASSERT(class_id == kTwoByteStringCid);
@@ -20538,10 +20540,11 @@ RawString* String::MakeExternal(void* array,
              (result.Hash() == String::Hash(ext_array, str_length)));
       ExternalTwoByteString::SetExternalData(result, ext_data);
       external_data = ext_data;
+      external_size = str_length * 2;
       finalizer = ExternalTwoByteString::Finalize;
     }
   }  // NoSafepointScope
-  AddFinalizer(result, external_data, finalizer);
+  AddFinalizer(result, external_data, finalizer, external_size);
   return this->raw();
 }
 
@@ -20996,7 +20999,8 @@ void OneByteString::SetPeer(const String& str,
   ASSERT(peer != NULL);
   ExternalStringData<uint8_t>* ext_data =
       new ExternalStringData<uint8_t>(NULL, peer, cback);
-  AddFinalizer(str, ext_data, OneByteString::Finalize);
+  intptr_t external_size = str.Length();

[siva, 2016/08/29 23:03:45]

For the same reason I am wondering if OneByteString::SetPeer and
TwoByteString::SetPeer should have a length argument that we pass in from the
API call implementation.

[rmacnak, 2016/08/30 20:49:17]

Pushed through from Dart_MakeExternalString.

+  AddFinalizer(str, ext_data, OneByteString::Finalize, external_size);
   Isolate::Current()->heap()->SetPeer(str.raw(), peer);
 }
 
@@ -21207,7 +21211,8 @@ void TwoByteString::SetPeer(const String& str,
   ASSERT(peer != NULL);
   ExternalStringData<uint16_t>* ext_data =
       new ExternalStringData<uint16_t>(NULL, peer, cback);
-  AddFinalizer(str, ext_data, TwoByteString::Finalize);
+  intptr_t external_size = str.Length() * 2;
+  AddFinalizer(str, ext_data, TwoByteString::Finalize, external_size);
   Isolate::Current()->heap()->SetPeer(str.raw(), peer);
 }
 
@@ -21245,7 +21250,9 @@ RawExternalOneByteString* ExternalOneByteString::New(
     result.SetHash(0);
     SetExternalData(result, external_data);
   }
-  AddFinalizer(result, external_data, ExternalOneByteString::Finalize);
+  intptr_t external_size = len;
+  AddFinalizer(result, external_data, ExternalOneByteString::Finalize,
+               external_size);
   return ExternalOneByteString::raw(result);
 }
 
@@ -21283,7 +21290,9 @@ RawExternalTwoByteString* ExternalTwoByteString::New(
     result.SetHash(0);
     SetExternalData(result, external_data);
   }
-  AddFinalizer(result, external_data, ExternalTwoByteString::Finalize);
+  intptr_t external_size = len * 2;
+  AddFinalizer(result, external_data, ExternalTwoByteString::Finalize,
+               external_size);
   return ExternalTwoByteString::raw(result);
 }
 
@@ -22077,8 +22086,10 @@ const char* TypedData::ToCString() const {
 
 
 FinalizablePersistentHandle* ExternalTypedData::AddFinalizer(
-    void* peer, Dart_WeakPersistentHandleFinalizer callback) const {
-  return dart::AddFinalizer(*this, peer, callback);
+    void* peer,
+    Dart_WeakPersistentHandleFinalizer callback,
+    intptr_t external_size) const {
+  return dart::AddFinalizer(*this, peer, callback, external_size);
 }