diff --git a/NativeScript/runtime/Interop.mm b/NativeScript/runtime/Interop.mm index 8a6544b2..4dacbe8c 100644 --- a/NativeScript/runtime/Interop.mm +++ b/NativeScript/runtime/Interop.mm @@ -270,18 +270,32 @@ inline bool isBool() { if (strArg->IsExternalOneByte()) { const v8::String::ExternalOneByteStringResource* resource = strArg->GetExternalOneByteStringResource(); - value = resource->data(); - } else { + if (OneByteStringResource::Owns(resource)) { + value = resource->data(); + } + } + + if (value == nullptr) { v8::String::Utf8Value utf8Value(isolate, arg); - value = strdup(*utf8Value); - // The external string only ties the strdup'd buffer's lifetime to the - // GC; it is never read as a string, and its one-byte (Latin-1) content - // matches the UTF-8 buffer only for ASCII. Length is the buffer's byte - // count, excluding the NUL. - OneByteStringResource* resource = - new OneByteStringResource(value, (size_t)utf8Value.length()); - bool success = v8::String::NewExternalOneByte(isolate, resource).ToLocal(&arg); - tns::Assert(success, isolate); + char* buffer = strdup(*utf8Value); + size_t byteLength = (size_t)utf8Value.length(); + value = buffer; + OneByteStringResource* resource = new OneByteStringResource(buffer, byteLength); + + // A one-byte external string's content is Latin-1, which coincides + // with the UTF-8 buffer exactly when every character is ASCII - i.e. + // when the byte count equals the UTF-16 length. Externalizing the + // argument itself lets later marshals of the same string take the + // reuse path above. + bool externalized = byteLength == (size_t)strArg->Length() && + strArg->CanMakeExternal(v8::String::ONE_BYTE_ENCODING) && + strArg->MakeExternal(isolate, resource); + if (!externalized) { + // The external string only ties the buffer's lifetime to the GC; it + // is never read as a string. + bool success = v8::String::NewExternalOneByte(isolate, resource).ToLocal(&arg); + tns::Assert(success, isolate); + } } Interop::SetValue(dest, value); } else { diff --git a/NativeScript/runtime/OneByteStringResource.cpp b/NativeScript/runtime/OneByteStringResource.cpp index e10086ca..4100a9ff 100644 --- a/NativeScript/runtime/OneByteStringResource.cpp +++ b/NativeScript/runtime/OneByteStringResource.cpp @@ -1,16 +1,35 @@ #include "OneByteStringResource.h" #include +#include +#include using namespace v8; +namespace { + +std::mutex registryMutex; + +std::unordered_set& Registry() { + static auto* registry = new std::unordered_set(); + return *registry; +} + +} // namespace + namespace tns { OneByteStringResource::OneByteStringResource(const char* data, size_t length): data_(data), length_(length) { + std::lock_guard lock(registryMutex); + Registry().insert(this); } OneByteStringResource::~OneByteStringResource() { + { + std::lock_guard lock(registryMutex); + Registry().erase(this); + } // data_ comes from strdup (see Interop::WriteValue's CStringEncoding path). std::free(const_cast(this->data_)); } @@ -23,4 +42,9 @@ size_t OneByteStringResource::length() const { return this->length_; } +bool OneByteStringResource::Owns( + const v8::String::ExternalOneByteStringResource* resource) { + std::lock_guard lock(registryMutex); + return Registry().count(resource) != 0; +} } diff --git a/NativeScript/runtime/OneByteStringResource.h b/NativeScript/runtime/OneByteStringResource.h index 0cf0a8fe..b81a5611 100644 --- a/NativeScript/runtime/OneByteStringResource.h +++ b/NativeScript/runtime/OneByteStringResource.h @@ -11,7 +11,13 @@ class OneByteStringResource : public v8::String::ExternalOneByteStringResource { ~OneByteStringResource() override; const char* data() const override; size_t length() const override; -private: + + // Whether this runtime created the resource. Only such resources are known + // to hold NUL-terminated UTF-8; V8's contract makes a foreign resource + // Latin-1 with no terminator guarantee. + static bool Owns(const v8::String::ExternalOneByteStringResource* resource); + + private: const char* data_; size_t length_; }; diff --git a/TestRunner/app/tests/Marshalling/ReferenceTests.js b/TestRunner/app/tests/Marshalling/ReferenceTests.js index c1810c3b..229bb1a5 100644 --- a/TestRunner/app/tests/Marshalling/ReferenceTests.js +++ b/TestRunner/app/tests/Marshalling/ReferenceTests.js @@ -337,6 +337,37 @@ describe(module.id, function () { interop.free(ptr); }); + it("reuses one marshalled buffer for repeated ASCII CString arguments", function () { + // Built at runtime so the string is a plain sequential string rather + // than an internalized literal, which V8 may refuse to externalize. + // V8 also refuses to externalize young-generation strings, so promote + // it to old space first - mirroring the real-world shape of a + // long-lived string marshalled repeatedly. + var str = Array(65).join("a"); + __collect(); + __collect(); + + // functionWithCharPtr echoes its argument pointer, exposing the + // address of the marshalled buffer: after the first call externalizes + // the string, later calls must reuse the same buffer instead of + // copying again. + var first = functionWithCharPtr(str); + var second = functionWithCharPtr(str); + expect(interop.handleof(first).toNumber()).toBe(interop.handleof(second).toNumber()); + expect(NSString.stringWithUTF8String(first).toString()).toBe(str); + }); + + it("marshals non-ASCII CString arguments as UTF-8 on every call", function () { + // Non-ASCII strings cannot be externalized as one-byte (Latin-1), so + // each marshal takes the copying path; the content must round-trip as + // UTF-8 both times. + var str = ["héllo", "wörld", "🙂"].join(" "); + var first = functionWithCharPtr(str); + expect(NSString.stringWithUTF8String(first).toString()).toBe(str); + var second = functionWithCharPtr(str); + expect(NSString.stringWithUTF8String(second).toString()).toBe(str); + }); + it("interops string from CString", function () { const str = "test"; const ptr = interop.alloc((str.length + 1) * interop.sizeof(interop.types.uint8));