Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix "Not implemented" exception on JSC #6028

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 8 additions & 6 deletions Common/cpp/Fabric/ShadowTreeCloner.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,10 @@ ShadowNode::Unshared cloneShadowTreeWithNewProps(
const auto props = source->getComponentDescriptor().cloneProps(
propsParserContext, source->getProps(), std::move(rawProps));

auto newChildNode = source->clone({/* .props = */ props, ShadowNodeFragment::childrenPlaceholder(), source->getState()});
auto newChildNode = source->clone(
{/* .props = */ props,
ShadowNodeFragment::childrenPlaceholder(),
source->getState()});

for (auto it = ancestors.rbegin(); it != ancestors.rend(); ++it) {
auto &parentNode = it->first.get();
Expand All @@ -50,11 +53,10 @@ ShadowNode::Unshared cloneShadowTreeWithNewProps(

children[childIndex] = newChildNode;

newChildNode = parentNode.clone({
ShadowNodeFragment::propsPlaceholder(),
std::make_shared<ShadowNode::ListOfShared>(children),
parentNode.getState()
});
newChildNode = parentNode.clone(
{ShadowNodeFragment::propsPlaceholder(),
std::make_shared<ShadowNode::ListOfShared>(children),
parentNode.getState()});
}

return std::const_pointer_cast<ShadowNode>(newChildNode);
Expand Down
70 changes: 56 additions & 14 deletions Common/cpp/SharedItems/Shareables.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@ using namespace facebook;

namespace reanimated {

std::atomic<NativeStateAccess> ShareableObject::nativeStateAccess_ =
NativeStateAccess::Unknown;

jsi::Function getValueUnpacker(jsi::Runtime &rt) {
auto valueUnpacker = rt.global().getProperty(rt, "__valueUnpacker");
assert(valueUnpacker.isObject() && "valueUnpacker not found");
Expand Down Expand Up @@ -99,8 +102,8 @@ jsi::Value makeShareableClone(
} else if (value.isSymbol()) {
// TODO: this is only a placeholder implementation, here we replace symbols
// with strings in order to make certain objects to be captured. There isn't
// yet any usecase for using symbols on the UI runtime so it is fine to keep
// it like this for now.
// yet any use case for using symbols on the UI runtime so it is fine to
// keep it like this for now.
shareable =
std::make_shared<ShareableString>(value.getSymbol(rt).toString(rt));
} else {
Expand Down Expand Up @@ -198,8 +201,11 @@ ShareableObject::ShareableObject(jsi::Runtime &rt, const jsi::Object &object)
auto value = extractShareableOrThrow(rt, object.getProperty(rt, key));
data_.emplace_back(key.utf8(rt), value);
}
if (object.hasNativeState(rt)) {
nativeState_ = object.getNativeState(rt);
if (nativeStateAccess_ == NativeStateAccess::Safe) {
makeNativeStateFromObject(rt, object);
} else if (nativeStateAccess_ == NativeStateAccess::Unknown) {
runWithNativeStateAccessProbe(
[&]() { makeNativeStateFromObject(rt, object); });
}
}

Expand All @@ -208,9 +214,11 @@ ShareableObject::ShareableObject(
const jsi::Object &object,
const jsi::Value &nativeStateSource)
: ShareableObject(rt, object) {
if (nativeStateSource.isObject() &&
nativeStateSource.asObject(rt).hasNativeState(rt)) {
nativeState_ = nativeStateSource.asObject(rt).getNativeState(rt);
if (nativeStateAccess_ == NativeStateAccess::Safe) {
makeNativeStateFromNativeStateSource(rt, nativeStateSource);
} else if (nativeStateAccess_ == NativeStateAccess::Unknown) {
runWithNativeStateAccessProbe(
[&]() { makeNativeStateFromNativeStateSource(rt, nativeStateSource); });
}
}

Expand All @@ -221,11 +229,45 @@ jsi::Value ShareableObject::toJSValue(jsi::Runtime &rt) {
rt, data_[i].first.c_str(), data_[i].second->getJSValue(rt));
}
if (nativeState_ != nullptr) {
obj.setNativeState(rt, nativeState_);
if (nativeStateAccess_ == NativeStateAccess::Safe) {
obj.setNativeState(rt, nativeState_);
} else if (nativeStateAccess_ == NativeStateAccess::Unknown) {
runWithNativeStateAccessProbe(
[&]() { obj.setNativeState(rt, nativeState_); });
}
}
return obj;
}

void ShareableObject::makeNativeStateFromObject(
jsi::Runtime &rt,
const jsi::Object &object) {
if (object.hasNativeState(rt)) {
nativeState_ = object.getNativeState(rt);
nativeStateAccess_ = NativeStateAccess::Safe;
}
}

void ShareableObject::makeNativeStateFromNativeStateSource(
jsi::Runtime &rt,
const jsi::Value &nativeStateSource) {
if (nativeStateSource.isObject() &&
nativeStateSource.asObject(rt).hasNativeState(rt)) {
nativeState_ = nativeStateSource.asObject(rt).getNativeState(rt);
nativeStateAccess_ = NativeStateAccess::Safe;
}
}

void ShareableObject::runWithNativeStateAccessProbe(
std::function<void()> &&block) {
try {
block();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why we need to pass an lambda here instead of just execute obj.setNativeState(rt, nativeState_);?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have more than just setNativeState function handled by it.

nativeStateAccess_ = NativeStateAccess::Safe;
} catch (...) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this 'Not implemented' exception have a specific type that we can implicitly catch here?

Copy link
Collaborator Author

@tjzel tjzel Jun 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to search through all of current implementations of it 🫣

nativeStateAccess_ = NativeStateAccess::Unsafe;
}
}

jsi::Value ShareableHostObject::toJSValue(jsi::Runtime &rt) {
return jsi::Object::createFromHostObject(rt, hostObject_);
}
Expand Down Expand Up @@ -270,12 +312,12 @@ jsi::Value ShareableHandle::toJSValue(jsi::Runtime &rt) {
rt, initObj, jsi::String::createFromAscii(rt, "Handle")));

// We are locking the initialization here since the thread that is
// initalizing can be pre-empted on runtime lock. E.g.
// UI thread can be pre-empted on initialization of a shared value and then
// JS thread can try to access the shared value, locking the whole runtime.
// If we put the lock on `getValueUnpacker` part (basically any part that
// requires runtime) we would get a deadlock since UI thread would never
// release it.
// initializing can be preempted on runtime lock. E.g.
// UI thread can be preempted on initialization of a shared value and
// then JS thread can try to access the shared value, locking the whole
// runtime. If we put the lock on `getValueUnpacker` part (basically any
// part that requires runtime) we would get a deadlock since UI thread
// would never release it.
std::unique_lock<std::mutex> lock(initializationMutex_);
if (remoteValue_ == nullptr) {
remoteValue_ = std::move(value);
Expand Down
18 changes: 18 additions & 0 deletions Common/cpp/SharedItems/Shareables.h
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,12 @@ class ShareableArray : public Shareable {
std::vector<std::shared_ptr<Shareable>> data_;
};

enum class NativeStateAccess {
Unknown,
Safe,
Unsafe,
};

class ShareableObject : public Shareable {
public:
ShareableObject(jsi::Runtime &rt, const jsi::Object &object);
Expand All @@ -191,8 +197,20 @@ class ShareableObject : public Shareable {
jsi::Value toJSValue(jsi::Runtime &rt) override;

protected:
void makeNativeStateFromObject(jsi::Runtime &rt, const jsi::Object &object);
void makeNativeStateFromNativeStateSource(
jsi::Runtime &rt,
const jsi::Value &nativeStateSource);

std::vector<std::pair<std::string, std::shared_ptr<Shareable>>> data_;
std::shared_ptr<jsi::NativeState> nativeState_;

// In some implementations of JSC - namely, macOS
// the methods that refer to the native state of an object
// are not implemented and they throw the error. Therefore
// we need to keep track of the access mode to the native state.
static std::atomic<NativeStateAccess> nativeStateAccess_;
static void runWithNativeStateAccessProbe(std::function<void()> &&block);
};

class ShareableHostObject : public Shareable {
Expand Down
6 changes: 3 additions & 3 deletions MacOSExample/macos/Podfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -1203,11 +1203,11 @@ EXTERNAL SOURCES:

SPEC CHECKSUMS:
boost: 0686b6af8cbd638c784fea5afb789be66699823c
DoubleConversion: acaf5db79676d2e9119015819153f0f99191de12
DoubleConversion: ca54355f8932558971f6643521d62b9bc8231cee
FBLazyVector: 6d4868013ba47ee362b596a3ccc6f256a0412519
FBReactNativeSpec: 030e69b320c718834248085bfd0bd6652141c28c
fmt: 03574da4b7ba40de39da59677ca66610ce8c4a02
glog: 6df0a3d6e2750a50609471fd1a01fd2948d405b5
glog: 3a72874c0322c7caf24931d3a2777cb7a3090529
RCT-Folly: 68e9c0fd4c0f05964afd447041d3ac2d67298f27
RCTRequired: 80f6978c000be199cc243f876974fd848651a247
RCTTypeSafety: cf3c24b03624d8ed3b7a1723f25ac159ff19ad70
Expand Down Expand Up @@ -1255,7 +1255,7 @@ SPEC CHECKSUMS:
RNReanimated: 102299541274020227cc99875a2bc080e23e82ce
RNSVG: ba3e7232f45e34b7b47e74472386cf4e1a676d0a
SocketRocket: f6c6249082c011e6de2de60ed641ef8bbe0cfac9
Yoga: d7c14f5598b68c9415cc426df87de9d5697e40c4
Yoga: 24ddb4963e2a3330df762423f0a9de2096325401

PODFILE CHECKSUM: bc6f61f87c0dc3e6c1b90a39188b38c3dcd174b0

Expand Down
4 changes: 2 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@
"format:plugin": "cd plugin && yarn format && cd ..",
"format:java": "node ./scripts/format-java.js",
"format:ios": "find apple/ -iname \"*.h\" -o -iname \"*.m\" -o -iname \"*.mm\" -o -iname \"*.cpp\" | xargs clang-format -i --Werror",
"format:android": "find android/src/ -iname *.h -o -iname *.cpp | xargs clang-format -i",
"format:common": "find Common/ -iname *.h -o -iname *.cpp | xargs clang-format -i",
"format:android": "find android/src/ -iname \"*.h\" -o -iname \"*.cpp\" | xargs clang-format -i",
"format:common": "find Common/ -iname \"*.h\" -o -iname \"*.cpp\" | xargs clang-format -i",
"format:docs": "cd docs && yarn format && cd ..",
"find-unused-code:js": "yarn ts-prune --ignore \"index|.web.\" --error ",
"type:check:src": "yarn tsc --noEmit",
Expand Down
4 changes: 2 additions & 2 deletions src/createAnimatedComponent/JSPropsUpdater.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@ class JSPropsUpdaterPaper implements IJSPropsUpdater {

constructor() {
this._reanimatedEventEmitter = new NativeEventEmitter(
// NativeEventEmitter only uses this parameter on iOS.
Platform.OS === 'ios'
// NativeEventEmitter only uses this parameter on iOS and macOS.
Platform.OS === 'ios' || Platform.OS === 'macos'
? (NativeReanimatedModule as unknown as NativeModule)
: undefined
);
Expand Down
Loading