Skip to content

Commit 096e03a

Browse files
DjDeveloperrDjDeveloperr
committed
fix(ffi): address direct engine review findings
1 parent 48707a0 commit 096e03a

9 files changed

Lines changed: 142 additions & 52 deletions

File tree

‎NativeScript/ffi/hermes/HermesFastNativeApi.mm‎

Lines changed: 20 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -586,10 +586,6 @@ napi_value TryCallHermesObjCMemberFastImpl(
586586
: nullptr;
587587

588588
if (frameDirectReturnInvoker != nullptr) {
589-
if (handled != nullptr) {
590-
*handled = true;
591-
}
592-
593589
EngineDirectRoundTripCacheFrameGuard roundTripCacheFrame(
594590
env, member->bridgeState, needsRoundTripFrame);
595591

@@ -611,6 +607,9 @@ EngineDirectRoundTripCacheFrameGuard roundTripCacheFrame(
611607
env, cif, reinterpret_cast<void*>(objc_msgSend), self,
612608
descriptor->selector, returnContext, hermesArgsBase,
613609
&directResult)) {
610+
if (handled != nullptr) {
611+
*handled = true;
612+
}
614613
return directResult;
615614
}
616615
} @catch (NSException* exception) {
@@ -656,10 +655,6 @@ EngineDirectRoundTripCacheFrameGuard roundTripCacheFrame(
656655
: nullptr;
657656

658657
if (directReturnInvoker != nullptr) {
659-
if (handled != nullptr) {
660-
*handled = true;
661-
}
662-
663658
EngineDirectRoundTripCacheFrameGuard roundTripCacheFrame(
664659
env, member->bridgeState, needsRoundTripFrame);
665660

@@ -684,6 +679,9 @@ EngineDirectRoundTripCacheFrameGuard roundTripCacheFrame(
684679
env, cif, reinterpret_cast<void*>(objc_msgSend), self,
685680
descriptor->selector, returnContext, directArgs,
686681
&directResult)) {
682+
if (handled != nullptr) {
683+
*handled = true;
684+
}
687685
return directResult;
688686
}
689687
} @catch (NSException* exception) {
@@ -704,10 +702,6 @@ EngineDirectRoundTripCacheFrameGuard roundTripCacheFrame(
704702
return nullptr;
705703
}
706704

707-
if (handled != nullptr) {
708-
*handled = true;
709-
}
710-
711705
EngineDirectRoundTripCacheFrameGuard roundTripCacheFrame(
712706
env, member->bridgeState, needsRoundTripFrame);
713707

@@ -744,6 +738,10 @@ EngineDirectRoundTripCacheFrameGuard roundTripCacheFrame(
744738
return nullptr;
745739
}
746740

741+
if (handled != nullptr) {
742+
*handled = true;
743+
}
744+
747745
return makeHermesObjCReturnValue(
748746
env, member, descriptor, cif, self, receiverIsClass, jsThis, rvalue,
749747
kind != EngineDirectMemberKind::Method);
@@ -803,10 +801,6 @@ napi_value TryCallHermesCFunctionFastImpl(
803801
: nullptr;
804802

805803
if (frameDirectReturnInvoker != nullptr) {
806-
if (handled != nullptr) {
807-
*handled = true;
808-
}
809-
810804
EngineDirectRoundTripCacheFrameGuard roundTripCacheFrame(
811805
env, bridgeState, needsRoundTripCacheFrame(cif));
812806

@@ -815,6 +809,9 @@ EngineDirectRoundTripCacheFrameGuard roundTripCacheFrame(
815809
NativeCallRuntimeUnlockScope unlockRuntime(env);
816810
if (frameDirectReturnInvoker(env, cif, function->fnptr, hermesArgsBase,
817811
&directResult)) {
812+
if (handled != nullptr) {
813+
*handled = true;
814+
}
818815
return directResult;
819816
}
820817
} @catch (NSException* exception) {
@@ -859,10 +856,6 @@ EngineDirectRoundTripCacheFrameGuard roundTripCacheFrame(
859856
: nullptr;
860857

861858
if (directReturnInvoker != nullptr) {
862-
if (handled != nullptr) {
863-
*handled = true;
864-
}
865-
866859
EngineDirectRoundTripCacheFrameGuard roundTripCacheFrame(
867860
env, bridgeState, needsRoundTripCacheFrame(cif));
868861

@@ -874,6 +867,9 @@ EngineDirectRoundTripCacheFrameGuard roundTripCacheFrame(
874867
NativeCallRuntimeUnlockScope unlockRuntime(env);
875868
if (directReturnInvoker(env, cif, function->fnptr, directArgs,
876869
&directResult)) {
870+
if (handled != nullptr) {
871+
*handled = true;
872+
}
877873
return directResult;
878874
}
879875
} @catch (NSException* exception) {
@@ -884,10 +880,6 @@ EngineDirectRoundTripCacheFrameGuard roundTripCacheFrame(
884880
}
885881
}
886882

887-
if (handled != nullptr) {
888-
*handled = true;
889-
}
890-
891883
EngineDirectRoundTripCacheFrameGuard roundTripCacheFrame(
892884
env, bridgeState, needsRoundTripCacheFrame(cif));
893885

@@ -927,6 +919,10 @@ EngineDirectRoundTripCacheFrameGuard roundTripCacheFrame(
927919
return nullptr;
928920
}
929921

922+
if (handled != nullptr) {
923+
*handled = true;
924+
}
925+
930926
return makeHermesCFunctionReturnValue(env, function, cif, perCallRValue);
931927
}
932928

‎NativeScript/ffi/hermes/HermesFastNativeApiPrivate.h‎

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -76,11 +76,7 @@ inline bool readHermesFiniteNumber(napi_value value, double* result) {
7676
}
7777

7878
inline napi_value makeHermesRawValue(Cif* cif, uint64_t raw) {
79-
if (cif != nullptr) {
80-
cif->hermesRawReturnSlot = raw;
81-
return reinterpret_cast<napi_value>(&cif->hermesRawReturnSlot);
82-
}
83-
79+
(void)cif;
8480
static thread_local uint64_t slots[64] = {};
8581
static thread_local unsigned int nextSlot = 0;
8682
uint64_t* slot = &slots[nextSlot++ & 63];

‎NativeScript/ffi/jsc/JSCFastNativeApi.mm‎

Lines changed: 25 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -46,13 +46,34 @@ id resolveJSCSelf(napi_env env, napi_value jsThis, ObjCClassMember* member) {
4646

4747
if (member != nullptr && member->cls != nullptr &&
4848
member->cls->nativeClass != nil) {
49-
if (member->classMethod) {
50-
return static_cast<id>(member->cls->nativeClass);
51-
}
52-
49+
bool shouldUseClassFallback = member->classMethod;
5350
napi_valuetype jsType = napi_undefined;
5451
if (jsThis != nullptr && napi_typeof(env, jsThis, &jsType) == napi_ok &&
5552
jsType == napi_function) {
53+
bool isSameConstructor = true;
54+
napi_value definingConstructor = nullptr;
55+
if (member->cls->constructor != nullptr) {
56+
napi_get_reference_value(env, member->cls->constructor,
57+
&definingConstructor);
58+
}
59+
if (definingConstructor != nullptr &&
60+
napi_strict_equals(env, jsThis, definingConstructor,
61+
&isSameConstructor) == napi_ok &&
62+
!isSameConstructor) {
63+
shouldUseClassFallback = false;
64+
} else {
65+
shouldUseClassFallback = true;
66+
}
67+
}
68+
69+
if (member->classMethod) {
70+
if (shouldUseClassFallback) {
71+
return static_cast<id>(member->cls->nativeClass);
72+
}
73+
return nil;
74+
}
75+
76+
if (shouldUseClassFallback) {
5677
return static_cast<id>(member->cls->nativeClass);
5778
}
5879
}

‎NativeScript/ffi/napi/Cif.h‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,6 @@ class Cif {
2424
bool generatedDispatchHasRoundTripCacheArgument = false;
2525
bool generatedDispatchUsesObjectReturnStorage = false;
2626
bool generatedDispatchSetsV8ReturnDirectly = false;
27-
uint64_t hermesRawReturnSlot = 0;
2827

2928
void* rvalue;
3029
void** avalues;

‎NativeScript/ffi/quickjs/QuickJSFastNativeApi.mm‎

Lines changed: 60 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,40 @@ bool isCompatCFunction(napi_env env, void* data) {
2828
strcmp(name, "NSApplicationMain") == 0;
2929
}
3030

31+
id normalizeQuickJSWrappedReceiver(napi_env env, void* wrapped) {
32+
if (wrapped == nullptr) {
33+
return nil;
34+
}
35+
36+
auto* state = nativescript::ObjCBridgeState::InstanceData(env);
37+
if (state != nullptr) {
38+
id nativeObject = state->nativeObjectForBridgeWrapper(wrapped);
39+
if (nativeObject != nil) {
40+
return nativeObject;
41+
}
42+
43+
for (const auto& entry : state->classes) {
44+
auto* bridgedClass = entry.second;
45+
if (bridgedClass == wrapped && bridgedClass->nativeClass != nil) {
46+
return static_cast<id>(bridgedClass->nativeClass);
47+
}
48+
}
49+
50+
for (const auto& entry : state->protocols) {
51+
auto* bridgedProtocol = entry.second;
52+
if (bridgedProtocol == wrapped) {
53+
if (Protocol* runtimeProtocol =
54+
objc_getProtocol(bridgedProtocol->name.c_str())) {
55+
return static_cast<id>(runtimeProtocol);
56+
}
57+
break;
58+
}
59+
}
60+
}
61+
62+
return static_cast<id>(wrapped);
63+
}
64+
3165
id resolveQuickJSSelf(napi_env env, napi_value jsThis,
3266
nativescript::ObjCClassMember* member) {
3367
id self = nil;
@@ -38,7 +72,7 @@ id resolveQuickJSSelf(napi_env env, napi_value jsThis,
3872
if (nativescript::TryUnwrapQuickJSNativeObjectFast(
3973
env, ToJSValue(jsThis), &wrapped) &&
4074
wrapped != nullptr) {
41-
return static_cast<id>(wrapped);
75+
return normalizeQuickJSWrappedReceiver(env, wrapped);
4276
}
4377
}
4478

@@ -56,13 +90,34 @@ id resolveQuickJSSelf(napi_env env, napi_value jsThis,
5690

5791
if (member != nullptr && member->cls != nullptr &&
5892
member->cls->nativeClass != nil) {
59-
if (member->classMethod) {
60-
return static_cast<id>(member->cls->nativeClass);
61-
}
62-
93+
bool shouldUseClassFallback = member->classMethod;
6394
napi_valuetype jsType = napi_undefined;
6495
if (jsThis != nullptr && napi_typeof(env, jsThis, &jsType) == napi_ok &&
6596
jsType == napi_function) {
97+
bool isSameConstructor = true;
98+
napi_value definingConstructor = nullptr;
99+
if (member->cls->constructor != nullptr) {
100+
napi_get_reference_value(env, member->cls->constructor,
101+
&definingConstructor);
102+
}
103+
if (definingConstructor != nullptr &&
104+
napi_strict_equals(env, jsThis, definingConstructor,
105+
&isSameConstructor) == napi_ok &&
106+
!isSameConstructor) {
107+
shouldUseClassFallback = false;
108+
} else {
109+
shouldUseClassFallback = true;
110+
}
111+
}
112+
113+
if (member->classMethod) {
114+
if (shouldUseClassFallback) {
115+
return static_cast<id>(member->cls->nativeClass);
116+
}
117+
return nil;
118+
}
119+
120+
if (shouldUseClassFallback) {
66121
return static_cast<id>(member->cls->nativeClass);
67122
}
68123
}

‎NativeScript/ffi/shared/SignatureDispatch.h‎

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -168,11 +168,7 @@ inline bool readHermesDispatchFiniteNumberRaw(uint64_t raw, double* result) {
168168
}
169169

170170
inline napi_value makeHermesDispatchRawValue(Cif* cif, uint64_t raw) {
171-
if (cif != nullptr) {
172-
cif->hermesRawReturnSlot = raw;
173-
return reinterpret_cast<napi_value>(&cif->hermesRawReturnSlot);
174-
}
175-
171+
(void)cif;
176172
static thread_local uint64_t slots[64] = {};
177173
static thread_local unsigned int nextSlot = 0;
178174
uint64_t* slot = &slots[nextSlot++ & 63];

‎NativeScript/ffi/v8/V8FastConversion.mm‎

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -270,7 +270,6 @@ bool TryFastConvertV8UInt16Argument(napi_env env, v8::Local<v8::Value> value, ui
270270
if (value->IsString()) {
271271
v8::String::Value chars(env->isolate, value);
272272
if (chars.length() != 1) {
273-
throwV8Error(env->isolate, "Expected a single-character string.");
274273
*result = 0;
275274
return false;
276275
}
@@ -336,7 +335,11 @@ bool TryFastConvertV8Argument(napi_env env, MDTypeKind kind, v8::Local<v8::Value
336335
case mdTypeSInt64:
337336
if (value->IsBigInt()) {
338337
bool lossless = false;
339-
*reinterpret_cast<int64_t*>(result) = value.As<v8::BigInt>()->Int64Value(&lossless);
338+
int64_t converted = value.As<v8::BigInt>()->Int64Value(&lossless);
339+
if (!lossless) {
340+
return false;
341+
}
342+
*reinterpret_cast<int64_t*>(result) = converted;
340343
return true;
341344
}
342345
return value->IntegerValue(env->context()).To(reinterpret_cast<int64_t*>(result));
@@ -345,8 +348,11 @@ bool TryFastConvertV8Argument(napi_env env, MDTypeKind kind, v8::Local<v8::Value
345348
case mdTypeUInt64:
346349
if (value->IsBigInt()) {
347350
bool lossless = false;
348-
*reinterpret_cast<uint64_t*>(result) =
349-
value.As<v8::BigInt>()->Uint64Value(&lossless);
351+
uint64_t converted = value.As<v8::BigInt>()->Uint64Value(&lossless);
352+
if (!lossless) {
353+
return false;
354+
}
355+
*reinterpret_cast<uint64_t*>(result) = converted;
350356
return true;
351357
} else {
352358
int64_t converted = 0;

‎NativeScript/ffi/v8/V8FastNativeApi.mm‎

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -163,7 +163,25 @@ id resolveSelf(napi_env env, v8::Local<v8::Value> jsThisValue, ObjCClassMember*
163163
if (method != nullptr && method->cls != nullptr && method->cls->nativeClass != nil) {
164164
if (method->classMethod) {
165165
shouldUseClassFallback = true;
166-
} else if (!jsThisValue.IsEmpty() && jsThisValue->IsFunction()) {
166+
}
167+
168+
if (!jsThisValue.IsEmpty() && jsThisValue->IsFunction() && jsThis != nullptr) {
169+
bool isSameConstructor = true;
170+
napi_value definingConstructor = nullptr;
171+
if (method->cls->constructor != nullptr) {
172+
napi_get_reference_value(env, method->cls->constructor,
173+
&definingConstructor);
174+
}
175+
if (definingConstructor != nullptr &&
176+
napi_strict_equals(env, jsThis, definingConstructor,
177+
&isSameConstructor) == napi_ok &&
178+
!isSameConstructor) {
179+
shouldUseClassFallback = false;
180+
} else {
181+
shouldUseClassFallback = true;
182+
}
183+
} else if (!method->classMethod && !jsThisValue.IsEmpty() &&
184+
jsThisValue->IsFunction()) {
167185
shouldUseClassFallback = true;
168186
}
169187
}

‎NativeScript/ffi/v8/V8FastNativeWrapper.mm‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -84,9 +84,12 @@ NS_V8_INTERCEPTED nativeWrapperNamedSetter(
8484
}
8585
}
8686

87-
definePlainValueProperty(info.GetIsolate()->GetCurrentContext(), holder,
88-
property, value);
89-
NS_V8_RETURN_YES;
87+
if (definePlainValueProperty(info.GetIsolate()->GetCurrentContext(), holder,
88+
property, value)) {
89+
NS_V8_RETURN_YES;
90+
}
91+
92+
NS_V8_RETURN_NO;
9093
}
9194

9295
NS_V8_INTERCEPTED nativeWrapperIndexedGetter(

0 commit comments

Comments
 (0)