AnimatedImplementation: Make sure we don't attempt to start an animation with index above array-length.

[runar-rkmedia]

Sometimes, the index for the animations-array will be outside the range of the array, so we need a nullcheck before attempting to do animations[current].start(onComplete);

fixes #17699

Motivation

Fix the bug.

Test Plan

Tested it in application, simple fix so should not have any side-effects.

Related PRs

(If this PR adds or changes functionality, please take some time to update the docs at https://github.com/facebook/react-native-website, and link to your PR here.)

Release Notes

[Android] [Fixes] - AnimatedImplementation : Make sure we don't attempt to start an animation with index above array-length.

[pull-bot]

@facebook-github-bot label Android

Generated by 🚫 dangerJS

[janicduplessis]

@runar-rkmedia Do you know why the index is out of range (maybe a repro example)? Would love to understand the cause of the bug before merging this fix.

[runar-rkmedia]

@janicduplessis First of all, thank you for looking into this.

I'm quite sorry, but I'm not quite sure what is causing this. In the linked issue #17699 there is a reproducible minimal example, but I fail to find the exact reason for this happening.

[cpojer]

It seems like this is an issue that only the original author @runar-rkmedia ran into and all of us, including the author of the PR, are unsure about what the issue is and how it happens. I'm not excited about merging a change to React Native where we have such little confidence so I'm gonna close this PR.

I'm happy to reopen and merge this with proper explanations and verification, though.

[hansencj06]

I was running this issue as well. I think the problem occurs when starting an animation sequence without resetting the animation.

Here's the implementation of Animation.sequence

const sequence = function(
  animations: Array<CompositeAnimation>,
): CompositeAnimation {
  let current = 0;
  return {
    start: function(callback?: ?EndCallback) {
      const onComplete = function(result) {
        if (!result.finished) {
          callback && callback(result);
          return;
        }

        current++;

        if (current === animations.length) {
          callback && callback(result);
          return;
        }

        animations[current].start(onComplete);
      };

      if (animations.length === 0) {
        callback && callback({finished: true});
      } else {
        animations[current].start(onComplete);
      }
    },

    stop: function() {
      if (current < animations.length) {
        animations[current].stop();
      }
    },

    reset: function() {
      animations.forEach((animation, idx) => {
        if (idx <= current) {
          animation.reset();
        }
      });
      current = 0;
    },

    ...
};

The variable current is incremented every time an animation in the sequence finishes. If you keep the sequence in a ref, current never gets reset unless you call reset. I don't know if this is desired behavior, but calling reset in a callback on start, resets current to 0 and the animation can be run again without issue.