diff --git a/NativeScript/runtime/FunctionReference.cpp b/NativeScript/runtime/FunctionReference.cpp index b7210eca..226a9e96 100644 --- a/NativeScript/runtime/FunctionReference.cpp +++ b/NativeScript/runtime/FunctionReference.cpp @@ -62,11 +62,19 @@ void FunctionReference::FunctionReferenceConstructorCallback( tns::Assert(info[0]->IsFunction(), isolate); Local arg = info[0].As(); + 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> poArg = ObjectManager::Register(context, arg); FunctionReferenceWrapper* wrapper = new FunctionReferenceWrapper(poArg); tns::SetValue(isolate, arg, wrapper); - info.GetReturnValue().Set(arg); } } // namespace tns diff --git a/NativeScript/runtime/Helpers.h b/NativeScript/runtime/Helpers.h index 5b48ade5..0f9718f0 100644 --- a/NativeScript/runtime/Helpers.h +++ b/NativeScript/runtime/Helpers.h @@ -285,6 +285,13 @@ void SetReleasedObjectPolicy(ReleasedObjectPolicy policy); BaseDataWrapper* GetValueOrReport(v8::Isolate* isolate, const v8::Local& val, const char* operation); void DeleteValue(v8::Isolate* isolate, const v8::Local& 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& fn, + BlockWrapper* wrapper); +BlockWrapper* GetJSBlockWrapper(v8::Isolate* isolate, const v8::Local& val); +void DeleteJSBlockWrapper(v8::Isolate* isolate, const v8::Local& val); bool DeleteWrapperIfUnused(v8::Isolate* isolate, const v8::Local& obj, BaseDataWrapper* value); std::vector> ArgsToVector(const v8::FunctionCallbackInfo& info); diff --git a/NativeScript/runtime/Helpers.mm b/NativeScript/runtime/Helpers.mm index ef35ddaa..494f6c5d 100644 --- a/NativeScript/runtime/Helpers.mm +++ b/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& fn, + BlockWrapper* wrapper) { + Local ext = External::New(isolate, wrapper, v8::kExternalPointerTypeTagDefault); + tns::SetPrivateValue(fn, tns::ToV8String(isolate, kJSBlockKey), ext); +} + +tns::BlockWrapper* tns::GetJSBlockWrapper(Isolate* isolate, const Local& val) { + if (val.IsEmpty() || !val->IsFunction()) { + return nullptr; + } + + Local prop = tns::GetPrivateValue(val.As(), tns::ToV8String(isolate, kJSBlockKey)); + if (prop.IsEmpty() || !prop->IsExternal()) { + return nullptr; + } + + return static_cast(prop.As()->Value(v8::kExternalPointerTypeTagDefault)); +} + +void tns::DeleteJSBlockWrapper(Isolate* isolate, const Local& val) { + if (val.IsEmpty() || !val->IsFunction()) { + return; + } + + Local obj = val.As(); + Local context; + bool success = obj->GetCreationContext(isolate).ToLocal(&context); + tns::Assert(success, isolate); + Local privateKey = Private::ForApi(isolate, tns::ToV8String(isolate, kJSBlockKey)); + + success = obj->DeletePrivate(context, privateKey).FromMaybe(false); + tns::Assert(success, isolate); +} + std::vector> tns::ArgsToVector(const FunctionCallbackInfo& info) { std::vector> args; args.reserve(info.Length()); diff --git a/NativeScript/runtime/Interop.mm b/NativeScript/runtime/Interop.mm index 998742e9..8cb6906b 100644 --- a/NativeScript/runtime/Interop.mm +++ b/NativeScript/runtime/Interop.mm @@ -30,6 +30,10 @@ static_cast(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()); - tns::Assert(wrapper != nullptr, isolate); + if (wrapper == nullptr) { + throw NativeScriptException(kNotAFunctionPointer); + } if (wrapper->Type() == WrapperType::Pointer) { PointerWrapper* pointerWrapper = static_cast(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(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(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(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(baseWrapper)->OwnsBlock()) { + // A native block; the wrapper's own Block_copy keeps it alive. + blockPtr = CFAutorelease(Block_copy(static_cast(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(wrapper->Block()))) { blockPtr = CFAutorelease(wrapper->Block()); } } @@ -592,7 +597,7 @@ inline bool isBool() { BlockWrapper* wrapper = new BlockWrapper((void*)blockPtr, blockTypeEncoding, false); reinterpret_cast((void*)blockPtr)->blockWrapper = wrapper; - tns::SetValue(isolate, arg.As(), wrapper); + tns::SetJSBlockWrapper(isolate, arg.As(), wrapper); } Interop::SetValue(dest, blockPtr); diff --git a/NativeScript/runtime/InteropTypes.mm b/NativeScript/runtime/InteropTypes.mm index a149d055..f51af9a5 100644 --- a/NativeScript/runtime/InteropTypes.mm +++ b/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(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(blockWrapper->Block()); + if (TryRetainJSBlock(block)) { + CFAutorelease(block); + return Pointer::NewInstance(context, block); + } + } } } else if (value->IsNull()) { return v8::Null(isolate); diff --git a/TestRunner/app/tests/FunctionReferenceBlockTests.js b/TestRunner/app/tests/FunctionReferenceBlockTests.js new file mode 100644 index 00000000..f8de0771 --- /dev/null +++ b/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(""); + done(); + }; + worker.postMessage(0); + }); + }); +}); diff --git a/TestRunner/app/tests/blockTeardownReleaseWorker.js b/TestRunner/app/tests/blockTeardownReleaseWorker.js index c60e22a5..4e14fbf8 100644 --- a/TestRunner/app/tests/blockTeardownReleaseWorker.js +++ b/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); diff --git a/TestRunner/app/tests/functionReferenceBlockWorker.js b/TestRunner/app/tests/functionReferenceBlockWorker.js new file mode 100644 index 00000000..fe86a234 --- /dev/null +++ b/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 }); +}; diff --git a/TestRunner/app/tests/index.js b/TestRunner/app/tests/index.js index f2f9e6fe..bcd27313 100644 --- a/TestRunner/app/tests/index.js +++ b/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");