fix: prevent JS function to native block leak (#223)

* fix: prevent JS function to native block leak

* fix: validate if isolate is alive

Author: edusperoni

NativeScript/runtime/Interop.mm

@@ -37,7 +37,7 @@
     .dispose = [](JSBlock* block) {
         if (block->descriptor == &JSBlock::kJSBlockDescriptor) {
             MethodCallbackWrapper* wrapper = static_cast<MethodCallbackWrapper*>(block->userData);
-            if (wrapper->isolateWrapper_.IsValid()) {
+            if (Runtime::IsAlive(wrapper->isolateWrapper_.Isolate()) && wrapper->isolateWrapper_.IsValid()) {
                 Isolate* isolate = wrapper->isolateWrapper_.Isolate();
                 v8::Locker locker(isolate);
                 Isolate::Scope isolate_scope(isolate);
@@ -80,6 +80,8 @@
 
     *blockPointer = {
         .isa = nullptr,
+        // this block is created with a refcount of 1
+        // this means we "own" it and need to free it
         .flags = JSBlock::BLOCK_HAS_COPY_DISPOSE | JSBlock::BLOCK_NEEDS_FREE | (1 /* ref count */ << 1),
         .reserved = 0,
         .invoke = (void*)result.first,
@@ -90,7 +92,8 @@
 
     object_setClass((__bridge id)blockPointer, objc_getClass("__NSMallocBlock__"));
 
-    return blockPointer;
+    // transfer ownership back to ARC (see refcount comment above)
+    return CFBridgingRelease(blockPointer);
 }
 
 Local<Value> Interop::CallFunction(CMethodCall& methodCall) {

TestFixtures/Marshalling/TNSObjCTypes.h

@@ -12,6 +12,7 @@ CFTypeRef TNSFunctionWithCreateCFTypeRefReturn() CF_RETURNS_RETAINED;
 typedef int (^NumberReturner)(int, int, int);
 
 @interface TNSObjCTypes : NSObject
+@property (nonatomic, copy) void (^retainedBlock)(void);
 + (void)methodWithComplexBlock:(id (^)(int, id, SEL, NSObject*, TNSOStruct))block;
 + (id)methodWithObject:(id)x;
 
@@ -22,6 +23,10 @@ typedef int (^NumberReturner)(int, int, int);
 - (void)methodWithSimpleBlock:(void (^)(void))block;
 - (void)methodWithComplexBlock:(id (^)(int, id, SEL, NSObject*, TNSOStruct))block;
 
+- (void)methodRetainingBlock:(void (^)(void))block;
+- (void)methodCallRetainingBlock;
+- (void)methodReleaseRetainingBlock;
+
 - (NumberReturner)methodWithBlockScope:(int)number;
 - (id)methodReturningBlockAsId:(int)number;
 

TestFixtures/Marshalling/TNSObjCTypes.m

@@ -48,6 +48,16 @@ - (void)methodWithSimpleBlock:(void (^)(void))block {
     block();
 }
 
+- (void)methodRetainingBlock:(void (^)(void))block {
+    _retainedBlock = block;
+}
+- (void)methodCallRetainingBlock {
+    _retainedBlock();
+}
+- (void)methodReleaseRetainingBlock {
+    _retainedBlock = NULL;
+}
+
 - (void)methodWithComplexBlock:(id (^)(int, id, SEL, NSObject*, TNSOStruct))block {
     TNSOStruct str = { 5, 6, 7 };
     id result = block(1, @2, @selector(init), @[@3, @4], str);

TestRunner/app/tests/Marshalling/ObjCTypesTests.js

@@ -65,6 +65,51 @@ describe(module.id, function () {
         expect(actual).toBe("simple block called");
     });
 
+    it("Block releases after call", function (done) {
+        const functionRef = new WeakRef(function () {
+            TNSLog('simple block called');
+        });
+        TNSObjCTypes.alloc().init().methodWithSimpleBlock(functionRef.deref());
+
+        var actual = TNSGetOutput();
+        expect(actual).toBe("simple block called");
+        gc();
+        setTimeout(() => {
+            gc();
+            expect(!!functionRef.deref()).toBe(false);
+            done();
+        });
+    });
+
+    it("Block retains and releases", function (done) {
+        const functionRef = new WeakRef(function () {
+            TNSLog('simple block called');
+        });
+        const instance = TNSObjCTypes.alloc().init();
+        instance.methodRetainingBlock(functionRef.deref());
+        function verifyBlockCall() {
+            instance.methodCallRetainingBlock();
+            var actual = TNSGetOutput();
+            expect(actual).toBe("simple block called");
+            TNSClearOutput();
+        }
+        verifyBlockCall();
+
+        gc();
+        setTimeout(() => {
+            gc();
+            expect(!!functionRef.deref()).toBe(true);
+            verifyBlockCall();
+            instance.methodReleaseRetainingBlock();
+            gc();
+            setTimeout(() => {
+                gc();
+                expect(!!functionRef.deref()).toBe(false);
+                done();    
+            })
+        });
+    });
+
     it("InstanceComplexBlock", function () {
         function block(i, id, sel, obj, str) {
             expect(i).toBe(1);