diff --git a/NativeScript/runtime/FFICall.h b/NativeScript/runtime/FFICall.h index c53ceef1..7a8531b8 100644 --- a/NativeScript/runtime/FFICall.h +++ b/NativeScript/runtime/FFICall.h @@ -3,7 +3,9 @@ #include +#include #include +#include #include "DataWrapper.h" #include "Metadata.h" @@ -92,11 +94,19 @@ class FFICall : public BaseCall { } ~FFICall() { + for (void* buffer : this->ownedBuffers_) { + std::free(buffer); + } + if (this->useDynamicBuffer_) { free(this->buffer_); } } + // Ties a malloc'd argument buffer to this call: it stays valid until after + // ffi_call returns and the result has been read. + inline void OwnBuffer(void* buffer) { this->ownedBuffers_.push_back(buffer); } + /** When calling this, always make another call to DisposeFFIType with the same parameters @@ -122,6 +132,7 @@ class FFICall : public BaseCall { static SpinMutex structInfosCacheMutex_; void** argsArray_; bool useDynamicBuffer_; + std::vector ownedBuffers_; uint8_t staticBuffer[512]; }; diff --git a/NativeScript/runtime/Interop.h b/NativeScript/runtime/Interop.h index db7bd20f..3c356977 100644 --- a/NativeScript/runtime/Interop.h +++ b/NativeScript/runtime/Interop.h @@ -107,7 +107,8 @@ class Interop { v8::Local arg); static void WriteValue(v8::Local context, const TypeEncoding* typeEncoding, void* dest, - v8::Local arg); + v8::Local arg, + FFICall* callOwner = nullptr); static id ToObject(v8::Local context, v8::Local arg); static v8::Local GetPrimitiveReturnType( v8::Local context, BinaryTypeEncodingType type, diff --git a/NativeScript/runtime/Interop.mm b/NativeScript/runtime/Interop.mm index 53f55aae..e6df1c08 100644 --- a/NativeScript/runtime/Interop.mm +++ b/NativeScript/runtime/Interop.mm @@ -148,7 +148,7 @@ enc = enc->next(); Local arg = args[i - initialParameterIndex]; void* argBuffer = call->ArgumentBuffer(i); - Interop::WriteValue(context, enc, argBuffer, arg); + Interop::WriteValue(context, enc, argBuffer, arg, call); } } @@ -236,7 +236,7 @@ inline bool isBool() { } void Interop::WriteValue(Local context, const TypeEncoding* typeEncoding, void* dest, - Local arg) { + Local arg, FFICall* callOwner) { Isolate* isolate = v8::Isolate::GetCurrent(); ExecuteWriteValueDebugValidationsIfInDebug(context, typeEncoding, dest, arg); ValueCache argHelper(arg); @@ -409,17 +409,15 @@ inline bool isBool() { tns::Assert(meta != nullptr && meta->type() == MetaType::Struct, isolate); const StructMeta* structMeta = static_cast(meta); StructInfo structInfo = FFICall::GetStructInfo(structMeta); - // TODO: How to free this? - // this is used when you have js obj and wants to pass the data as a struct ponter - // (MyStruct*) we create a new MyStruct with a snapshot of the jsObject and pass that in but - // when should we delete it? currently it's up to the function called to delete it we could - // delete after the function call, but if the fuction stores that then it's a memory leak we - // could also just store it as a wrapper in the object, binding it to the object lifecycle - // but that also means refactoring a lot of "if(wrapper == nullptr)" because essentially the - // wrapper should be treated as a nullptr, except when deating with - // StructDeclarationReference + // A plain JS object written into a MyStruct* slot is snapshotted into a fresh + // buffer. With an owner the callee only borrows it for the duration of the call. + // Without one (writes into an interop.Reference slot) the pointer is stored in + // memory that outlives any call, so the buffer must stay allocated. data = malloc(structInfo.FFIType()->size); Interop::InitializeStruct(context, data, structInfo.Fields(), arg); + if (callOwner != nullptr) { + callOwner->OwnBuffer(data); + } } else { if (wrapper == nullptr) { bool isArrayBuffer = false; diff --git a/TestFixtures/TNSTestNativeCallbacks.h b/TestFixtures/TNSTestNativeCallbacks.h index c5fc3272..69591b2b 100644 --- a/TestFixtures/TNSTestNativeCallbacks.h +++ b/TestFixtures/TNSTestNativeCallbacks.h @@ -58,6 +58,10 @@ + (void)recordsPointer:(TNSSimpleStruct*)object; +// Reads the pointee back out so callers can assert the marshalled values +// without going through the shared log buffer. ++ (TNSSimpleStruct)recordsPointerEcho:(TNSSimpleStruct*)object; + + (void)apiNSMutableArrayMethods:(NSMutableArray*)object; + (void)apiSwizzle:(TNSSwizzleKlass*)object; diff --git a/TestFixtures/TNSTestNativeCallbacks.m b/TestFixtures/TNSTestNativeCallbacks.m index abd0a28d..17c9943a 100644 --- a/TestFixtures/TNSTestNativeCallbacks.m +++ b/TestFixtures/TNSTestNativeCallbacks.m @@ -330,6 +330,10 @@ + (void)recordsPointer:(TNSSimpleStruct*)object { TNSLog([NSString stringWithFormat:@"%d %d", object->x, object->y]); } ++ (TNSSimpleStruct)recordsPointerEcho:(TNSSimpleStruct*)object { + return *object; +} + + (void)apiNSMutableArrayMethods:(NSMutableArray*)object { [object addObject:@"b"]; [object addObject:@"x"]; diff --git a/TestRunner/app/tests/Marshalling/RecordTests.js b/TestRunner/app/tests/Marshalling/RecordTests.js index 254edc7f..eb0a8a2f 100644 --- a/TestRunner/app/tests/Marshalling/RecordTests.js +++ b/TestRunner/app/tests/Marshalling/RecordTests.js @@ -358,4 +358,51 @@ describe(module.id, function () { TNSTestNativeCallbacks.recordsPointer(obj); expect(TNSGetOutput()).toBe("1 2"); }); + + it("Marshalling struct pointers from object literals repeatedly", () => { + for (let i = 0; i < 1000; i++) { + const echoed = TNSTestNativeCallbacks.recordsPointerEcho({ x: i, y: i + 1 }); + expect(echoed instanceof TNSSimpleStruct).toBe(true); + expect(echoed.x).toBe(i); + expect(echoed.y).toBe(i + 1); + } + }); + + it("Marshalling struct pointers from a wrapped struct leaves the wrapper's buffer intact", () => { + const record = new TNSSimpleStruct(); + record.x = 3; + record.y = 4; + + for (let i = 0; i < 100; i++) { + const echoed = TNSTestNativeCallbacks.recordsPointerEcho(record); + expect(echoed.x).toBe(3); + expect(echoed.y).toBe(4); + } + + expect(record.x).toBe(3); + expect(record.y).toBe(4); + }); + + it("Marshalling struct pointers from an interop.Reference leaves the reference readable", () => { + const record = new TNSSimpleStruct(); + record.x = 5; + record.y = 6; + + const reference = new interop.Reference(record); + + const echoed = TNSTestNativeCallbacks.recordsPointerEcho(reference); + expect(echoed.x).toBe(5); + expect(echoed.y).toBe(6); + + expect(reference.value.x).toBe(5); + expect(reference.value.y).toBe(6); + }); + + it("Marshalling structs by value from object literals repeatedly", () => { + for (let i = 0; i < 1000; i++) { + const echoed = TNSTestNativeCallbacks.recordsSimpleStruct({ x: i, y: i + 1 }); + expect(echoed.x).toBe(i); + expect(echoed.y).toBe(i + 1); + } + }); });