Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Android][Fixed] - Fix the IndexOutOfBoundsException when wrong tag is used in manageChildren because of animation. #23530

Closed
wants to merge 1 commit into from

Conversation

zhangmincons
Copy link

Fix the IndexOutOfBoundsException when wrong tag is used in manageChildren because of animation.

Summary

When using animation, some views may not be removed immediately. Instead, they will be removed when the animation is finished. This can cause incorrect index or even crashes.

  • Bad Case:
import React, { Component } from 'react';
import { Text, View, UIManager, LayoutAnimation } from 'react-native';

class Screen extends Component {
  showB = true;
  toggle = () => {
    this.showB = !this.showB;
    this.forceUpdate();
  };

  componentWillUpdate(/* A garder : nextProps , nextState */) {
    LayoutAnimation.easeInEaseOut();
  }

  render() {
    return (
    <View style={{flex: 1, backgroundColor: '#F5FCFF'}}>
      <View style={{ margin: 40 }}>
        <Text onPress={this.toggle}>Click 2 times here to make app crash</Text>
        {
          [
            <View key={0}>
              <Text>A</Text>
              {
                this.showB ?
                  <Text>B</Text>
                  :
                  null
              }
            </View>,
            <View key={1}>
              <Text>A</Text>
              {
                this.showB ?
                  <Text>B</Text>
                  :
                  null
              }
            </View>,
          ]
        }
      </View>
    </View>
    );
  }
}

export default Screen;
  • Root Cause:

image

  1. Index 2(the first B) is supposed to be removed, but it is not removed immediately because of animation.
  2. Index 3(the second B) is supposed to be removed then, but the first B has not be removed yet. Therefore the second A is removed by mistake.
  • Solution:
    A mark is used to adjust the index. Using an delete-mark to represent the view going to be deleted can help find out the correct index when the animation finish and therefore fix the problem. In mDeleteMark, Integer(0) represents a normal view while Integer(1) represents the view that should be removed but because of animation it should be removed later. The true index can be gotten by calculating the number of Integer(1).

  • Issue:App crash with error "index=4 count=3" using LayoutAnimation.easeInEaseOut() on Android #17118

Changelog

[Android][Fixed] - Fix the IndexOutOfBoundsException when wrong tag is used in manageChildren because of animation.

Test Plan

  • Open the Android emulator or an Android device.
  • Create a react-native-app with the bad case code above.
  • Tap the button and see if it will crash.

@facebook-github-bot 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 Feb 19, 2019
@cpojer cpojer requested a review from mdvacca February 20, 2019 05:56
Copy link
Contributor

@mdvacca mdvacca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @zhangmincons, thanks for working on this. I added few comments
Moving the PR back to you to review my comments.

@todorone
Copy link

@zhangmincons @mdvacca Isn't this PR already trying to fix the same problem?
#22776

@zhangmincons
Copy link
Author

@zhangmincons @mdvacca Isn't this PR already trying to fix the same problem?
#22776

Nope, I've read that PR, and it fixed a different problem.

@zhangmincons
Copy link
Author

Hey @zhangmincons, thanks for working on this. I added few comments
Moving the PR back to you to review my comments.

Thank you, I will read the comments and fix the problems.

@zhangmincons zhangmincons force-pushed the master branch 2 times, most recently from d4d3d9d to ea816ea Compare February 27, 2019 06:21
@zhangmincons
Copy link
Author

hi, @mdvacca, the problems are solved. Can you review the code again? Thanks!

…s used in manageChildren because of animation.
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@RWOverdijk
Copy link

Is there any progress on this?

@lunaleaps
Copy link
Contributor

Hi @zhangmincons this was fixed internally here:
20b4879

Apologies about the lack of synchronization on this!

@cpojer
Copy link
Contributor

cpojer commented Apr 29, 2019

Based on @lunaleaps's comment, I'll close this PR here since the underlying problem should already have been fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Platform: Android Android applications.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants