Native Animated - Restore default values when removing props on iOS #11819

janicduplessis · 2017-01-10T23:31:14Z

This fixes a bug that causes properties to keep stale values because they were not restored to their default after being removed when their value was controlled by native animated.

To fix this we restore default values in disconnectFromView by updating views with null values for all props that we modified previously. However this causes another issue where we lose any props that were set by the normal process because NativeAnimated operations are always executed after UIManager operatations. To fix this I added a way to hook into UIManager view updating process to be able to execute NativeAnimated operations either before or after updating native views.

In the case of disconnecting we want to do it before updating views so that it does: Value changed by native animated -> value restored to default -> (optional) value updated by normal prop.

This PR also depends on #10658.

Test plan
Tested that this fixed a particular bug in an app that uses ex-navigation + native animations where a navbar would not get it's opacity restored to 1 when an opacity prop driven by native animated was removed. Also tested that other animations still worked properly as well as the examples in UIExplorer.

facebook-github-bot · 2017-01-10T23:31:24Z

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

janicduplessis · 2017-01-27T21:04:55Z

#10658 landed so this is ready for review, cc @ryangomba @ide @javache

ide · 2017-02-10T09:26:22Z

Libraries/NativeAnimation/Nodes/RCTPropsAnimatedNode.m

@@ -33,6 +45,17 @@ - (void)connectToView:(NSNumber *)viewTag

 - (void)disconnectFromView:(NSNumber *)viewTag
 {
+  // Restore the default value for all props that were modified by this node.
+  for (NSString *key in [_propsDictionary allKeys]) {
+    [_propsDictionary setObject:[NSNull null] forKey:key];


Use modern syntax: _propsDictionary[key] = [NSNull null]

ide · 2017-02-10T09:27:14Z

Libraries/NativeAnimation/Nodes/RCTPropsAnimatedNode.m

@@ -33,6 +45,17 @@ - (void)connectToView:(NSNumber *)viewTag

 - (void)disconnectFromView:(NSNumber *)viewTag
 {
+  // Restore the default value for all props that were modified by this node.
+  for (NSString *key in [_propsDictionary allKeys]) {


allKeys is documented as a property, write _propsDictionary.allKeys: https://developer.apple.com/reference/foundation/nsdictionary/1409150-allkeys?language=objc

ide · 2017-02-10T09:37:30Z

Libraries/NativeAnimation/Nodes/RCTPropsAnimatedNode.m


    } else if ([parentNode isKindOfClass:[RCTValueAnimatedNode class]]) {
      NSString *property = [self propertyNameForParentTag:parentTag];
      CGFloat value = [(RCTValueAnimatedNode *)parentNode value];
-      [props setObject:@(value) forKey:property];
+      [_propsDictionary setObject:@(value) forKey:property];
    }

  }];



If there are props that were previously set, but now are no longer set -- that is, if the old value of _propsDictionary has keys that wouldn't be in props (in the old code) -- should we set those entries in _propsDictionary to [NSNull null]?

A props node always keep the same props, if the props change we create a new one. See https://github.com/facebook/react-native/blob/master/Libraries/Animated/src/AnimatedImplementation.js#L1777 which is called from componentWillReceiveProps.

ide · 2017-02-10T09:38:27Z

Libraries/NativeAnimation/RCTNativeAnimatedModule.m

    [nodesManager connectAnimatedNodeToView:nodeTag viewTag:viewTag viewName:viewName];
  }];
 }

 RCT_EXPORT_METHOD(disconnectAnimatedNodeFromView:(nonnull NSNumber *)nodeTag
                  viewTag:(nonnull NSNumber *)viewTag)
 {
-  [_operations addObject:^(RCTNativeAnimatedNodesManager *nodesManager) {
+  // Disconnecting a view also restores it's default values so we have to make


it's -> its

ide · 2017-02-10T09:58:16Z

Libraries/NativeAnimation/RCTNativeAnimatedModule.m

+  NSMutableArray<AnimatedOperation> *_preOperations;
+  // Lock used for _operations and _preOperations. This is needed since operations
+  // are added from the UIManager queue and executed from the main queue.
+  NSLock *_operationsLock;


I would prefer to come up with a solution that doesn't use the lock on each operation to further ensure the code is correct, plus perhaps better performance due to less contention.

My concern is that we may process half a JS frame's worth of operations, rendering an inconsistent state to the user, regardless of whether the JS thread runs faster than the UI thread or vice versa. I believe we should treat all the operations in a given JS frame as one atomic set -- the code should be structured in a way that makes it impossible to do otherwise.

Here is one idea: we split _operations into two arrays (same for _preOperations): NSMutableArray<AnimOp> *_operations and NSMutableArray<NSArray<AnimOp> *> *_operationLists. The first array is accessible only from the UIManager queue and the second only from the native main queue (mostly).

The UIManager queue still writes to _operations the same way. When we know it has written all of its operations for the current JS frame, we run:

// On the UI queue [_operationsLock lock]; // Add both operation sets to be accessible from the main queue [_preOperationsLists addObject[_preOperations]; [_operationsLists addObject:_operations]; // We are on the UIManager queue at the end of the frame, this is safe: _preOperations = [NSMutableArray new]; _operations = [NSMutableArray new]; [_operationsLock unlock];

There's a little trickiness on the main queue to also use the above lock (plus probably more efficient solutions than what I proposed).

I see, working on a fix.

ide · 2017-02-10T09:59:42Z

React/Modules/RCTUIManager.m

@@ -723,6 +725,16 @@ - (void)_amendPendingUIBlocksWithStylePropagationUpdateForShadowView:(RCTShadowV
  }
 }

+- (void)addUIManagerObserver:(id<RCTUIManagerObserver>)observer


@javache any reservations about adding this API?

janicduplessis · 2017-02-17T23:08:05Z

@ide Updated, this adds a staged queue that we update only at the end of the batch. This avoid having to use locking when adding operations and makes sure operations are batched properly.

cychiuae · 2017-02-23T03:31:05Z

Any chance this PR will be included in 0.42?

janicduplessis · 2017-02-23T03:46:56Z

Would love to, ping @ide

ide · 2017-02-23T23:49:07Z

Libraries/NativeAnimation/RCTNativeAnimatedModule.m

 - (void)batchDidComplete
 {
-  NSArray *operations = _operations;
-  _operations = [NSMutableArray new];
+  [_operationsLock lock];


Add an assertion or a comment about which queue this runs on. It's important that this runs on the the method queue (RCTGetUIManagerQueue in practice) because we don't want [self addOperationBlock:] and this method to run concurrently. I think it might run on the JS queue..

AFAIK it will run on the same queue as the module specifies, will double check.

ide · 2017-02-23T23:54:16Z

Libraries/NativeAnimation/RCTNativeAnimatedModule.m

+  _stagedOperations = [NSMutableArray new];
+  _preOperations = [NSMutableArray new];
+  _stagedPreOperations = [NSMutableArray new];
+  _operationsLock = [NSLock new];


What do you think about naming this _stagedOperationsLock since it's only (I believe) the staged operation arrays that are accessed from different queues (UIManager queue and main queue)?

Yea you are right

ide · 2017-02-23T23:55:19Z

Libraries/NativeAnimation/RCTNativeAnimatedModule.m

+  _stagedOperations = [NSMutableArray new];
+  [_operationsLock unlock];
+
+  [operations enumerateObjectsUsingBlock:^(AnimatedOperation operation, NSUInteger i, BOOL *stop) {


Use for in instead of enumerateObjectsUsingBlock? Same above too.

ide · 2017-02-24T00:05:26Z

I realized there might be a potential bug with the locking, imagine this:

UIManager notifies observers that it willFlushUIBlocks. We acquire the operations lock, safely get the preOperations, release the lock, and execute the preOperations.
The JS frame happens to finish and batchDidComplete is called. We acquire the operations lock, add new operations to the preOperations and operations arrays, and release the lock.
UIManager flushes its UI blocks. Then it notifies observers that it didFlushUIBlocks. We acquire the operations lock, get all the operations (including the ones that were just added in step 2), release the lock, and execute the operations.

The problem is that we've run the preOperations for frame 1 but the operations for frames 1 and 2. Maybe this doesn't ever happen but I think it can because the UIManager does its work on the main queue while batchDidComplete is called from the JS thread.

ide · 2017-02-27T20:42:04Z

React/Modules/RCTUIManager.m

+
+      // Copy the observers set to avoid having to lock for too long while running ui blocks.
+      [_uiManagerObserversLock lock];
+      NSSet *observers = [[NSSet alloc] initWithSet:_uiManagerObservers];


Just do [_uiManagerObservers copy]

ide · 2017-02-27T20:43:47Z

React/Modules/RCTUIManager.m

@@ -1183,6 +1201,16 @@ - (void)flushUIBlocks
    RCTProfileBeginFlowEvent();
    dispatch_async(dispatch_get_main_queue(), ^{
      RCTProfileEndFlowEvent();
+
+      // Copy the observers set to avoid having to lock for too long while running ui blocks.


More importantly this avoids needing to think about concurrent modification of the observers set while the observers run (i.e. when an observer adds or removes other observers)

Yea this actually avoids a deadlock too haha, will update the comment a bit.

ide · 2017-02-27T20:47:38Z

Libraries/NativeAnimation/RCTNativeAnimatedModule.m

-    }];
-    [self->_nodesManager updateAnimations];
-  });
+  [self.bridge.uiManager addUIManagerObserver:batch];


I think you should go back to your first approach of adding just one observer. The issue here is that the observers are stored in an NSSet, which doesn't preserve order. So if batchDidComplete is called twice, the observer set might store the OperationBatch objects in the wrong order.

So I think it would be better to have one NSMutableArray<OperationBatch *> in RCTNativeAnimatedModule and make the module be the only observer.

btw I think this batch approach is good, this is what we do for GL calls in exgl.

Good call about set ordering, didnt think about that, I'll try the approach you suggested.

ide · 2017-02-27T20:49:32Z

React/Modules/RCTUIManager.m

@@ -723,6 +727,20 @@ - (void)_amendPendingUIBlocksWithStylePropagationUpdateForShadowView:(RCTShadowV
  }
 }

+- (void)addUIManagerObserver:(id<RCTUIManagerObserver>)observer
+{
+  [_uiManagerObserversLock lock];


I think it would also be safe to remove the lock and replace it with dispatch_async(main queue) since the main queue is a serial queue.

I think it's preferable that adding observers is synchronous, with the current impl that adds an observer for each batch this is needed otherwise the observer could be added after the UI operations are completed.

The order of operations that currently touch the lock (addUIManagerObserver, removeUIManagerObserver, the code within flushUIBlocks) should still happen in the same order with dispatch_async, which keeps the order of blocks (it uses FIFO). I believe that "otherwise the observer could be added after the UI operations are completed" is about as equally likely with the locks as with dispatch_async.

The case Im worried about is:
UIManager batchDidComplete -> dispatch async the ui operations block
NativeAnimatedModule batchDidComplete -> dispatch async addObserver.

In this case the observer would get added after operations are executed. It's preferable not to assume the order in which the batchDidComplete methods get executed in.

We still have a similar bug with the locks:

[UIManager batchDidComplete] -> dispatch_async to the main queue to run the UI blocks

Main thread starts running and acquires the observers lock

[NativeAnimatedModule batchDidComplete] -> acquires the observers lock

Yea I see, so now it's a race instead of happening all the time. Anyway if I make the change to use one observer this locking won't be needed anymore.

ide · 2017-02-27T20:55:02Z

React/Modules/RCTUIManager.m

@@ -224,6 +224,8 @@ @implementation RCTUIManager
  NSDictionary *_componentDataByName;

  NSMutableSet<id<RCTComponent>> *_bridgeTransactionListeners;
+  NSMutableSet<id<RCTUIManagerObserver>> *_uiManagerObservers;


This should hold weak references -- otherwise I think there is a cycle from the animation module -> bridge -> UIManager.

You might be able to use [NSPointerArray weakObjectsPointerArray] or an NSArray of [NSValue valueWithNonretainedObjectValue:] but be very careful especially around the semantics of what happens when the values are dealloc'd and whether the references are properly zeroed.

Also trying to get the array to behave well might not be a good use of time now, you could change the property to be just __weak id<RCTUIManagerObserver> _observer similar to a traditional delegate.

If we properly remove the observer when we dealloc the animated module it shouldn't be an issue right?

Yes, are you sure the animated module eventually gets deallocated? Might be good to verify that dealloc is called.

Yes I can verify that.

I assume all native modules get deallocated at the same time, I guess the bridge is where it could leak.

Native modules never seemed to get deallocated during runtime so it shouldn't be an issue, even if they did shouldn't leak since we are removing the observer.

They get dealloc'd upon reload, make sure you test that case.

I did test reloading but dealloc never got called for neither UIManager nor AnimatedModule.

Well makes sense that dealloc never gets called if it leaks lol. Moved the code that cleans up the module to invalidate and now everything gets dealloc'ed properly on reload.

janicduplessis · 2017-02-27T21:35:52Z

I've also noticed that this bug is present on android too, didn't notice it because I am using different transitions on Android. Working on a similar fix.

janicduplessis · 2017-03-01T06:07:33Z

@ide Ok I think all race conditions are fixed.

ide · 2017-03-01T08:17:29Z

React/Modules/RCTUIManager.m

@@ -224,6 +224,8 @@ @implementation RCTUIManager
  NSDictionary *_componentDataByName;

  NSMutableSet<id<RCTComponent>> *_bridgeTransactionListeners;
+  NSMutableSet<id<RCTUIManagerObserver>> *_uiManagerObservers;


They get dealloc'd upon reload, make sure you test that case.

ide · 2017-03-04T01:23:05Z

Libraries/NativeAnimation/RCTNativeAnimatedModule.m

+  [_operationBatches removeAllObjects];
+  [_operationBatchesLock unlock];
+
+  for (OperationBatch *batch in _stagedOperationBatches) {


One thing I'm concerned about (please sanity check me, would be great if I'm mistaken), is that we might have multiple operation batches for several JS frames (ex: batches for JS frames 1 and 2) and if the main thread is lagging, then when the UIManager flushes the UI blocks for native/main-thread frame 1, we'll run the pre/post-operations for JS frames 1 and 2.

Sorry this is so tricky...

Yea you are right, I checked how UIManager works it it will dispatch to the main queue once per batch and execute only the operations from that batch instead of every it has available. I think what could work well here is only process one batch at a time, this will work because for each completed batch there will be a call to the UIManagerObserver methods.

janicduplessis · 2017-03-04T22:23:34Z

@ide Ok, we're getting there! 😅

Tested that the module properly gets dealloc'ed on reload and batches should be processed properly now.

janicduplessis · 2017-03-04T22:44:19Z

So I made things a lot harder than they should have been, I realized we can just leverage the UIManager operation queue to schedule our operations when we want. I simply added some API to also be able to add a block at the start of the queue.

…animated on iOS

janicduplessis · 2017-03-22T02:33:36Z

@ide Added the extra comments you mentioned.

ide · 2017-03-22T03:59:25Z

@facebook-github-bot shipit

facebook-github-bot · 2017-03-22T04:00:02Z

@ide has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

javache · 2017-03-23T11:44:13Z

This is not landing internally because we have -Wimplicit-retain-self turned on. Any reference to an ivar inside a block will need to be prefixed with self->

janicduplessis · 2017-03-23T18:22:37Z

@javache Thanks! Could you sync up clang flags between OSS and fb internal build to avoid these import failures in the future?

janicduplessis · 2017-03-23T22:55:29Z

@ide @javache Updated to use explicit self retains, pending blocks are nil'ed in UIManager invalidate so there is no risks of leaks here, tested that both RCTUIManager and RCTNativeAnimatedModule get dealloc'ed on app reload to make sure.

ide · 2017-03-24T00:34:08Z

@facebook-github-bot shipit

facebook-github-bot · 2017-03-24T00:35:00Z

@ide has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

janicduplessis · 2017-03-28T00:59:53Z

@javache Could you check why this is not landing?

nihgwu · 2017-03-28T15:40:41Z

@janicduplessis just test with the master branch on my app, seems this commit breaks Native Animation, then I revert this commit locally, it works perfectly as before

janicduplessis · 2017-03-28T15:55:02Z

@nihgwu Thanks for testing this, looks like there is an issue when there is an value listener. Looking into it.

nihgwu · 2017-03-28T15:56:43Z

with the latest master:

after revert this commit:

You can see that the stickySectionheader is flickering and the stickyheader( implemented in Native Animation ) is jumping

nihgwu · 2017-03-28T16:01:42Z

I've to say that this commit should solve some of the issues when I play with Native Animation, for example the flashing screen issue when navigate using NavigationExperimental I guess, so I've been waiting for this PR so long

Thanks for you quick feedback, I believe you can solve the problem soon , and great thanks for your work on Native Animation

nihgwu · 2017-03-28T16:04:49Z

I've tried to remove the listener but still the same

janicduplessis · 2017-03-28T16:15:22Z

It's actually related to rerendering, I should have a fix soon.

Summary: This call got lost in the refactor of #11819 and caused some view updating issues. Closes #13183 Differential Revision: D4787565 Pulled By: javache fbshipit-source-id: 8f7d456824c67abee6ac1d5f906e4c831ede889b

Summary: This fixes a bug that causes properties to keep stale values because they were not restored to their default after being removed when their value was controlled by native animated. To fix this we restore default values in `disconnectFromView` by updating views with null values for all props that we modified previously. However this causes another issue where we lose any props that were set by the normal process because NativeAnimated operations are always executed after UIManager operatations. To fix this I added a way to hook into UIManager view updating process to be able to execute NativeAnimated operations either before or after updating native views. In the case of disconnecting we want to do it before updating views so that it does: Value changed by native animated -> value restored to default -> (optional) value updated by normal prop. This PR also depends on facebook#10658. **Test plan** Tested that this fixed a particular bug in an app that uses ex-navigation + native animations where a navbar w Closes facebook#11819 Differential Revision: D4752566 Pulled By: javache fbshipit-source-id: 68ee28200ffeba859ae1b98ac753bd7dcb8910f0

Summary: This call got lost in the refactor of facebook#11819 and caused some view updating issues. Closes facebook#13183 Differential Revision: D4787565 Pulled By: javache fbshipit-source-id: 8f7d456824c67abee6ac1d5f906e4c831ede889b

Summary: Same as #11819 but for Android. I didn't notice the bug initially in my app because I was using different animations on Android which did not trigger this issue. **Test plan** Created a simple repro example and tested that this fixes it. https://gist.github.com/janicduplessis/0f3eb362dae63fedf99a0d3ee041796a Closes #12842 Differential Revision: D5556439 Pulled By: hramos fbshipit-source-id: d13f4ad258d03cca46c793751ebc49d942b99152

Summary: Changelog: [Internal] # Problem Before every update, restoreDefaults is called on animated nodes. In paper this makes sure no stale properties are on animated nodes. In paper it works fine because restoreDefaults is called before mounting and animations are triggered after mounting within single commit. Details: #11819 In Fabric however it is called outside of other mounting operations and it applies default values to the view and then re-applies animated values. Reviewed By: JoshuaGross Differential Revision: D21786765 fbshipit-source-id: a2cb6d6d9cbd39d4c403c97c2f51e7d92078102f

facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Jan 10, 2017

janicduplessis added Animated Platform: iOS iOS applications. labels Jan 10, 2017

janicduplessis force-pushed the native-animations-default-values branch from 566cde7 to 46b073f Compare January 12, 2017 05:01

janicduplessis mentioned this pull request Jan 22, 2017

NavigationExperimental is broken when native animations are used #11377

Closed

janicduplessis force-pushed the native-animations-default-values branch from 46b073f to c31d6ff Compare January 27, 2017 21:00

janicduplessis mentioned this pull request Feb 8, 2017

Header of first screen rendered behind second screen header react-navigation/react-navigation#229

Closed

satya164 mentioned this pull request Feb 9, 2017

Header title covers half of back button on iOS react-navigation/react-navigation#197

Closed

ide self-requested a review February 9, 2017 20:04

ide suggested changes Feb 10, 2017

View reviewed changes

janicduplessis force-pushed the native-animations-default-values branch from cb68c83 to 0a15f9a Compare February 17, 2017 22:48

ide suggested changes Feb 24, 2017

View reviewed changes

ide reviewed Feb 27, 2017

View reviewed changes

ide reviewed Mar 4, 2017

View reviewed changes

Properly restore default values when removing props driven by native …

eb5152f

…animated on iOS

janicduplessis force-pushed the native-animations-default-values branch from 9465c1a to eb5152f Compare March 4, 2017 23:24

janicduplessis added 2 commits March 5, 2017 17:57

Merge branch 'master' into native-animations-default-values

a018a54

Merge branch 'master' into native-animations-default-values

1649b1f

janicduplessis mentioned this pull request Mar 10, 2017

Native Animated - Restore default values when removing props on Android #12842

Closed

ide added GH Review: accepted and removed GH Review: review-needed labels Mar 21, 2017

janicduplessis added 2 commits March 21, 2017 22:20

Merge branch 'master' into native-animations-default-values

e94c8e1

Update comments

ff5b2fe

ide approved these changes Mar 22, 2017

View reviewed changes

janicduplessis added 2 commits March 23, 2017 18:24

Merge branch 'master' into native-animations-default-values

4bbc66c

Fix implicit retains

483bf36

facebook-github-bot closed this in c9fae2f Mar 28, 2017

janicduplessis mentioned this pull request Mar 28, 2017

NativeAnimated - Fix missing update at the end of the batch #13183

Closed

janicduplessis deleted the native-animations-default-values branch March 31, 2017 20:08

Native Animated - Restore default values when removing props on iOS #11819

Native Animated - Restore default values when removing props on iOS #11819

Conversation

janicduplessis commented Jan 10, 2017 • edited Loading

facebook-github-bot commented Jan 10, 2017

janicduplessis commented Jan 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janicduplessis commented Feb 17, 2017

cychiuae commented Feb 23, 2017

janicduplessis commented Feb 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ide commented Feb 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janicduplessis commented Feb 27, 2017

janicduplessis commented Mar 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janicduplessis commented Mar 4, 2017

janicduplessis commented Mar 4, 2017

janicduplessis commented Mar 22, 2017

ide commented Mar 22, 2017

facebook-github-bot commented Mar 22, 2017

javache commented Mar 23, 2017

janicduplessis commented Mar 23, 2017

janicduplessis commented Mar 23, 2017

ide commented Mar 24, 2017

facebook-github-bot commented Mar 24, 2017

janicduplessis commented Mar 28, 2017

nihgwu commented Mar 28, 2017

janicduplessis commented Mar 28, 2017 • edited Loading

nihgwu commented Mar 28, 2017

nihgwu commented Mar 28, 2017

nihgwu commented Mar 28, 2017

janicduplessis commented Mar 28, 2017

janicduplessis commented Jan 10, 2017 •

edited

Loading

janicduplessis commented Mar 28, 2017 •

edited

Loading