fix: crash when copying TurboModules between runtimes#53111
Conversation
| @@ -150,6 +151,7 @@ class JSI_EXPORT TurboModule : public jsi::HostObject { | |||
| private: | |||
| friend class TurboModuleBinding; | |||
| std::unique_ptr<jsi::WeakObject> jsRepresentation_; | |||
| const jsi::Runtime* representationRuntime_; | |||
There was a problem hiding this comment.
shouldn't this be a shared_ptr, instead of a plain pointer?
There was a problem hiding this comment.
You probably don't want to hold a shared_ptr to the runtime in each TurboModule. This plain ptr is only to compare the addresses of runtimes - if the runtime the called the get of HostObject is the same as the one that holds the jsRepresentation.
I made it const so you'd not be able to invoke anything on that runtime since it's held as a plain ptr.
There was a problem hiding this comment.
We almost never use regular pointers in React Native. This should be either a shared_ptr or a weak_ptr if you don't want for it to keep the runtime alive.
There was a problem hiding this comment.
I don't think there's any way to obtain a weak_ptr here. This pointer is used only literally to compare addresses. Maybe we could store it as std::uintptr_t to signify it has a numerical meaning?
|
Hey @cipolleschi could you take another look at it? |
be86ba1 to
34af43e
Compare
|
This PR is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days. |
Summary:
Since TurboModules got optimized for property lookups it became impossible to copy them between runtimes. These is because the C++ code tried to use
jsRepresentationwhich was defined on another runtime. I addedrepresentationRuntimemember toTurboModuleclass so thejsRepresentationwould be used only if we are on the correct runtime. On other runtimes we should resolve to "slow" property lookup. The runtime can then make the same optimization on its own.The code that works now:
Changelog:
[INTERNAL] [FIXED] - TurboModules don't try to use
jsRepresentationfor property lookups on wrong runtimesTest Plan:
Running TurboModule examples in RNTester didn't show any regressions and the code snippet above works now.