fix(runtime): keep a JS function's block apart from its own wrapper

NativeScript/runtime/FunctionReference.cpp

@@ -62,11 +62,19 @@ void FunctionReference::FunctionReferenceConstructorCallback(
   tns::Assert(info[0]->IsFunction(), isolate);
 
   Local<v8::Function> arg = info[0].As<v8::Function>();
+  info.GetReturnValue().Set(arg);
+
+  // The existing wrapper may already hold the trampoline native code calls.
+  BaseDataWrapper* existing = tns::GetValue(isolate, arg);
+  if (existing != nullptr &&
+      existing->Type() == WrapperType::FunctionReference) {
+    return;
+  }
+
   std::shared_ptr<Persistent<v8::Value>> poArg =
       ObjectManager::Register(context, arg);
   FunctionReferenceWrapper* wrapper = new FunctionReferenceWrapper(poArg);
   tns::SetValue(isolate, arg, wrapper);
-  info.GetReturnValue().Set(arg);
 }
 
 }  // namespace tns

NativeScript/runtime/Helpers.h

@@ -285,6 +285,13 @@ void SetReleasedObjectPolicy(ReleasedObjectPolicy policy);
 BaseDataWrapper* GetValueOrReport(v8::Isolate* isolate, const v8::Local<v8::Value>& val,
                                   const char* operation);
 void DeleteValue(v8::Isolate* isolate, const v8::Local<v8::Value>& val);
+// The block a JS function was last marshalled as (see Interop::JSBlock, which
+// owns the wrapper). Kept apart from GetValue's slot so the function's own
+// wrapper and its block never evict each other.
+void SetJSBlockWrapper(v8::Isolate* isolate, const v8::Local<v8::Function>& fn,
+                       BlockWrapper* wrapper);
+BlockWrapper* GetJSBlockWrapper(v8::Isolate* isolate, const v8::Local<v8::Value>& val);
+void DeleteJSBlockWrapper(v8::Isolate* isolate, const v8::Local<v8::Value>& val);
 bool DeleteWrapperIfUnused(v8::Isolate* isolate, const v8::Local<v8::Value>& obj,
                            BaseDataWrapper* value);
 std::vector<v8::Local<v8::Value>> ArgsToVector(const v8::FunctionCallbackInfo<v8::Value>& info);

NativeScript/runtime/Helpers.mm

@@ -527,6 +527,46 @@ void WriteDebugLine(tns::LogCategory category, const char* message) {
   tns::Assert(success, isolate);
 }
 
+namespace {
+
+constexpr const char* kJSBlockKey = "jsBlock";
+
+}  // namespace
+
+void tns::SetJSBlockWrapper(Isolate* isolate, const Local<v8::Function>& fn,
+                            BlockWrapper* wrapper) {
+  Local<External> ext = External::New(isolate, wrapper, v8::kExternalPointerTypeTagDefault);
+  tns::SetPrivateValue(fn, tns::ToV8String(isolate, kJSBlockKey), ext);
+}
+
+tns::BlockWrapper* tns::GetJSBlockWrapper(Isolate* isolate, const Local<Value>& val) {
+  if (val.IsEmpty() || !val->IsFunction()) {
+    return nullptr;
+  }
+
+  Local<Value> prop = tns::GetPrivateValue(val.As<Object>(), tns::ToV8String(isolate, kJSBlockKey));
+  if (prop.IsEmpty() || !prop->IsExternal()) {
+    return nullptr;
+  }
+
+  return static_cast<BlockWrapper*>(prop.As<External>()->Value(v8::kExternalPointerTypeTagDefault));
+}
+
+void tns::DeleteJSBlockWrapper(Isolate* isolate, const Local<Value>& val) {
+  if (val.IsEmpty() || !val->IsFunction()) {
+    return;
+  }
+
+  Local<Object> obj = val.As<Object>();
+  Local<Context> context;
+  bool success = obj->GetCreationContext(isolate).ToLocal(&context);
+  tns::Assert(success, isolate);
+  Local<Private> privateKey = Private::ForApi(isolate, tns::ToV8String(isolate, kJSBlockKey));
+
+  success = obj->DeletePrivate(context, privateKey).FromMaybe(false);
+  tns::Assert(success, isolate);
+}
+
 std::vector<Local<Value>> tns::ArgsToVector(const FunctionCallbackInfo<Value>& info) {
   std::vector<Local<Value>> args;
   args.reserve(info.Length());

NativeScript/runtime/Interop.mm

@@ -30,6 +30,10 @@
     static_cast<int64_t>(kUint64AllBitsSet << 53) + 1;        // -9007199254740991 (-(2^53-1))
 static constexpr int64_t kMaxSafeInteger = -kMinSafeInteger;  // 9007199254740991 (2^53-1)
 
+static constexpr const char* kNotAFunctionPointer =
+    "A function pointer argument takes an interop.Pointer, a native function pointer or a "
+    "function wrapped in interop.FunctionReference.";
+
 Interop::JSBlock::JSBlockDescriptor Interop::JSBlock::kJSBlockDescriptor = {
     .reserved = 0,
     .size = sizeof(JSBlock),
@@ -61,8 +65,8 @@
                   if (!callback.IsEmpty() && callback->IsObject()) {
                     // The slot may hold another wrapper by now; only our own is
                     // cleared from it.
-                    if (tns::GetValue(isolate, callback) == blockWrapper) {
-                      tns::DeleteValue(isolate, callback);
+                    if (tns::GetJSBlockWrapper(isolate, callback) == blockWrapper) {
+                      tns::DeleteJSBlockWrapper(isolate, callback);
                     }
                   }
                   // Unconditional: an already-detached callback still owns its
@@ -518,7 +522,9 @@ inline bool isBool() {
   } else if (argHelper.isObject() &&
              typeEncoding->type == BinaryTypeEncodingType::FunctionPointerEncoding) {
     BaseDataWrapper* wrapper = tns::GetValue(isolate, arg.As<Object>());
-    tns::Assert(wrapper != nullptr, isolate);
+    if (wrapper == nullptr) {
+      throw NativeScriptException(kNotAFunctionPointer);
+    }
     if (wrapper->Type() == WrapperType::Pointer) {
       PointerWrapper* pointerWrapper = static_cast<PointerWrapper*>(wrapper);
       void* data = pointerWrapper->Data();
@@ -528,7 +534,6 @@ inline bool isBool() {
       void* data = functionWrapper->Data();
       Interop::SetValue(dest, data);
     } else if (wrapper->Type() == WrapperType::FunctionReference) {
-      tns::Assert(wrapper != nullptr && wrapper->Type() == WrapperType::FunctionReference, isolate);
       FunctionReferenceWrapper* funcWrapper = static_cast<FunctionReferenceWrapper*>(wrapper);
       const TypeEncoding* functionTypeEncoding =
           typeEncoding->details.functionPointer.signature.first();
@@ -557,28 +562,28 @@ inline bool isBool() {
 
       Interop::SetValue(dest, functionPointer);
     } else {
-      tns::Assert(false, isolate);
+      throw NativeScriptException(kNotAFunctionPointer);
     }
   } else if (arg->IsFunction() && typeEncoding->type == BinaryTypeEncodingType::BlockEncoding) {
     const TypeEncoding* blockTypeEncoding = typeEncoding->details.block.signature.first();
     int argsCount = typeEncoding->details.block.signature.count - 1;
 
     CFTypeRef blockPtr = nullptr;
+    // The callee takes the block at +0 and copies it if it needs to keep it,
+    // so the reference that keeps it alive across the call must be balanced:
+    // the JSBlock dispose helper owns the ffi closure and the callback wrapper
+    // and only runs once the last reference goes away.
     BaseDataWrapper* baseWrapper = tns::GetValue(isolate, arg);
-    if (baseWrapper != nullptr && baseWrapper->Type() == WrapperType::Block) {
-      BlockWrapper* wrapper = static_cast<BlockWrapper*>(baseWrapper);
-      // The callee takes the block at +0 and copies it if it needs to keep it,
-      // so the reference that keeps it alive across the call must be balanced:
-      // the JSBlock dispose helper owns the ffi closure and the callback wrapper
-      // and only runs once the last reference goes away.
-      if (wrapper->OwnsBlock()) {
-        // A native block; the wrapper's own Block_copy keeps it alive.
-        blockPtr = CFAutorelease(Block_copy(wrapper->Block()));
-      } else if (TryRetainJSBlock(static_cast<JSBlock*>(wrapper->Block()))) {
-        // Reading the block is safe even when its last release raced ahead:
-        // until the isolate's teardown closes its gate, dispose clears this
-        // slot under the Locker this thread holds, before libclosure frees
-        // the block.
+    if (baseWrapper != nullptr && baseWrapper->Type() == WrapperType::Block &&
+        static_cast<BlockWrapper*>(baseWrapper)->OwnsBlock()) {
+      // A native block; the wrapper's own Block_copy keeps it alive.
+      blockPtr = CFAutorelease(Block_copy(static_cast<BlockWrapper*>(baseWrapper)->Block()));
+    } else if (BlockWrapper* wrapper = tns::GetJSBlockWrapper(isolate, arg)) {
+      // Reading the block is safe even when its last release raced ahead:
+      // until the isolate's teardown closes its gate, dispose clears this
+      // slot under the Locker this thread holds, before libclosure frees
+      // the block.
+      if (TryRetainJSBlock(static_cast<JSBlock*>(wrapper->Block()))) {
         blockPtr = CFAutorelease(wrapper->Block());
       }
     }
@@ -592,7 +597,7 @@ inline bool isBool() {
 
       BlockWrapper* wrapper = new BlockWrapper((void*)blockPtr, blockTypeEncoding, false);
       reinterpret_cast<JSBlock*>((void*)blockPtr)->blockWrapper = wrapper;
-      tns::SetValue(isolate, arg.As<v8::Function>(), wrapper);
+      tns::SetJSBlockWrapper(isolate, arg.As<v8::Function>(), wrapper);
     }
 
     Interop::SetValue(dest, blockPtr);

NativeScript/runtime/InteropTypes.mm

@@ -589,6 +589,10 @@ new PrimitiveDataWrapper(sizeof(void*),
                        }
                      }
 
+                     if (size == 0 && tns::GetJSBlockWrapper(isolate, arg) != nullptr) {
+                       size = sizeof(void*);
+                     }
+
                      if (size == 0) {
                        throw NativeScriptException("Unknown type");
                      } else {
@@ -696,20 +700,22 @@ new PrimitiveDataWrapper(sizeof(void*),
             if (blockWrapper->OwnsBlock()) {
               return Pointer::NewInstance(context, blockWrapper->Block());
             }
-            // A JS function does not keep its block alive: only native
-            // references do. A live block is kept for the rest of the turn,
-            // like one passed to a native call; a dying one is no handle.
-            JSBlock* block = static_cast<JSBlock*>(blockWrapper->Block());
-            if (TryRetainJSBlock(block)) {
-              CFAutorelease(block);
-              return Pointer::NewInstance(context, block);
-            }
             break;
           }
           default:
             break;
         }
       }
+      if (BlockWrapper* blockWrapper = tns::GetJSBlockWrapper(isolate, obj)) {
+        // A JS function does not keep its block alive: only native
+        // references do. A live block is kept for the rest of the turn,
+        // like one passed to a native call; a dying one is no handle.
+        JSBlock* block = static_cast<JSBlock*>(blockWrapper->Block());
+        if (TryRetainJSBlock(block)) {
+          CFAutorelease(block);
+          return Pointer::NewInstance(context, block);
+        }
+      }
     }
   } else if (value->IsNull()) {
     return v8::Null(isolate);

TestRunner/app/tests/FunctionReferenceBlockTests.js

@@ -0,0 +1,131 @@
+// A function can be marshalled both as a block and, once wrapped in
+// interop.FunctionReference, as a C function pointer. Neither use may take over
+// the state the other keeps on the function.
+describe("Function used as a block and as a function pointer", function () {
+    function square(x) {
+        return x * x;
+    }
+
+    afterEach(function () {
+        TNSClearOutput();
+    });
+
+    it("is called through a function pointer after being a block", function () {
+        var blockCalls = [];
+        var fn = new interop.FunctionReference(function (x) {
+            blockCalls.push(x);
+            return square(x);
+        });
+
+        TNSTestNativeCallbacks.repeatPausingAfter(2, fn);
+        expect(blockCalls).toEqual([0, 1]);
+
+        functionWithSimpleFunctionPointer(fn);
+        expect(TNSGetOutput()).toBe("4");
+    });
+
+    it("is called as a block after being a function pointer", function () {
+        var blockCalls = [];
+        var fn = new interop.FunctionReference(function (x) {
+            blockCalls.push(x);
+            return square(x);
+        });
+
+        functionWithSimpleFunctionPointer(fn);
+        expect(TNSGetOutput()).toBe("4");
+
+        TNSTestNativeCallbacks.repeatPausingAfter(2, fn);
+        expect(blockCalls).toEqual([2, 0, 1]);
+
+        TNSClearOutput();
+        functionWithSimpleFunctionPointer(fn);
+        expect(TNSGetOutput()).toBe("4");
+    });
+
+    it("keeps its function pointer across a second interop.FunctionReference", function () {
+        var fn = new interop.FunctionReference(square);
+        functionWithSimpleFunctionPointer(fn);
+        var trampoline = interop.handleof(fn).toNumber();
+
+        expect(new interop.FunctionReference(fn)).toBe(fn);
+        expect(interop.handleof(fn).toNumber()).toBe(trampoline);
+    });
+
+    it("keeps its cached block across interop.FunctionReference", function () {
+        var fn = function () {};
+        TNSTestNativeCallbacks.keepBlockForMilliseconds(fn, 1000);
+        var block = interop.handleof(fn).toNumber();
+
+        expect(new interop.FunctionReference(fn)).toBe(fn);
+        expect(interop.handleof(fn).toNumber()).toBe(block);
+
+        TNSTestNativeCallbacks.keepBlockForMilliseconds(fn, 1000);
+        expect(interop.handleof(fn).toNumber()).toBe(block);
+    });
+
+    it("is reported by interop.handleof as its function pointer once it has one", function () {
+        var fn = function () {};
+        TNSTestNativeCallbacks.keepBlockForMilliseconds(fn, 1000);
+        var block = interop.handleof(fn).toNumber();
+
+        new interop.FunctionReference(fn);
+        functionWithSimpleFunctionPointer(fn);
+        var trampoline = interop.handleof(fn).toNumber();
+        expect(trampoline).not.toBe(block);
+
+        TNSTestNativeCallbacks.keepBlockForMilliseconds(fn, 1000);
+        expect(interop.handleof(fn).toNumber()).toBe(trampoline);
+    });
+
+    it("throws when passed as a function pointer without interop.FunctionReference", function () {
+        var fn = function () {};
+        TNSTestNativeCallbacks.repeatPausingAfter(1, fn);
+        expect(function () {
+            functionWithSimpleFunctionPointer(fn);
+        }).toThrowError(/FunctionReference/);
+        expect(function () {
+            functionWithSimpleFunctionPointer(function () {});
+        }).toThrowError(/FunctionReference/);
+    });
+
+    it("is collected once its blocks are gone", function (done) {
+        for (var i = 0; i < 50; i++) {
+            var fn = new interop.FunctionReference(function () {});
+            if (i % 2) {
+                TNSTestNativeCallbacks.repeatPausingAfter(1, fn);
+            } else {
+                TNSTestNativeCallbacks.keepBlockForMilliseconds(fn, 5);
+            }
+        }
+        setTimeout(function () {
+            __collect();
+            __collect();
+            done();
+        }, 50);
+    });
+
+    describe("in a worker", function () {
+        var originalTimeout;
+        beforeEach(function () {
+            originalTimeout = jasmine.DEFAULT_TIMEOUT_INTERVAL;
+            jasmine.DEFAULT_TIMEOUT_INTERVAL = 10000;
+        });
+        afterEach(function () {
+            jasmine.DEFAULT_TIMEOUT_INTERVAL = originalTimeout;
+        });
+
+        it("is torn down while native code still holds its block", function (done) {
+            var worker = new Worker("./functionReferenceBlockWorker.js");
+            worker.onmessage = function (msg) {
+                expect(msg.data).toEqual({ pointerOutput: "4", blockKept: true });
+                worker.terminate();
+                setTimeout(done, 600);
+            };
+            worker.onerror = function (e) {
+                expect(String(e && e.message ? e.message : e)).toBe("<no worker error>");
+                done();
+            };
+            worker.postMessage(0);
+        });
+    });
+});

TestRunner/app/tests/blockTeardownReleaseWorker.js

@@ -1,7 +1,7 @@
 // Leaves a native object as the only owner of a block built from a function
 // that interop.FunctionReference registered first. Teardown disposes registered
-// objects newest first, so it releases the block (and runs its dispose) before
-// it reaches the function whose slot still points at the block's wrapper.
+// objects newest first, so it releases the block (and runs its dispose, which
+// frees the block's wrapper) before it disposes the function's own wrapper.
 onmessage = function () {
     var fn = function () {};
     new interop.FunctionReference(fn);

TestRunner/app/tests/functionReferenceBlockWorker.js

@@ -0,0 +1,17 @@
+// Leaves native code holding a block built from a function that is also an
+// interop.FunctionReference with a trampoline, past this worker's teardown.
+onmessage = function () {
+    var fn = new interop.FunctionReference(function (x) {
+        return x * x;
+    });
+    TNSTestNativeCallbacks.keepBlockForMilliseconds(fn, 300);
+    var block = interop.handleof(fn).toNumber();
+
+    TNSClearOutput();
+    functionWithSimpleFunctionPointer(fn);
+    var pointerOutput = String(TNSGetOutput());
+    TNSClearOutput();
+
+    TNSTestNativeCallbacks.keepBlockForMilliseconds(fn, 300);
+    postMessage({ pointerOutput: pointerOutput, blockKept: block !== 0 });
+};

TestRunner/app/tests/index.js

@@ -131,6 +131,7 @@ require("./ApiTests");
 require("./NsRuntimeTests");
 require("./GCFinalizerTests");
 require("./BlockCacheRaceTests");
+require("./FunctionReferenceBlockTests");
 require("./WorkerConcurrentStartupTests");
 require("./WorkerOptionsTests");
 require("./WorkerResourceLimitsTests");