fix(🧵): thread safety in RNSkJsiViewApi.h (#2481)

Shopify · Jun 14, 2024 · 76ee745 · 76ee745
1 parent 6a8e64e
commit 76ee745
Show file tree

Hide file tree

Showing 7 changed files with 26 additions and 95 deletions.
diff --git 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<std::mutex> lock(_mutex);
     auto info = getEnsuredViewInfo(nativeId);
 
-    std::lock_guard<std::mutex> 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<std::mutex> 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<SkImage> image;
-    auto info = getEnsuredViewInfo(nativeId);
-    if (info->view != nullptr) {
+    std::shared_ptr<RNSkView> view;
+    {
+      std::lock_guard<std::mutex> 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<RNSkView> view;
+    {
+      std::lock_guard<std::mutex> 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<RNJsi::JsiPromises::Promise> 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<RNSkPlatformContext> 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<std::mutex> lock(_mutex);
     // Unregister all views
     auto tempList = _viewInfos;
     for (const auto &info : tempList) {
       unregisterSkiaView(info.first);
     }
-    std::lock_guard<std::mutex> 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<RNSkView> view) {
-    auto info = getEnsuredViewInfo(nativeId);
     std::lock_guard<std::mutex> 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<std::mutex> lock(_mutex);
     if (_viewInfos.count(nativeId) == 0) {
       return;
     }
     auto info = getEnsuredViewInfo(nativeId);
 
-    std::lock_guard<std::mutex> 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<RNSkView> view) {
+    std::lock_guard<std::mutex> lock(_mutex);
     if (_viewInfos.find(nativeId) == _viewInfos.end()) {
       return;
     }
     auto info = getEnsuredViewInfo(nativeId);
-    std::lock_guard<std::mutex> 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<std::mutex> lock(_mutex);
       _viewInfos.emplace(nativeId, info);
     }
     return &_viewInfos.at(nativeId);

diff --git 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
@@ -173,17 +173,6 @@ class RNSkView : public std::enable_shared_from_this<RNSkView> {
     // 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
@@ -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
@@ -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
@@ -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
@@ -48,21 +48,8 @@ export type TouchHandlers = {
 
 export type TouchHandler = (touchInfo: Array<Array<TouchInfo>>) => void;
 
-/**
- * Listener interface for value changes
- */
-export interface ValueListener {
-  addListener: (callback: () => void) => number;
-  removeListener: (id: number) => void;
-}
-
 export interface ISkiaViewApi {
   setJsiProperty: <T>(nativeId: number, name: string, value: T) => void;
-  callJsiMethod: <T extends Array<unknown>>(
-    nativeId: number,
-    name: string,
-    ...args: T
-  ) => void;
   requestRedraw: (nativeId: number) => void;
   makeImageSnapshot: (nativeId: number, rect?: SkRect) => SkImage;
   makeImageSnapshotAsync: (nativeId: number, rect?: SkRect) => Promise<SkImage>;