From 76ee745ed3840fd9a8e0b2e90fa9f810875214f3 Mon Sep 17 00:00:00 2001 From: William Candillon Date: Fri, 14 Jun 2024 19:13:49 +0200 Subject: [PATCH] =?UTF-8?q?fix(=F0=9F=A7=B5):=20thread=20safety=20in=20RNS?= =?UTF-8?q?kJsiViewApi.h=20(#2481)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- package/cpp/rnskia/RNSkJsiViewApi.h | 92 ++++++++------------------- package/cpp/rnskia/RNSkManager.cpp | 2 +- package/cpp/rnskia/RNSkView.h | 11 ---- package/src/views/SkiaDomView.tsx | 1 - package/src/views/SkiaJSDomView.tsx | 1 - package/src/views/SkiaPictureView.tsx | 1 - package/src/views/types.ts | 13 ---- 7 files changed, 26 insertions(+), 95 deletions(-) diff --git a/package/cpp/rnskia/RNSkJsiViewApi.h b/package/cpp/rnskia/RNSkJsiViewApi.h index 54343db101..82195da9b0 100644 --- a/package/cpp/rnskia/RNSkJsiViewApi.h +++ b/package/cpp/rnskia/RNSkJsiViewApi.h @@ -51,10 +51,11 @@ class RNSkJsiViewApi : public RNJsi::JsiHostObject, return jsi::Value::undefined(); } + auto nativeId = arguments[0].asNumber(); + std::lock_guard lock(_mutex); auto info = getEnsuredViewInfo(nativeId); - std::lock_guard lock(_mutex); info->props.insert_or_assign(arguments[1].asString(runtime).utf8(runtime), RNJsi::JsiValueWrapper(runtime, arguments[2])); @@ -69,52 +70,6 @@ class RNSkJsiViewApi : public RNJsi::JsiHostObject, return jsi::Value::undefined(); } - /** - Calls a custom command / method on a view by the view id. - */ - JSI_HOST_FUNCTION(callJsiMethod) { - if (count < 2) { - _platformContext->raiseError( - std::string("callCustomCommand: Expected at least 2 arguments, got " + - std::to_string(count) + ".")); - - return jsi::Value::undefined(); - } - - if (!arguments[0].isNumber()) { - _platformContext->raiseError( - "callCustomCommand: First argument must be a number"); - - return jsi::Value::undefined(); - } - - if (!arguments[1].isString()) { - _platformContext->raiseError("callCustomCommand: Second argument must be " - "the name of the action to call."); - - return jsi::Value::undefined(); - } - - auto nativeId = arguments[0].asNumber(); - auto action = arguments[1].asString(runtime).utf8(runtime); - - auto info = getEnsuredViewInfo(nativeId); - - if (info->view == nullptr) { - throw jsi::JSError( - runtime, std::string("callCustomCommand: Could not call action " + - action + " on view - view not ready.") - .c_str()); - - return jsi::Value::undefined(); - } - - // Get arguments - size_t paramsCount = count - 2; - const jsi::Value *params = paramsCount > 0 ? &arguments[2] : nullptr; - return info->view->callJsiMethod(runtime, action, params, paramsCount); - } - JSI_HOST_FUNCTION(requestRedraw) { if (count != 1) { _platformContext->raiseError( @@ -133,7 +88,7 @@ class RNSkJsiViewApi : public RNJsi::JsiHostObject, // find Skia View int nativeId = arguments[0].asNumber(); - + std::lock_guard lock(_mutex); auto info = getEnsuredViewInfo(nativeId); if (info->view != nullptr) { info->view->requestRedraw(); @@ -158,13 +113,18 @@ class RNSkJsiViewApi : public RNJsi::JsiHostObject, // find Skia view int nativeId = arguments[0].asNumber(); sk_sp image; - auto info = getEnsuredViewInfo(nativeId); - if (info->view != nullptr) { + std::shared_ptr view; + { + std::lock_guard lock(_mutex); + auto info = getEnsuredViewInfo(nativeId); + view = info->view; + } + if (view != nullptr) { if (count > 1 && !arguments[1].isUndefined() && !arguments[1].isNull()) { auto rect = JsiSkRect::fromValue(runtime, arguments[1]); - image = info->view->makeImageSnapshot(rect.get()); + image = view->makeImageSnapshot(rect.get()); } else { - image = info->view->makeImageSnapshot(nullptr); + image = view->makeImageSnapshot(nullptr); } if (image == nullptr) { throw jsi::JSError(runtime, @@ -194,20 +154,25 @@ class RNSkJsiViewApi : public RNJsi::JsiHostObject, // find Skia view int nativeId = arguments[0].asNumber(); - auto info = getEnsuredViewInfo(nativeId); + std::shared_ptr view; + { + std::lock_guard lock(_mutex); + auto info = getEnsuredViewInfo(nativeId); + view = info->view; + } auto context = _platformContext; auto bounds = count > 1 && !arguments[1].isUndefined() && !arguments[1].isNull() ? JsiSkRect::fromValue(runtime, arguments[1]) : nullptr; return RNJsi::JsiPromises::createPromiseAsJSIValue( - runtime, [context = std::move(context), info, bounds]( + runtime, [context = std::move(context), view, bounds]( jsi::Runtime &runtime, std::shared_ptr promise) { - context->runOnMainThread([&runtime, info = std::move(info), + context->runOnMainThread([&runtime, view = std::move(view), promise = std::move(promise), context = std::move(context), bounds]() { - auto image = info->view->makeImageSnapshot( + auto image = view->makeImageSnapshot( bounds == nullptr ? nullptr : bounds.get()); context->runOnJavascriptThread( [&runtime, context = std::move(context), @@ -225,7 +190,6 @@ class RNSkJsiViewApi : public RNJsi::JsiHostObject, } JSI_EXPORT_FUNCTIONS(JSI_EXPORT_FUNC(RNSkJsiViewApi, setJsiProperty), - JSI_EXPORT_FUNC(RNSkJsiViewApi, callJsiMethod), JSI_EXPORT_FUNC(RNSkJsiViewApi, requestRedraw), JSI_EXPORT_FUNC(RNSkJsiViewApi, makeImageSnapshotAsync), JSI_EXPORT_FUNC(RNSkJsiViewApi, makeImageSnapshot)) @@ -237,21 +201,16 @@ class RNSkJsiViewApi : public RNJsi::JsiHostObject, explicit RNSkJsiViewApi(std::shared_ptr platformContext) : JsiHostObject(), _platformContext(platformContext) {} - /** - * Invalidates the Skia View Api object - */ - void invalidate() { unregisterAll(); } - /** Call to remove all draw view infos */ void unregisterAll() { + std::lock_guard lock(_mutex); // Unregister all views auto tempList = _viewInfos; for (const auto &info : tempList) { unregisterSkiaView(info.first); } - std::lock_guard lock(_mutex); _viewInfos.clear(); } @@ -261,8 +220,8 @@ class RNSkJsiViewApi : public RNJsi::JsiHostObject, * @param view View to register */ void registerSkiaView(size_t nativeId, std::shared_ptr view) { - auto info = getEnsuredViewInfo(nativeId); std::lock_guard lock(_mutex); + auto info = getEnsuredViewInfo(nativeId); info->view = view; info->view->setNativeId(nativeId); info->view->setJsiProperties(info->props); @@ -274,12 +233,12 @@ class RNSkJsiViewApi : public RNJsi::JsiHostObject, * @param nativeId View id */ void unregisterSkiaView(size_t nativeId) { + std::lock_guard lock(_mutex); if (_viewInfos.count(nativeId) == 0) { return; } auto info = getEnsuredViewInfo(nativeId); - std::lock_guard lock(_mutex); info->view = nullptr; _viewInfos.erase(nativeId); } @@ -291,11 +250,11 @@ class RNSkJsiViewApi : public RNJsi::JsiHostObject, or a valid view, effectively toggling the view's availability. */ void setSkiaView(size_t nativeId, std::shared_ptr view) { + std::lock_guard lock(_mutex); if (_viewInfos.find(nativeId) == _viewInfos.end()) { return; } auto info = getEnsuredViewInfo(nativeId); - std::lock_guard lock(_mutex); if (view != nullptr) { info->view = view; info->view->setNativeId(nativeId); @@ -315,7 +274,6 @@ class RNSkJsiViewApi : public RNJsi::JsiHostObject, RNSkViewInfo *getEnsuredViewInfo(size_t nativeId) { if (_viewInfos.count(nativeId) == 0) { RNSkViewInfo info; - std::lock_guard lock(_mutex); _viewInfos.emplace(nativeId, info); } return &_viewInfos.at(nativeId); diff --git a/package/cpp/rnskia/RNSkManager.cpp b/package/cpp/rnskia/RNSkManager.cpp index 70ec7fe748..87ea6c8c69 100644 --- a/package/cpp/rnskia/RNSkManager.cpp +++ b/package/cpp/rnskia/RNSkManager.cpp @@ -46,7 +46,7 @@ void RNSkManager::invalidate() { _isInvalidated = true; // Invalidate members - _viewApi->invalidate(); + _viewApi->unregisterAll(); _platformContext->invalidate(); } diff --git a/package/cpp/rnskia/RNSkView.h b/package/cpp/rnskia/RNSkView.h index beb07f038e..90c46f4a4f 100644 --- a/package/cpp/rnskia/RNSkView.h +++ b/package/cpp/rnskia/RNSkView.h @@ -173,17 +173,6 @@ class RNSkView : public std::enable_shared_from_this { // Nothing here... } - /** - Calls a custom action. - */ - virtual jsi::Value callJsiMethod(jsi::Runtime &runtime, - const std::string &name, - const jsi::Value *arguments, size_t count) { - throw std::runtime_error( - "The base Skia View does not support any commands. Command " + name + - " not found."); - } - /** * Repaints the Skia view using the underlying context and the drawcallback. * This method schedules a draw request that will be run on the correct diff --git a/package/src/views/SkiaDomView.tsx b/package/src/views/SkiaDomView.tsx index 3dbd850a1a..25d07873a0 100644 --- a/package/src/views/SkiaDomView.tsx +++ b/package/src/views/SkiaDomView.tsx @@ -114,7 +114,6 @@ const assertSkiaViewApi = () => { if ( SkiaViewApi === null || SkiaViewApi.setJsiProperty === null || - SkiaViewApi.callJsiMethod === null || SkiaViewApi.requestRedraw === null || SkiaViewApi.makeImageSnapshot === null ) { diff --git a/package/src/views/SkiaJSDomView.tsx b/package/src/views/SkiaJSDomView.tsx index 15923bd6c6..52c3c0faf6 100644 --- a/package/src/views/SkiaJSDomView.tsx +++ b/package/src/views/SkiaJSDomView.tsx @@ -121,7 +121,6 @@ const assertSkiaViewApi = () => { if ( SkiaViewApi === null || SkiaViewApi.setJsiProperty === null || - SkiaViewApi.callJsiMethod === null || SkiaViewApi.requestRedraw === null || SkiaViewApi.makeImageSnapshot === null ) { diff --git a/package/src/views/SkiaPictureView.tsx b/package/src/views/SkiaPictureView.tsx index e2f8d9a7bf..bb91bd2049 100644 --- a/package/src/views/SkiaPictureView.tsx +++ b/package/src/views/SkiaPictureView.tsx @@ -78,7 +78,6 @@ const assertSkiaViewApi = () => { if ( SkiaViewApi === null || SkiaViewApi.setJsiProperty === null || - SkiaViewApi.callJsiMethod === null || SkiaViewApi.requestRedraw === null || SkiaViewApi.makeImageSnapshot === null ) { diff --git a/package/src/views/types.ts b/package/src/views/types.ts index 9048ef1ab8..022c941d90 100644 --- a/package/src/views/types.ts +++ b/package/src/views/types.ts @@ -48,21 +48,8 @@ export type TouchHandlers = { export type TouchHandler = (touchInfo: Array>) => void; -/** - * Listener interface for value changes - */ -export interface ValueListener { - addListener: (callback: () => void) => number; - removeListener: (id: number) => void; -} - export interface ISkiaViewApi { setJsiProperty: (nativeId: number, name: string, value: T) => void; - callJsiMethod: >( - nativeId: number, - name: string, - ...args: T - ) => void; requestRedraw: (nativeId: number) => void; makeImageSnapshot: (nativeId: number, rect?: SkRect) => SkImage; makeImageSnapshotAsync: (nativeId: number, rect?: SkRect) => Promise;