Skip to content

Commit df472eb

Browse files
DjDeveloperrDjDeveloperr
committed
fix: address follow-up CodeRabbit comments
1 parent 387abce commit df472eb

3 files changed

Lines changed: 206 additions & 69 deletions

File tree

‎NativeScript/ffi/CFunction.mm‎

Lines changed: 60 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
#include "CFunction.h"
22
#include <dispatch/dispatch.h>
3+
#include <cstddef>
34
#include <cstdlib>
45
#include <cstdint>
56
#include <cstring>
@@ -33,6 +34,50 @@ inline bool isCompatOrMainCFunctionName(const char* name) {
3334
strcmp(name, "NSApplicationMain") == 0;
3435
}
3536

37+
class CFunctionReturnStorage final {
38+
public:
39+
explicit CFunctionReturnStorage(Cif* cif) {
40+
size_t size = 0;
41+
if (cif != nullptr) {
42+
size = cif->rvalueLength;
43+
if (size == 0 && cif->cif.rtype != nullptr) {
44+
size = cif->cif.rtype->size;
45+
}
46+
}
47+
if (size == 0) {
48+
size = sizeof(void*);
49+
}
50+
51+
if (size <= kInlineSize) {
52+
rvalue_ = inlineBuffer_;
53+
std::memset(rvalue_, 0, size);
54+
return;
55+
}
56+
57+
rvalue_ = std::malloc(size);
58+
if (rvalue_ != nullptr) {
59+
std::memset(rvalue_, 0, size);
60+
}
61+
}
62+
63+
~CFunctionReturnStorage() {
64+
if (rvalue_ != nullptr && rvalue_ != inlineBuffer_) {
65+
std::free(rvalue_);
66+
}
67+
}
68+
69+
CFunctionReturnStorage(const CFunctionReturnStorage&) = delete;
70+
CFunctionReturnStorage& operator=(const CFunctionReturnStorage&) = delete;
71+
72+
bool isValid() const { return rvalue_ != nullptr; }
73+
void* rvalue() const { return rvalue_; }
74+
75+
private:
76+
static constexpr size_t kInlineSize = 32;
77+
alignas(max_align_t) unsigned char inlineBuffer_[kInlineSize];
78+
void* rvalue_ = nullptr;
79+
};
80+
3681
class CFunctionInvocationFrame final {
3782
public:
3883
explicit CFunctionInvocationFrame(Cif* cif)
@@ -433,17 +478,18 @@ inline void ensureCFunctionDispatchLookup(CFunction* function, Cif* cif) {
433478

434479
const bool isMainEntrypoint =
435480
strcmp(name, "UIApplicationMain") == 0 || strcmp(name, "NSApplicationMain") == 0;
436-
auto invocationFrame = std::make_shared<CFunctionInvocationFrame>(cif);
437-
if (!invocationFrame->isValid()) {
438-
napi_throw_error(env, "NativeScriptException",
439-
"Unable to allocate C function invocation storage.");
440-
return nullptr;
441-
}
442-
void* rvalue = invocationFrame->rvalue();
443481

444482
if ((engineDirectInvoker != nullptr ||
445483
(napiInvoker != nullptr && !cif->skipGeneratedNapiDispatch)) &&
446484
!isMainEntrypoint) {
485+
CFunctionReturnStorage returnStorage(cif);
486+
if (!returnStorage.isValid()) {
487+
napi_throw_error(env, "NativeScriptException",
488+
"Unable to allocate C function return storage.");
489+
return nullptr;
490+
}
491+
492+
void* rvalue = returnStorage.rvalue();
447493
@try {
448494
NativeCallRuntimeUnlockScope unlockRuntime(env);
449495
bool invoked = engineDirectInvoker != nullptr
@@ -468,6 +514,13 @@ inline void ensureCFunctionDispatchLookup(CFunction* function, Cif* cif) {
468514
return cif->returnType->toJS(env, rvalue, toJSFlags);
469515
}
470516

517+
auto invocationFrame = std::make_shared<CFunctionInvocationFrame>(cif);
518+
if (!invocationFrame->isValid()) {
519+
napi_throw_error(env, "NativeScriptException",
520+
"Unable to allocate C function invocation storage.");
521+
return nullptr;
522+
}
523+
void* rvalue = invocationFrame->rvalue();
471524
void** avalues = invocationFrame->avalues();
472525

473526
bool shouldFreeAny = false;

‎NativeScript/ffi/JSCFastNativeApi.mm‎

Lines changed: 69 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,12 @@
3636
CFunction = 5,
3737
};
3838

39+
enum class JSCEngineDirectResult {
40+
NotHandled,
41+
Handled,
42+
Failed,
43+
};
44+
3945
inline bool needsRoundTripCacheFrame(Cif* cif) {
4046
return cif != nullptr && cif->generatedDispatchHasRoundTripCacheArgument;
4147
}
@@ -687,20 +693,18 @@ bool makeJSCCFunctionReturnValue(napi_env env, CFunction* function, Cif* cif,
687693
return true;
688694
}
689695

690-
bool tryCallJSCObjCEngineDirect(napi_env env, ObjCClassMember* member,
691-
napi_value jsThis, size_t argc,
692-
const napi_value* argv,
693-
EngineDirectMemberKind kind,
694-
JSValueRef* result) {
696+
JSCEngineDirectResult tryCallJSCObjCEngineDirect(
697+
napi_env env, ObjCClassMember* member, napi_value jsThis, size_t argc,
698+
const napi_value* argv, EngineDirectMemberKind kind, JSValueRef* result) {
695699
if (env == nullptr || member == nullptr || member->bridgeState == nullptr ||
696700
result == nullptr) {
697-
return false;
701+
return JSCEngineDirectResult::NotHandled;
698702
}
699703

700704
MethodDescriptor* descriptor = nullptr;
701705
Cif* cif = jscMemberCif(env, member, kind, &descriptor);
702706
if (cif == nullptr || cif->isVariadic || cif->returnType == nullptr) {
703-
return false;
707+
return JSCEngineDirectResult::NotHandled;
704708
}
705709

706710
const bool canUseGeneratedInvoker =
@@ -712,23 +716,23 @@ bool tryCallJSCObjCEngineDirect(napi_env env, ObjCClassMember* member,
712716

713717
if (isJSCNSErrorOutSignature(descriptor, cif) ||
714718
isJSCBlockFallbackSelector(descriptor->selector)) {
715-
return false;
719+
return JSCEngineDirectResult::NotHandled;
716720
}
717721

718722
id self = resolveJSCSelf(env, jsThis, member);
719723
if (self == nil) {
720-
return false;
724+
return JSCEngineDirectResult::NotHandled;
721725
}
722726

723727
const bool receiverIsClass = object_isClass(self);
724728
Class receiverClass = receiverIsClass ? static_cast<Class>(self) : object_getClass(self);
725729
if (receiverClassRequiresJSCSuperCall(receiverClass)) {
726-
return false;
730+
return JSCEngineDirectResult::NotHandled;
727731
}
728732

729733
JSCFastReturnStorage rvalueStorage(cif);
730734
if (!rvalueStorage.valid()) {
731-
return false;
735+
return JSCEngineDirectResult::NotHandled;
732736
}
733737

734738
void* rvalue = rvalueStorage.get();
@@ -748,34 +752,44 @@ JSCFastRoundTripCacheFrameGuard roundTripCacheFrame(
748752
std::string message = exception.description.UTF8String;
749753
NativeScriptException nativeScriptException(message);
750754
nativeScriptException.ReThrowToJS(env);
751-
return false;
755+
return JSCEngineDirectResult::Failed;
752756
}
753757

754-
return didInvoke &&
755-
makeJSCObjCReturnValue(env, member, descriptor, cif, self,
756-
receiverIsClass, jsThis, rvalue,
757-
kind != EngineDirectMemberKind::Method, result);
758+
if (!didInvoke) {
759+
if (invoker == nullptr && env->last_exception != nullptr) {
760+
return JSCEngineDirectResult::Failed;
761+
}
762+
return JSCEngineDirectResult::NotHandled;
763+
}
764+
765+
if (!makeJSCObjCReturnValue(env, member, descriptor, cif, self,
766+
receiverIsClass, jsThis, rvalue,
767+
kind != EngineDirectMemberKind::Method, result)) {
768+
return JSCEngineDirectResult::Failed;
769+
}
770+
771+
return JSCEngineDirectResult::Handled;
758772
}
759773

760-
bool tryCallJSCCFunctionEngineDirect(napi_env env, MDSectionOffset offset,
761-
size_t argc, const napi_value* argv,
762-
JSValueRef* result) {
774+
JSCEngineDirectResult tryCallJSCCFunctionEngineDirect(
775+
napi_env env, MDSectionOffset offset, size_t argc, const napi_value* argv,
776+
JSValueRef* result) {
763777
if (env == nullptr || result == nullptr) {
764-
return false;
778+
return JSCEngineDirectResult::NotHandled;
765779
}
766780

767781
ObjCBridgeState* bridgeState = ObjCBridgeState::InstanceData(env);
768782
if (bridgeState == nullptr ||
769783
isCompatCFunction(env, reinterpret_cast<void*>(
770784
static_cast<uintptr_t>(offset)))) {
771-
return false;
785+
return JSCEngineDirectResult::NotHandled;
772786
}
773787

774788
CFunction* function = bridgeState->getCFunction(env, offset);
775789
Cif* cif = function != nullptr ? function->cif : nullptr;
776790
if (function == nullptr || cif == nullptr || cif->isVariadic ||
777791
cif->returnType == nullptr) {
778-
return false;
792+
return JSCEngineDirectResult::NotHandled;
779793
}
780794

781795
const bool canUseGeneratedInvoker =
@@ -788,7 +802,7 @@ bool tryCallJSCCFunctionEngineDirect(napi_env env, MDSectionOffset offset,
788802
JSCFastRoundTripCacheFrameGuard roundTripCacheFrame(env, bridgeState, cif);
789803
JSCFastReturnStorage rvalueStorage(cif);
790804
if (!rvalueStorage.valid()) {
791-
return false;
805+
return JSCEngineDirectResult::NotHandled;
792806
}
793807
void* rvalue = rvalueStorage.get();
794808
@try {
@@ -802,10 +816,21 @@ bool tryCallJSCCFunctionEngineDirect(napi_env env, MDSectionOffset offset,
802816
std::string message = exception.description.UTF8String;
803817
NativeScriptException nativeScriptException(message);
804818
nativeScriptException.ReThrowToJS(env);
805-
return false;
819+
return JSCEngineDirectResult::Failed;
820+
}
821+
822+
if (!didInvoke) {
823+
if (invoker == nullptr && env->last_exception != nullptr) {
824+
return JSCEngineDirectResult::Failed;
825+
}
826+
return JSCEngineDirectResult::NotHandled;
827+
}
828+
829+
if (!makeJSCCFunctionReturnValue(env, function, cif, rvalue, result)) {
830+
return JSCEngineDirectResult::Failed;
806831
}
807832

808-
return didInvoke && makeJSCCFunctionReturnValue(env, function, cif, rvalue, result);
833+
return JSCEngineDirectResult::Handled;
809834
}
810835

811836
void initializeFastFunction(JSContextRef ctx, JSObjectRef object) {
@@ -879,30 +904,30 @@ JSValueRef callFastFunction(JSContextRef ctx, JSObjectRef function,
879904
}
880905
napi_value jsThis = ToNapi(effectiveThis);
881906
JSValueRef directResult = nullptr;
882-
bool didUseDirectResult = false;
907+
JSCEngineDirectResult directCallResult = JSCEngineDirectResult::NotHandled;
883908
switch (binding->kind) {
884909
case JSCFastNativeKind::ObjCMethod:
885-
didUseDirectResult = tryCallJSCObjCEngineDirect(
910+
directCallResult = tryCallJSCObjCEngineDirect(
886911
env, static_cast<ObjCClassMember*>(binding->data), jsThis,
887912
argumentCount, argv, EngineDirectMemberKind::Method,
888913
&directResult);
889914
break;
890915
case JSCFastNativeKind::ObjCGetter:
891-
didUseDirectResult = tryCallJSCObjCEngineDirect(
916+
directCallResult = tryCallJSCObjCEngineDirect(
892917
env, static_cast<ObjCClassMember*>(binding->data), jsThis, 0,
893918
nullptr, EngineDirectMemberKind::Getter, &directResult);
894919
break;
895920
case JSCFastNativeKind::ObjCSetter: {
896921
JSValueRef undefined = JSValueMakeUndefined(ctx);
897922
napi_value value =
898923
argumentCount > 0 ? ToNapi(arguments[0]) : ToNapi(undefined);
899-
didUseDirectResult = tryCallJSCObjCEngineDirect(
924+
directCallResult = tryCallJSCObjCEngineDirect(
900925
env, static_cast<ObjCClassMember*>(binding->data), jsThis, 1,
901926
&value, EngineDirectMemberKind::Setter, &directResult);
902927
break;
903928
}
904929
case JSCFastNativeKind::CFunction:
905-
didUseDirectResult = tryCallJSCCFunctionEngineDirect(
930+
directCallResult = tryCallJSCCFunctionEngineDirect(
906931
env,
907932
static_cast<MDSectionOffset>(
908933
reinterpret_cast<uintptr_t>(binding->data)),
@@ -912,7 +937,7 @@ JSValueRef callFastFunction(JSContextRef ctx, JSObjectRef function,
912937
break;
913938
}
914939

915-
if (didUseDirectResult) {
940+
if (directCallResult == JSCEngineDirectResult::Handled) {
916941
if (env->last_exception != nullptr) {
917942
if (exception != nullptr) {
918943
*exception = env->last_exception;
@@ -922,6 +947,19 @@ JSValueRef callFastFunction(JSContextRef ctx, JSObjectRef function,
922947
}
923948
return directResult != nullptr ? directResult : JSValueMakeUndefined(ctx);
924949
}
950+
951+
if (directCallResult == JSCEngineDirectResult::Failed) {
952+
if (env->last_exception == nullptr) {
953+
napi_throw_error(env, "NativeScriptException",
954+
"NativeScript fast native call failed.");
955+
}
956+
if (exception != nullptr) {
957+
*exception = env->last_exception;
958+
}
959+
env->last_exception = nullptr;
960+
return JSValueMakeUndefined(ctx);
961+
}
962+
925963
env->last_exception = nullptr;
926964

927965
napi_value result = nullptr;

0 commit comments

Comments
 (0)