Uses a finalizer to delete zlib filter resources

runtime/bin/filter.cc

Index: runtime/bin/filter.cc
diff --git a/runtime/bin/filter.cc b/runtime/bin/filter.cc
index 5e87f62a599d6f17d4727760511fba1cb19f04e0..ef3683eca97f17ccebce674b26dfaf7a9f4871a5 100644
--- a/runtime/bin/filter.cc
+++ b/runtime/bin/filter.cc
@@ -28,12 +28,8 @@ static Filter* GetFilter(Dart_Handle filter_obj) {
   return filter;
 }
 
-static void EndFilter(Dart_Handle filter_obj, Filter* filter) {
-  Filter::SetFilterPointerNativeField(filter_obj, NULL);
-  delete filter;
-}
 
-static uint8_t* copyDictionary(Dart_Handle dictionary_obj) {
+static uint8_t* CopyDictionary(Dart_Handle dictionary_obj) {
   uint8_t* src = NULL;
   intptr_t size;
   Dart_TypedData_Type type;
@@ -44,11 +40,7 @@ static uint8_t* copyDictionary(Dart_Handle dictionary_obj) {
   }
 
   uint8_t* dictionary = new uint8_t[size];
-
-  if (dictionary == NULL) {
-    Dart_ThrowException(DartUtils::NewInternalError(
-        "Failed to allocate buffer for the zlib dictionary"));
-  }
+  ASSERT(dictionary != NULL);

[Cutch, 2016/03/10 17:09:59]

Are we sure we want to move from throwing an exception to ASSERT (which won't do
anything in a released SDK)?

[zra, 2016/03/10 18:48:09]

I don't think we have consistent practices around this question. Using DartUtils
to throw an exception is probably wrong though. Changed to return NULL here, and
to directly call Dart_PropagateError(Dart_NewApiError(...)) at call sites, which
is also not great, but doesn't rely on DartUtils checking for error handles.

 
   Dart_Handle result = Dart_TypedDataAcquireData(
       dictionary_obj, &type, reinterpret_cast<void**>(&src), &size);
@@ -66,6 +58,7 @@ static uint8_t* copyDictionary(Dart_Handle dictionary_obj) {
   return dictionary;
 }
 
+
 void FUNCTION_NAME(Filter_CreateZLibInflate)(Dart_NativeArguments args) {
   Dart_Handle filter_obj = Dart_GetNativeArgument(args, 0);
   Dart_Handle window_bits_obj = Dart_GetNativeArgument(args, 1);
@@ -74,11 +67,10 @@ void FUNCTION_NAME(Filter_CreateZLibInflate)(Dart_NativeArguments args) {
   uint8_t* dictionary = NULL;
   intptr_t dictionary_length = 0;
   if (!Dart_IsNull(dict_obj)) {
-    dictionary = copyDictionary(dict_obj);
-    if (dictionary != NULL) {
-      dictionary_length = 0;
-      Dart_ListLength(dict_obj, &dictionary_length);
-    }
+    dictionary = CopyDictionary(dict_obj);
+    ASSERT(dictionary != NULL);
+    dictionary_length = 0;
+    Dart_ListLength(dict_obj, &dictionary_length);
   }
   Dart_Handle raw_obj = Dart_GetNativeArgument(args, 3);
   bool raw;
@@ -86,20 +78,23 @@ void FUNCTION_NAME(Filter_CreateZLibInflate)(Dart_NativeArguments args) {
     Dart_ThrowException(DartUtils::NewInternalError(
         "Failed to get 'raw' parameter"));
   }
-  Filter* filter = new ZLibInflateFilter(static_cast<int32_t>(window_bits),
-                                         dictionary, dictionary_length, raw);
+  ZLibInflateFilter* filter = new ZLibInflateFilter(
+      static_cast<int32_t>(window_bits), dictionary, dictionary_length, raw);
+  ASSERT(filter != NULL);
   if (!filter->Init()) {
     delete filter;
     Dart_ThrowException(DartUtils::NewInternalError(
         "Failed to create ZLibInflateFilter"));
   }
-  Dart_Handle result = Filter::SetFilterPointerNativeField(filter_obj, filter);
+  Dart_Handle result = Filter::SetFilterPointerNativeField(
+      filter_obj, filter, sizeof(*filter) + dictionary_length);
   if (Dart_IsError(result)) {
     delete filter;
     Dart_PropagateError(result);
   }
 }
 
+
 void FUNCTION_NAME(Filter_CreateZLibDeflate)(Dart_NativeArguments args) {
   Dart_Handle filter_obj = Dart_GetNativeArgument(args, 0);
   Dart_Handle gzip_obj = Dart_GetNativeArgument(args, 1);
@@ -117,31 +112,35 @@ void FUNCTION_NAME(Filter_CreateZLibDeflate)(Dart_NativeArguments args) {
   uint8_t* dictionary = NULL;
   intptr_t dictionary_length = 0;
   if (!Dart_IsNull(dict_obj)) {
-    dictionary = copyDictionary(dict_obj);
-    if (dictionary != NULL) {
-      dictionary_length = 0;
-      Dart_ListLength(dict_obj, &dictionary_length);
-    }
+    dictionary = CopyDictionary(dict_obj);
+    ASSERT(dictionary != NULL);
+    dictionary_length = 0;
+    Dart_ListLength(dict_obj, &dictionary_length);
   }
   Dart_Handle raw_obj = Dart_GetNativeArgument(args, 7);
   bool raw = DartUtils::GetBooleanValue(raw_obj);
-  Filter* filter = new ZLibDeflateFilter(gzip, static_cast<int32_t>(level),
-                                         static_cast<int32_t>(window_bits),
-                                         static_cast<int32_t>(mem_level),
-                                         static_cast<int32_t>(strategy),
-                                         dictionary, dictionary_length, raw);
+  ZLibDeflateFilter* filter = new ZLibDeflateFilter(
+      gzip,
+      static_cast<int32_t>(level),
+      static_cast<int32_t>(window_bits),
+      static_cast<int32_t>(mem_level),
+      static_cast<int32_t>(strategy),
+      dictionary, dictionary_length, raw);
+  ASSERT(filter != NULL);
   if (!filter->Init()) {
     delete filter;
     Dart_ThrowException(DartUtils::NewInternalError(
         "Failed to create ZLibDeflateFilter"));
   }
-  Dart_Handle result = Filter::SetFilterPointerNativeField(filter_obj, filter);
+  Dart_Handle result = Filter::SetFilterPointerNativeField(
+      filter_obj, filter, sizeof(*filter) + dictionary_length);
   if (Dart_IsError(result)) {
     delete filter;
     Dart_PropagateError(result);
   }
 }
 
+
 void FUNCTION_NAME(Filter_Process)(Dart_NativeArguments args) {
   Dart_Handle filter_obj = Dart_GetNativeArgument(args, 0);
   Filter* filter = GetFilter(filter_obj);
@@ -163,11 +162,7 @@ void FUNCTION_NAME(Filter_Process)(Dart_NativeArguments args) {
           "Invalid argument passed to Filter_Process"));
     }
     uint8_t* zlib_buffer = new uint8_t[chunk_length];
-    if (zlib_buffer == NULL) {
-      Dart_TypedDataReleaseData(data_obj);
-      Dart_ThrowException(DartUtils::NewInternalError(
-          "Failed to allocate buffer for zlib"));
-    }
+    ASSERT(zlib_buffer != NULL);
     memmove(zlib_buffer, buffer + start, chunk_length);
     Dart_TypedDataReleaseData(data_obj);
     buffer = zlib_buffer;
@@ -177,6 +172,7 @@ void FUNCTION_NAME(Filter_Process)(Dart_NativeArguments args) {
           "Failed to get list length"));
     }
     buffer = new uint8_t[chunk_length];
+    ASSERT(buffer != NULL);
     if (Dart_IsError(Dart_ListGetAsBytes(
             data_obj, start, buffer, chunk_length))) {
       delete[] buffer;
@@ -187,7 +183,6 @@ void FUNCTION_NAME(Filter_Process)(Dart_NativeArguments args) {
   // Process will take ownership of buffer, if successful.
   if (!filter->Process(buffer, chunk_length)) {
     delete[] buffer;
-    EndFilter(filter_obj, filter);
     Dart_ThrowException(DartUtils::NewInternalError(
         "Call to Process while still processing data"));
   }
@@ -214,8 +209,6 @@ void FUNCTION_NAME(Filter_Processed)(Dart_NativeArguments args) {
                                     flush,
                                     end);
   if (read < 0) {
-    // Error, end filter.
-    EndFilter(filter_obj, filter);
     Dart_ThrowException(DartUtils::NewInternalError(
         "Filter error, bad data"));
   } else if (read == 0) {
@@ -229,19 +222,30 @@ void FUNCTION_NAME(Filter_Processed)(Dart_NativeArguments args) {
 }
 
 
-void FUNCTION_NAME(Filter_End)(Dart_NativeArguments args) {
-  Dart_Handle filter_obj = Dart_GetNativeArgument(args, 0);
-  Filter* filter = GetFilter(filter_obj);
-  EndFilter(filter_obj, filter);
+static void DeleteFilter(
+    void* isolate_data,
+    Dart_WeakPersistentHandle handle,
+    void* filter_pointer) {
+  Filter* filter = reinterpret_cast<Filter*>(filter_pointer);
+  delete filter;
 }
 
 
 Dart_Handle Filter::SetFilterPointerNativeField(Dart_Handle filter,

[Cutch, 2016/03/10 17:09:59]

Consider renaming this method as it now does a lot more than set the native
field.

[zra, 2016/03/10 18:48:08]

Done.

-                                                Filter* filter_pointer) {
-  return Dart_SetNativeInstanceField(
+                                                Filter* filter_pointer,
+                                                intptr_t size) {
+  Dart_Handle err = Dart_SetNativeInstanceField(
       filter,
       kFilterPointerNativeField,
       reinterpret_cast<intptr_t>(filter_pointer));
+  if (Dart_IsError(err)) {
+    return err;
+  }
+  Dart_NewWeakPersistentHandle(filter,
+                               reinterpret_cast<void*>(filter_pointer),
+                               size,
+                               DeleteFilter);
+  return err;
 }
 
 
@@ -255,9 +259,15 @@ Dart_Handle Filter::GetFilterPointerNativeField(Dart_Handle filter,
 
 
 ZLibDeflateFilter::~ZLibDeflateFilter() {
-  delete[] dictionary_;
-  delete[] current_buffer_;
-  if (initialized()) deflateEnd(&stream_);
+  if (dictionary_ != NULL) {
+    delete[] dictionary_;

[Cutch, 2016/03/10 17:09:59]

delete is null safe. Here and elsewhere.

[zra, 2016/03/10 18:48:08]

Done.

+  }
+  if (current_buffer_ != NULL) {
+    delete[] current_buffer_;
+  }
+  if (initialized()) {
+    deflateEnd(&stream_);
+  }
 }
 
 
@@ -277,7 +287,7 @@ bool ZLibDeflateFilter::Init() {
   if (result != Z_OK) {
     return false;
   }
-  if (dictionary_ != NULL && !gzip_ && !raw_) {
+  if ((dictionary_ != NULL) && !gzip_ && !raw_) {
     result = deflateSetDictionary(&stream_, dictionary_, dictionary_length_);
     delete[] dictionary_;
     dictionary_ = NULL;
@@ -291,7 +301,9 @@ bool ZLibDeflateFilter::Init() {
 
 
 bool ZLibDeflateFilter::Process(uint8_t* data, intptr_t length) {
-  if (current_buffer_ != NULL) return false;
+  if (current_buffer_ != NULL) {
+    return false;
+  }
   stream_.avail_in = length;
   stream_.next_in = current_buffer_ = data;
   return true;
@@ -321,17 +333,25 @@ intptr_t ZLibDeflateFilter::Processed(uint8_t* buffer,
         error = true;
   }
 
-  delete[] current_buffer_;
-  current_buffer_ = NULL;
+  if (current_buffer_ != NULL) {
+    delete[] current_buffer_;
+    current_buffer_ = NULL;
+  }
   // Either 0 Byte processed or error
   return error ? -1 : 0;
 }
 
 
 ZLibInflateFilter::~ZLibInflateFilter() {
-  delete[] dictionary_;
-  delete[] current_buffer_;
-  if (initialized()) inflateEnd(&stream_);
+  if (dictionary_ != NULL) {
+    delete[] dictionary_;
+  }
+  if (current_buffer_ != NULL) {
+    delete[] current_buffer_;
+  }
+  if (initialized()) {
+    inflateEnd(&stream_);
+  }
 }
 
 
@@ -355,7 +375,9 @@ bool ZLibInflateFilter::Init() {
 
 
 bool ZLibInflateFilter::Process(uint8_t* data, intptr_t length) {
-  if (current_buffer_ != NULL) return false;
+  if (current_buffer_ != NULL) {
+    return false;
+  }
   stream_.avail_in = length;
   stream_.next_in = current_buffer_ = data;
   return true;
@@ -405,8 +427,10 @@ intptr_t ZLibInflateFilter::Processed(uint8_t* buffer,
       error = true;
   }
 
-  delete[] current_buffer_;
-  current_buffer_ = NULL;
+  if (current_buffer_ != NULL) {
+    delete[] current_buffer_;
+    current_buffer_ = NULL;
+  }
   // Either 0 Byte processed or error
   return error ? -1 : 0;
 }