Permalink
Browse files

Fix NativeAnimation invalidation & races on iOS

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 #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 #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 #10663

Differential Revision: D4120301

Pulled By: mkonicek

fbshipit-source-id: e247afcb5d8c15999b8328c664b9f7e764d76a75
  • Loading branch information...
1 parent bf901d9 commit c858420b2d4eb989f2411a7dc6a884a660d15f85 @ryangomba ryangomba committed with Facebook Github Bot Nov 28, 2016
@@ -7,6 +7,7 @@
* of patent rights can be found in the PATENTS file in the same directory.
*/
+#import <Foundation/Foundation.h>
#import <CoreGraphics/CoreGraphics.h>
#import <React/RCTBridgeModule.h>
@@ -31,8 +32,7 @@ NS_ASSUME_NONNULL_BEGIN
- (void)stopAnimation;
- (void)stepAnimation;
- (void)removeAnimation;
-- (void)cleanupAnimationUpdate;
-
-@end
NS_ASSUME_NONNULL_END
+
+@end
@@ -150,9 +150,4 @@ - (void)updateOutputWithFrameOutput:(CGFloat)frameOutput
[_valueNode setNeedsUpdate];
}
-- (void)cleanupAnimationUpdate
-{
- [_valueNode cleanupAnimationUpdate];
-}
-
@end
@@ -193,9 +193,4 @@ - (void)onUpdate:(CGFloat)outputValue
[_valueNode setNeedsUpdate];
}
-- (void)cleanupAnimationUpdate
-{
- [_valueNode cleanupAnimationUpdate];
-}
-
@end
@@ -21,7 +21,6 @@
@property (nonatomic, copy, readonly) NSDictionary<NSNumber *, RCTAnimatedNode *> *parentNodes;
@property (nonatomic, readonly) BOOL needsUpdate;
-@property (nonatomic, readonly) BOOL hasUpdated;
/**
* Marks a node and its children as needing update.
@@ -38,11 +37,6 @@
*/
- (void)performUpdate NS_REQUIRES_SUPER;
-/**
- * Cleans up after a round of updates.
- */
-- (void)cleanupAnimationUpdate NS_REQUIRES_SUPER;
-
- (void)addChild:(RCTAnimatedNode *)child NS_REQUIRES_SUPER;
- (void)removeChild:(RCTAnimatedNode *)child NS_REQUIRES_SUPER;
@@ -93,30 +93,15 @@ - (void)detachNode
- (void)setNeedsUpdate
{
- if (_needsUpdate) {
- // Has already been marked. Stop branch.
- return;
- }
_needsUpdate = YES;
for (RCTAnimatedNode *child in _childNodes.allValues) {
[child setNeedsUpdate];
}
}
-- (void)cleanupAnimationUpdate
-{
- if (_hasUpdated) {
- _needsUpdate = NO;
- _hasUpdated = NO;
- for (RCTAnimatedNode *child in _childNodes.allValues) {
- [child cleanupAnimationUpdate];
- }
- }
-}
-
- (void)updateNodeIfNecessary
{
- if (_needsUpdate && !_hasUpdated) {
+ if (_needsUpdate) {
for (RCTAnimatedNode *parent in _parentNodes.allValues) {
[parent updateNodeIfNecessary];
}
@@ -126,7 +111,7 @@ - (void)updateNodeIfNecessary
- (void)performUpdate
{
- _hasUpdated = YES;
+ _needsUpdate = NO;
// To be overidden by subclasses
// This method is called on a node only if it has been marked for update
// during the current update loop
@@ -9,14 +9,14 @@
#import "RCTAnimatedNode.h"
-@class RCTNativeAnimatedModule;
+@class RCTUIManager;
@class RCTViewPropertyMapper;
@interface RCTPropsAnimatedNode : RCTAnimatedNode
@property (nonatomic, readonly) RCTViewPropertyMapper *propertyMapper;
-- (void)connectToView:(NSNumber *)viewTag animatedModule:(RCTNativeAnimatedModule *)animationModule;
+- (void)connectToView:(NSNumber *)viewTag uiManager:(RCTUIManager *)uiManager;
- (void)disconnectFromView:(NSNumber *)viewTag;
- (void)performViewUpdatesIfNecessary;
@@ -8,35 +8,17 @@
*/
#import "RCTPropsAnimatedNode.h"
+
#import "RCTAnimationUtils.h"
-#import "RCTNativeAnimatedModule.h"
#import "RCTStyleAnimatedNode.h"
+#import "RCTValueAnimatedNode.h"
#import "RCTViewPropertyMapper.h"
@implementation RCTPropsAnimatedNode
-{
- RCTStyleAnimatedNode *_parentNode;
-}
-
-- (void)onAttachedToNode:(RCTAnimatedNode *)parent
-{
- [super onAttachedToNode:parent];
- if ([parent isKindOfClass:[RCTStyleAnimatedNode class]]) {
- _parentNode = (RCTStyleAnimatedNode *)parent;
- }
-}
-
-- (void)onDetachedFromNode:(RCTAnimatedNode *)parent
-{
- [super onDetachedFromNode:parent];
- if (_parentNode == parent) {
- _parentNode = nil;
- }
-}
-- (void)connectToView:(NSNumber *)viewTag animatedModule:(RCTNativeAnimatedModule *)animationModule
+- (void)connectToView:(NSNumber *)viewTag uiManager:(RCTUIManager *)uiManager
{
- _propertyMapper = [[RCTViewPropertyMapper alloc] initWithViewTag:viewTag animationModule:animationModule];
+ _propertyMapper = [[RCTViewPropertyMapper alloc] initWithViewTag:viewTag uiManager:uiManager];
}
- (void)disconnectFromView:(NSNumber *)viewTag
@@ -50,11 +32,36 @@ - (void)performUpdate
[self performViewUpdatesIfNecessary];
}
+- (NSString *)propertyNameForParentTag:(NSNumber *)parentTag
+{
+ __block NSString *propertyName;
+ [self.config[@"props"] enumerateKeysAndObjectsUsingBlock:^(NSString * _Nonnull property, NSNumber * _Nonnull tag, BOOL * _Nonnull stop) {
+ if ([tag isEqualToNumber:parentTag]) {
+ propertyName = property;
+ *stop = YES;
+ }
+ }];
+ return propertyName;
+}
+
- (void)performViewUpdatesIfNecessary
{
- NSDictionary *updates = [_parentNode updatedPropsDictionary];
- if (updates.count) {
- [_propertyMapper updateViewWithDictionary:updates];
+ NSMutableDictionary *props = [NSMutableDictionary dictionary];
+ [self.parentNodes enumerateKeysAndObjectsUsingBlock:^(NSNumber * _Nonnull parentTag, RCTAnimatedNode * _Nonnull parentNode, BOOL * _Nonnull stop) {
+
+ if ([parentNode isKindOfClass:[RCTStyleAnimatedNode class]]) {
+ [props addEntriesFromDictionary:[(RCTStyleAnimatedNode *)parentNode propsDictionary]];
+
+ } else if ([parentNode isKindOfClass:[RCTValueAnimatedNode class]]) {
+ NSString *property = [self propertyNameForParentTag:parentTag];
+ CGFloat value = [(RCTValueAnimatedNode *)parentNode value];
+ [props setObject:@(value) forKey:property];
+ }
+
+ }];
+
+ if (props.count) {
+ [_propertyMapper updateViewWithDictionary:props];
}
}
@@ -11,6 +11,6 @@
@interface RCTStyleAnimatedNode : RCTAnimatedNode
-- (NSDictionary<NSString *, NSObject *> *)updatedPropsDictionary;
+- (NSDictionary<NSString *, NSObject *> *)propsDictionary;
@end
@@ -14,21 +14,21 @@
@implementation RCTStyleAnimatedNode
{
- NSMutableDictionary<NSString *, NSObject *> *_updatedPropsDictionary;
+ NSMutableDictionary<NSString *, NSObject *> *_propsDictionary;
}
- (instancetype)initWithTag:(NSNumber *)tag
config:(NSDictionary<NSString *, id> *)config;
{
if ((self = [super initWithTag:tag config:config])) {
- _updatedPropsDictionary = [NSMutableDictionary new];
+ _propsDictionary = [NSMutableDictionary new];
}
return self;
}
-- (NSDictionary *)updatedPropsDictionary
+- (NSDictionary *)propsDictionary
{
- return _updatedPropsDictionary;
+ return _propsDictionary;
}
- (void)performUpdate
@@ -38,22 +38,16 @@ - (void)performUpdate
NSDictionary<NSString *, NSNumber *> *style = self.config[@"style"];
[style enumerateKeysAndObjectsUsingBlock:^(NSString *property, NSNumber *nodeTag, __unused BOOL *stop) {
RCTAnimatedNode *node = self.parentNodes[nodeTag];
- if (node && node.hasUpdated) {
+ if (node) {
if ([node isKindOfClass:[RCTValueAnimatedNode class]]) {
RCTValueAnimatedNode *parentNode = (RCTValueAnimatedNode *)node;
- [self->_updatedPropsDictionary setObject:@(parentNode.value) forKey:property];
+ [self->_propsDictionary setObject:@(parentNode.value) forKey:property];
} else if ([node isKindOfClass:[RCTTransformAnimatedNode class]]) {
RCTTransformAnimatedNode *parentNode = (RCTTransformAnimatedNode *)node;
- [self->_updatedPropsDictionary addEntriesFromDictionary:parentNode.updatedPropsDictionary];
+ [self->_propsDictionary addEntriesFromDictionary:parentNode.propsDictionary];
}
}
}];
}
-- (void)cleanupAnimationUpdate
-{
- [super cleanupAnimationUpdate];
- [_updatedPropsDictionary removeAllObjects];
-}
-
@end
@@ -11,6 +11,6 @@
@interface RCTTransformAnimatedNode : RCTAnimatedNode
-- (NSDictionary<NSString *, NSObject *> *)updatedPropsDictionary;
+- (NSDictionary<NSString *, NSObject *> *)propsDictionary;
@end
@@ -12,21 +12,21 @@
@implementation RCTTransformAnimatedNode
{
- NSMutableDictionary<NSString *, NSObject *> *_updatedPropsDictionary;
+ NSMutableDictionary<NSString *, NSObject *> *_propsDictionary;
}
- (instancetype)initWithTag:(NSNumber *)tag
config:(NSDictionary<NSString *, id> *)config;
{
if ((self = [super initWithTag:tag config:config])) {
- _updatedPropsDictionary = [NSMutableDictionary new];
+ _propsDictionary = [NSMutableDictionary new];
}
return self;
}
-- (NSDictionary *)updatedPropsDictionary
+- (NSDictionary *)propsDictionary
{
- return _updatedPropsDictionary;
+ return _propsDictionary;
}
- (void)performUpdate
@@ -44,7 +44,7 @@ - (void)performUpdate
if ([type isEqualToString: @"animated"]) {
NSNumber *nodeTag = transformConfig[@"nodeTag"];
RCTAnimatedNode *node = self.parentNodes[nodeTag];
- if (!node.hasUpdated || ![node isKindOfClass:[RCTValueAnimatedNode class]]) {
+ if (![node isKindOfClass:[RCTValueAnimatedNode class]]) {
continue;
}
RCTValueAnimatedNode *parentNode = (RCTValueAnimatedNode *)node;
@@ -82,13 +82,7 @@ - (void)performUpdate
}
}
- _updatedPropsDictionary[@"transform"] = [NSValue valueWithCATransform3D:transform];
-}
-
-- (void)cleanupAnimationUpdate
-{
- [super cleanupAnimationUpdate];
- [_updatedPropsDictionary removeAllObjects];
+ _propsDictionary[@"transform"] = [NSValue valueWithCATransform3D:transform];
}
@end
@@ -38,8 +38,10 @@
5C9894951D999639008027DB /* RCTDivisionAnimatedNode.m in Sources */ = {isa = PBXBuildFile; fileRef = 5C9894941D999639008027DB /* RCTDivisionAnimatedNode.m */; };
944244D01DB962DA0032A02B /* RCTFrameAnimation.m in Sources */ = {isa = PBXBuildFile; fileRef = 94C1294D1D4069170025F25C /* RCTFrameAnimation.m */; };
944244D11DB962DC0032A02B /* RCTSpringAnimation.m in Sources */ = {isa = PBXBuildFile; fileRef = 94C1294F1D4069170025F25C /* RCTSpringAnimation.m */; };
+ 9476E8EC1DC9232D005D5CD1 /* RCTNativeAnimatedNodesManager.m in Sources */ = {isa = PBXBuildFile; fileRef = 94DA09171DC7971C00AEA8C9 /* RCTNativeAnimatedNodesManager.m */; };
94C129511D40692B0025F25C /* RCTFrameAnimation.m in Sources */ = {isa = PBXBuildFile; fileRef = 94C1294D1D4069170025F25C /* RCTFrameAnimation.m */; };
94C129521D40692B0025F25C /* RCTSpringAnimation.m in Sources */ = {isa = PBXBuildFile; fileRef = 94C1294F1D4069170025F25C /* RCTSpringAnimation.m */; };
+ 94DA09181DC7971C00AEA8C9 /* RCTNativeAnimatedNodesManager.m in Sources */ = {isa = PBXBuildFile; fileRef = 94DA09171DC7971C00AEA8C9 /* RCTNativeAnimatedNodesManager.m */; };
94DAE3F91D7334A70059942F /* RCTModuloAnimatedNode.m in Sources */ = {isa = PBXBuildFile; fileRef = 94DAE3F81D7334A70059942F /* RCTModuloAnimatedNode.m */; };
/* End PBXBuildFile section */
@@ -79,6 +81,8 @@
94C1294D1D4069170025F25C /* RCTFrameAnimation.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = RCTFrameAnimation.m; sourceTree = "<group>"; };
94C1294E1D4069170025F25C /* RCTSpringAnimation.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; lineEnding = 0; path = RCTSpringAnimation.h; sourceTree = "<group>"; xcLanguageSpecificationIdentifier = xcode.lang.objcpp; };
94C1294F1D4069170025F25C /* RCTSpringAnimation.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = RCTSpringAnimation.m; sourceTree = "<group>"; };
+ 94DA09161DC7971C00AEA8C9 /* RCTNativeAnimatedNodesManager.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = RCTNativeAnimatedNodesManager.h; sourceTree = "<group>"; };
+ 94DA09171DC7971C00AEA8C9 /* RCTNativeAnimatedNodesManager.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = RCTNativeAnimatedNodesManager.m; sourceTree = "<group>"; };
94DAE3F71D7334A70059942F /* RCTModuloAnimatedNode.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; lineEnding = 0; path = RCTModuloAnimatedNode.h; sourceTree = "<group>"; xcLanguageSpecificationIdentifier = xcode.lang.objcpp; };
94DAE3F81D7334A70059942F /* RCTModuloAnimatedNode.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = RCTModuloAnimatedNode.m; sourceTree = "<group>"; };
/* End PBXFileReference section */
@@ -130,6 +134,8 @@
13E501C81D07A644005F35D8 /* RCTViewPropertyMapper.m */,
13E501BD1D07A644005F35D8 /* RCTNativeAnimatedModule.h */,
13E501BE1D07A644005F35D8 /* RCTNativeAnimatedModule.m */,
+ 94DA09161DC7971C00AEA8C9 /* RCTNativeAnimatedNodesManager.h */,
+ 94DA09171DC7971C00AEA8C9 /* RCTNativeAnimatedNodesManager.m */,
94C129491D4069170025F25C /* Drivers */,
13E501D51D07A6C9005F35D8 /* Nodes */,
134814211AA4EA7D00B7C361 /* Products */,
@@ -242,6 +248,7 @@
2D3B5EF31D9B0B3400451313 /* RCTViewPropertyMapper.m in Sources */,
944244D01DB962DA0032A02B /* RCTFrameAnimation.m in Sources */,
944244D11DB962DC0032A02B /* RCTSpringAnimation.m in Sources */,
+ 9476E8EC1DC9232D005D5CD1 /* RCTNativeAnimatedNodesManager.m in Sources */,
);
runOnlyForDeploymentPostprocessing = 0;
};
@@ -251,6 +258,7 @@
files = (
94C129511D40692B0025F25C /* RCTFrameAnimation.m in Sources */,
94C129521D40692B0025F25C /* RCTSpringAnimation.m in Sources */,
+ 94DA09181DC7971C00AEA8C9 /* RCTNativeAnimatedNodesManager.m in Sources */,
13E501F01D07A6C9005F35D8 /* RCTValueAnimatedNode.m in Sources */,
94DAE3F91D7334A70059942F /* RCTModuloAnimatedNode.m in Sources */,
193F64F41D776EC6004D1CAA /* RCTDiffClampAnimatedNode.m in Sources */,
@@ -6,6 +6,7 @@
* LICENSE file in the root directory of this source tree. An additional grant
* of patent rights can be found in the PATENTS file in the same directory.
*/
+
#import <React/RCTBridgeModule.h>
#import <React/RCTEventDispatcher.h>
#import <React/RCTEventEmitter.h>
Oops, something went wrong.

0 comments on commit c858420

Please sign in to comment.