Fix NativeAnimation invalidation & races on iOS #10663

ryangomba · 2016-10-31T22:01:49Z

This diff attempts to fix a number of iOS native animation bugs related to improper node invalidation and a race with view creation. The major issues were presented in #9120 as problems 3 and 3b, but I'll recap here:

Incomplete node invalidation

The invalidation model we use is overly complicated and incomplete. The proper combination of _needsUpdate and _hasUpdated will result in nodes values being recomputed. However, we do not invalidate nodes in all the places we should, e.g. if we create a new view and attach it to an existing value node (see example in #9120). This diff chooses to remove the _hasUpdated flag, and simply relies on the _needsUpdate flag to mark a node as dirty.

We mark nodes as dirty when they are:

created
updated
attached to new parents
detached from old parents
attached to a view

Calling updateNodeIfNecessary will, if necessary, compute all invalidated parent values before recomputing the node value. It will then apply the update, and mark the node as clean.

Note: the simplification also allows us to remove the cleanupAnimationUpdate routine.

Race condition on view creation

Sometimes, we would attempt to set native animated props on a non-existent view (see example in #9120). This is because we have a race condition with UIManager: sometimes, we run animated updates before UIManager can create the view. To synchronize RCTUIManager and RCTNativeAnimatedModule, we do the following:

place RCTNativeAnimatedModule calls on the UIManager queue
gather all native animated operations, but don't apply them
wait for a batchDidComplete call to flush the operation queue
let RCTNativeAnimatedNodesManager apply the updates on the main queue

This module + nodeManager structure mimics the one on Android, and I think it works quite well.

facebook-github-bot · 2016-10-31T22:02:12Z

By analyzing the blame information on this pull request, we identified @buba447 and @janicduplessis to be potential reviewers.

ryangomba · 2016-10-31T22:02:22Z

@janicduplessis here it is!

ryangomba · 2016-10-31T22:03:35Z

fwiw, I'm working on a large application that makes extensive use of the NativeAnimated API. This diff fixed a boatload of hard to track down bugs.

janicduplessis · 2016-11-01T09:30:10Z

Awesome! Tested it and it does fix the node update issues that I had + the weird race conditions that caused some transitions to sometimes not happen at all. Going to review this tomorrow.

janicduplessis

Great work! Tested this quite a bit and it did fix the issues you mentioned.

janicduplessis · 2016-11-02T20:26:44Z

Libraries/NativeAnimation/RCTNativeAnimatedNodesManager.h

@@ -0,0 +1,78 @@
+//


FB copyright header

janicduplessis · 2016-11-02T20:36:34Z

Libraries/NativeAnimation/RCTNativeAnimatedNodesManager.m

@@ -0,0 +1,350 @@
+//


FB copyright header

janicduplessis · 2016-11-02T21:23:06Z

@facebook-github-bot shipit

facebook-github-bot · 2016-11-03T11:48:11Z

I tried to merge this pull request into the Facebook internal repo but some checks failed. To unblock yourself please check the following: Does this pull request pass all open source tests on GitHub? If not please fix those. Does the code still apply cleanly on top of GitHub master? If not can please rebase. In all other cases this means some internal test failed, for example a part of a fb app won't work with this pull request. I've added the Import Failed label to this pull request so it is easy for someone at fb to find the pull request and check what failed. If you don't see anyone comment in a few days feel free to comment mentioning one of the core contributors to the project so they get a notification.

ryangomba · 2016-11-11T20:38:22Z

Should we try this again? Any context on the failed import @javache ?

hramos · 2016-11-11T20:40:09Z

Looking.

nihgwu · 2016-11-15T04:11:28Z

what's going on this?

janicduplessis · 2016-11-15T15:51:21Z

Ping @hramos

janicduplessis · 2016-11-17T03:15:01Z

@facebook-github-bot shipit

facebook-github-bot · 2016-11-17T03:48:07Z

I tried to merge this pull request into the Facebook internal repo but some checks failed. To unblock yourself please check the following: Does this pull request pass all open source tests on GitHub? If not please fix those. Does the code still apply cleanly on top of GitHub master? If not can please rebase. In all other cases this means some internal test failed, for example a part of a fb app won't work with this pull request. I've added the Import Failed label to this pull request so it is easy for someone at fb to find the pull request and check what failed. If you don't see anyone comment in a few days feel free to comment mentioning one of the core contributors to the project so they get a notification.

nihgwu · 2016-11-21T15:34:33Z

why the import failed? is anyone looking into it? I've test this PR with my app, it works so great

mkonicek · 2016-11-22T19:22:28Z

Looks like the import failed with an unrelated error. Let's try again.

@facebook-github-bot shipit

facebook-github-bot · 2016-11-22T21:48:26Z

I tried to merge this pull request into the Facebook internal repo but some checks failed. To unblock yourself please check the following: Does this pull request pass all open source tests on GitHub? If not please fix those. Does the code still apply cleanly on top of GitHub master? If not can please rebase. In all other cases this means some internal test failed, for example a part of a fb app won't work with this pull request. I've added the Import Failed label to this pull request so it is easy for someone at fb to find the pull request and check what failed. If you don't see anyone comment in a few days feel free to comment mentioning one of the core contributors to the project so they get a notification.

janicduplessis · 2016-11-22T21:51:07Z

@mkonicek Talked with @hramos and this causes some buck errors internally, someone from fb will have to look at it.

mkonicek · 2016-11-25T14:12:56Z

@janicduplessis @ryangomba I'm not sure it's Buck-related. Here are the errors that blocked this from landing internally. I know little about Obj-C, do they make sense to you?

Libraries/NativeAnimation/RCTNativeAnimatedModule.m:181:17: error: block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior [-Werror,-Wimplicit-retain-self]
operation(_nodesManager);
            ^
Libraries/NativeAnimation/RCTNativeAnimatedModule.m:183:6: error: block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior [-Werror,-Wimplicit-retain-self]
[_nodesManager updateAnimations];

Looks like this PR needs to be rebased too, sorry about that!

janicduplessis · 2016-11-25T18:16:31Z

@mkonicek Looks like we're compiling with different warning flags internally and in OSS. Are you using buck to build objc internally? If so I guess it has its own config that probably add different compiler warning flags as the xcbuild one. You might want to ping some ios person about it as it is easy to fix and would prevent import failures like this.

mkonicek · 2016-11-28T11:10:29Z

Thanks @janicduplessis! Yes we use Buck. Pinged @mmmulani.

mkonicek · 2016-11-28T11:12:59Z

Is there any way you can make the code compile independently of compiler flags to unblock yourself before @mmmulani has time to look into this?

ryangomba · 2016-11-28T17:05:08Z

I can follow up on this now that I understand the import error

Summary: This diff attempts to fix a number of iOS native animation bugs related to improper node invalidation and a race with view creation. The major issues were presented in facebook#9120 as problems 3 and 3b, but I'll recap here: The invalidation model we use is overly complicated and incomplete. The proper combination of `_needsUpdate` and `_hasUpdated` will result in nodes values being recomputed. However, we do not invalidate nodes in all the places we should, e.g. if we create a new view and attach it to an existing value node (see example in facebook#9120). This diff chooses to remove the `_hasUpdated` flag, and simply relies on the `_needsUpdate` flag to mark a node as dirty. We mark nodes as dirty when they are: - created - updated - attached to new parents - detached from old parents - attached to a view Calling `updateNodeIfNecessary` will, if necessary, compute all invalidated parent values before recomputing the node value. It will then apply the update, and mark the no Closes facebook#10663 Differential Revision: D4120301 Pulled By: mkonicek fbshipit-source-id: e247afcb5d8c15999b8328c664b9f7e764d76a75

Fix iOS native animation (non-nil/non-zero) assertion failure (on initial parentNode then childNode), after nav to bot 4th webview tab (unable to load pages, likely caused by current excluded RN polyfillGlobal) - Df (Foundation) *** Assertion failure in - disconnectAnimatedNodes:childTag - E [com.facebook.react.log:native] Exception thrown while executing UI block: 'parentNode' is a required parameter - Df (Foundation) *** Assertion failure in - connectAnimatedNodes:childTag - E [com.facebook.react.log:native] Exception thrown while executing UI block: 'childNode' is a required parameter Exclude iOS native animation (non-nil/non-zero) assertion macros - for now, when nodes (pointers to tags) attach/detach (to old/new parents and new views) QA: no empty animation frames observed ✅ Low probability possible risks introduced - incomplete node invalidation (outdated nodes) - race condition: prop updated before UIManager created view (outdated props) QA: no outdated animation frames observed ✅ iOS native animation assertion refs - Summary: facebook/react-native@c858420 - PR: facebook/react-native#10663 - Examples: facebook/react-native#9120 - nb: mimics ReactAndroid (i.e. NativeAnimatedNodesManager.java) nb: metro-react-native-babel-preset (0.72.3) - @babel/plugin-transform-regenerator has been removed since initial investigation - @babel/plugin-transform-runtime (removed) - 4 fewer SES warnings - @babel/plugin-transform-runtime > regenerator: false - immediate error thrown (recurring) Todo: Fix webview page loading (likely caused by current excluded RN promisePolyfill), thus fixing these assertion failures on nav, then revert this patch - Problem: Including promisePolyfill (default RN) causes app to boot empty root view Todo: Root cause of above 'regenerator: false' causing nil/zero parent/child nodes

Fix iOS native animation (non-nil/non-zero) assertion failure (on initial parentNode then childNode), after nav to bot 4th WebView tab (unable to load pages, likely caused by current excluded RN Promise polyfillGlobal) - Df (Foundation) *** Assertion failure in - disconnectAnimatedNodes:childTag - E [com.facebook.react.log:native] Exception thrown while executing UI block: 'parentNode' is a required parameter - Df (Foundation) *** Assertion failure in - connectAnimatedNodes:childTag - E [com.facebook.react.log:native] Exception thrown while executing UI block: 'childNode' is a required parameter Exclude iOS native animation (non-nil/non-zero) assertion macros - for now, when nodes (pointers to tags) attach/detach (to old/new parents and new views) QA: no empty animation frames observed ✅ Low probability possible risks introduced - incomplete node invalidation (outdated nodes) - race condition: prop updated before UIManager created view (outdated props) QA: no outdated animation frames observed ✅ iOS native animation assertion refs - Summary: facebook/react-native@c858420 - PR: facebook/react-native#10663 - Examples: facebook/react-native#9120 - nb: mimics ReactAndroid (i.e. NativeAnimatedNodesManager.java) nb: metro-react-native-babel-preset (0.72.3) - @babel/plugin-transform-regenerator has been removed since initial investigation - @babel/plugin-transform-runtime (removed) - 4 fewer SES warnings - intrinsics: Object.setPrototypeOf.default, Object.setPrototypeOf.__esModule, Reflect.construct.default, Reflect.construct.__esModule - @babel/plugin-transform-runtime > regenerator: false - immediate error thrown (recurring) Todo: Fix WebView page load (likely caused by current excluded RN Promise polyfillGlobal), thus fixing these assertion failures on nav, then revert this patch - Problem: Including (default) RN Promise polyfillGlobal causing app to boot empty root view Todo: Root cause of above 'regenerator: false' causing nil/zero parent/child nodes immediately to reoccur

facebook-github-bot added the GH Review: review-needed label Oct 31, 2016

ryangomba force-pushed the native-animation-invalidate branch from 216ec54 to a40dca3 Compare October 31, 2016 22:06

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 31, 2016

ryangomba force-pushed the native-animation-invalidate branch 2 times, most recently from 1dc9155 to b634c97 Compare October 31, 2016 22:26

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 31, 2016

janicduplessis added Platform: iOS iOS applications. Animated labels Nov 1, 2016

janicduplessis self-assigned this Nov 1, 2016

ryangomba force-pushed the native-animation-invalidate branch 2 times, most recently from b89a9c5 to 806bbf0 Compare November 1, 2016 19:23

janicduplessis suggested changes Nov 2, 2016

View reviewed changes

janicduplessis added GH Review: needs-revision and removed GH Review: review-needed labels Nov 2, 2016

ryangomba force-pushed the native-animation-invalidate branch from 806bbf0 to 78afc74 Compare November 2, 2016 21:16

facebook-github-bot added GH Review: review-needed and removed GH Review: needs-revision labels Nov 2, 2016

janicduplessis approved these changes Nov 2, 2016

View reviewed changes

facebook-github-bot added GH Review: accepted Import Started This pull request has been imported. This does not imply the PR has been approved. and removed GH Review: review-needed labels Nov 2, 2016

ryangomba mentioned this pull request Nov 2, 2016

Optionally create NativeAnimated value with native flag #10657

Closed

facebook-github-bot added Import Failed and removed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Nov 3, 2016

facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Nov 17, 2016

facebook-github-bot removed the Import Started This pull request has been imported. This does not imply the PR has been approved. label Nov 17, 2016

facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Nov 22, 2016

facebook-github-bot removed the Import Started This pull request has been imported. This does not imply the PR has been approved. label Nov 22, 2016

facebook-github-bot added the Failed Commands label Nov 25, 2016

ryangomba force-pushed the native-animation-invalidate branch from 6954ca4 to 0e5ab15 Compare November 28, 2016 17:32

Fix NativeAnimation invalidation & races on iOS

ad5d967

ryangomba force-pushed the native-animation-invalidate branch from 0e5ab15 to ad5d967 Compare November 28, 2016 18:44

facebook-github-bot closed this in c858420 Nov 28, 2016

kocburak mentioned this pull request Apr 13, 2025

Android - Animated.event useNativeDriver:true sets incorrect translate position when scrolling interrupted by user #50496

Open

Fix NativeAnimation invalidation & races on iOS #10663

Fix NativeAnimation invalidation & races on iOS #10663

Uh oh!

Conversation

ryangomba commented Oct 31, 2016

Incomplete node invalidation

Race condition on view creation

Uh oh!

facebook-github-bot commented Oct 31, 2016

Uh oh!

ryangomba commented Oct 31, 2016

Uh oh!

ryangomba commented Oct 31, 2016

Uh oh!

janicduplessis commented Nov 1, 2016

Uh oh!

janicduplessis left a comment

Choose a reason for hiding this comment

Uh oh!

janicduplessis Nov 2, 2016

Choose a reason for hiding this comment

Uh oh!

janicduplessis Nov 2, 2016

Choose a reason for hiding this comment

Uh oh!

janicduplessis commented Nov 2, 2016

Uh oh!

facebook-github-bot commented Nov 3, 2016

Uh oh!

ryangomba commented Nov 11, 2016

Uh oh!

hramos commented Nov 11, 2016

Uh oh!

nihgwu commented Nov 15, 2016

Uh oh!

janicduplessis commented Nov 15, 2016

Uh oh!

janicduplessis commented Nov 17, 2016

Uh oh!

facebook-github-bot commented Nov 17, 2016

Uh oh!

nihgwu commented Nov 21, 2016

Uh oh!

mkonicek commented Nov 22, 2016

Uh oh!

facebook-github-bot commented Nov 22, 2016

Uh oh!

janicduplessis commented Nov 22, 2016

Uh oh!

mkonicek commented Nov 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

janicduplessis commented Nov 25, 2016

Uh oh!

mkonicek commented Nov 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mkonicek commented Nov 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryangomba commented Nov 28, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mkonicek commented Nov 25, 2016 •

edited

Loading

mkonicek commented Nov 28, 2016 •

edited

Loading

mkonicek commented Nov 28, 2016 •

edited

Loading