Skip to content

Commit

Permalink
fix: Cache StoreUser (JSIStoreValueUser) on a per-`RuntimeManager…
Browse files Browse the repository at this point in the history
…`-basis (#2219)

## Description

<!--
Description and motivation for this PR.

Inlude Fixes #<number> if this is fixing some issue.

Fixes # .
-->

Until now, the `JSIStoreValueUser` class was used to cache `jsi::Value`s. This worked fine for Reanimated since there was only one separate Runtime apart from the React-JS runtime, but once another library tries to use the workletization code, this will break since `JSIStoreValueUser` is accessed from multiple runtimes. Instead, it should only be used on a per-runtime basis, so I moved the static code to instance code, and added a Shared Pointer to `RuntimeManager`.

This fixes SIGABRT crashes for when trying to use a captured variable (such as `console`) in multiple runtimes (REA, VisionCamera, react-native-multithreading, ...):

![image](https://user-images.githubusercontent.com/15199031/126974869-f40ec8b3-ba94-4ce8-9c0f-ef412b24e8fc.png)

The above crash happened because a `jsi::Value` (`console`) cached inside `StoreUser` was accessed from the VisionCamera runtime, but the value was initially created on the REA runtime.

## Changes

<!--
Please describe things you've changed here, make a **high level** overview, if change is simple you can omit this section.

For example:

- Added `foo` method which add bouncing animation
- Updated `about.md` docs
- Added caching in CI builds

-->
* Refactored `JSIStoreValueUser` to be an instance variable of `RuntimeManager` instead of a static one.
* Clear store on `RuntimeManager` dealloc (dtor)
* Fix a `isWorkletRuntime` check which should instead strictly compare if the runtimes are exactly same

<!--

## Screenshots / GIFs

Here you can add screenshots / GIFs documenting your change.

You can add before / after section if you're changing some behavior.

### Before

### After

-->

## Test code and steps to reproduce

1. Create a normal function on the React-JS Thread
2. Verify that you can call that function inside of a REA UI Worklet (`useAnimatedStyle`, or `runOnUI`) using `runOnJS`
3. Verify that you can call that same function inside of a VisionCamera FP Worklet (`useFrameProcessor`) using `runOnJS`

<!--
Please include code that can be used to test this change and short description how this example should work.
This snippet should be as minimal as possible and ready to be pasted into editor (don't exclude exports or remove "not important" parts of reproduction example)
-->

## Checklist

- [ ] Included code example that can be used to test this change
- [ ] Updated TS types
- [ ] Added TS types tests
- [ ] Added unit / integration tests
- [ ] Updated documentation
- [ ] Ensured that CI passes
  • Loading branch information
mrousavy authored Jul 26, 2021
1 parent 285b64a commit 0f5e7fb
Show file tree
Hide file tree
Showing 10 changed files with 51 additions and 40 deletions.
15 changes: 5 additions & 10 deletions Common/cpp/NativeModules/NativeReanimatedModule.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ NativeReanimatedModule::NativeReanimatedModule(std::shared_ptr<CallInvoker> jsIn
std::shared_ptr<ErrorHandler> errorHandler,
std::function<jsi::Value(jsi::Runtime &, const int, const jsi::String &)> propObtainer,
std::shared_ptr<LayoutAnimationsProxy> layoutAnimationsProxy,
PlatformDepMethodsHolder platformDepMethodsHolder) :
PlatformDepMethodsHolder platformDepMethodsHolder) :
NativeReanimatedModuleSpec(jsInvoker),
RuntimeManager(rt, errorHandler, scheduler, RuntimeType::UI),
mapperRegistry(std::make_shared<MapperRegistry>()),
Expand All @@ -76,9 +76,9 @@ NativeReanimatedModule::NativeReanimatedModule(std::shared_ptr<CallInvoker> jsIn
frameCallbacks.push_back(callback);
maybeRequestRender();
};

this->layoutAnimationsProxy = layoutAnimationsProxy;

RuntimeDecorator::decorateUIRuntime(*runtime,
platformDepMethodsHolder.updaterFunction,
requestAnimationFrame,
Expand Down Expand Up @@ -127,7 +127,7 @@ jsi::Value NativeReanimatedModule::startMapper(
auto mapperShareable = ShareableValue::adapt(rt, worklet, this);
auto inputMutables = extractMutablesFromArray(rt, inputs.asObject(rt).asArray(rt), this);
auto outputMutables = extractMutablesFromArray(rt, outputs.asObject(rt).asArray(rt), this);

int optimalizationLvl = 0;
auto optimalization = updater.asObject(rt).getProperty(rt, "__optimalization");
if(optimalization.isNumber()) {
Expand All @@ -139,7 +139,7 @@ jsi::Value NativeReanimatedModule::startMapper(
scheduler->scheduleOnUI([=] {
auto mapperFunction = mapperShareable->getValue(*runtime).asObject(*runtime).asFunction(*runtime);
std::shared_ptr<jsi::Function> mapperFunctionPointer = std::make_shared<jsi::Function>(std::move(mapperFunction));

std::shared_ptr<Mapper> mapperPointer = std::make_shared<Mapper>(
this,
newMapperId,
Expand Down Expand Up @@ -276,9 +276,4 @@ void NativeReanimatedModule::onRender(double timestampMs)
}
}

NativeReanimatedModule::~NativeReanimatedModule()
{
StoreUser::clearStore();
}

} // namespace reanimated
2 changes: 1 addition & 1 deletion Common/cpp/SharedItems/MutableValue.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ std::vector<jsi::PropNameID> MutableValue::getPropertyNames(jsi::Runtime &rt) {
}

MutableValue::MutableValue(jsi::Runtime &rt, const jsi::Value &initial, RuntimeManager *runtimeManager, std::shared_ptr<Scheduler> s):
StoreUser(s), runtimeManager(runtimeManager), value(ShareableValue::adapt(rt, initial, runtimeManager)) {
StoreUser(s, *runtimeManager), runtimeManager(runtimeManager), value(ShareableValue::adapt(rt, initial, runtimeManager)) {
}

unsigned long int MutableValue::addListener(unsigned long id, std::function<void ()> listener) {
Expand Down
4 changes: 3 additions & 1 deletion Common/cpp/SharedItems/ShareableValue.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,8 @@ std::shared_ptr<ShareableValue> ShareableValue::adapt(jsi::Runtime &rt, const js

jsi::Value ShareableValue::getValue(jsi::Runtime &rt) {
// TODO: maybe we can cache toJSValue results on a per-runtime basis, need to avoid ref loops
if (RuntimeDecorator::isWorkletRuntime(rt)) {
if (&rt == runtimeManager->runtime.get()) {
// Getting value on the same runtime where it was created, prepare remoteValue
if (remoteValue.expired()) {
remoteValue = getWeakRef(rt);
}
Expand All @@ -187,6 +188,7 @@ jsi::Value ShareableValue::getValue(jsi::Runtime &rt) {
}
return jsi::Value(rt, *remoteValue.lock());
} else {
// Getting value on a different runtime than where it was created from, prepare hostValue
if (hostValue.get() == nullptr) {
hostValue = std::make_unique<jsi::Value>(toJSValue(rt));
}
Expand Down
13 changes: 3 additions & 10 deletions Common/cpp/Tools/JSIStoreValueUser.cpp
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
#include "JSIStoreValueUser.h"
#include "RuntimeManager.h"

namespace reanimated {

std::shared_ptr<StaticStoreUser> StoreUser::staticStoreUserData = std::make_shared<StaticStoreUser>();

std::weak_ptr<jsi::Value> StoreUser::getWeakRef(jsi::Runtime &rt) {
const std::lock_guard<std::recursive_mutex> lock(storeUserData->storeMutex);
if (storeUserData->store.count(identifier) == 0) {
Expand All @@ -15,8 +14,8 @@ std::weak_ptr<jsi::Value> StoreUser::getWeakRef(jsi::Runtime &rt) {
return sv;
}

StoreUser::StoreUser(std::shared_ptr<Scheduler> s): scheduler(s) {
storeUserData = StoreUser::staticStoreUserData;
StoreUser::StoreUser(std::shared_ptr<Scheduler> s, RuntimeManager &runtimeManager): scheduler(s) {
storeUserData = runtimeManager.storeUserData;
identifier = storeUserData->ctr++;
}

Expand All @@ -34,10 +33,4 @@ StoreUser::~StoreUser() {
}
}


void StoreUser::clearStore() {
const std::lock_guard<std::recursive_mutex> lock(StoreUser::staticStoreUserData->storeMutex);
StoreUser::staticStoreUserData->store.clear();
}

}
4 changes: 1 addition & 3 deletions Common/cpp/headers/NativeModules/NativeReanimatedModule.h
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ class NativeReanimatedModule : public NativeReanimatedModuleSpec, public Runtime
{
friend ShareableValue;
friend MutableValue;

public:
NativeReanimatedModule(std::shared_ptr<CallInvoker> jsInvoker,
std::shared_ptr<Scheduler> scheduler,
Expand All @@ -35,8 +35,6 @@ class NativeReanimatedModule : public NativeReanimatedModuleSpec, public Runtime
std::shared_ptr<LayoutAnimationsProxy> layoutAnimationsProxy,
PlatformDepMethodsHolder platformDepMethodsHolder);

virtual ~NativeReanimatedModule();

void installCoreFunctions(jsi::Runtime &rt, const jsi::Value &valueSetter) override;

jsi::Value makeShareable(jsi::Runtime &rt, const jsi::Value &value) override;
Expand Down
2 changes: 1 addition & 1 deletion Common/cpp/headers/SharedItems/RemoteObject.h
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ class RemoteObject: public jsi::HostObject, public StoreUser {
public:
void maybeInitializeOnWorkletRuntime(jsi::Runtime &rt);
RemoteObject(jsi::Runtime &rt, jsi::Object &object, RuntimeManager *runtimeManager, std::shared_ptr<Scheduler> s):
StoreUser(s), initializer(std::make_unique<FrozenObject>(rt, object, runtimeManager)) {}
StoreUser(s, *runtimeManager), initializer(std::make_unique<FrozenObject>(rt, object, runtimeManager)) {}
void set(jsi::Runtime &rt, const jsi::PropNameID &name, const jsi::Value &value);
jsi::Value get(jsi::Runtime &rt, const jsi::PropNameID &name);
std::vector<jsi::PropNameID> getPropertyNames(jsi::Runtime &rt);
Expand Down
29 changes: 23 additions & 6 deletions Common/cpp/headers/SharedItems/RuntimeManager.h
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#include "Scheduler.h"
#include "WorkletsCache.h"
#include "RuntimeDecorator.h"
#include "JSIStoreValueUser.h"
#include <jsi/jsi.h>
#include <memory>

Expand All @@ -22,14 +23,20 @@ class RuntimeManager {
std::shared_ptr<ErrorHandler> errorHandler,
std::shared_ptr<Scheduler> scheduler,
RuntimeType runtimeType = RuntimeType::Worklet
) :
runtime(runtime),
errorHandler(errorHandler),
scheduler(scheduler),
workletsCache(std::make_unique<WorkletsCache>())
{
) :
runtime(runtime),
errorHandler(errorHandler),
scheduler(scheduler),
workletsCache(std::make_unique<WorkletsCache>()),
storeUserData(std::make_shared<StaticStoreUser>())
{
RuntimeDecorator::registerRuntime(this->runtime.get(), runtimeType);
}

virtual ~RuntimeManager() {
clearStore();
}

public:
/**
Holds the jsi::Function worklet that is responsible for updating values in JS.
Expand All @@ -52,6 +59,16 @@ class RuntimeManager {
Holds a list of adapted Worklets which are cached to avoid unneccessary recreation.
*/
std::unique_ptr<WorkletsCache> workletsCache;
/**
Holds the JSI-Value Store where JSI::Values are cached on a per-RuntimeManager basis.
*/
std::shared_ptr<StaticStoreUser> storeUserData;

private:
void clearStore() {
const std::lock_guard<std::recursive_mutex> lock(storeUserData->storeMutex);
storeUserData->store.clear();
}
};

}
2 changes: 1 addition & 1 deletion Common/cpp/headers/SharedItems/ShareableValue.h
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ friend void extractMutables(jsi::Runtime &rt,
std::weak_ptr<jsi::Value> remoteValue;
bool containsHostFunction = false;

ShareableValue(RuntimeManager *runtimeManager, std::shared_ptr<Scheduler> s): StoreUser(s), runtimeManager(runtimeManager) {}
ShareableValue(RuntimeManager *runtimeManager, std::shared_ptr<Scheduler> s): StoreUser(s, *runtimeManager), runtimeManager(runtimeManager) {}

jsi::Value toJSValue(jsi::Runtime &rt);
jsi::Object createHost(jsi::Runtime &rt, std::shared_ptr<jsi::HostObject> host);
Expand Down
8 changes: 4 additions & 4 deletions Common/cpp/headers/Tools/JSIStoreValueUser.h
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ using namespace facebook;

namespace reanimated {

class RuntimeManager;

struct StaticStoreUser {
std::atomic<int> ctr;
std::unordered_map<int, std::vector<std::shared_ptr<jsi::Value>>> store;
Expand All @@ -21,17 +23,15 @@ struct StaticStoreUser {

class StoreUser {
int identifier = 0;
static std::shared_ptr<StaticStoreUser> staticStoreUserData;
std::shared_ptr<StaticStoreUser> storeUserData;
std::weak_ptr<Scheduler> scheduler;
std::shared_ptr<StaticStoreUser> storeUserData;

public:
StoreUser(std::shared_ptr<Scheduler> s);
StoreUser(std::shared_ptr<Scheduler> s, RuntimeManager &runtimeManager);

std::weak_ptr<jsi::Value> getWeakRef(jsi::Runtime &rt);
void removeRefs();

static void clearStore();
virtual ~StoreUser();
};

Expand Down
12 changes: 9 additions & 3 deletions Example/ios/Podfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@ PODS:
- React-Core (= 0.64.1)
- React-jsi (= 0.64.1)
- ReactCommon/turbomodule/core (= 0.64.1)
- Folly (2016.09.26.00):
- boost-for-react-native
- DoubleConversion
- glog
- glog (0.3.5)
- RCT-Folly (2020.01.13.00):
- boost-for-react-native
Expand Down Expand Up @@ -287,8 +291,8 @@ PODS:
- DoubleConversion
- FBLazyVector
- FBReactNativeSpec
- Folly
- glog
- RCT-Folly
- RCTRequired
- RCTTypeSafety
- React
Expand Down Expand Up @@ -360,6 +364,7 @@ DEPENDENCIES:
SPEC REPOS:
trunk:
- boost-for-react-native
- Folly

EXTERNAL SOURCES:
DoubleConversion:
Expand Down Expand Up @@ -435,7 +440,8 @@ SPEC CHECKSUMS:
boost-for-react-native: 39c7adb57c4e60d6c5479dd8623128eb5b3f0f2c
DoubleConversion: cf9b38bf0b2d048436d9a82ad2abe1404f11e7de
FBLazyVector: 7b423f9e248eae65987838148c36eec1dbfe0b53
FBReactNativeSpec: 8a41e9f7b718116d3460ddef00151fe020714e83
FBReactNativeSpec: 260beeb2d85223aee47e15223a73048e6115e84d
Folly: 211775e49d8da0ca658aebc8eab89d642935755c
glog: 73c2498ac6884b13ede40eda8228cb1eee9d9d62
RCT-Folly: ec7a233ccc97cc556cf7237f0db1ff65b986f27c
RCTRequired: ec2ebc96b7bfba3ca5c32740f5a0c6a014a274d2
Expand Down Expand Up @@ -463,7 +469,7 @@ SPEC CHECKSUMS:
ReactCommon: bedc99ed4dae329c4fcf128d0c31b9115e5365ca
RNCMaskedView: 5a8ec07677aa885546a0d98da336457e2bea557f
RNGestureHandler: 5e58135436aacc1c5d29b75547d3d2b9430d052c
RNReanimated: daebbd404c0cd9df6daa248d63dd940086bea9ff
RNReanimated: 86cf5f9e41f15386d2f13813d44e8a9a0dfe041f
RNScreens: b6c9607e6fe47c1b6e2f1910d2acd46dd7ecea3a
RNSVG: ce9d996113475209013317e48b05c21ee988d42e
Yoga: a7de31c64fe738607e7a3803e3f591a4b1df7393
Expand Down

0 comments on commit 0f5e7fb

Please sign in to comment.