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

fix: check if view is not null #671

Merged
merged 1 commit into from
Nov 4, 2020
Merged

Conversation

WoLewicki
Copy link
Member

@WoLewicki WoLewicki commented Oct 19, 2020

Added a check for if View is a null. Should resolve #646.

@kmagiera
Copy link
Member

Already accepted this, but after looking at #646 it'd be good to understand what are the cases when the view is null. It may be the case that we should never expect view to be null at this point and the fact it isn't defined is some other issue surfaced this way

@WoLewicki
Copy link
Member Author

It can be caused by the asynchronous transitions between screens, but I am not sure. cc @janicduplessis, maybe you have a clue why there can be null already? Also, @kmagiera should I merge it as a temporary workaround?

@janicduplessis
Copy link
Contributor

I think it could be caused by the fragment being destroyed during the animation. Not sure how this can happen, maybe if an ancestor view of the fragment is removed?

@kmagiera
Copy link
Member

kmagiera commented Nov 2, 2020

IMO this is safe to be merged. The crash means that fragment that ends animation is no longer mounted. In that case there see to be no benefit of dispatching the "end transition" event we use the animation listener for because the receiver no longer exists anyway.

@WoLewicki WoLewicki merged commit 77ef5f6 into master Nov 4, 2020
@WoLewicki WoLewicki deleted the @wolewicki/fix-NPE-in-stack branch November 4, 2020 15:03
@owinter86
Copy link

owinter86 commented Nov 4, 2020

@kmagiera and @janicduplessis I can confirm this does fix the issue, but if you are still trying to work out how to replicate it, its with any nested stack navigator and spamming the android back button to dismiss the nested stack. Navigate to the "Yellow" screen and spam the android device back button in quick succession and it will cause the crash at #646

import React from 'react';
import { NavigationContainer } from '@react-navigation/native';
import { createNativeStackNavigator } from 'react-native-screens/native-stack';
import { Button, View } from 'react-native';

const MainStack = createNativeStackNavigator();
export default function NavigationTest() {
  return (
    <NavigationContainer>
      <MainStack.Navigator>
        <MainStack.Screen name="Red" component={Red} />
        <MainStack.Screen name="ColourNav" component={ColourNav} />
      </MainStack.Navigator>
    </NavigationContainer>
  );
}

const ColourStack = createNativeStackNavigator();
function ColourNav() {
  return (
    <ColourStack.Navigator>
      <ColourStack.Screen name="Blue" component={Blue} />
      <ColourStack.Screen name="Yellow" component={Yellow} />
    </ColourStack.Navigator>
  );
}

function Red({ navigation }) {
  return (
    <View
      style={{
        backgroundColor: 'red',
        flex: 1,
        alignItems: 'center',
        justifyContent: 'center',
      }}
    >
      <Button title="Go To Blue" onPress={() => navigation.navigate('ColourNav')} />
    </View>
  );
}
function Blue({ navigation }) {
  return (
    <View
      style={{
        backgroundColor: 'blue',
        flex: 1,
        alignItems: 'center',
        justifyContent: 'center',
      }}
    >
      <Button title="Go To Yellow" onPress={() => navigation.navigate('Yellow')} />
    </View>
  );
}

function Yellow({ navigation }) {
  return (
    <View
      style={{
        backgroundColor: 'yellow',
        flex: 1,
        alignItems: 'center',
        justifyContent: 'center',
      }}
    >
      <Button title="Go Back" onPress={() => navigation.goBack()} />
    </View>
  );
}

@kmagiera
Copy link
Member

kmagiera commented Nov 4, 2020

Thanks @owinter86 for sending that. I still want to investigate to see if the fix does not actually put the stack in some weird state regardless of the fact that it no longer crashes. Will try that later this week. Thanks so much!

@kmagiera
Copy link
Member

kmagiera commented Nov 4, 2020

The repro case @owinter86 worked and managed to get the crash w/o this patch applied. This also helped me investigate the case in more depth and now have full confidence about this fix.

As suspected the crash was due to the fact that the stack would unmount while performing animation. In the repro case we do exacly that by hitting back button twice: first back starts transition on the nested stack while the second is transitions the parent stack. In such a case the proposed patch would prevent us from calling onViewAppearTransitionEnd. However, in such a case it is completely fine to skip the call as this can only occur when the screen is being unmounted. We call onViewAppearTransitionEnd in this place because it is the only way for us to tell when the animation is finished, but the place where we call this method does not know if this is an entering or exiting animation while we are only interested in the former. This check happens inside the onViewAppearTransitionEnd method (see it here. When getView returns null it means the screen is completely unmounted and thus we can be sure that there was an exit transition, hence there is no need to enter onViewAppearTransitionEnd (it is therefore not an "appear" transition)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Android: android.view.ViewParent android.view.View.getParent()' on a null object reference
4 participants