Skip to content

Commit

Permalink
Fix setImmediate/clearTimeout mismatch in NativeAnimatedHelper (#46525)
Browse files Browse the repository at this point in the history
Summary:
Pull Request resolved: #46525

While debugging I noticed that my setTimout was being cleared by someone else. Upon further inspection I found NativeAnimatedHelper which was mismatching setImmediate timer handle with a clearTimeout. This isn't safe and needs to be fixed.

I used this to rename the timer to be more clear to reduce the changes of mismatches.

Changelog:
[General][Fixed] - Fix setImmediate/clearTimeout mismatch in NativeAnimatedHelper that could clear an unrelated setTimeout.

Reviewed By: javache, yungsters

Differential Revision: D62775703

fbshipit-source-id: c1669c60bd08f13a59dd6159be2f471a6c1beebd
  • Loading branch information
Benoit Girard authored and facebook-github-bot committed Sep 18, 2024
1 parent bebd653 commit a5dd1be
Show file tree
Hide file tree
Showing 3 changed files with 24 additions and 9 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -298,6 +298,11 @@ const definitions: FeatureFlagDefinitions = {
defaultValue: false,
description: 'Enables Animated to skip non-allowlisted props and styles.',
},
enableAnimatedClearImmediateFix: {
defaultValue: true,
description:
'Enables an experimental to use the proper clearIntermediate instead of calling the wrong clearTimeout and canceling another timer.',
},
enableAnimatedPropsMemo: {
defaultValue: false,
description:
Expand Down
20 changes: 12 additions & 8 deletions packages/react-native/src/private/animated/NativeAnimatedHelper.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ const isSingleOpBatching =
Platform.OS === 'android' &&
NativeAnimatedModule?.queueAndExecuteBatchedOperations != null &&
ReactNativeFeatureFlags.animatedShouldUseSingleOp();
let flushQueueTimeout = null;
let flushQueueImmediate = null;

const eventListenerGetValueCallbacks: {
[number]: (value: number) => void,
Expand Down Expand Up @@ -142,9 +142,13 @@ const API = {
queueOperations = true;
if (
ReactNativeFeatureFlags.animatedShouldDebounceQueueFlush() &&
flushQueueTimeout
flushQueueImmediate
) {
clearTimeout(flushQueueTimeout);
if (ReactNativeFeatureFlags.enableAnimatedClearImmediateFix()) {
clearImmediate(flushQueueImmediate);
} else {
clearTimeout(flushQueueImmediate);
}
}
},

Expand All @@ -161,9 +165,9 @@ const API = {
invariant(NativeAnimatedModule, 'Native animated module is not available');

if (ReactNativeFeatureFlags.animatedShouldDebounceQueueFlush()) {
const prevTimeout = flushQueueTimeout;
clearImmediate(prevTimeout);
flushQueueTimeout = setImmediate(API.flushQueue);
const prevImmediate = flushQueueImmediate;
clearImmediate(prevImmediate);
flushQueueImmediate = setImmediate(API.flushQueue);
} else {
API.flushQueue();
}
Expand All @@ -176,7 +180,7 @@ const API = {
NativeAnimatedModule || process.env.NODE_ENV === 'test',
'Native animated module is not available',
);
flushQueueTimeout = null;
flushQueueImmediate = null;

if (singleOpQueue.length === 0) {
return;
Expand All @@ -198,7 +202,7 @@ const API = {
NativeAnimatedModule || process.env.NODE_ENV === 'test',
'Native animated module is not available',
);
flushQueueTimeout = null;
flushQueueImmediate = null;

if (queue.length === 0) {
return;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @generated SignedSource<<a1d731250e59d99ac390c9d01c4dbd2f>>
* @generated SignedSource<<da9c8c080b54d86a30c4f671365ba990>>
* @flow strict
*/

Expand Down Expand Up @@ -32,6 +32,7 @@ export type ReactNativeFeatureFlagsJsOnly = {
animatedShouldUseSingleOp: Getter<boolean>,
enableAccessToHostTreeInFabric: Getter<boolean>,
enableAnimatedAllowlist: Getter<boolean>,
enableAnimatedClearImmediateFix: Getter<boolean>,
enableAnimatedPropsMemo: Getter<boolean>,
enableOptimisedVirtualizedCells: Getter<boolean>,
isLayoutAnimationEnabled: Getter<boolean>,
Expand Down Expand Up @@ -124,6 +125,11 @@ export const enableAccessToHostTreeInFabric: Getter<boolean> = createJavaScriptF
*/
export const enableAnimatedAllowlist: Getter<boolean> = createJavaScriptFlagGetter('enableAnimatedAllowlist', false);

/**
* Enables an experimental to use the proper clearIntermediate instead of calling the wrong clearTimeout and canceling another timer.
*/
export const enableAnimatedClearImmediateFix: Getter<boolean> = createJavaScriptFlagGetter('enableAnimatedClearImmediateFix', true);

/**
* Enables Animated to analyze props to minimize invalidating `AnimatedProps`.
*/
Expand Down

0 comments on commit a5dd1be

Please sign in to comment.