fix(ffi): resolve protocol-only method metadata for block parameters - #71
Open
DjDeveloperr wants to merge 276 commits into
Open
fix(ffi): resolve protocol-only method metadata for block parameters#71DjDeveloperr wants to merge 276 commits into
DjDeveloperr wants to merge 276 commits into
Conversation
- Skip redundant sel_registerName + class_getInstanceMethod when the prepared invocation is already cached (first-call-only overhead). - Use raw pointer for receiver host object lookup (avoids atomic ref count increment on every method call). - Only acquire shared_ptr for init methods that need disown handling. - Add v8HostObjectRaw<T> template for zero-overhead receiver access. Tests: v8 713/0
Switch V8 HostObject interceptor from kNone to kNonMasking. With kNonMasking, V8 checks own properties and prototype chain BEFORE calling the interceptor. This means method calls and property getters installed on the prototype (by installClassMembers) are found directly by V8's inline caches without any C++ interceptor overhead. Add toString to the host object template so it overrides Object.prototype.toString (which would otherwise shadow it with kNonMasking). Benchmark: 732ms total (was 1250ms) — 42% improvement, now matching legacy iOS V8 performance (728ms). Known: 9 test failures related to function pointer resolution, instanceof, and readonly property error messages. These are edge cases that need the interceptor but aren't on the hot path. Tests: 713 total, 9 failures (704 pass)
This reverts commit 1000f70.
Add a separate V8 object template for NativeApiObjectHostObject that uses kNonMasking interceptor flag. This allows V8 to check the prototype chain before calling the interceptor for native object instances, enabling faster property access for methods and getters installed on the prototype. Also skip superclass/class/constructor/debugDescription from prototype property installation so the interceptor's special handling is used (these properties need to return wrapped class constructors). Install toString on the native object template to override Object.prototype.toString with kNonMasking. Tests: v8 713/0
Use kNonMasking interceptor on native object instances only (not class or bridge host objects). This allows V8 to find prototype properties without calling the interceptor, giving a 40% speedup. Benchmark: 773ms (was 1250ms) Tests: 713 total, 7 failures remaining (superclass/instanceof edge cases) Also fix readonly property test expectations to accept V8's native error message with kNonMasking.
Skip superclass/class/constructor/className/debugDescription from both prototype property installation AND selector group installation. This ensures the interceptor handles these properties (which need special wrapping) even with kNonMasking on native instances. Fixed: SimpleInheritance, NSArray constructor, instanceof, TaggedPointers, readonly property errors. Remaining: 1 Swift class name test (constructor.name is empty string instead of the mangled Swift class name). Benchmark: 773ms (40% improvement from 1250ms baseline) Tests: 713 total, 1 failure, 10 skipped
Improve constructor handler to try cached class value and global lookup before falling back to makeNativeClassValue. Also skip className from prototype installation. Tests: 712/713 (1 Swift class name edge case remaining) Benchmark: ~773ms
Update the Swift marshalling test to use className property instead of class_getName(constructor) which fails when the constructor is a class host object that can't be converted to a pointer. Tests: 713/0
…object-arg fast path + cached invocation flags
…on selector-group data
…property lookups)
…undant JS root property lookup)
…kJS/Hermes hot paths
…e interceptor (skips per-access metadata discovery for JSI engines)
We need to ensure the errors created via public API JS_NewError include .stack property.
Squashed Android C++ runtime refactor and Apple platform organization work after CI passed.
* refactor(runtime): split engine backends and harden lifetimes * refactor(runtime): consolidate engine selector dispatch
…regressions Rebase repair for two bugs introduced while re-stacking refactor onto main's upstream fixes: - shared/bridge/Install.mm: the zero-arg branch of the native-class constructable() referenced an undeclared `instance` (ReferenceError at runtime) — the alloc()+init() fallback block was dropped while merging the derived-receiver static-allocator fix (#61) into the constructor path. Restored it. - The Generated Signature Dispatch (GSD) fast paths and the V8/QuickJS/JSC Marshalling.mm fast paths are refactor-only infrastructure that never existed on main, so upstream's "unsigned short returns are always numbers" fix (#55) never reached them: setUInt16/mdTypeUShort still projected printable-ASCII values (32-126) to single-character strings. Made them always return numeric, and added mdTypeUnichar handling (UTF-8 encoded single-character string projection) to the Marshalling.mm slow-ish path used for calls GSD declines to fast-path (mdTypeUnichar is intentionally excluded from GSD's fast-scalar allowlist, so this is the path unichar returns actually take). Found via the macOS TestRunner suite (previously green build masked the regression — check:ffi-boundaries and the build itself don't execute JS).
NativeScript.mm and napi/ClassMember.mm each carried a leftover #include "runtime/NativeScriptException.h" at the pre-reorg path (the file now lives at runtime/apple/NativeScriptException.h) alongside the correct include — dead weight that happened to survive several rebase-conflict resolutions unnoticed until a from-scratch build hit "file not found".
… not alloc()+init() Follow-up to e7e244d: that commit fixed a ReferenceError (an undeclared `instance`) in constructable()'s zero-arg path, but restored the WRONG pre-refactor content — the old two-step `nativeClass.alloc()` + `instance.init()` sequence — instead of refactor's own one-step `nativeClass.new()` call that the original conflict resolution (commit 749756f's merge) had correctly kept. The two-step sequence bypasses whatever construction-state bookkeeping `nativeClass.new()` does internally, and specifically for JS-extended classes it let a JS override of `-init` re-enter the dispatch path without the "under construction" guard being set — manifesting as an EXC_BREAKPOINT crash in object_getClass on macOS and a hang on iOS in ObjCConstructors.js's "ParameterlessConstructor" spec (`new (TNSCInterface.extend({}))()`), both CI-only (not reproducible against two different local Xcode toolchains, which is why it wasn't caught locally).
findClassForRuntimeClass only ever consulted the concrete class hierarchy (class_getSuperclass), so a selector declared solely on a conformed Objective-C protocol had no path to its metadata whenever the class that actually implements it doesn't carry that conformance in a header the metadata generator parsed -- a category/class-extension conformance, or (as with UIViewControllerTransitionCoordinator) a fully private concrete class. createEngineCallback then threw "Native callback metadata is unavailable." for any block parameter on such a method, forcing callers to hand-supply the ObjC encoding via interop.Block(fn, "..."). Adds a runtime protocol-conformance fallback (class_copyProtocolList + protocol_copyProtocolList, walked alongside the existing class walk), consulted only once the ordinary class/protocol-declared-on-header lookup misses, and cached per runtime Class so well-declared classes pay nothing extra. Deterministic tie-break for a class conforming to several protocols declaring the same selector: most-derived class first, then ancestors; within one class's own adopted-protocol list, class_copyProtocolList's order; each protocol's inherited protocols expanded depth-first ahead of its next sibling. First match wins. Covered by two new fixtures/tests mirroring the real-world shape (hidden vs. declared protocol conformance) plus the existing 717-test macOS suite (unchanged 8 pre-existing DBL_MAX failures, no new ones). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BQPGUU8YECUhbBvwkW4Kek
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
This was referenced Aug 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What was broken
If a method your app calls is declared only on an Objective-C protocol
(not on any real class in the object's chain), and takes a block
parameter, calling it from JS with a plain closure threw:
Error: Native callback metadata is unavailable.This happens whenever the class that actually implements the method
doesn't declare that conformance in a header the metadata generator
parsed -- e.g. conformance added via a category/class-extension, or a
fully private concrete class (
UIViewControllerTransitionCoordinatoris the classic real-world case: every method lives on the protocol,
the concrete class is private). The only workaround was hand-supplying
the ObjC encoding:
interop.Block(fn, "v@?@").Root cause:
findClassForRuntimeClass(ObjCBridge.mm) only walked theconcrete class hierarchy via
class_getSuperclass, never consultingprotocol conformance, so method lookup for such selectors came up
empty and the block parameter's metadata was never resolved.
The fix
Adds a protocol-conformance fallback (
class_copyProtocolList+protocol_copyProtocolList, walked alongside the existing class walk),consulted only once the ordinary class-hierarchy lookup misses, cached
per runtime
Classso classes with complete metadata pay nothingextra.
Duplicate-selector rule: most-derived class first, then ancestors;
within one class's own adopted-protocol list,
class_copyProtocolList'sorder (stable, reflects declaration order); each protocol's inherited
protocols expand depth-first before its next sibling. First match wins.
Verification
Two new fixtures (hidden vs. declared protocol conformance, same
selectors) prove the fix and that the already-working class path is
unaffected. Confirmed genuine repro: same tests fail with exactly this
error on the pre-fix code. Full macOS suite: 717 tests, 8 pre-existing
DBL_MAX-formatting failures (unchanged), 0 new failures.
Co-Authored-By: Claude Opus 5 (1M context) noreply@anthropic.com
https://claude.ai/code/session_01BQPGUU8YECUhbBvwkW4Kek