fix: polluted [RCTBridge currentBridge] #2760

alexandrius · 2024-11-21T11:57:31Z

Hello. This PR changes [RCTBridge currentBridge] to the bridge instance the ViewManager was created. For example react-native-threads creates its own bridge for process separation.

Thanks for all amazing work @chrfalch @wcandillon

alexandrius · 2024-11-21T12:43:17Z

I have signed the CLA!

wcandillon · 2024-12-04T11:57:33Z

@Kudo Thank you for looking into this. Here are the two comments that came up:

Make the originalBridgeInstance a weak reference to avoid retaining the React instance from native modules.
Consider renaming originalBridge to bridge, as the "original" prefix might not be necessary.

wcandillon · 2024-12-04T12:26:45Z

@Kudo @alexandrius is this potentially a fix for #2039?

Kudo · 2024-12-04T14:30:39Z

sorry for late to share comment here. i don't like the singleton actually but i didn't find a better way in fabric.
this is my proposed change:

we can get bridge from old arch ViewManager, so we don't need to access singleton.
accessing singleton bridge actually equals to accessing singleton SkiaManager. i turn to provide a SkManager rather than bridge.
keeps the singleton weak to prevent retain cycle.

diff --git a/node_modules/@shopify/react-native-skia/ios/RNSkia-iOS/SkiaDomView.mm b/node_modules/@shopify/react-native-skia/ios/RNSkia-iOS/SkiaDomView.mm
index 1f1bdb5..bc0831a 100644
--- a/node_modules/@shopify/react-native-skia/ios/RNSkia-iOS/SkiaDomView.mm
+++ b/node_modules/@shopify/react-native-skia/ios/RNSkia-iOS/SkiaDomView.mm
@@ -24,10 +24,10 @@ using namespace facebook::react;
 
 - (instancetype)initWithFrame:(CGRect)frame {
   if (self = [super initWithFrame:frame]) {
-    auto skManager = [[self skiaManager] skManager];
-    // Pass SkManager as a raw pointer to avoid circular dependenciesr
+    // Pass SkManager as a raw pointer to avoid circular dependencies
+    auto skManager = [SkiaManager latestActiveSkManager].get();
     [self
-        initCommon:skManager.get()
+        initCommon:skManager
            factory:[](std::shared_ptr<RNSkia::RNSkPlatformContext> context) {
              return std::make_shared<RNSkiOSView<RNSkia::RNSkDomView>>(context);
            }];
diff --git a/node_modules/@shopify/react-native-skia/ios/RNSkia-iOS/SkiaDomViewManager.mm b/node_modules/@shopify/react-native-skia/ios/RNSkia-iOS/SkiaDomViewManager.mm
index 30f53eb..d77f3b8 100644
--- a/node_modules/@shopify/react-native-skia/ios/RNSkia-iOS/SkiaDomViewManager.mm
+++ b/node_modules/@shopify/react-native-skia/ios/RNSkia-iOS/SkiaDomViewManager.mm
@@ -15,7 +15,8 @@
 RCT_EXPORT_MODULE(SkiaDomView)
 
 - (SkiaManager *)skiaManager {
-  auto bridge = [RCTBridge currentBridge];
+  auto bridge = self.bridge;
+  RCTAssert(bridge, @"Bridge must not be nil.");
   auto skiaModule = (RNSkiaModule *)[bridge moduleForName:@"RNSkiaModule"];
   return [skiaModule manager];
 }
diff --git a/node_modules/@shopify/react-native-skia/ios/RNSkia-iOS/SkiaManager.h b/node_modules/@shopify/react-native-skia/ios/RNSkia-iOS/SkiaManager.h
index 8f5457f..07d41dc 100644
--- a/node_modules/@shopify/react-native-skia/ios/RNSkia-iOS/SkiaManager.h
+++ b/node_modules/@shopify/react-native-skia/ios/RNSkia-iOS/SkiaManager.h
@@ -16,4 +16,10 @@
                      jsInvoker:(std::shared_ptr<facebook::react::CallInvoker>)
                                    jsInvoker;
 
+#ifdef RCT_NEW_ARCH_ENABLED
+// Fabric components do not have a better way to interact with TurboModules.
+// Workaround to get the SkManager instance from singleton.
++ (std::shared_ptr<RNSkia::RNSkManager>)latestActiveSkManager;
+#endif // RCT_NEW_ARCH_ENABLED
+
 @end
diff --git a/node_modules/@shopify/react-native-skia/ios/RNSkia-iOS/SkiaManager.mm b/node_modules/@shopify/react-native-skia/ios/RNSkia-iOS/SkiaManager.mm
index 7d3cf59..b32ea39 100644
--- a/node_modules/@shopify/react-native-skia/ios/RNSkia-iOS/SkiaManager.mm
+++ b/node_modules/@shopify/react-native-skia/ios/RNSkia-iOS/SkiaManager.mm
@@ -8,6 +8,8 @@
 
 #import "RNSkiOSPlatformContext.h"
 
+static __weak SkiaManager *sharedInstance = nil;
+
 @implementation SkiaManager {
   std::shared_ptr<RNSkia::RNSkManager> _skManager;
   __weak RCTBridge *weakBridge;
@@ -29,6 +31,7 @@
                                    jsInvoker {
   self = [super init];
   if (self) {
+    sharedInstance = self;
     RCTCxxBridge *cxxBridge = (RCTCxxBridge *)bridge;
     if (cxxBridge.runtime) {
 
@@ -45,4 +48,18 @@
   return self;
 }
 
+- (void)dealloc
+{
+  sharedInstance = nil;
+}
+
+#ifdef RCT_NEW_ARCH_ENABLED
++ (std::shared_ptr<RNSkia::RNSkManager>)latestActiveSkManager
+{
+  if (sharedInstance != nil) {
+    return [sharedInstance skManager];
+  }
+  return nullptr;
+}
+#endif // RCT_NEW_ARCH_ENABLED
 @end
diff --git a/node_modules/@shopify/react-native-skia/ios/RNSkia-iOS/SkiaPictureView.mm b/node_modules/@shopify/react-native-skia/ios/RNSkia-iOS/SkiaPictureView.mm
index 9bf2f61..b0730f6 100644
--- a/node_modules/@shopify/react-native-skia/ios/RNSkia-iOS/SkiaPictureView.mm
+++ b/node_modules/@shopify/react-native-skia/ios/RNSkia-iOS/SkiaPictureView.mm
@@ -24,9 +24,9 @@ using namespace facebook::react;
 
 - (instancetype)initWithFrame:(CGRect)frame {
   if (self = [super initWithFrame:frame]) {
-    auto skManager = [[self skiaManager] skManager];
-    // Pass SkManager as a raw pointer to avoid circular dependenciesr
-    [self initCommon:skManager.get()
+    // Pass SkManager as a raw pointer to avoid circular dependencies
+    auto skManager = [SkiaManager latestActiveSkManager].get();
+    [self initCommon:skManager
              factory:[](std::shared_ptr<RNSkia::RNSkPlatformContext> context) {
                return std::make_shared<RNSkiOSView<RNSkia::RNSkPictureView>>(
                    context);
diff --git a/node_modules/@shopify/react-native-skia/ios/RNSkia-iOS/SkiaPictureViewManager.mm b/node_modules/@shopify/react-native-skia/ios/RNSkia-iOS/SkiaPictureViewManager.mm
index 8ebccb1..c123db1 100644
--- a/node_modules/@shopify/react-native-skia/ios/RNSkia-iOS/SkiaPictureViewManager.mm
+++ b/node_modules/@shopify/react-native-skia/ios/RNSkia-iOS/SkiaPictureViewManager.mm
@@ -15,7 +15,8 @@
 RCT_EXPORT_MODULE(SkiaPictureView)
 
 - (SkiaManager *)skiaManager {
-  auto bridge = [RCTBridge currentBridge];
+  auto bridge = self.bridge;
+  RCTAssert(bridge, @"Bridge must not be nil.");
   auto skiaModule = (RNSkiaModule *)[bridge moduleForName:@"RNSkiaModule"];
   return [skiaModule manager];
 }
diff --git a/node_modules/@shopify/react-native-skia/ios/RNSkia-iOS/SkiaUIView.h b/node_modules/@shopify/react-native-skia/ios/RNSkia-iOS/SkiaUIView.h
index 8335d42..c3fb2e2 100644
--- a/node_modules/@shopify/react-native-skia/ios/RNSkia-iOS/SkiaUIView.h
+++ b/node_modules/@shopify/react-native-skia/ios/RNSkia-iOS/SkiaUIView.h
@@ -29,7 +29,6 @@
            factory:(std::function<std::shared_ptr<RNSkBaseiOSView>(
                         std::shared_ptr<RNSkia::RNSkPlatformContext>)>)factory;
 - (std::shared_ptr<RNSkBaseiOSView>)impl;
-- (SkiaManager *)skiaManager;
 
 - (void)setDebugMode:(bool)debugMode;
 - (void)setOpaque:(bool)opaque;
diff --git a/node_modules/@shopify/react-native-skia/ios/RNSkia-iOS/SkiaUIView.mm b/node_modules/@shopify/react-native-skia/ios/RNSkia-iOS/SkiaUIView.mm
index 8ec714e..2dbe373 100644
--- a/node_modules/@shopify/react-native-skia/ios/RNSkia-iOS/SkiaUIView.mm
+++ b/node_modules/@shopify/react-native-skia/ios/RNSkia-iOS/SkiaUIView.mm
@@ -48,12 +48,6 @@
   _factory = factory;
 }
 
-- (SkiaManager *)skiaManager {
-  auto bridge = [RCTBridge currentBridge];
-  auto skiaModule = (RNSkiaModule *)[bridge moduleForName:@"RNSkiaModule"];
-  return [skiaModule manager];
-}
-
 - (void)willInvalidateModules {
   _impl = nullptr;
   _manager = nullptr;
@@ -110,7 +104,7 @@
     // this flag is only set when the view is inserted and we want to set the
     // manager here since the view could be recycled or the app could be
     // refreshed and we would have a stale manager then
-    _manager = [[self skiaManager] skManager].get();
+    _manager = [SkiaManager latestActiveSkManager].get();
   }
 }
 #endif // RCT_NEW_ARCH_ENABLED

Kudo · 2024-12-04T14:31:27Z

@Kudo @alexandrius is this potentially a fix for #2039?

not pretty sure for this. i cannot repro this issue previously.

alexandrius · 2024-12-06T09:23:58Z

@Kudo @alexandrius is this potentially a fix for #2039?

It doesn't look like the same issue to me.

@wcandillon @Kudo

I'm a bit confused with the patch, are you planning to refactor it with a different approach? Should I close this PR?

Kudo · 2024-12-06T17:15:48Z

@alexandrius i would like to keep your pr ongoing since you are the first one raise the issue and propose a solution. so please try to update this pr if my patch makes sense to you. the whole idea of my patch is to move away [RCTBridge currentBridge] as much as possible. if there's anything unclear to you, please feel free to let me know and i can explain further. thanks again for your help.

This reverts commit a9a8b6b.

alexandrius · 2024-12-09T08:13:04Z

@Kudo @wcandillon Updated to @Kudo's patch. Tested works fine

Kudo

nice. just having some out-of-sync change and should be good to merge. thanks @alexandrius

packages/skia/ios/RNSkia-iOS/SkiaDomView.mm

packages/skia/ios/RNSkia-iOS/SkiaManager.mm

Co-authored-by: Kudo Chien <[email protected]>

github-actions · 2024-12-10T15:46:10Z

🎉 This PR is included in version 1.7.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

--------- Co-authored-by: William Candillon <[email protected]> Co-authored-by: Kudo Chien <[email protected]>

fix: polluted bridge

a9a8b6b

github-actions bot added the cla-needed label Nov 21, 2024

alexandrius changed the title ~~fix: polluted currentBridge~~ fix: polluted [RCTBridge currentBridge] Nov 21, 2024

github-actions bot removed the cla-needed label Nov 21, 2024

Merge branch 'main' into ap/fix_polluted_bridge

03e06c1

Merge branch 'main' into ap/fix_polluted_bridge

c1d36af

Merge branch 'main' into ap/fix_polluted_bridge

5df5d85

wcandillon and others added 3 commits December 6, 2024 18:36

Merge branch 'main' into ap/fix_polluted_bridge

3e25780

Revert "fix: polluted bridge"

6987dc6

This reverts commit a9a8b6b.

refactor: to Kudo's patch

12360aa

Kudo approved these changes Dec 9, 2024

View reviewed changes

packages/skia/ios/RNSkia-iOS/SkiaDomView.mm Outdated Show resolved Hide resolved

packages/skia/ios/RNSkia-iOS/SkiaManager.mm Outdated Show resolved Hide resolved

wcandillon and others added 3 commits December 9, 2024 16:11

Merge branch 'main' into ap/fix_polluted_bridge

d6fd536

Update packages/skia/ios/RNSkia-iOS/SkiaManager.mm

5c3570d

Co-authored-by: Kudo Chien <[email protected]>

Update packages/skia/ios/RNSkia-iOS/SkiaDomView.mm

484caa0

Co-authored-by: Kudo Chien <[email protected]>

Kudo approved these changes Dec 9, 2024

View reviewed changes

wcandillon merged commit 2cfa890 into Shopify:main Dec 9, 2024
8 checks passed

github-actions bot added the released label Dec 10, 2024

stevengoldberg pushed a commit to stevengoldberg/react-native-skia that referenced this pull request May 18, 2025

fix(🍏): use dedicated bridge (Shopify#2760)

d3fde86

--------- Co-authored-by: William Candillon <[email protected]> Co-authored-by: Kudo Chien <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: polluted [RCTBridge currentBridge] #2760

fix: polluted [RCTBridge currentBridge] #2760

Uh oh!

alexandrius commented Nov 21, 2024 •

edited

Loading

Uh oh!

alexandrius commented Nov 21, 2024

Uh oh!

wcandillon commented Dec 4, 2024

Uh oh!

wcandillon commented Dec 4, 2024

Uh oh!

Kudo commented Dec 4, 2024

Uh oh!

Kudo commented Dec 4, 2024

Uh oh!

alexandrius commented Dec 6, 2024

Uh oh!

Kudo commented Dec 6, 2024

Uh oh!

alexandrius commented Dec 9, 2024

Uh oh!

Kudo left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Dec 10, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fix: polluted [RCTBridge currentBridge] #2760

fix: polluted [RCTBridge currentBridge] #2760

Uh oh!

Conversation

alexandrius commented Nov 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexandrius commented Nov 21, 2024

Uh oh!

wcandillon commented Dec 4, 2024

Uh oh!

wcandillon commented Dec 4, 2024

Uh oh!

Kudo commented Dec 4, 2024

Uh oh!

Kudo commented Dec 4, 2024

Uh oh!

alexandrius commented Dec 6, 2024

Uh oh!

Kudo commented Dec 6, 2024

Uh oh!

alexandrius commented Dec 9, 2024

Uh oh!

Kudo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Dec 10, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

alexandrius commented Nov 21, 2024 •

edited

Loading