| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting. Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR migrates NativeScript’s iOS V8 integration to V8 14.9, adds checksum-verified prebuilt artifact installation and inspector vendoring, updates runtime bindings and callbacks, removes obsolete vendored V8 sources, changes module and teardown behavior, and updates CI, documentation, and version metadata. ChangesV8 14.9 migration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: nathanwalker Poem 🚥 Pre-merge checks | ✅ 4 | ❌ 1 ❌ Failed checks (1 warning)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. ❤️ ShareComment @coderabbitai help to get the list of available commands. |
Sorry, something went wrong.
Mechanical, except where noted: - Context::GetIsolate() removed -> Isolate::GetCurrent() - External::New/Value() and the aligned internal-field accessors take a type tag - ScriptOrigin no longer takes an isolate; AccessControl is gone - GetInternalField returns Local<Data> - GetCreationContext[Checked]() need an isolate - V8Inspector::connect takes a trust level; createForConsoleAPI takes a span; the dynamic-import callback is reshaped - String::Value -> String::ValueView, remembering that a one-byte V8 string is Latin-1 rather than UTF-8 - Local is trivially destructible now, so unused locals trip -Wunused-variable Accessors are the part that is not mechanical. PropertyCallbackInfo no longer exposes the receiver, and SetNativeDataProperty is not a drop-in for SetAccessor on anything inherited from. ClassBuilder's super (on the implementation object) and Reference's value (on a prototype template) both need the receiver, and are now function-backed. Static properties on constructor functions matter most: SetNativeDataProperty installs something data-like, so assigning through a derived class shadowed the base with an own property and never reached the native setter. Interceptors now return v8::Intercepted. A path that set a return value or threw becomes kYes; a path that returned without setting one -- including falling off the end -- becomes kNo. The fall-through case is load-bearing: the swizzling definer depends on V8 still running the real defineProperty, and the setters depend on V8 still performing the store. Two parity traps worth recording. Object::SetAccessor defaulted to PropertyAttribute::None, so super's attributes are passed explicitly rather than assumed. ArrayBuffer::Detach now returns Maybe<bool> and is warn-unused-result, which invites a .Check() that aborts the process on a detach-key mismatch; the result is discarded instead, matching the void Detach it replaced. Isolate::VisitHandlesWithClassIds was removed, so teardown disposal walks an intrusive list of registered wrappers that unlink themselves from their GC finalizer, rather than enumerating V8's handles.
The libraries and the headers that must match them now come from a pinned release of NativeScript/v8-buildscripts, installed by ./download_v8.sh and verified against the SHA256SUMS published with it. V8_RELEASE is the pin. The full matrix cannot be produced on any single machine -- the Apple variants need macOS, the 32-bit Android ABIs need an ia32-capable host -- so hand-assembled binaries are neither reproducible nor verifiable. That is also why fetch_v8.sh and build_v8_source*.sh go: producing V8 belongs in the build repo, which can build all of it, and a second copy of the gn args here is just two sets that can disagree. The vendored headers are ignored along with the libraries. Keeping a copy in git is how it drifts out of step with the libraries it describes: third_party/inspector_protocol/crdtp was still on 10.3 while libcrdtp.a was built from 14.9. Sourcing both from one verified artifact makes that impossible. The inspector's internals are re-derived by closure from the release's src-headers artifact, which also prunes the tree from 164 files to the 66 actually reachable from the glue. It is a standalone script rather than an Xcode build phase, matching download_llvm.sh: a prerequisite you run once, a no-op when the artifacts are in place, and V8_SKIP_DOWNLOAD=1 to keep a local V8 build. build_nativescript.sh calls it, so the vision path is covered too. libffi stays tracked; it is not V8.
Under docs/knowledge rather than the repo root: they are a record of a completed migration, not instructions, and next to error-handling.md they would read as the latter.
V8 14.9's v8-memory-span.h defines MemorySpan<T>::Iterator, a nested class with no element_type. Xcode 15.4's libc++ leaves the primary __pointer_traits_element_type undefined, so <memory> instantiating it for that type is a hard error rather than a SFINAE miss; newer libc++ defines it as an empty struct and the instantiation drops out. The floor is the standard library, not our code, so the runner and Xcode both move up. Xcode 16 ships the iOS 18 runtime, so the destination's OS=17.2 pin no longer resolves -- it is OS=latest on an iPhone 16 Pro now. download_v8.sh moves into setup-build-env because the test job drives xcodebuild directly and never reaches build_nativescript.sh, which is what installs V8 for the build job. Shared setup is the one place both jobs go through. It is idempotent, so the build job running it twice costs nothing.
An unresolvable bare specifier resolved to a placeholder module whose exports was a Proxy that threw on property access, so requiring a missing module without dereferencing it succeeded silently. The ESM resolver had the same construct, plus debug-mode branches that returned an empty MaybeLocal without scheduling an exception, which is not a valid resolve callback result. The placeholder never worked in the first place: the module name was interpolated unquoted into a single-quoted JS string literal, so the generated proxy script was always a SyntaxError. On V8 10.3 the pending exception left by that failed compile escaped the require callback, which is the only reason "should throw error if cant find node module" passed. Both paths now throw "Cannot find module '<specifier>'".
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)NativeScript/runtime/URLPatternImpl.cpp (1)142-148: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Reject foreign receivers before reading internal fields.
URLPatternImpl::GetPointer and URLImpl::GetPointer are called by ordinary property accessors, so JS can invoke them with .call()/.apply() on an unrelated object. Guard each receiver with InternalFieldCount() < 1 before GetAlignedPointerFromInternalField(0, ...) to avoid V8 undefined behavior/type confusion.
🤖 Prompt for AI AgentsVerify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@NativeScript/runtime/URLPatternImpl.cpp` around lines 142 - 148, Guard both URLPatternImpl::GetPointer in NativeScript/runtime/URLPatternImpl.cpp (lines 142-148) and URLImpl::GetPointer in NativeScript/runtime/URLImpl.cpp (lines 23-30) by returning nullptr when object->InternalFieldCount() is less than 1, before calling GetAlignedPointerFromInternalField; preserve the existing null-pointer handling afterward.
docs/knowledge/v8-14-migration.md (1)🤖 Prompt for all review comments with AI agentsNativeScript/runtime/ModuleInternal.mm (1)215-227: 🩺 Stability & Availability | 🔵 Trivial
Track the intermittent deadlock as a release risk.
A ten-minute lock-order hang is an availability issue even if it has not reproduced recently. Please attach a tracked issue and add a regression/stress scenario or explicit CI timeout diagnostics before treating the suite as fully green.
🤖 Prompt for AI AgentsVerify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/knowledge/v8-14-migration.md` around lines 215 - 227, Update the “An intermittent worker/notification deadlock” section to reference a tracked issue and document either a regression/stress scenario or explicit CI timeout diagnostics for this lock-order hang. Keep the existing reproduction details and require these safeguards before considering the suite fully green.NativeScript/runtime/WorkerWrapper.mm (1)403-405: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Duplicated "Cannot find module" framing produces a doubly-wrapped message.
ResolvePath already throws the detailed Cannot find module '<name>' error, and LoadImpl catches it and re-wraps it with Failed to resolve module: '<name>' plus the same base-dir context. The user-visible string ends up repeating the specifier and base directory three times. Consider letting the ResolvePath message pass through unwrapped so the thrown text matches the Node-style Cannot find module '<specifier>' the PR advertises.
Also applies to: 1045-1060
🤖 Prompt for AI AgentsVerify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@NativeScript/runtime/ModuleInternal.mm` around lines 403 - 405, Remove the duplicate “Cannot find module” exception construction in ResolvePath and let its existing detailed error propagate through LoadImpl without re-wrapping. Update the LoadImpl catch/error path as needed so the user-visible message remains the Node-style “Cannot find module '<specifier>'” without repeated module or base-directory context.NativeScript/runtime/MetadataBuilder.mm (1)219-219: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Inconsistent isolate source within functions that already hold the right isolate as a member.
CallOnErrorHandlers/PassUncaughtExceptionFromWorkerToMain/ConstructErrorObject all mix v8::Isolate::GetCurrent() with the already-available this->workerIsolate_ / this->mainIsolate_ members in the same function body. Current call sites all establish the matching Isolate::Scope first, so this works today, but relying on GetCurrent() when the correct isolate is already a class member is a latent trap for future callers.
♻️ Suggested direction- Isolate* isolate = v8::Isolate::GetCurrent(); - tns::Assert(success, isolate); + tns::Assert(success, this->workerIsolate_);Also applies to: 247-247, 380-380
🤖 Prompt for AI AgentsVerify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@NativeScript/runtime/WorkerWrapper.mm` at line 219, Replace the local v8::Isolate::GetCurrent() usages in CallOnErrorHandlers, PassUncaughtExceptionFromWorkerToMain, and ConstructErrorObject with the matching workerIsolate_ or mainIsolate_ member used by each function. Keep the existing isolate scopes and behavior unchanged.NativeScript/runtime/Timers.cpp (2)904-932: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
Minor asymmetry: empty-name struct write is silently swallowed vs. "field not found" falling through.
StructPropertySetterCallback returns Intercepted::kYes for propertyName == "" (blocks the ordinary store) but Intercepted::kNo for "field not found" (allows the ordinary store). Since struct property names are never realistically empty strings, this is unlikely to matter in practice.
🤖 Prompt for AI AgentsVerify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@NativeScript/runtime/MetadataBuilder.mm` around lines 904 - 932, Update StructPropertySetterCallback so an empty propertyName follows the same fallback behavior as an unknown struct field: return Intercepted::kNo instead of swallowing the write. Preserve the existing field lookup and Interop::SetStructPropertyValue flow for recognized fields.127-130: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Discarded NewFunctionTemplate result.
This call creates a FunctionTemplate and an External that are never used — the actual bindings come from the SetMethod calls below. Since you are already touching these lines for the pointer tag, dropping the dead call removes a per-isolate allocation and some confusion.
🤖 Prompt for AI AgentsVerify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@NativeScript/runtime/Timers.cpp` around lines 127 - 130, Remove the unused tns::NewFunctionTemplate call involving Timers::SetTimeoutCallback and its v8::External allocation; retain the SetMethod bindings below as the sole registration path.
171-172: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Prefer the already-scoped isolate local over Isolate::GetCurrent().
isolate (from task->isolate_) is the isolate whose Isolate::Scope/Locker is active here, so passing it directly is both clearer and independent of ambient current-isolate state.
♻️ Use the local isolate🤖 Prompt for AI Agents- v8::Local<v8::Context> context = - cb->GetCreationContextChecked(v8::Isolate::GetCurrent()); + v8::Local<v8::Context> context = cb->GetCreationContextChecked(isolate);Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@NativeScript/runtime/Timers.cpp` around lines 171 - 172, Update the context lookup in the timer callback to pass the already-scoped isolate local from task->isolate_ to GetCreationContextChecked, replacing the ambient Isolate::GetCurrent() lookup while preserving the existing callback and context flow.
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. Inline comments: In `@docs/knowledge/v8-14-migration.md`: - Around line 193-198: Update the migration documentation’s NativeScript/lib architecture follow-up to resolve all six remaining V8 10.3 slices before release: rebuild or remove each supported slice, or explicitly document that download_v8.sh replaces them. Ensure the documented state no longer permits packaging older libraries alongside V8 14.9 headers. In `@docs/knowledge/v8-resurrecting-finalizers.md`: - Around line 137-140: Renumber the “Resurrect-then-rearm, end to end” checklist entry from item 3 to item 2 so the test plan numbering is sequential. - Around line 3-7: Run the finalizer runtime acceptance tests against the patched V8, covering resurrect/rearm, nested-GC, young-generation, and worker scenarios. Update the documented Status and test listing in v8-resurrecting-finalizers.md to record the runtime results, retaining the compilation status and clearly indicating any failures or unresolved validation. In `@download_v8.sh`: - Around line 55-58: Update the V8_SKIP_DOWNLOAD early-exit branch in download_v8.sh so local builds still synchronize NativeScript/inspector internals: require the local source and generated paths and rerun the vendoring step before exiting, or explicitly reject local builds whose V8 revision does not match. Preserve the existing skip behavior only for compatible, already-synchronized builds. In `@NativeScript/runtime/ExtVector.cpp`: - Around line 55-59: Update the out-of-range branch in the vector getter to return v8::Intercepted::kNo instead of setting undefined and returning kYes, preserving V8’s normal own/prototype lookup and matching the setter’s fall-through behavior. In `@NativeScript/runtime/ModuleInternalCallbacks.mm`: - Around line 2406-2413: Remove the unreachable __nsHmrRequestModule fetch-retry path and its related callbacks, including the code around ImportModuleDynamicallyCallback and the external-pointer handling at the referenced sections, or hoist the retry logic before the existing maybeModule.IsEmpty() early return so it can execute. Ensure the chosen change eliminates the dead guard and preserves valid dynamic import behavior. In `@NativeScript/runtime/Pointer.cpp`: - Around line 159-161: The PointerWrapper retrieval in Pointer.cpp must validate that info.This() has an internal field containing an External and that the resulting PointerWrapper* is non-null before accessing its data; throw a TypeError for invalid receivers. In ObjectManager.mm, validate superField->IsValue() before calling As<v8::Value>() when reading internal field 1. In `@NativeScript/runtime/SymbolIterator.mm`: - Around line 27-29: Update the Symbol.iterator installation in the surrounding iterator setup to assert the result of Set, matching the other Set calls in SymbolIterator.mm; do not leave the assigned success value unchecked. In `@NativeScript/runtime/UnmanagedType.mm`: - Around line 67-90: Update UnmanagedType::TakeValue to mark the wrapper as consumed after Interop::GetResult completes, covering both retained and unretained paths before returning. Preserve the existing ValueTaken guard and ensure the retained release occurs only once. In `@NativeScript/runtime/URLImpl.cpp`: - Around line 76-90: Update URLImpl::Ctor to handle count == 0 before accessing args[0], and only call tns::ToString after confirming the argument is a string; preserve the existing TypeError behavior for present non-string values. Apply the same validation to URLImpl::CanParse for info[0] and info[1], avoiding unchecked .As<v8::String>() casts and preserving its expected behavior for omitted arguments. --- Outside diff comments: In `@NativeScript/runtime/URLPatternImpl.cpp`: - Around line 142-148: Guard both URLPatternImpl::GetPointer in NativeScript/runtime/URLPatternImpl.cpp (lines 142-148) and URLImpl::GetPointer in NativeScript/runtime/URLImpl.cpp (lines 23-30) by returning nullptr when object->InternalFieldCount() is less than 1, before calling GetAlignedPointerFromInternalField; preserve the existing null-pointer handling afterward. --- Nitpick comments: In `@docs/knowledge/v8-14-migration.md`: - Around line 215-227: Update the “An intermittent worker/notification deadlock” section to reference a tracked issue and document either a regression/stress scenario or explicit CI timeout diagnostics for this lock-order hang. Keep the existing reproduction details and require these safeguards before considering the suite fully green. In `@NativeScript/runtime/MetadataBuilder.mm`: - Around line 904-932: Update StructPropertySetterCallback so an empty propertyName follows the same fallback behavior as an unknown struct field: return Intercepted::kNo instead of swallowing the write. Preserve the existing field lookup and Interop::SetStructPropertyValue flow for recognized fields. In `@NativeScript/runtime/ModuleInternal.mm`: - Around line 403-405: Remove the duplicate “Cannot find module” exception construction in ResolvePath and let its existing detailed error propagate through LoadImpl without re-wrapping. Update the LoadImpl catch/error path as needed so the user-visible message remains the Node-style “Cannot find module '<specifier>'” without repeated module or base-directory context. In `@NativeScript/runtime/Timers.cpp`: - Around line 127-130: Remove the unused tns::NewFunctionTemplate call involving Timers::SetTimeoutCallback and its v8::External allocation; retain the SetMethod bindings below as the sole registration path. - Around line 171-172: Update the context lookup in the timer callback to pass the already-scoped isolate local from task->isolate_ to GetCreationContextChecked, replacing the ambient Isolate::GetCurrent() lookup while preserving the existing callback and context flow. In `@NativeScript/runtime/WorkerWrapper.mm`: - Line 219: Replace the local v8::Isolate::GetCurrent() usages in CallOnErrorHandlers, PassUncaughtExceptionFromWorkerToMain, and ConstructErrorObject with the matching workerIsolate_ or mainIsolate_ member used by each function. Keep the existing isolate scopes and behavior unchanged.
Fix all unresolved CodeRabbit comments on this PR:
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 965742ff-aedf-4dd9-9e33-85af7ef1f02f
📥 CommitsReviewing files that changed from the base of the PR and between 55ffdef and cd8671e.
📒 Files selected for processing (432)
Sorry, something went wrong.
| Nothing blocking. Two follow-ups: | ||
|
|
||
| - Only `arm64-iphonesimulator` has been rebuilt; the other six architectures in | ||
| `NativeScript/lib` still hold 10.3 libraries. | ||
| - `DisposerPHV.{h,mm}` is now dead code -- `Isolate::VisitHandlesWithClassIds` no longer exists, | ||
| so the visitor can never be driven. Its logic moved to `ObjectManager::DisposeAllRegistered()`. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repository files of interest:"
git ls-files | rg '(^docs/knowledge/v8-14-migration.md$|download_v8\.sh$|NativeScript/lib|v8-14-migration|NativeScript)' || true
echo
echo "--- docs/knowledge/v8-14-migration.md relevant lines ---"
if [ -f docs/knowledge/v8-14-migration.md ]; then
nl -ba docs/knowledge/v8-14-migration.md | sed -n '160,220p'
fi
echo
echo "--- NativeScript related files ---"
git ls-files | rg 'NativeScript/(lib|.*\.framework|.*\.dylib|.*\.so)|libNativeScript|NativeScript\.framework|Frameworks' || true
echo
echo "--- download_v8.sh if present ---"
if [ -f download_v8.sh ]; then
nl -ba download_v8.sh | sed -n '1,220p'
fi
echo
echo "--- v8 version references ---"
rg -n "10\.3|14\.9|NativeScript/lib|arm64-iphonesimulator|iphonesimulator|iphoneos|iphoneos|device" docs/knowledge/v8-14-migration.md download_v8.sh 2>/dev/null || trueRepository: NativeScript/ios
Length of output: 2017
Resolve the remaining NativeScript/lib architecture artifacts before release.
The migration doc still lists six NativeScript/lib slices holding V8 10.3 libraries. If those slices are consumed or packaged, they can mix older 10.3 libraries with the newer V8 14.9 headers; rebuild, remove, or document that download_v8.sh replaces each supported slice.
🤖 Prompt for AI AgentsVerify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/knowledge/v8-14-migration.md` around lines 193 - 198, Update the migration documentation’s NativeScript/lib architecture follow-up to resolve all six remaining V8 10.3 slices before release: rebuild or remove each supported slice, or explicitly document that download_v8.sh replaces them. Ensure the documented state no longer permits packaging older libraries alongside V8 14.9 headers.
Sorry, something went wrong.
| if [ "${V8_SKIP_DOWNLOAD:-0}" != "0" ]; then | ||
| echo "V8_SKIP_DOWNLOAD is set; leaving NativeScript/lib and the headers alone." | ||
| exit 0 | ||
| fi |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Keep inspector internals synchronized for local V8 builds.
Line 55 exits before the vendoring step. A local V8 build at another revision therefore compiles against stale NativeScript/inspector internals, despite the documented local-build workflow. Require local source/gen paths and rerun vendoring, or explicitly reject version-mismatched local builds.
🤖 Prompt for AI AgentsVerify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@download_v8.sh` around lines 55 - 58, Update the V8_SKIP_DOWNLOAD early-exit branch in download_v8.sh so local builds still synchronize NativeScript/inspector internals: require the local source and generated paths and rerun the vendoring step before exiting, or explicitly reject local builds whose V8 revision does not match. Preserve the existing skip behavior only for compatible, already-synchronized builds.
Sorry, something went wrong.
| if (offset >= ffiType->size) { | ||
| // Trying to access an element outside of the vector size | ||
| info.GetReturnValue().SetUndefined(); | ||
| return v8::Intercepted::kYes; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Out-of-range reads now swallow ordinary lookup.
The old void getter fell through for offset >= ffiType->size, so V8 continued with the normal own/prototype lookup. Returning kYes with undefined intercepts the read instead — combined with the setter returning kNo (which lets V8 perform the ordinary store), an out-of-range index can be written but never read back. Returning kNo preserves the previous semantics and stays symmetric with the setter.
♻️ Preserve fall-through for out-of-range indices ffi_type* ffiType = extVectorWrapper->FFIType();
if (offset >= ffiType->size) {
- // Trying to access an element outside of the vector size
- info.GetReturnValue().SetUndefined();
- return v8::Intercepted::kYes;
+ // Outside of the vector size: not intercepted, let V8 perform the
+ // ordinary lookup (what the old void-returning callback did).
+ return v8::Intercepted::kNo;
}‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (offset >= ffiType->size) { | |
| // Trying to access an element outside of the vector size | |
| info.GetReturnValue().SetUndefined(); | |
| return v8::Intercepted::kYes; | |
| } | |
| if (offset >= ffiType->size) { | |
| // Outside of the vector size: not intercepted, let V8 perform the | |
| // ordinary lookup (what the old void-returning callback did). | |
| return v8::Intercepted::kNo; | |
| } |
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@NativeScript/runtime/ExtVector.cpp` around lines 55 - 59, Update the out-of-range branch in the vector getter to return v8::Intercepted::kNo instead of setting undefined and returning kYes, preserving V8’s normal own/prototype lookup and matching the setter’s fall-through behavior.
Sorry, something went wrong.
| v8::Isolate* isolateInner = info.GetIsolate(); | ||
| v8::HandleScope hs(isolateInner); | ||
| if (!info.Data()->IsExternal()) return; | ||
| auto* d = static_cast<FetchRetryData*>(info.Data().As<v8::External>()->Value()); | ||
| auto* d = static_cast<FetchRetryData*>( | ||
| info.Data().As<v8::External>()->Value(v8::kExternalPointerTypeTagDefault)); | ||
| v8::Local<v8::Context> ctx = isolateInner->GetCurrentContext(); | ||
| v8::Local<v8::Promise::Resolver> res = d->resolver.Get(isolateInner); | ||
| v8::Local<v8::String> specLocal = d->spec.Get(isolateInner); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
These external-pointer updates sit in an unreachable block.
ImportModuleDynamicallyCallback already returns at Line 2347 whenever maybeModule.IsEmpty(), so the if (maybeModule.IsEmpty()) guard at Line 2364 — and the whole __nsHmrRequestModule fetch-retry path including the callbacks touched here — can never execute. Either hoist this retry before the early-return at Lines 2347-2360 or delete the dead path; migrating tags inside it gives a false sense of coverage.
Also applies to: 2449-2460, 2462-2475
🤖 Prompt for AI AgentsVerify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@NativeScript/runtime/ModuleInternalCallbacks.mm` around lines 2406 - 2413, Remove the unreachable __nsHmrRequestModule fetch-retry path and its related callbacks, including the code around ImportModuleDynamicallyCallback and the external-pointer handling at the referenced sections, or hoist the retry logic before the existing maybeModule.IsEmpty() early return so it can execute. Ensure the chosen change eliminates the dead guard and preserves valid dynamic import behavior.
Sorry, something went wrong.
| PointerWrapper* wrapper = static_cast<PointerWrapper*>( | ||
| info.This()->GetInternalField(0).As<External>()->Value( | ||
| v8::kExternalPointerTypeTagDefault)); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unchecked Local<Data> downcasts on internal fields. V8 13+ returns Local<Data> from GetInternalField, and both sites downcast with As<...>() without verifying the field actually holds the expected type — unchecked in release, a CHECK failure in debug.
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@NativeScript/runtime/Pointer.cpp` around lines 159 - 161, The PointerWrapper retrieval in Pointer.cpp must validate that info.This() has an internal field containing an External and that the resulting PointerWrapper* is non-null before accessing its data; throw a TypeError for invalid receivers. In ObjectManager.mm, validate superField->IsValue() before calling As<v8::Value>() when reading internal field 1.
Sorry, something went wrong.
| Local<Value> symbolIteratorKey = Symbol::GetIterator(isolate); | ||
| success = object.As<Object>()->Set(context, symbolIteratorKey, iterator).FromMaybe(false); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
success is assigned and never checked.
A failed Symbol.iterator install is silently swallowed here, unlike every other Set in this file which asserts. Add the missing assertion.
🐛 Proposed fix Local<Value> symbolIteratorKey = Symbol::GetIterator(isolate);
success = object.As<Object>()->Set(context, symbolIteratorKey, iterator).FromMaybe(false);
+ tns::Assert(success, isolate);
}‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Local<Value> symbolIteratorKey = Symbol::GetIterator(isolate); | |
| success = object.As<Object>()->Set(context, symbolIteratorKey, iterator).FromMaybe(false); | |
| } | |
| Local<Value> symbolIteratorKey = Symbol::GetIterator(isolate); | |
| success = object.As<Object>()->Set(context, symbolIteratorKey, iterator).FromMaybe(false); | |
| tns::Assert(success, isolate); | |
| } |
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@NativeScript/runtime/SymbolIterator.mm` around lines 27 - 29, Update the Symbol.iterator installation in the surrounding iterator setup to assert the result of Set, matching the other Set calls in SymbolIterator.mm; do not leave the assigned success value unchecked.
Sorry, something went wrong.
| Local<Value> UnmanagedType::TakeValue(const FunctionCallbackInfo<Value>& info, bool retained) { | ||
| Isolate* isolate = info.GetIsolate(); | ||
| Local<Context> context = isolate->GetCurrentContext(); | ||
| Isolate* isolate = info.GetIsolate(); | ||
| Local<Context> context = isolate->GetCurrentContext(); | ||
|
|
||
| BaseDataWrapper* baseWrapper = tns::GetValue(isolate, info.This()); | ||
| UnmanagedTypeWrapper* wrapper = static_cast<UnmanagedTypeWrapper*>(baseWrapper); | ||
|
|
||
| if (wrapper->ValueTaken()) { | ||
| throw NativeScriptException("Unmanaged value has already been consumed."); | ||
| } | ||
| BaseDataWrapper* baseWrapper = tns::GetValue(isolate, info.This()); | ||
| UnmanagedTypeWrapper* wrapper = static_cast<UnmanagedTypeWrapper*>(baseWrapper); | ||
|
|
||
| uint8_t* data = wrapper->Data(); | ||
| const TypeEncoding* typeEncoding = wrapper->TypeEncoding(); | ||
| if (wrapper->ValueTaken()) { | ||
| throw NativeScriptException("Unmanaged value has already been consumed."); | ||
| } | ||
|
|
||
| BaseCall call((uint8_t*)&data); | ||
| Local<Value> result = Interop::GetResult(context, typeEncoding, &call, false); | ||
| uint8_t* data = wrapper->Data(); | ||
| const TypeEncoding* typeEncoding = wrapper->TypeEncoding(); | ||
|
|
||
| if (retained) { | ||
| id value = static_cast<id>((void*)data); | ||
| [value release]; | ||
| } | ||
| BaseCall call((uint8_t*)&data); | ||
| Local<Value> result = Interop::GetResult(context, typeEncoding, &call, false); | ||
|
|
||
| return result; | ||
| } | ||
| if (retained) { | ||
| id value = static_cast<id>((void*)data); | ||
| [value release]; | ||
| } | ||
|
|
||
| return result; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain🏁 Script executed:
#!/bin/bash
fd -e h -e mm -e cpp | xargs rg -n 'ValueTaken|UnmanagedTypeWrapper' -A3 -B3Repository: NativeScript/ios
Length of output: 2046
🏁 Script executed:
#!/bin/bash
set -u
printf 'PWD: %s\n' "$(pwd)"
printf '\nFiles matching UnmanagedType (case-insensitive name):\n'
fd -i 'UnmanagedType' . 2>/dev/null || true
printf '\nMatching symbols (ValueTaken/UnmanagedTypeWrapper) in tracked files:\n'
git ls-files '*.[hmmcpp]' 2>/dev/null | xargs -r rg -n 'ValueTaken|UnmanagedTypeWrapper|takeRetainedValue|SetValueTaken' -A4 -B4 || trueRepository: NativeScript/ios
Length of output: 2017
🌐 Web query:
NativeScript ios UnmanagedTypeWrapper ValueTaken TakeValue SetValueTaken UnmanagedType.mm
💡 Result:
In the NativeScript iOS runtime, UnmanagedTypeWrapper, ValueTaken, TakeValue, and SetValueTaken are internal implementation details related to the handling of unmanaged objects—specifically Core Foundation objects—that lack clear memory management annotations [1][2]. These terms are associated with the C++/Objective-C++ implementation of the NativeScript iOS runtime (specifically within files like UnmanagedType.mm or UnmanagedType.cpp in older versions of the runtime) [2]. The logic is designed to facilitate the conversion between native Core Foundation types and JavaScript-managed objects [1][2]. Key technical aspects include: 1. Handling Unmanaged Objects: When a native function returns a Core Foundation object without annotations indicating how it should be memory managed (e.g., whether it is retained or unretained), NativeScript wraps it in an Unmanaged type [1][3]. This places the responsibility on the developer to manage the object's lifecycle [1][4]. 2. Internal Methods: Methods such as takeValue, takeRetainedValue, and takeUnretainedValue are used to bridge these objects to JavaScript [2]. Specifically, takeRetainedValue returns the object and decrements the reference count (consuming the "unbalanced retain"), while takeUnretainedValue returns the object without altering the reference count [1][4]. 3. Implementation: Internally, logic within the runtime (historically referenced in PRs like #386 in the ios-runtime repository) uses helper methods to distinguish between these two states to ensure proper memory management behavior, such as calling [result release] when a value is "taken" in a retained context [2]. These internal wrappers ensure that NativeScript can safely interact with iOS APIs that do not adhere to standard Objective-C memory management rules, allowing developers to use JavaScript to interact with memory-sensitive C-based frameworks [1][2][4].
Citations:
🌐 Web query:
site:github.com/NativeScript/ios UnmanagedTypeWrapper.cpp SetValueTaken ValueTaken TakeValue
💡 Result:
Based on the available search results, there is no public technical documentation or specific code reference for a file named UnmanagedTypeWrapper.cpp containing the functions SetValueTaken, ValueTaken, or TakeValue. The search results for the NativeScript/ios repository, which frequently uses terms like UnmanagedType, Interop, and ObjectManager, do not reference these specific identifiers [1][2][3][4][5]. It is possible that these identifiers are part of a private codebase, a specific internal utility, or were misnamed in the query.
Citations:
Set the taken flag after consuming the unmanaged value.
TakeValue checks wrapper->ValueTaken() but never sets it in either path, so repeated takeRetainedValue() / takeUnretainedValue() calls would bypass the guard and double-release the retained Objective-C object. Mark the value consumed before returning, or track consumption in a shared path after Interop::GetResult.
🤖 Prompt for AI AgentsVerify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@NativeScript/runtime/UnmanagedType.mm` around lines 67 - 90, Update UnmanagedType::TakeValue to mark the wrapper as consumed after Interop::GetResult completes, covering both retained and unretained paths before returning. Preserve the existing ValueTaken guard and ensure the retained release occurs only once.
Sorry, something went wrong.
| void URLImpl::Ctor(const v8::FunctionCallbackInfo<v8::Value>& args) { | ||
| auto count = args.Length(); | ||
| auto value = args[0]; | ||
| auto isolate = args.GetIsolate(); | ||
| auto context = isolate->GetCurrentContext(); | ||
| if (count >= 1 && !value->IsString()) { | ||
| isolate->ThrowException(v8::Exception::TypeError(ToV8String(isolate, ""))); | ||
| return; | ||
| } | ||
|
|
||
| url_aggregator url; | ||
|
|
||
| auto url_string = tns::ToString(isolate, args[0].As<v8::String>()); | ||
|
|
||
| if (count > 1) { | ||
| if (args[1]->IsString()) { | ||
| auto base_string = tns::ToString(isolate, args[1].As<v8::String>()); | ||
| std::string_view base_string_view(base_string.data(), base_string.length()); | ||
|
|
||
| if (!can_parse(url_string, &base_string_view)) { | ||
| isolate->ThrowException( | ||
| v8::Exception::TypeError(ToV8String(isolate, ""))); | ||
| return; | ||
| } | ||
| auto base_url = ada::parse<ada::url_aggregator>(base_string_view, nullptr); | ||
|
|
||
| auto result = ada::parse<ada::url_aggregator>(url_string, &base_url.value()); | ||
|
|
||
| if (result) { | ||
| url = result.value(); | ||
| } else { | ||
| isolate->ThrowException( | ||
| v8::Exception::TypeError(ToV8String(isolate, ""))); | ||
| return; | ||
| } | ||
| } else if (args[1]->IsObject()) { | ||
| auto base_string = tns::ToString(isolate, | ||
| args[1]->ToString(context).ToLocalChecked()); | ||
| std::string_view base_string_view(base_string.data(), base_string.length()); | ||
| if (!can_parse(std::string_view(url_string.data(), url_string.length()), | ||
| &base_string_view)) { | ||
| isolate->ThrowException( | ||
| v8::Exception::TypeError(ToV8String(isolate, ""))); | ||
| return; | ||
| } | ||
|
|
||
|
|
||
| auto base_url = ada::parse<ada::url_aggregator>(base_string_view, nullptr); | ||
|
|
||
| auto result = ada::parse<ada::url_aggregator>(url_string, &base_url.value()); | ||
|
|
||
| if (result) { | ||
| url = result.value(); | ||
| } else { | ||
| isolate->ThrowException( | ||
| v8::Exception::TypeError(ToV8String(isolate, ""))); | ||
| return; | ||
| } | ||
| } else { | ||
| // treat 2nd arg as undefined otherwise. | ||
| auto result = ada::parse<ada::url_aggregator>(url_string, nullptr); | ||
| if (result) { | ||
| url = result.value(); | ||
| } else { | ||
| isolate->ThrowException( | ||
| v8::Exception::TypeError(ToV8String(isolate, ""))); | ||
| return; | ||
| } | ||
| } | ||
| } else { | ||
| auto result = ada::parse<ada::url_aggregator>(url_string, nullptr); | ||
| if (result) { | ||
| url = result.value(); | ||
| } else { | ||
| isolate->ThrowException( | ||
| v8::Exception::TypeError(ToV8String(isolate, ""))); | ||
| return; | ||
| } | ||
| url_aggregator url; | ||
|
|
||
| } | ||
| auto url_string = tns::ToString(isolate, args[0].As<v8::String>()); | ||
|
|
||
| auto ret = args.This(); | ||
| if (count > 1) { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unchecked .As<v8::String>() cast on missing/non-string arguments.
In Ctor, when count == 0 the count >= 1 && !value->IsString() guard is skipped, so args[0] (Undefined) still gets .As<v8::String>()-cast and passed to tns::ToString. CanParse has the same problem: info[0]/info[1] are cast unconditionally without an IsString() check. .As<T>() performs no runtime type check, so calling String-specific accessors on an Undefined/non-String handle is undefined behavior — reachable via plain new URL() or URL.canParse(). URLPatternImpl::Ctor and URLSearchParamsImpl::Ctor both handle the zero-argument case safely; URLImpl does not.
🐛 Proposed fix auto count = args.Length();
auto value = args[0];
auto isolate = args.GetIsolate();
auto context = isolate->GetCurrentContext();
- if (count >= 1 && !value->IsString()) {
+ if (count < 1 || !value->IsString()) {
isolate->ThrowException(v8::Exception::TypeError(ToV8String(isolate, "")));
return;
} void URLImpl::CanParse(const v8::FunctionCallbackInfo<v8::Value>& info) {
bool value;
auto count = info.Length();
-
auto isolate = info.GetIsolate();
- if (count > 1) {
+ if (count < 1 || !info[0]->IsString() ||
+ (count > 1 && !info[1]->IsString())) {
+ value = false;
+ } else if (count > 1) {
auto url_string = tns::ToString(isolate, info[0].As<v8::String>());
auto base_string = tns::ToString(isolate, info[1].As<v8::String>());
std::string_view base_string_view(base_string.data(), base_string.length());
value = can_parse(url_string, &base_string_view);
} else {
value = can_parse(tns::ToString(isolate, info[0].As<v8::String>()).c_str(),
nullptr);
}Also applies to: 474-490
🤖 Prompt for AI AgentsVerify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@NativeScript/runtime/URLImpl.cpp` around lines 76 - 90, Update URLImpl::Ctor to handle count == 0 before accessing args[0], and only call tns::ToString after confirming the argument is a string; preserve the existing TypeError behavior for present non-string values. Apply the same validation to URLImpl::CanParse for info[0] and info[1], avoiding unchecked .As<v8::String>() casts and preserving its expected behavior for omitted arguments.
Sorry, something went wrong.
URLImpl::Ctor guards with `count >= 1 && !value->IsString()`, so a zero-argument `new URL()` skipped the guard and still cast args[0] to Local<String>; CanParse never checked its arguments at all. tns::ToString takes a Local<Value> and coerces, so the cast was a round trip that only mistyped the handle -- harmless here, a CheckCast abort under a V8_ENABLE_CHECKS build. Dropping it preserves behaviour exactly.
The stamp records which release was installed, not that its files are still on disk, so a checkout that lost the header or inspector tree -- which happens when moving across the commit that untracks them -- kept reporting the release as installed.
The migration note still listed six NativeScript/lib slices holding 10.3 libraries, which the move to the pinned release resolved. The finalizer note still said the patch had never run; the suite now exercises it, so the status distinguishes that from the flag-driven scenarios that remain unrun. Also renumbers a list that skipped an item.
There was a problem hiding this comment.
docs/knowledge/v8-resurrecting-finalizers.md (1)🤖 Prompt for all review comments with AI agents3-4: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Clarify the acceptance-gate scope.
The document reports the 904-test suite as passing, but also lists five correctness-critical, flag-dependent scenarios as unverified. Calling the existing suite “the acceptance gate” is ambiguous; rename it to “baseline regression gate” or explicitly state that the unrun scenarios remain required acceptance work.
Also applies to: 120-121, 154-156
🤖 Prompt for AI AgentsVerify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/knowledge/v8-resurrecting-finalizers.md` around lines 3 - 4, Clarify the acceptance criteria in the document’s status and related test-result sections: rename the 904-test suite to “baseline regression gate” and explicitly state that the five unrun flag-dependent stress scenarios remain required acceptance work. Apply this wording consistently to the referenced sections without changing the reported test results.
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. Nitpick comments: In `@docs/knowledge/v8-resurrecting-finalizers.md`: - Around line 3-4: Clarify the acceptance criteria in the document’s status and related test-result sections: rename the 904-test suite to “baseline regression gate” and explicitly state that the five unrun flag-dependent stress scenarios remain required acceptance work. Apply this wording consistently to the referenced sections without changing the reported test results.
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7043eef8-8b78-4a70-9c17-e8de48a9d53c
📥 CommitsReviewing files that changed from the base of the PR and between cd8671e and 7e56a01.
📒 Files selected for processing (4)
Sorry, something went wrong.
Heap::CollectGarbage now holds a DisallowJavascriptExecution across the whole collection and entering JS from a callback is a GRACEFUL_FATAL, so "3. Post-pause invocation" no longer described what V8 does. Also records the ordering hazard where a wrapper referenced by a reviving one is finalized anyway, and why neither deferral nor strong-rooting fixes it.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)docs/knowledge/v8-resurrecting-finalizers.md (1)🤖 Prompt for all review comments with AI agents71-92: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
Make the JS-execution bounds explicit in the patch diff.
The document describes only closing AllowJavascriptExecution before InvokeSecondPassPhantomCallbacks(), but it does not show the patch change for that scope. Add that to the referenced patch/diff so the “narrow drain-only” contract is not just described.
🤖 Prompt for AI AgentsVerify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/knowledge/v8-resurrecting-finalizers.md` around lines 71 - 92, Update the referenced patch/diff to explicitly add an AllowJavascriptExecution scope around only the finalizer drain, including ObjectManager::FinalizerCallback processing. Ensure the scope is closed before InvokeSecondPassPhantomCallbacks(), preserving its existing DisallowJavascriptExecution requirement and making the narrow drain-only boundary visible.
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. Inline comments: In `@docs/knowledge/v8-resurrecting-finalizers.md`: - Around line 148-189: Update the migration guidance around the husk mitigation to require a single checked native-pointer accessor that detects released wrappers before any pointer dereference and raises the defined “native object has already been released” error. Ensure all shared access paths, including Pointer.cpp, use this accessor, and require an end-to-end resurrection regression test before adoption is considered complete. --- Nitpick comments: In `@docs/knowledge/v8-resurrecting-finalizers.md`: - Around line 71-92: Update the referenced patch/diff to explicitly add an AllowJavascriptExecution scope around only the finalizer drain, including ObjectManager::FinalizerCallback processing. Ensure the scope is closed before InvokeSecondPassPhantomCallbacks(), preserving its existing DisallowJavascriptExecution requirement and making the narrow drain-only boundary visible.
Fix all unresolved CodeRabbit comments on this PR:
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6df1bb27-302a-47e3-b895-a6d19fc4863d
📥 CommitsReviewing files that changed from the base of the PR and between 7e56a01 and 49c81cd.
📒 Files selected for processing (1)
Sorry, something went wrong.
| ## Known hazard: a finalized object can outlive its native half | ||
|
|
||
| `IdentifyDeadFinalizerHandles()` snapshots the dead set *before* `IterateFinalizerHandlesAsRoots()` | ||
| and the re-drain, so the queue is built against pre-resurrection marking. If two wrappers are | ||
| both JS-unreachable and one references the other, **both** are queued: | ||
|
|
||
| - `n1` holds `n2`; neither is reachable from a JS root, so both are queued. | ||
| - The keep-alive marks `n1`'s whole closure — `n2` included — live, so nothing is swept. | ||
| - `n2`'s callback runs first and disposes: `[N2 release]`, wrapper deleted. | ||
| - `n1`'s callback finds `IsGcProtected()` and re-arms. | ||
|
|
||
| `n1` comes back holding a JS object whose native half is gone. `DisposeValue` does neuter the | ||
| husk (`delete wrapper` then `tns::DeleteValue`, which sets internal field 0 to `Undefined`), so | ||
| it is not a dangling `BaseDataWrapper*` — but consumers that read the field without checking, | ||
| `Pointer.cpp` among them, will read `Undefined` as an `External` and dereference it. | ||
|
|
||
| **This is not fixable by deferring the release or by re-checking liveness after the drain.** | ||
| Every queued node is rooted by `IterateFinalizerHandlesAsRoots()` and treated as a strong | ||
| retainer for the rest of the cycle, so immediately after the GC *every* disposed object still | ||
| looks alive; the check cannot separate "alive because a referrer revived" from "alive because | ||
| it was rooted for its own finalization". Waiting a cycle does not converge either: `n1` re-arms | ||
| weak, so the next `IdentifyDeadFinalizerHandles()` finds both unmarked again and re-queues the | ||
| same pair. | ||
|
|
||
| Nor is it fixable by making `GcProtect()` a strong root instead of a disposal veto. Disposal | ||
| here is driven by whether *native* still needs the object, not by JS reachability, and that is | ||
| precisely what lets a JS↔ObjC cycle collect at all: an unprotected object is released even | ||
| though surviving JS still references it. Turning protection into an opaque strong root would | ||
| convert every such cycle into a permanent leak. | ||
|
|
||
| The real fix is to stop expressing liveness with roots and express it by *tracing* — `CppHeap`, | ||
| `v8::Object::Wrap`/`Unwrap`, `TracedReference` and cppgc `Trace()`, which mark *through* the | ||
| embedder graph and so collect cross-heap cycles without resurrection. That is | ||
| [RESURRECTION_TO_REACHABILITY.md](RESURRECTION_TO_REACHABILITY.md). Note that the intermediate | ||
| option is gone: `EmbedderRootsHandler::IsRoot()`, which used to let an embedder declare a | ||
| `TracedReference` a root per-GC, no longer exists in 14.9 — only `ResetRoot`/`TryResetRoot` | ||
| remain. | ||
|
|
||
| Until then, the cheap mitigation is to give the husk defined behaviour: throw "native object | ||
| has already been released" from a single checked accessor rather than letting each call site | ||
| read `Undefined` as a pointer. | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 6 \
'DeleteValue|Pointer.cpp|ToExternal|External::Cast|Undefined|IsGcProtected|FinalizerCallback' \
NativeScript
rg -n -C 5 \
'resurrect|rearm|released|finalizer' \
TestRunnerRepository: NativeScript/ios
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '## Relevant tracked files\n'
git ls-files | rg '(^|/)(Pointer\.cpp|Helpers\.(cpp|mm|hpp|h)|Finalizers|v8-resurrecting-finalizers\.md|RESURRECTION_TO_REACHABILITY\.md)$|Pointer' | sed -n '1,200p'
printf '\n## Knowledge doc section\n'
cat -n docs/knowledge/v8-resurrecting-finalizers.md | sed -n '140,192p'
printf '\n## Search exact terms (focused)\n'
rg -n --glob '!NativeScript/v8runtime/**' --glob '!NativeScript/jsi/**' \
'external::Cast|ToExternal|External::Cast|GetValue|DeleteValue|GcProtect|GcUnprotect|IsGcProtected|Finalize|Finalizer|Weak|WeakCallback|delete wrapper' \
NativeScript/runtime NativeScript/v8runtime docs TestRunner 2>/dev/null | sed -n '1,240p'Repository: NativeScript/ios
Length of output: 24698
Gate the migration on a checked husk-access path.
The document describes a reachable crash where a resurrected wrapper leaves native pointer accessors reading Undefined as an external pointer. Treat the suggested “single checked accessor” as a required fix: gate adoption until every shared native-pointer access path rejects released wrappers with a defined native error and an end-to-end resurrection regression test is in place.
🤖 Prompt for AI AgentsVerify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/knowledge/v8-resurrecting-finalizers.md` around lines 148 - 189, Update the migration guidance around the husk mitigation to require a single checked native-pointer accessor that detects released wrappers before any pointer dereference and raises the defined “native object has already been released” error. Ensure all shared access paths, including Pointer.cpp, use this accessor, and require an end-to-end resurrection regression test before adoption is considered complete.
Sorry, something went wrong.
v8config.h rejects anything at or below C++17, so an app or plugin whose sources reach a V8 header cannot build at the templates' gnu++0x. Both templates move to gnu++20, matching the runtime. A plugin pinning a lower standard in its xcconfig still wins over this project-level setting. GCC_C_LANGUAGE_STANDARD goes gnu99 -> gnu17 separately: it is the only language setting the metadata generator reads, and gnu99 predates C11. Regenerating against the iOS 26.2 SDK under both is byte-identical across the binary, the umbrella header and all 193 YAML modules.
XcconfigService.mergeFiles is first-writer-wins and the app's file is merged after every plugin's, so the app's value is discarded rather than merely losing sometimes.
v8-14.9.207.39-2 carries the patch change that lifts V8 14.9's DisallowJavascriptExecution for the finalizer drain. Without it, disposing a wrapper whose -dealloc reaches a JS override aborts the process from inside the GC epilogue. The accompanying spec covers that path. It is pinned together with the release deliberately: against -1 it does not fail, it kills the test host.
The repo formats staged C/C++/ObjC sources through lint-staged, but the `clang-format` on PATH here resolved to depot_tools' wrapper, which needs a Chromium checkout and exits non-zero without one. The hook failed without aborting the commit, so everything this branch added went in unformatted while main's sources stayed formatted. Normalized with clang-format 20.1.8 (Homebrew LLVM), verified to reproduce byte-identical output on all 30 runtime files #412 touched, so this matches the style the V8 upgrade was written in. Scoped with `git clang-format origin/main` to the lines this branch actually changed: main's own pre-existing drift in files like ConcurrentQueue.cpp is left alone rather than swept into this branch's diff. Formatting only — no behavior change.
The repo formats staged C/C++/ObjC sources through lint-staged, but the `clang-format` on PATH here resolved to depot_tools' wrapper, which needs a Chromium checkout and exits non-zero without one. The hook failed without aborting the commit, so everything this branch added went in unformatted while main's sources stayed formatted. Normalized with clang-format 20.1.8 (Homebrew LLVM), verified to reproduce byte-identical output on all 30 runtime files #412 touched, so this matches the style the V8 upgrade was written in. Scoped with `git clang-format origin/main` to the lines this branch actually changed: main's own pre-existing drift in files like ConcurrentQueue.cpp is left alone rather than swept into this branch's diff. Formatting only — no behavior change.
The repo formats staged C/C++/ObjC sources through lint-staged, but the `clang-format` on PATH here resolved to depot_tools' wrapper, which needs a Chromium checkout and exits non-zero without one. The hook failed without aborting the commit, so everything this branch added went in unformatted while main's sources stayed formatted. Normalized with clang-format 20.1.8 (Homebrew LLVM), verified to reproduce byte-identical output on all 30 runtime files #412 touched, so this matches the style the V8 upgrade was written in. Scoped with `git clang-format origin/main` to the lines this branch actually changed: main's own pre-existing drift in files like ConcurrentQueue.cpp is left alone rather than swept into this branch's diff. Formatting only — no behavior change.
The repo formats staged C/C++/ObjC sources through lint-staged, but the `clang-format` on PATH here resolved to depot_tools' wrapper, which needs a Chromium checkout and exits non-zero without one. The hook failed without aborting the commit, so everything this branch added went in unformatted while main's sources stayed formatted. Normalized with clang-format 20.1.8 (Homebrew LLVM), verified to reproduce byte-identical output on all 30 runtime files #412 touched, so this matches the style the V8 upgrade was written in. Scoped with `git clang-format origin/main` to the lines this branch actually changed: main's own pre-existing drift in files like ConcurrentQueue.cpp is left alone rather than swept into this branch's diff. Formatting only — no behavior change.
| Back | FazBrowse Home | New Git URL |
Upgrades the runtime from V8 10.3.22 → 14.9.207.39. Companion to NativeScript/android#1987.
Status
The eleven skips are the pre-existing xit()s already in the suite.
The libraries are no longer committed
./download_v8.sh installs them from the release pinned in V8_RELEASE, verifying every archive against the SHA256SUMS published with it. It is a no-op once they are in place, V8_SKIP_DOWNLOAD=1 skips it entirely, and build_nativescript.sh calls it — so the vision path is covered by the same call.
It is a standalone script rather than an Xcode build phase, deliberately matching download_llvm.sh: a prerequisite you run once, trivial to skip, and easy to override with a local V8 build. Keeping it out of the build also means a stale or unreachable release cannot wedge an otherwise working build.
Why they cannot stay in git: the full matrix cannot be produced on any single machine. The Apple variants need macOS; the 32-bit Android ABIs need an ia32-capable host, because mksnapshot becomes a 32-bit x86 host binary and v8config.h refuses anything else. Artifacts assembled by hand are therefore stitched together from several machines, and nobody can reproduce or verify them. On Android two of the monoliths now also exceed GitHub's 100 MiB per-file push limit.
fetch_v8.sh and build_v8_source*.sh go for the same reason: producing V8 belongs in v8-buildscripts, which builds all nine targets in CI. A second copy of the gn args here is just two sets that can disagree — and these are args where disagreeing is expensive.
The vendored headers are ignored along with the libraries, not just the binaries. Keeping a copy in git is how it drifts out of step with the libraries it describes, and it already had: third_party/inspector_protocol/crdtp was still on 10.3 while libcrdtp.a was built from 14.9. It compiled and the inspector worked, so it was latent rather than broken — but it is exactly the mismatch this change removes. libffi stays tracked; it is not V8.
tools/v8/vendor_inspector_sources.py re-derives the inspector's internals by closure from the release's src-headers artifact. That also prunes the tree from 164 files to the 66 actually reachable from the glue — verified by compiling all five roots against the closure alone.
The API migration
Mostly mechanical; the full table is in docs/knowledge/v8-14-migration.md. The part that is not:
PropertyCallbackInfo no longer exposes the receiver at all, and SetNativeDataProperty is not a drop-in for SetAccessor on anything inherited from. ClassBuilder's super (on the implementation object) and Reference's value (on a prototype template) both need the receiver and are now function-backed. Static properties on constructor functions matter most: SetNativeDataProperty installs something data-like, so assigning through a derived class shadowed the base with an own property and never reached the native setter — silent, and not covered by any test.
Interceptors now return v8::Intercepted. A path that set a return value or threw becomes kYes; a path that returned without setting one, including falling off the end, becomes kNo. The fall-through case is load-bearing: the swizzling definer depends on V8 still running the real defineProperty, and the setters depend on V8 still performing the store. Getting it backwards is silent in both directions.
Two parity traps, neither covered by a test:
Isolate::VisitHandlesWithClassIds was removed, so teardown disposal walks an intrusive list of registered wrappers that unlink themselves from their GC finalizer, rather than enumerating V8's handles.
Module resolution now fails loudly
should throw error if cant find node module was the one test the upgrade broke, and the cause was worth more than a patch. An unresolvable specifier resolved to a placeholder module whose exports was meant to be a Proxy that threw on property access, so require() of a missing module succeeded and only failed if you dereferenced it.
That placeholder never worked on any V8 version. The generated script interpolates the module name unquoted into a single-quoted string literal:
Always a SyntaxError, so exports was never set. On 10.3 the pending exception left by that failed compile — there is no TryCatch around it — escaped the require callback, and that accident is the only reason the test passed. On 14.9 it no longer escapes, so require returned undefined.
The mechanism is removed rather than repaired: both the CommonJS and ESM paths now throw Cannot find module '<specifier>'. The ESM resolver additionally had debug-mode branches returning an empty MaybeLocal without scheduling an exception, which is not a valid resolve-callback result; those now throw too. node: built-in polyfills are untouched.
Every bare specifier still live in the suite is either inside a try/catch or behind an isV8iOS ? xit skip, so nothing depended on the placeholder.
What is not verified
Notes for review
Summary by CodeRabbit