fix: memory leaks & WeakRefs (#111)

rigor789 · darind · brysem · web-flow · commit 431e8167ed5d · 2021-07-03T10:55:08.000-07:00
* fix: don't allocate persistent empty objects * fix: Blocks memory leak (#100) * feat: drop custom WeakRef implementation & use built-in * fix: free allocated memory in ReferenceWrapper * fix: DictionaryAdapter Hanging References (#114) * Clean up hanging references before deallocating Reduces total dictionary related memory leaks by 36% * fix * chore: more leak fixes * fix: only release enumerator_ when set (#117) * fix: handle methods with pointer type params fixes #109 * fix: referenceWrapper memory leak & CString leak * refactor: ReferenceWrapper dispose to be managed internally * fix: pre-allocate memory even for empty values * fix: undo invalid fix (needs a different way) Co-authored-by: Darin Dimitrov <darin.dimitrov@gmail.com> Co-authored-by: Bryse Meijer <brysem@users.noreply.github.com> Co-authored-by: logikgate <nick.fredricks@flypaper.com>
diff --git a/NativeScript/runtime/ArgConverter.mm b/NativeScript/runtime/ArgConverter.mm
@@ -267,6 +267,7 @@
                 id adapter = [[DictionaryAdapter alloc] initWithJSObject:value.As<Object>() isolate:isolate];
                 memset(retValue, 0, sizeof(id));
                 *static_cast<id __strong *>(retValue) = adapter;
+                CFAutorelease(adapter);
                 return;
             }
         } else if (type == BinaryTypeEncodingType::StructDeclarationReference) {
diff --git a/NativeScript/runtime/DataWrapper.h b/NativeScript/runtime/DataWrapper.h
@@ -259,6 +259,17 @@ class ReferenceWrapper: public BaseDataWrapper {
           disposeData_(false) {
     }
 
+    ~ReferenceWrapper() {
+        if(this->value_ != nullptr) {
+            value_->Reset();
+            delete value_;
+        }
+        
+        if (this->data_ != nullptr) {
+            std::free(this->data_);
+        }
+    }
+
     const WrapperType Type() {
         return WrapperType::Reference;
     }
@@ -291,16 +302,12 @@ class ReferenceWrapper: public BaseDataWrapper {
     }
 
     void SetData(void* data, bool disposeData = false) {
-        if (this->data_ != nullptr && data != nullptr && this->disposeData_) {
+        if (this->data_ != nullptr && this->disposeData_) {
             std::free(this->data_);
         }
         this->data_ = data;
         this->disposeData_ = disposeData;
     }
-
-    bool ShouldDisposeData() {
-        return this->disposeData_;
-    }
 private:
     BaseDataWrapper* typeWrapper_;
     v8::Persistent<v8::Value>* value_;
diff --git a/NativeScript/runtime/DictionaryAdapter.mm b/NativeScript/runtime/DictionaryAdapter.mm
@@ -55,6 +55,10 @@ - (id)nextObject {
 }
 
 - (void)dealloc {
+    self->isolate_ = nullptr;
+    self->map_ = nil;
+    self->cache_ = nil;
+    
     [super dealloc];
 }
 
@@ -138,6 +142,10 @@ - (NSArray*)allObjects {
 }
 
 - (void)dealloc {
+    self->isolate_ = nullptr;
+    self->dictionary_ = nil;
+    self->cache_ = nil;
+    
     [super dealloc];
 }
 
@@ -147,6 +155,7 @@ @implementation DictionaryAdapter {
     Isolate* isolate_;
     std::shared_ptr<Persistent<Value>> object_;
     std::shared_ptr<Caches> cache_;
+    NSEnumerator* enumerator_;
 }
 
 - (instancetype)initWithJSObject:(Local<Object>)jsObject isolate:(Isolate*)isolate {
@@ -225,10 +234,14 @@ - (NSEnumerator*)keyEnumerator {
     Local<Value> obj = self->object_->Get(self->isolate_);
 
     if (obj->IsMap()) {
-        return [[DictionaryAdapterMapKeysEnumerator alloc] initWithMap:self->object_ isolate:self->isolate_ cache:self->cache_];
+        self->enumerator_ = [[DictionaryAdapterMapKeysEnumerator alloc] initWithMap:self->object_ isolate:self->isolate_ cache:self->cache_];
+        
+        return self->enumerator_;
     }
 
-    return [[DictionaryAdapterObjectKeysEnumerator alloc] initWithProperties:self->object_ isolate:self->isolate_ cache:self->cache_];
+    self->enumerator_ = [[DictionaryAdapterObjectKeysEnumerator alloc] initWithProperties:self->object_ isolate:self->isolate_ cache:self->cache_];
+    
+    return self->enumerator_;
 }
 
 - (void)dealloc {
@@ -240,6 +253,15 @@ - (void)dealloc {
         delete wrapper;
     }
     self->object_->Reset();
+    self->isolate_ = nullptr;
+    self->cache_ = nil;
+    self->object_ = nil;
+    
+    if (self->enumerator_ != nullptr) {
+        CFAutorelease(self->enumerator_);
+        self->enumerator_ = nullptr;
+    }
+    
     [super dealloc];
 }
 
diff --git a/NativeScript/runtime/Helpers.mm b/NativeScript/runtime/Helpers.mm
@@ -252,6 +252,7 @@
         return Local<Value>();
     }
 
+    v8::Locker locker(isolate);
     Local<Value> result;
     if (!obj->GetPrivate(context, privateKey).ToLocal(&result)) {
         tns::Assert(false, isolate);
diff --git a/NativeScript/runtime/Interop.h b/NativeScript/runtime/Interop.h
@@ -126,6 +126,7 @@ class Interop {
     static id ToObject(v8::Local<v8::Context> context, v8::Local<v8::Value> arg);
     static v8::Local<v8::Value> GetPrimitiveReturnType(v8::Local<v8::Context> context, BinaryTypeEncodingType type, BaseCall* call);
 private:
+    static std::pair<IMP, ffi_closure*> CreateMethodInternal(const uint8_t initialParamIndex, const uint8_t argsCount, const TypeEncoding* typeEncoding, FFIMethodCallback callback, void* userData);
     static CFTypeRef CreateBlock(const uint8_t initialParamIndex, const uint8_t argsCount, const TypeEncoding* typeEncoding, FFIMethodCallback callback, void* userData);
     template <typename T>
     static void SetStructValue(v8::Local<v8::Value> value, void* destBuffer, ptrdiff_t position);
@@ -187,7 +188,8 @@ class Interop {
         const void* invoke;
         JSBlockDescriptor* descriptor;
         void* userData;
-
+        ffi_closure* ffiClosure;
+        
         static JSBlockDescriptor kJSBlockDescriptor;
     } JSBlock;
 };
diff --git a/NativeScript/runtime/Interop.mm b/NativeScript/runtime/Interop.mm
@@ -46,43 +46,53 @@
                     v8::Locker locker(isolate);
                     Isolate::Scope isolate_scope(isolate);
                     HandleScope handle_scope(isolate);
+                    BlockWrapper* blockWrapper = static_cast<BlockWrapper*>(tns::GetValue(isolate, callback));
                     tns::DeleteValue(isolate, callback);
                     wrapper->callback_->Reset();
+                    delete blockWrapper;
                 }
             }
             delete wrapper;
+            ffi_closure_free(block->ffiClosure);
             block->~JSBlock();
         }
     }
 };
 
-IMP Interop::CreateMethod(const uint8_t initialParamIndex, const uint8_t argsCount, const TypeEncoding* typeEncoding, FFIMethodCallback callback, void* userData) {
+std::pair<IMP, ffi_closure*> Interop::CreateMethodInternal(const uint8_t initialParamIndex, const uint8_t argsCount, const TypeEncoding* typeEncoding, FFIMethodCallback callback, void* userData) {
     void* functionPointer;
     ffi_closure* closure = static_cast<ffi_closure*>(ffi_closure_alloc(sizeof(ffi_closure), &functionPointer));
     ParametrizedCall* call = ParametrizedCall::Get(typeEncoding, initialParamIndex, initialParamIndex + argsCount);
     ffi_status status = ffi_prep_closure_loc(closure, call->Cif, callback, userData, functionPointer);
     tns::Assert(status == FFI_OK);
 
-    return (IMP)functionPointer;
+    return std::make_pair((IMP)functionPointer, closure);
+
+}
+
+IMP Interop::CreateMethod(const uint8_t initialParamIndex, const uint8_t argsCount, const TypeEncoding* typeEncoding, FFIMethodCallback callback, void* userData) {
+    std::pair<IMP, ffi_closure*> result = Interop::CreateMethodInternal(initialParamIndex, argsCount, typeEncoding, callback, userData);
+    return result.first;
 }
 
 CFTypeRef Interop::CreateBlock(const uint8_t initialParamIndex, const uint8_t argsCount, const TypeEncoding* typeEncoding, FFIMethodCallback callback, void* userData) {
     JSBlock* blockPointer = reinterpret_cast<JSBlock*>(malloc(sizeof(JSBlock)));
-    void* functionPointer = (void*)CreateMethod(initialParamIndex, argsCount, typeEncoding, callback, userData);
+
+    std::pair<IMP, ffi_closure*> result = Interop::CreateMethodInternal(initialParamIndex, argsCount, typeEncoding, callback, userData);
 
     *blockPointer = {
         .isa = nullptr,
         .flags = JSBlock::BLOCK_HAS_COPY_DISPOSE | JSBlock::BLOCK_NEEDS_FREE | (1 /* ref count */ << 1),
         .reserved = 0,
-        .invoke = functionPointer,
+        .invoke = (void*)result.first,
         .descriptor = &JSBlock::kJSBlockDescriptor,
+        .userData = userData,
+        .ffiClosure = result.second,
     };
 
-    blockPointer->userData = userData;
-
     object_setClass((__bridge id)blockPointer, objc_getClass("__NSMallocBlock__"));
 
-    return blockPointer;
+    return CFAutorelease(blockPointer);
 }
 
 Local<Value> Interop::CallFunction(CMethodCall& methodCall) {
@@ -295,8 +305,10 @@
                     Local<Value> value = referenceWrapper->Value() != nullptr ? referenceWrapper->Value()->Get(isolate) : Local<Value>();
                     ffi_type* ffiType = FFICall::GetArgumentType(innerType);
                     data = calloc(1, ffiType->size);
-                    referenceWrapper->SetData(data);
+
+                    referenceWrapper->SetData(data, true);
                     referenceWrapper->SetEncoding(innerType);
+
                     // Initialize the ref/out parameter value before passing it to the function call
                     if (!value.IsEmpty()) {
                         Interop::WriteValue(context, innerType, data, value);
@@ -350,7 +362,7 @@
         BaseDataWrapper* baseWrapper = tns::GetValue(isolate, arg);
         if (baseWrapper != nullptr && baseWrapper->Type() == WrapperType::Block) {
             BlockWrapper* wrapper = static_cast<BlockWrapper*>(baseWrapper);
-            blockPtr = wrapper->Block();
+            blockPtr = Block_copy(wrapper->Block());
         } else {
             std::shared_ptr<Persistent<Value>> poCallback = std::make_shared<Persistent<Value>>(isolate, arg);
             MethodCallbackWrapper* userData = new MethodCallbackWrapper(isolate, poCallback, 1, argsCount, blockTypeEncoding);
@@ -512,13 +524,16 @@
             Local<ArrayBuffer> buffer = arg.As<ArrayBuffer>();
             NSDataAdapter* adapter = [[NSDataAdapter alloc] initWithJSObject:buffer isolate:isolate];
             Interop::SetValue(dest, adapter);
+            CFAutorelease(adapter);
         } else if (tns::IsArrayOrArrayLike(isolate, obj)) {
             Local<v8::Array> array = Interop::ToArray(obj);
             ArrayAdapter* adapter = [[ArrayAdapter alloc] initWithJSObject:array isolate:isolate];
             Interop::SetValue(dest, adapter);
+            CFAutorelease(adapter);
         } else {
             DictionaryAdapter* adapter = [[DictionaryAdapter alloc] initWithJSObject:obj isolate:isolate];
             Interop::SetValue(dest, adapter);
+            CFAutorelease(adapter);
         }
     } else {
         tns::Assert(false, isolate);
@@ -574,6 +589,7 @@
         } else {
             Local<Object> obj = arg.As<Object>();
             DictionaryAdapter* adapter = [[DictionaryAdapter alloc] initWithJSObject:obj isolate:isolate];
+            CFAutorelease(adapter);
             return adapter;
         }
     }
diff --git a/NativeScript/runtime/ObjectManager.mm b/NativeScript/runtime/ObjectManager.mm
@@ -112,9 +112,6 @@
         case WrapperType::Reference: {
             ReferenceWrapper* referenceWrapper = static_cast<ReferenceWrapper*>(wrapper);
             if (referenceWrapper->Data() != nullptr) {
-                if (referenceWrapper->ShouldDisposeData()) {
-                    std::free(referenceWrapper->Data());
-                }
                 referenceWrapper->SetData(nullptr);
                 referenceWrapper->SetEncoding(nullptr);
             }
diff --git a/NativeScript/runtime/Reference.cpp b/NativeScript/runtime/Reference.cpp
@@ -58,7 +58,7 @@ Local<v8::Function> Reference::GetInteropReferenceCtorFunc(Local<Context> contex
     Local<Object> prototype = prototypeValue.As<Object>();
     Reference::RegisterToStringMethod(context, prototype);
 
-    cache->InteropReferenceCtorFunc = std::make_unique<Persistent<v8::Function>>(isolate, ctorFunc);
+    cache->InteropReferenceCtorFunc = std::make_unique<Persistent<v8::Function> >(isolate, ctorFunc);
 
     return ctorFunc;
 }
@@ -80,6 +80,10 @@ void Reference::ReferenceConstructorCallback(const FunctionCallbackInfo<Value>&
             val = new Persistent<Value>(isolate, info[1]);
         }
     }
+    
+    if(val != nullptr) {
+        val->SetWeak();
+    }
 
     ReferenceWrapper* wrapper = new ReferenceWrapper(typeWrapper, val);
     Local<Object> thiz = info.This();
diff --git a/NativeScript/runtime/Runtime.mm b/NativeScript/runtime/Runtime.mm
@@ -105,7 +105,7 @@
 
     DefineGlobalObject(context);
     DefineCollectFunction(context);
-    PromiseProxy::Init(context);
+//    PromiseProxy::Init(context);
     Console::Init(context);
     WeakRef::Init(context);
 
diff --git a/NativeScript/runtime/WeakRef.cpp b/NativeScript/runtime/WeakRef.cpp
diff --git a/NativeScript/runtime/WeakRef.h b/NativeScript/runtime/WeakRef.h
diff --git a/TestRunner/app/tests/shared/WeakRef.js b/TestRunner/app/tests/shared/WeakRef.js

Original file line number	Diff line number	Diff line change
`@@ -267,6 +267,7 @@`
`267`	`267`	`id adapter = [[DictionaryAdapter alloc] initWithJSObject:value.As<Object>() isolate:isolate];`
`268`	`268`	`memset(retValue, 0, sizeof(id));`
`269`	`269`	`static_cast<id __strong >(retValue) = adapter;`
	`270`	`+ CFAutorelease(adapter);`
`270`	`271`	`return;`
`271`	`272`	`}`
`272`	`273`	`} else if (type == BinaryTypeEncodingType::StructDeclarationReference) {`
Original file line number	Diff line number	Diff line change
`@@ -252,6 +252,7 @@`
`252`	`252`	`return Local<Value>();`
`253`	`253`	`}`
`254`	`254`
	`255`	`+ v8::Locker locker(isolate);`
`255`	`256`	`Local<Value> result;`
`256`	`257`	`if (!obj->GetPrivate(context, privateKey).ToLocal(&result)) {`
`257`	`258`	`tns::Assert(false, isolate);`
Original file line number	Diff line number	Diff line change
`@@ -58,7 +58,7 @@ Local<v8::Function> Reference::GetInteropReferenceCtorFunc(Local<Context> contex`
`58`	`58`	`Local<Object> prototype = prototypeValue.As<Object>();`
`59`	`59`	`Reference::RegisterToStringMethod(context, prototype);`
`60`	`60`
`61`		`- cache->InteropReferenceCtorFunc = std::make_unique<Persistent<v8::Function>>(isolate, ctorFunc);`
	`61`	`+ cache->InteropReferenceCtorFunc = std::make_unique<Persistent<v8::Function> >(isolate, ctorFunc);`
`62`	`62`
`63`	`63`	`return ctorFunc;`
`64`	`64`	`}`
`@@ -80,6 +80,10 @@ void Reference::ReferenceConstructorCallback(const FunctionCallbackInfo<Value>&`
`80`	`80`	`val = new Persistent<Value>(isolate, info[1]);`
`81`	`81`	`}`
`82`	`82`	`}`
	`83`	`+`
	`84`	`+ if(val != nullptr) {`
	`85`	`+ val->SetWeak();`
	`86`	`+ }`
`83`	`87`
`84`	`88`	`ReferenceWrapper* wrapper = new ReferenceWrapper(typeWrapper, val);`
`85`	`89`	`Local<Object> thiz = info.This();`