Skip to content

Commit

Permalink
Use atomic pointer for animationDelegate_ to prevent race during tear…
Browse files Browse the repository at this point in the history
…down

Summary:
Changelog: [internal]

[This assumption](https://fburl.com/diffusion/yeqtvxru) in `~Scheduler` might not be completely correct. There can be a race between JS thread and main thread. With main thread settings animationDelegate_ to nullptr and JS thread reading the value. Even though they always happen one after the other, JS thread can have the value of animationDelegate_ in its cache line and changing it from main thread might not invalidate it right away.

Reviewed By: JoshuaGross

Differential Revision: D29237290

fbshipit-source-id: 6cb898caf7c225cf8162e7560921b563dec514b1
  • Loading branch information
sammy-SC authored and facebook-github-bot committed Jun 18, 2021
1 parent d1e7551 commit 52a9fed
Show file tree
Hide file tree
Showing 2 changed files with 9 additions and 7 deletions.
14 changes: 8 additions & 6 deletions ReactCommon/react/renderer/uimanager/UIManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -345,8 +345,9 @@ void UIManager::configureNextLayoutAnimation(
RawValue const &config,
jsi::Value const &successCallback,
jsi::Value const &failureCallback) const {
if (animationDelegate_) {
animationDelegate_->uiManagerDidConfigureNextLayoutAnimation(
auto animationDelegate = animationDelegate_.load();
if (animationDelegate) {
animationDelegate->uiManagerDidConfigureNextLayoutAnimation(
runtime,
config,
std::move(successCallback),
Expand Down Expand Up @@ -435,14 +436,15 @@ void UIManager::setAnimationDelegate(UIManagerAnimationDelegate *delegate) {
}

void UIManager::stopSurfaceForAnimationDelegate(SurfaceId surfaceId) const {
if (animationDelegate_ != nullptr) {
animationDelegate_->stopSurface(surfaceId);
auto animationDelegate = animationDelegate_.load();
if (animationDelegate) {
animationDelegate->stopSurface(surfaceId);
}
}

void UIManager::animationTick() {
if (animationDelegate_ != nullptr &&
animationDelegate_->shouldAnimateFrame()) {
auto animationDelegate = animationDelegate_.load();
if (animationDelegate && animationDelegate->shouldAnimateFrame()) {
shadowTreeRegistry_.enumerate(
[&](ShadowTree const &shadowTree, bool &stop) {
shadowTree.notifyDelegatesOfUpdates();
Expand Down
2 changes: 1 addition & 1 deletion ReactCommon/react/renderer/uimanager/UIManager.h
Original file line number Diff line number Diff line change
Expand Up @@ -184,8 +184,8 @@ class UIManager final : public ShadowTreeDelegate {
ShadowTreeRegistry const &getShadowTreeRegistry() const;

SharedComponentDescriptorRegistry componentDescriptorRegistry_;
UIManagerAnimationDelegate *animationDelegate_{nullptr};
std::atomic<UIManagerDelegate *> delegate_;
std::atomic<UIManagerAnimationDelegate *> animationDelegate_{nullptr};
RuntimeExecutor const runtimeExecutor_{};
ShadowTreeRegistry shadowTreeRegistry_{};
BackgroundExecutor const backgroundExecutor_{};
Expand Down

0 comments on commit 52a9fed

Please sign in to comment.