From 0c3cd66016b811a2795b847d4e70647b7b9c864b Mon Sep 17 00:00:00 2001 From: Marc Rousavy Date: Tue, 21 Feb 2023 15:44:43 +0100 Subject: [PATCH] fix: Improve C++ safety by attaching Cache Invalidator to `jsi::Runtime`'s lifecycle (#1488) * fix: fix C++ lint * fix: attach `InvalidateCacheOnDestroy` to `jsi::Runtime` --- .github/workflows/validate-cpp.yml | 1 + android/src/main/cpp/FrameProcessorRuntimeManager.cpp | 7 +++++++ android/src/main/cpp/FrameProcessorRuntimeManager.h | 2 +- android/src/main/cpp/JSIJNIConversion.h | 4 ++-- cpp/JSITypedArray.cpp | 11 ++++++++--- cpp/JSITypedArray.h | 6 ++++-- ios/Frame Processor/FrameProcessorRuntimeManager.mm | 7 +++++++ scripts/cpplint.sh | 2 +- 8 files changed, 31 insertions(+), 9 deletions(-) diff --git a/.github/workflows/validate-cpp.yml b/.github/workflows/validate-cpp.yml index b92ab23..c976460 100644 --- a/.github/workflows/validate-cpp.yml +++ b/.github/workflows/validate-cpp.yml @@ -30,6 +30,7 @@ jobs: ,-readability/todo\ ,-build/namespaces\ ,-whitespace/comments\ + ,-runtime/references\ ,-build/include_order\ ,-build/c++11\ " diff --git a/android/src/main/cpp/FrameProcessorRuntimeManager.cpp b/android/src/main/cpp/FrameProcessorRuntimeManager.cpp index db59050..5ec9171 100644 --- a/android/src/main/cpp/FrameProcessorRuntimeManager.cpp +++ b/android/src/main/cpp/FrameProcessorRuntimeManager.cpp @@ -15,6 +15,7 @@ #include "JSIJNIConversion.h" #include "java-bindings/JImageProxy.h" #include "java-bindings/JFrameProcessorPlugin.h" +#include "JSITypedArray.h" namespace vision { @@ -151,6 +152,12 @@ void FrameProcessorRuntimeManager::installJSIBindings() { auto& jsiRuntime = *_jsRuntime; + // HostObject that attaches the cache to the lifecycle of the Runtime. On Runtime destroy, we destroy the cache. + auto propNameCacheObject = std::make_shared(jsiRuntime); + jsiRuntime.global().setProperty(jsiRuntime, + "__visionCameraPropNameCache", + jsi::Object::createFromHostObject(jsiRuntime, propNameCacheObject)); + auto setFrameProcessor = JSI_HOST_FUNCTION_LAMBDA { __android_log_write(ANDROID_LOG_INFO, TAG, "Setting new Frame Processor..."); diff --git a/android/src/main/cpp/FrameProcessorRuntimeManager.h b/android/src/main/cpp/FrameProcessorRuntimeManager.h index 3f545e5..1126baa 100644 --- a/android/src/main/cpp/FrameProcessorRuntimeManager.h +++ b/android/src/main/cpp/FrameProcessorRuntimeManager.h @@ -45,7 +45,7 @@ class FrameProcessorRuntimeManager : public jni::HybridClass plugin); void logErrorToJS(const std::string& message); - void setFrameProcessor(jsi::Runtime& runtime, // NOLINT(runtime/references) + void setFrameProcessor(jsi::Runtime& runtime, int viewTag, const jsi::Value& frameProcessor); void unsetFrameProcessor(int viewTag); diff --git a/android/src/main/cpp/JSIJNIConversion.h b/android/src/main/cpp/JSIJNIConversion.h index 4af3126..5f031ec 100644 --- a/android/src/main/cpp/JSIJNIConversion.h +++ b/android/src/main/cpp/JSIJNIConversion.h @@ -14,9 +14,9 @@ namespace JSIJNIConversion { using namespace facebook; -jobject convertJSIValueToJNIObject(jsi::Runtime& runtime, const jsi::Value& value); // NOLINT(runtime/references) +jobject convertJSIValueToJNIObject(jsi::Runtime& runtime, const jsi::Value& value); -jsi::Value convertJNIObjectToJSIValue(jsi::Runtime& runtime, const jni::local_ref& object); // NOLINT(runtime/references) +jsi::Value convertJNIObjectToJSIValue(jsi::Runtime& runtime, const jni::local_ref& object); } // namespace JSIJNIConversion diff --git a/cpp/JSITypedArray.cpp b/cpp/JSITypedArray.cpp index 4642a2f..aedd71e 100644 --- a/cpp/JSITypedArray.cpp +++ b/cpp/JSITypedArray.cpp @@ -12,6 +12,11 @@ #include "JSITypedArray.h" #include +#include +#include +#include +#include +#include namespace vision { @@ -97,7 +102,7 @@ TypedArrayKind TypedArrayBase::getKind(jsi::Runtime &runtime) const { .asString(runtime) .utf8(runtime); return getTypedArrayKindForName(constructorName); -}; +} size_t TypedArrayBase::size(jsi::Runtime &runtime) const { return getProperty(runtime, propNameIDCache.get(runtime, Prop::Length)).asNumber(); @@ -191,13 +196,13 @@ void arrayBufferUpdate( } template -TypedArray::TypedArray(jsi::Runtime &runtime, size_t size) : TypedArrayBase(runtime, size, T){}; +TypedArray::TypedArray(jsi::Runtime &runtime, size_t size) : TypedArrayBase(runtime, size, T) {} template TypedArray::TypedArray(jsi::Runtime &runtime, std::vector> data) : TypedArrayBase(runtime, data.size(), T) { update(runtime, data); -}; +} template TypedArray::TypedArray(TypedArrayBase &&base) : TypedArrayBase(std::move(base)) {} diff --git a/cpp/JSITypedArray.h b/cpp/JSITypedArray.h index a2ec5ed..eb470da 100644 --- a/cpp/JSITypedArray.h +++ b/cpp/JSITypedArray.h @@ -13,6 +13,8 @@ #pragma once #include +#include +#include namespace jsi = facebook::jsi; @@ -77,7 +79,7 @@ struct typedArrayTypeMap { // the cache object is connected to the lifecycle of the js runtime class InvalidateCacheOnDestroy : public jsi::HostObject { public: - InvalidateCacheOnDestroy(jsi::Runtime &runtime); + explicit InvalidateCacheOnDestroy(jsi::Runtime &runtime); virtual ~InvalidateCacheOnDestroy(); virtual jsi::Value get(jsi::Runtime &, const jsi::PropNameID &name) { return jsi::Value::null(); @@ -139,9 +141,9 @@ void arrayBufferUpdate( template class TypedArray : public TypedArrayBase { public: + explicit TypedArray(TypedArrayBase &&base); TypedArray(jsi::Runtime &runtime, size_t size); TypedArray(jsi::Runtime &runtime, std::vector> data); - TypedArray(TypedArrayBase &&base); TypedArray(TypedArray &&) = default; TypedArray &operator=(TypedArray &&) = default; diff --git a/ios/Frame Processor/FrameProcessorRuntimeManager.mm b/ios/Frame Processor/FrameProcessorRuntimeManager.mm index 88ae71f..8cc3555 100644 --- a/ios/Frame Processor/FrameProcessorRuntimeManager.mm +++ b/ios/Frame Processor/FrameProcessorRuntimeManager.mm @@ -27,6 +27,7 @@ #import "FrameProcessorUtils.h" #import "FrameProcessorCallback.h" #import "../React Utils/JSIUtils.h" +#import "../../cpp/JSITypedArray.h" // Forward declarations for the Swift classes __attribute__((objc_runtime_name("_TtC12VisionCamera12CameraQueues"))) @@ -130,6 +131,12 @@ __attribute__((objc_runtime_name("_TtC12VisionCamera10CameraView"))) } jsi::Runtime& jsiRuntime = *(jsi::Runtime*)cxxBridge.runtime; + + // HostObject that attaches the cache to the lifecycle of the Runtime. On Runtime destroy, we destroy the cache. + auto propNameCacheObject = std::make_shared(jsiRuntime); + jsiRuntime.global().setProperty(jsiRuntime, + "__visionCameraPropNameCache", + jsi::Object::createFromHostObject(jsiRuntime, propNameCacheObject)); // Install the Worklet Runtime in the main React JS Runtime [self setupWorkletContext:jsiRuntime]; diff --git a/scripts/cpplint.sh b/scripts/cpplint.sh index 4abec7c..6b19a86 100755 --- a/scripts/cpplint.sh +++ b/scripts/cpplint.sh @@ -1,7 +1,7 @@ #!/bin/bash if which cpplint >/dev/null; then - cpplint --linelength=230 --filter=-legal/copyright,-readability/todo,-build/namespaces,-whitespace/comments,-build/include_order,-build/c++11 --quiet --recursive cpp android/src/main/cpp + cpplint --linelength=230 --filter=-legal/copyright,-readability/todo,-build/namespaces,-runtime/references,-whitespace/comments,-build/include_order,-build/c++11 --quiet --recursive cpp android/src/main/cpp else echo "warning: cpplint not installed, download from https://github.com/cpplint/cpplint" fi