fix: serialize extended-class allocate/register to prevent duplicate class names - #421
Conversation
…class names objc_allocateClassPair only rejects names that are already registered - the name enters the lookup table in objc_registerClassPair, not at allocation. Concurrent extends of the same class name (e.g. several workers requiring a module that extends a named class) could therefore both pass the collision check and register duplicate same-named classes; objc keeps both, logs 'Class X is implemented in both', and name-based class resolution becomes ambiguous. That ambiguity let a worker first-touch another isolate's freshly extended class, whose synthesized +initialize takes a v8::Locker on the creating isolate inside the objc class-init monitor - closing a lock-order cycle with the main thread's synchronous invocation of worker-owned closures and wedging the app until the test harness's 600s timeout (the long-standing intermittent CI failure). Serialize the allocate -> register window under a process-global os_unfair_lock. Captured live via thread samples and per-class probes; with the lock, 42 consecutive local suite runs are clean versus a roughly 1-in-8 baseline wedge rate. Primary fix for #420.
📝 WalkthroughWalkthrough
ChangesClass registration synchronization
Estimated code review effort: 3 (Moderate) | ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
NativeScript/runtime/ClassBuilder.cpp (1)
36-49: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUse RAII for
extendedClassRegistrationLock.The retry loop calls
std::stringassignments while holding the lock; if C++ exceptions are enabled, an allocation failure will bypassos_unfair_lock_unlock(&extendedClassRegistrationLock)and leave future class-extension calls blocked. Wrap the lock in a scope guard/guard object centered around the allocate/register window.🤖 Prompt for AI Agents
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/ClassBuilder.cpp` around lines 36 - 49, Replace the manual lock/unlock sequence around class allocation and registration with an RAII guard for extendedClassRegistrationLock, ensuring the guard is acquired before objc_allocateClassPair and released automatically after objc_registerClassPair, including when string operations or allocation throw. Keep the retry behavior in the existing allocation loop unchanged.
🤖 Prompt for all review comments with AI agents
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 `@NativeScript/runtime/ClassBuilder.cpp`:
- Around line 33-36: Update the class allocation/registration retry loop guarded
by extendedClassRegistrationLock to count failed objc_allocateClassPair
attempts, distinguish repeated name collisions from other allocation failures,
and stop after a bounded number of attempts. Always unlock
extendedClassRegistrationLock before returning the allocation failure so later
class creation is not blocked.
---
Outside diff comments:
In `@NativeScript/runtime/ClassBuilder.cpp`:
- Around line 36-49: Replace the manual lock/unlock sequence around class
allocation and registration with an RAII guard for
extendedClassRegistrationLock, ensuring the guard is acquired before
objc_allocateClassPair and released automatically after objc_registerClassPair,
including when string operations or allocation throw. Keep the retry behavior in
the existing allocation loop unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e974c5ff-0144-4fb4-be6d-3500e46884b8
📒 Files selected for processing (1)
NativeScript/runtime/ClassBuilder.cpp
Review round on the registration lock: - Promote the os_unfair_lock wrapper to UnfairLock.h as a BasicLockable UnfairMutex driven by std::lock_guard, so other call sites can use it and no raw lock/unlock pairs remain. Benchmarks against the in-tree SpinLock (8-thread burst: ~30x faster at ~33x less CPU; sleeping holder: ~100x less CPU) are summarized in the header, with a pointer from SpinLock.h. The kernel-informed adaptive-spin variant (private API, the Firefox approach) is documented behind NATIVESCRIPT_UNFAIR_LOCK_ADAPTIVE_SPIN for experiments only. - Replace the unbounded collision retry ladder with a stateless gallop + binary search over objc_getClass probes: same-name reuse (worker-heavy suites, HMR re-extends) stays O(log N) per extend with no cap to outgrow and no side state, and the remaining bound only trips on consecutive allocation failures, i.e. allocation broken for reasons other than a name collision. - Assert non-nil extended classes at both GetExtendedClass call sites.
|
Review round in b39662f:
Lock benchmark backing the primitive choice (M-series 14-core, best-of-3, wall/CPU):
The sleeping-holder row is this lock's actual regime (the section blocks on objc's Full suite green on the final state; cumulative post-fix validation across all variants: 48+ consecutive clean runs vs a ~1-in-8 baseline wedge rate. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
NativeScript/runtime/UnfairLock.h (1)
42-58: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDelete copy/move on
UnfairMutexto matchos_unfair_lock's address-stability requirement.
os_unfair_lockis a value type whose kernel-side identity is tied to its memory address; copying/moving it silently produces two independent locks with no crash, just broken mutual exclusion.UnfairMutexdoesn't currently prevent this the waystd::mutexdoes. Not a live bug today (single static instance), but a cheap guard against a very well-known footgun class if this type is ever stored in a copyable object or reallocated container.🔒 Suggested fix
struct UnfairMutex { os_unfair_lock lock_ = OS_UNFAIR_LOCK_INIT; + + UnfairMutex() = default; + UnfairMutex(const UnfairMutex&) = delete; + UnfairMutex& operator=(const UnfairMutex&) = delete;🤖 Prompt for AI Agents
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/UnfairLock.h` around lines 42 - 58, Delete copy and move construction and assignment for UnfairMutex, keeping its default construction available. Add the deleted special-member declarations directly in the UnfairMutex definition so instances cannot be copied or moved while preserving the existing lock(), try_lock(), and unlock() behavior.
🤖 Prompt for all review comments with AI agents
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 `@NativeScript/runtime/ClassBuilder.mm`:
- Line 66: Update ExtendCallback around the GetExtendedClass call to detect and
handle a pending NullptrException before evaluating tns::Assert(extendedClass,
isolate). Ensure the exception is propagated or returned through the existing
error path so execution cannot continue into later allocation work when
GetExtendedClass fails.
---
Nitpick comments:
In `@NativeScript/runtime/UnfairLock.h`:
- Around line 42-58: Delete copy and move construction and assignment for
UnfairMutex, keeping its default construction available. Add the deleted
special-member declarations directly in the UnfairMutex definition so instances
cannot be copied or moved while preserving the existing lock(), try_lock(), and
unlock() behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 145d0c0f-97d6-432d-9d43-9f3562cb1fe1
📒 Files selected for processing (4)
NativeScript/runtime/ClassBuilder.cppNativeScript/runtime/ClassBuilder.mmNativeScript/runtime/SpinLock.hNativeScript/runtime/UnfairLock.h
|
|
||
| Class extendedClass = ClassBuilder::GetExtendedClass(baseClassName, staticClassName, | ||
| std::to_string(isolateId) + "_"); | ||
| tns::Assert(extendedClass != nil, isolate); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Confirm tns::Assert's failure behavior (abort/throw) and that it isn't a debug-only no-op.
rg -n -A 15 'void Assert' NativeScript/runtime/Helpers.h NativeScript/runtime/Helpers.mm NativeScript/runtime/Helpers.cpp 2>/dev/nullRepository: NativeScript/ios
Length of output: 1521
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate Assert definition/usages =="
rg -n -C 4 'Assert\(|StopExecutionAndLogStackTrace|asserts?|NATIVE_ASSERT|AssertError' NativeScript/runtime 2>/dev/null | head -n 240
echo
echo "== Helpers.h section =="
sed -n '318,365p' NativeScript/runtime/Helpers.h 2>/dev/null || true
echo
echo "== Helpers.c/cpp/mm files =="
fd 'Helpers\.(c|cpp|mm|mm.h)$' NativeScript/runtime -t f | tr '\n' '\n'Repository: NativeScript/ios
Length of output: 21637
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 8 '^void tns::Assert|tns::Assert\b|^Assert\b\(bool' NativeScript/runtime -g '*.mm' -g '*.cpp' -g '*.hpp' | head -n 220Repository: NativeScript/ios
Length of output: 16303
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== NativeScript/runtime/Helpers.mm first 500 lines =="
sed -n '1,350p' NativeScript/runtime/Helpers.mm
echo
echo "== Assert implementations beyond Helpers =="
rg -n -C 10 'void\s+Assert|Assert\b\(bool' NativeScript -g '*.cpp' -g '*.mm' -g '*.h' -g '*.hpp' | head -n 300Repository: NativeScript/ios
Length of output: 14908
Guard against NullptrException in GetExtendedClass before asserting extendedClass.
The two assertions prevent a nil class when allocation fails, but GetExtendedClass can also throw NullptrException(isolate) directly; callers of ExtendCallback need to ensure that exception is handled before this guard is evaluated so the failure does not fall through to later allocation work.
🤖 Prompt for AI Agents
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/ClassBuilder.mm` at line 66, Update ExtendCallback
around the GetExtendedClass call to detect and handle a pending NullptrException
before evaluating tns::Assert(extendedClass, isolate). Ensure the exception is
propagated or returned through the existing error path so execution cannot
continue into later allocation work when GetExtendedClass fails.
2 participants
Primary fix for #420 — the long-standing intermittent
exceeded 600 seconds ... Jasmine testsCI wedge.Root cause (captured live — full analysis in #420)
objc_allocateClassPaironly rejects names that are already registered: the name enters the runtime's lookup table inobjc_registerClassPair→addNamedClass, not at allocation (verified against objc4 source).GetExtendedClass's allocate-as-collision-check strategy is therefore racy across threads: several workers extending the same class name concurrently (e.g. eachrequire-ing a module that extendsTimerTargetImpl) can all pass the nil-check inside the retry ladder and register duplicate same-named classes. objc keeps both and logsClass X is implemented in both …— that warning appears in every captured failure, CI and local.With duplicate names, name-based class resolution is ambiguous. Instrumented runs caught the consequence in-frame: two workers minted
TimerTargetImpl102 ms apart, and one then ran+initializeof the other worker's class — whose synthesized+initializetakes av8::Lockeron the creating isolate, inside the objc class-init monitor. That closes a lock-order cycle with the main thread's synchronous invocation of worker-owned closures (nil-queueNSNotificationCenterblock observers), wedging the app until the test harness times out.Fix
Serialize the allocate → register window under a process-global
os_unfair_lock.Why not the in-tree
SpinLock: by that header's own criteria the section doesn't qualify — it takes objc'sruntimeLockinternally (the holder can sleep) and contenders run at worker-chosen QoS (iosPriority), so a spinning waiter burns CPU against a sleeping holder and risks priority inversion.os_unfair_lockdonates priority and doesn't spin. The lock is a leaf — the section never acquires av8::Locker— so it cannot form a cycle with the isolate locks.User-facing class names are unchanged: first claimant of a name still gets it verbatim (
MyAppDelegatestaysMyAppDelegate), later same-name extends still walk the existing retry ladder — the lock only makes the ladder's collision detection actually atomic.Validation
Local repro loop (samples the app when it outlives its normal lifetime; recipe in #420):
implemented in bothwarnings.os_unfair_lockform), zero duplicate-class warnings. P ≈ 0.4% of that happening by chance at the baseline rate.Related: #419 hardens the harness/CI side (report delivery retries, fail-fast sentinel, diagnostics upload) so any residual wedge variant stays diagnosable.
Summary by CodeRabbit