Skip to content

Commit a3ab3d1

Browse files
committed
feat: enhance module resolution and memory management
- Improved module resolution by normalizing module paths and supporting extensionless "main" entries in package.json. - Enhanced memory management in Cif and Reference classes, ensuring proper cleanup of allocated resources. - Added functionality to handle JSON modules and built-in Node.js module specifiers. - Refactored code for better clarity and maintainability, including updates to error handling and path management.
1 parent 351e7c4 commit a3ab3d1

File tree

11 files changed

+335
-72
lines changed

11 files changed

+335
-72
lines changed

NativeScript/cli/main.cpp

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,16 @@ void bootFromBytecode(std::string baseDir, const void* data, size_t size) {
3232

3333
void bootFromModuleSpec(std::string baseDir, std::string spec) {
3434
RuntimeConfig.BaseDir = baseDir;
35+
RuntimeConfig.ApplicationPath = baseDir;
36+
37+
// In CLI mode, map "~" imports to the directory of the entry file when
38+
// possible so app bundles that use "~/package.json" resolve consistently.
39+
std::error_code ec;
40+
std::filesystem::path specPath(spec);
41+
auto absoluteSpecPath = std::filesystem::absolute(specPath, ec);
42+
if (!ec && !absoluteSpecPath.empty()) {
43+
RuntimeConfig.ApplicationPath = absoluteSpecPath.parent_path().string();
44+
}
3545

3646
auto runtime = Runtime();
3747

NativeScript/ffi/CFunction.mm

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -125,10 +125,6 @@
125125
return cif->returnType->toJS(env, rvalue);
126126
}
127127

128-
CFunction::~CFunction() {
129-
if (cif != nullptr) {
130-
delete cif;
131-
}
132-
}
128+
CFunction::~CFunction() { cif = nullptr; }
133129

134130
} // namespace nativescript

NativeScript/ffi/Cif.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,9 @@ class Cif {
2121

2222
void* rvalue;
2323
void** avalues;
24+
ffi_type** atypes = nullptr;
25+
unsigned int avaluesAllocStart = 0;
26+
unsigned int avaluesAllocCount = 0;
2427

2528
std::shared_ptr<TypeConv> returnType;
2629
std::vector<std::shared_ptr<TypeConv>> argTypes;

NativeScript/ffi/Cif.mm

Lines changed: 28 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
#include "ObjCBridge.h"
88
#include "TypeConv.h"
99
#include "Util.h"
10+
#include <cstring>
1011

1112
namespace nativescript {
1213

@@ -68,13 +69,13 @@
6869
this->argc = (int)numberOfArguments - 2;
6970
this->argv = (napi_value*)malloc(sizeof(napi_value) * this->argc);
7071

71-
unsigned int argc = (unsigned int)numberOfArguments;
72+
unsigned int totalArgc = (unsigned int)numberOfArguments;
7273

7374
const char* returnType = signature.methodReturnType;
7475
this->returnType = TypeConv::Make(env, &returnType);
7576

7677
ffi_type* rtype = this->returnType->type;
77-
ffi_type** atypes = (ffi_type**)malloc(sizeof(ffi_type*) * argc);
78+
this->atypes = (ffi_type**)malloc(sizeof(ffi_type*) * totalArgc);
7879

7980
unsigned long methodReturnLength = signature.methodReturnLength;
8081
unsigned long frameLength = signature.frameLength;
@@ -83,16 +84,19 @@
8384
this->rvalueLength = methodReturnLength;
8485
this->frameLength = frameLength;
8586

86-
this->avalues = (void**)malloc(sizeof(void*) * argc);
87+
this->avalues = (void**)malloc(sizeof(void*) * totalArgc);
88+
memset(this->avalues, 0, sizeof(void*) * totalArgc);
8789
this->shouldFree = (bool*)malloc(sizeof(bool) * this->argc);
8890
memset(this->shouldFree, false, sizeof(bool) * this->argc);
8991
this->shouldFreeAny = false;
92+
this->avaluesAllocStart = 2;
93+
this->avaluesAllocCount = 0;
9094

9195
for (int i = 0; i < numberOfArguments; i++) {
9296
const char* argenc = [signature getArgumentTypeAtIndex:i];
9397

9498
auto argTypeInfo = TypeConv::Make(env, &argenc);
95-
atypes[i] = argTypeInfo->type;
99+
this->atypes[i] = argTypeInfo->type;
96100

97101
if (i >= 2) {
98102
this->argTypes.push_back(argTypeInfo);
@@ -101,16 +105,17 @@
101105

102106
[signature release];
103107

104-
ffi_status status = ffi_prep_cif(&cif, FFI_DEFAULT_ABI, argc, rtype, atypes);
105-
106-
for (int i = 2; i < argc; i++) {
107-
this->avalues[i] = malloc(cif.arg_types[i]->size);
108-
}
108+
ffi_status status = ffi_prep_cif(&cif, FFI_DEFAULT_ABI, totalArgc, rtype, this->atypes);
109109

110110
if (status != FFI_OK) {
111111
std::cout << "Failed to prepare CIF, libffi returned error:" << status << std::endl;
112112
return;
113113
}
114+
115+
for (unsigned int i = 2; i < totalArgc; i++) {
116+
this->avalues[i] = malloc(cif.arg_types[i]->size);
117+
this->avaluesAllocCount++;
118+
}
114119
}
115120

116121
Cif::Cif(napi_env env, MDMetadataReader* reader, MDSectionOffset offset, bool isMethod,
@@ -121,11 +126,12 @@
121126

122127
returnType = TypeConv::Make(env, reader, &offset);
123128

124-
ffi_type** atypes = nullptr;
125-
126129
auto implicitArgs = isMethod ? 2 : isBlock ? 1 : 0;
127130

128131
shouldFreeAny = false;
132+
atypes = nullptr;
133+
avaluesAllocStart = 0;
134+
avaluesAllocCount = 0;
129135

130136
if (next || isMethod || isBlock) {
131137
while (next) {
@@ -146,6 +152,7 @@
146152

147153
atypes = (ffi_type**)malloc(sizeof(ffi_type*) * totalArgc);
148154
avalues = (void**)malloc(sizeof(void*) * argc);
155+
memset(avalues, 0, sizeof(void*) * argc);
149156

150157
if (isMethod) {
151158
atypes[0] = &ffi_type_pointer;
@@ -177,6 +184,7 @@
177184

178185
for (int i = 0; i < argc; i++) {
179186
avalues[i] = malloc(cif.arg_types[i + implicitArgs]->size);
187+
avaluesAllocCount++;
180188
}
181189

182190
rvalue = malloc(cif.rtype->size);
@@ -191,8 +199,17 @@
191199
free(argv);
192200
}
193201
if (avalues != nullptr) {
202+
for (unsigned int i = 0; i < avaluesAllocCount; i++) {
203+
auto index = avaluesAllocStart + i;
204+
if (avalues[index] != nullptr) {
205+
free(avalues[index]);
206+
}
207+
}
194208
free(avalues);
195209
}
210+
if (atypes != nullptr) {
211+
free(atypes);
212+
}
196213
if (shouldFree != nullptr) {
197214
free(shouldFree);
198215
}

NativeScript/ffi/Interop.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,9 @@ class Reference {
6060
~Reference();
6161

6262
// data = nullptr means the reference is not initialized.
63+
napi_env env = nullptr;
6364
void* data = nullptr;
65+
bool ownsData = false;
6466
std::shared_ptr<TypeConv> type = nullptr;
6567
napi_ref initValue = nullptr;
6668
};

NativeScript/ffi/Interop.mm

Lines changed: 21 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -802,7 +802,7 @@ napi_value interop_bufferFromData(napi_env env, napi_callback_info info) {
802802
napi_get_cb_info(env, info, &argc, argv, &jsThis, nullptr);
803803

804804
Reference* reference = new Reference();
805-
bool owned = true;
805+
reference->env = env;
806806

807807
if (argc == 1) {
808808
reference->initValue = make_ref(env, argv[0]);
@@ -816,9 +816,10 @@ napi_value interop_bufferFromData(napi_env env, napi_callback_info info) {
816816

817817
if (argtype == napi_object && Pointer::isInstance(env, argv[1])) {
818818
reference->data = Pointer::unwrap(env, argv[1])->data;
819-
owned = false;
819+
reference->ownsData = false;
820820
} else {
821821
reference->data = malloc(reference->type->type->size);
822+
reference->ownsData = true;
822823
bool shouldFree;
823824
reference->type->toNative(env, argv[1], reference->data, &shouldFree, &shouldFree);
824825
}
@@ -832,12 +833,7 @@ napi_value interop_bufferFromData(napi_env env, napi_callback_info info) {
832833
return nullptr;
833834
}
834835

835-
if (owned) {
836-
napi_ref ref;
837-
napi_wrap(env, jsThis, reference, Reference::finalize, nullptr, &ref);
838-
} else {
839-
napi_wrap(env, jsThis, reference, nullptr, nullptr, nullptr);
840-
}
836+
napi_wrap(env, jsThis, reference, Reference::finalize, nullptr, nullptr);
841837

842838
return jsThis;
843839
}
@@ -897,8 +893,13 @@ napi_value interop_bufferFromData(napi_env env, napi_callback_info info) {
897893
}
898894

899895
Reference::~Reference() {
900-
if (data != nullptr) {
896+
if (initValue != nullptr && env != nullptr) {
897+
napi_delete_reference(env, initValue);
898+
initValue = nullptr;
899+
}
900+
if (data != nullptr && ownsData) {
901901
free(data);
902+
data = nullptr;
902903
}
903904
}
904905

@@ -935,7 +936,17 @@ napi_value interop_bufferFromData(napi_env env, napi_callback_info info) {
935936
return jsThis;
936937
}
937938

938-
FunctionReference::~FunctionReference() {}
939+
FunctionReference::~FunctionReference() {
940+
// If closure is already created, it shares the same JS function ref.
941+
// Clear it there and delete exactly once here.
942+
if (closure != nullptr && closure->func == ref) {
943+
closure->func = nullptr;
944+
}
945+
if (ref != nullptr && env != nullptr) {
946+
napi_delete_reference(env, ref);
947+
ref = nullptr;
948+
}
949+
}
939950

940951
void* FunctionReference::getFunctionPointer(MDSectionOffset offset) {
941952
if (closure == nullptr) {

NativeScript/ffi/ObjCBridge.mm

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -113,11 +113,6 @@ void finalize_bridge_data(napi_env env, void* data, void* hint) {
113113
}
114114
mdBlockSignatureCache.clear();
115115

116-
for (auto& pair : mdFunctionSignatureCache) {
117-
delete pair.second;
118-
}
119-
mdFunctionSignatureCache.clear();
120-
121116
// Clean up ObjCClass objects
122117
for (auto& pair : classes) {
123118
delete pair.second;
@@ -142,6 +137,11 @@ void finalize_bridge_data(napi_env env, void* data, void* hint) {
142137
}
143138
cFunctionCache.clear();
144139

140+
for (auto& pair : mdFunctionSignatureCache) {
141+
delete pair.second;
142+
}
143+
mdFunctionSignatureCache.clear();
144+
145145
// if (objc_autoreleasePool != nullptr)
146146
// objc_autoreleasePoolPop(objc_autoreleasePool);
147147

NativeScript/ffi/TypeConv.mm

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -715,6 +715,7 @@ void toNative(napi_env env, napi_value value, void* result, bool* shouldFree,
715715
if (ref->data == nullptr) {
716716
ref->type = pointeeType;
717717
ref->data = malloc(pointeeType->type->size);
718+
ref->ownsData = true;
718719
if (ref->initValue) {
719720
napi_value initValue = get_ref_value(env, ref->initValue);
720721
bool shouldFree;

NativeScript/napi/v8/v8-api.cpp

Lines changed: 70 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,11 @@
11
#include <algorithm>
2+
#include <chrono>
23
#include <climits> // INT_MAX
34
#include <cmath>
5+
#include <filesystem>
46
#include <sstream>
57
#include <string_view> // string_view, u16string_view
8+
#include <thread>
69

710
#define NAPI_EXPERIMENTAL
811

@@ -3301,6 +3304,28 @@ napi_status NAPI_CDECL napi_run_script_as_module(napi_env env,
33013304
v8::Isolate* isolate = env->isolate;
33023305

33033306
v8::TryCatch module_try_catch(isolate);
3307+
auto set_last_exception = [&](v8::Local<v8::Value> exception) {
3308+
if (!env->last_exception.IsEmpty()) {
3309+
env->last_exception.Reset();
3310+
}
3311+
env->last_exception.Reset(env->isolate, exception);
3312+
};
3313+
3314+
// Normalize module path to keep cache keys stable across equivalent path
3315+
// spellings (e.g. ".", "..", and symlink traversals).
3316+
std::error_code pathError;
3317+
auto normalizedModulePath = std::filesystem::absolute(source_url, pathError);
3318+
if (pathError) {
3319+
pathError.clear();
3320+
normalizedModulePath = std::filesystem::path(source_url);
3321+
}
3322+
normalizedModulePath = normalizedModulePath.lexically_normal();
3323+
auto canonicalModulePath =
3324+
std::filesystem::weakly_canonical(normalizedModulePath, pathError);
3325+
if (!pathError) {
3326+
normalizedModulePath = canonicalModulePath;
3327+
}
3328+
const std::string modulePath = normalizedModulePath.string();
33043329

33053330
// Initialize ES module system on first use
33063331
static bool es_module_initialized = false;
@@ -3312,7 +3337,7 @@ napi_status NAPI_CDECL napi_run_script_as_module(napi_env env,
33123337
// Create script origin for ES module
33133338
v8::ScriptOrigin origin(
33143339
isolate,
3315-
v8::String::NewFromUtf8(isolate, source_url, v8::NewStringType::kNormal).ToLocalChecked(),
3340+
v8::String::NewFromUtf8(isolate, modulePath.c_str(), v8::NewStringType::kNormal).ToLocalChecked(),
33163341
0, 0, false, -1, v8::Local<v8::Value>(), false, false,
33173342
true // is_module = true for ES modules
33183343
);
@@ -3330,10 +3355,6 @@ napi_status NAPI_CDECL napi_run_script_as_module(napi_env env,
33303355

33313356
v8::Local<v8::Module> module = maybe_module.ToLocalChecked();
33323357

3333-
// Register the module in our module registry for resolution
3334-
// Use the source_url as the module path
3335-
std::string modulePath = source_url;
3336-
33373358
// Safe Global handle management: Clear any existing entry first
33383359
auto it = v8impl::g_moduleRegistry.find(modulePath);
33393360
if (it != v8impl::g_moduleRegistry.end()) {
@@ -3363,20 +3384,55 @@ napi_status NAPI_CDECL napi_run_script_as_module(napi_env env,
33633384
if (!module->InstantiateModule(context, &v8impl::ResolveModuleCallback).FromMaybe(false)) {
33643385
if (instantiate_try_catch.HasCaught()) {
33653386
// Store the exception in env->last_exception instead of throwing
3366-
v8::Local<v8::Value> exception = instantiate_try_catch.Exception();
3367-
v8::String::Utf8Value error(isolate, exception);
3368-
3369-
if (!env->last_exception.IsEmpty()) {
3370-
env->last_exception.Reset();
3371-
}
3372-
env->last_exception.Reset(env->isolate, instantiate_try_catch.Exception());
3387+
set_last_exception(instantiate_try_catch.Exception());
33733388
}
33743389
return napi_set_last_error(env, napi_generic_failure);
33753390
}
33763391

33773392
// Evaluate the module
3393+
v8::TryCatch evaluate_try_catch(isolate);
33783394
v8::MaybeLocal<v8::Value> maybe_result = module->Evaluate(context);
3379-
CHECK_MAYBE_EMPTY_WITH_PREAMBLE(env, maybe_result, napi_generic_failure);
3395+
if (maybe_result.IsEmpty()) {
3396+
if (evaluate_try_catch.HasCaught()) {
3397+
set_last_exception(evaluate_try_catch.Exception());
3398+
} else if (module->GetStatus() == v8::Module::kErrored) {
3399+
set_last_exception(module->GetException());
3400+
}
3401+
return napi_set_last_error(env, napi_generic_failure);
3402+
}
3403+
3404+
v8::Local<v8::Value> evaluate_result = maybe_result.ToLocalChecked();
3405+
3406+
// Evaluate may return a promise; surface rejection as module-load failure.
3407+
if (evaluate_result->IsPromise()) {
3408+
v8::Local<v8::Promise> promise = evaluate_result.As<v8::Promise>();
3409+
constexpr int kMaxAttempts = 100;
3410+
int attempts = 0;
3411+
while (attempts < kMaxAttempts &&
3412+
promise->State() == v8::Promise::kPending) {
3413+
isolate->PerformMicrotaskCheckpoint();
3414+
std::this_thread::sleep_for(std::chrono::milliseconds(1));
3415+
attempts++;
3416+
}
3417+
3418+
if (promise->State() == v8::Promise::kRejected) {
3419+
set_last_exception(promise->Result());
3420+
return napi_set_last_error(env, napi_generic_failure);
3421+
}
3422+
3423+
if (promise->State() == v8::Promise::kPending) {
3424+
set_last_exception(v8::Exception::Error(
3425+
v8::String::NewFromUtf8(isolate,
3426+
"Module evaluation did not settle")
3427+
.ToLocalChecked()));
3428+
return napi_set_last_error(env, napi_generic_failure);
3429+
}
3430+
}
3431+
3432+
if (module->GetStatus() == v8::Module::kErrored) {
3433+
set_last_exception(module->GetException());
3434+
return napi_set_last_error(env, napi_generic_failure);
3435+
}
33803436

33813437
// Get the module namespace as the result
33823438
v8::Local<v8::Value> namespace_obj = module->GetModuleNamespace();
@@ -3914,4 +3970,4 @@ napi_status napi_is_host_object(napi_env env, napi_value object, bool* result) {
39143970
*result = true;
39153971

39163972
return napi_ok;
3917-
}
3973+
}

0 commit comments

Comments
 (0)