Skip to content

Conversation

maciekstosio
Copy link
Contributor

Description

Fixes #2810

This PR should fix a crash when going back from the screen that has flat list.

Changes

There are two exceptions thrown:

First:

Error: Exception in HostFunction: android.view.ViewRootImpl$CalledFromWrongThreadException: Only the original thread that created a view hierarchy can touch its views.

Second:

java.lang.IndexOutOfBoundsException: getChildDrawingOrder() returned invalid index 1 (child count is 1)

And it fails around this place in Screen.kt:

(...)
                if (parent is SwipeRefreshLayout && child is ImageView) {
                    // SwipeRefreshLayout class which has CircleImageView as a child,
                    // does not handle `startViewTransition` properly.
                    // It has a custom `getChildDrawingOrder` method which returns
                    // wrong index if we called `startViewTransition` on the views on new arch.
                    // We add a simple View to bump the number of children to make it work.
                    // TODO: find a better way to handle this scenario
                        it.addView(View(context), i)
                } else {
                    child?.let { view -> it.startViewTransition(view) }
                }
(...)

Based on the comment, I assume it.addView(View(context), i) is to prevent the second crash. Because the first exception, we do not add "workaround" view, thus we run into second. We get the first exception, as the operation is performed from different thread, thus I ensure the adding view is performed from UIThread.

Test code and steps to reproduce

Source code
import * as React from 'react';
import { View, Text, FlatList } from 'react-native';
import { NavigationContainer, useNavigation } from '@react-navigation/native';
import { createNativeStackNavigator, NativeStackScreenProps } from '@react-navigation/native-stack';
import { Button } from '@react-navigation/elements';
import { SafeAreaProvider } from 'react-native-safe-area-context';

type StackParamList = {
  Home: undefined;
  Details: undefined;
};

function HomeScreen() {
  const navigation = useNavigation<NativeStackScreenProps<StackParamList, 'Home'>['navigation']>();

  return (
    <View style={{ flex: 1, alignItems: 'center', justifyContent: 'center' }}>
      <Text>Home Screen</Text>
      <Button onPress={() => navigation.navigate('Details')}>
        Go to Details
      </Button>
    </View>
  );
}

function DetailsScreen() {
  const [isRefreshing, setIsRefreshing] = React.useState(false);

  return (
    <View style={{ flex: 1, alignItems: 'center', justifyContent: 'center' }}>
      <FlatList
        data={[1, 2, 3, 4, 5]}
        renderItem={({ item }) => <Text>{item}</Text>}
        onRefresh={() => {
          return new Promise<void>((resolve) => {
            setIsRefreshing(true);
            setTimeout(() => {
              resolve();
            }, 3000);
          }).then(() => {
            setIsRefreshing(false);
          });
        }}
        refreshing={isRefreshing}
      />
    </View>
  );
}

const Stack = createNativeStackNavigator<StackParamList>();

function NestedStack() {
  return (
    <Stack.Navigator>
      <Stack.Screen name="Home" component={HomeScreen} />
      <Stack.Screen name="Details" component={DetailsScreen} />
    </Stack.Navigator>
  );
}

export default function App() {
  return (
    <SafeAreaProvider>
      <NavigationContainer>
        <NestedStack />
      </NavigationContainer>
    </SafeAreaProvider>
  );
};

Checklist

  • Included code example that can be used to test this change
  • Ensured that CI passes

Copy link
Member

@kkafar kkafar left a comment

Choose a reason for hiding this comment

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

Yeah I understand the issue. This won't be the way-to-go to fix it. mostly because concurrency problems. What we should do in lieu of this approach, is to schedule calls to startTransitionRecursive on UI thread.

I was trying to do this already some time ago, but decided to not merge since "there weren't any problems with current approach".

Can we have a call so that I can explain the fix I see here to you?

@kkafar
Copy link
Member

kkafar commented Jun 6, 2025

This is the approach I've mentioned to you: https://github.com/software-mansion/react-native-screens/pull/2532/files, however I'm not happy with this PR neither. Let's have a talk today & I'll explain.

NativeProxy

We should be fine doing this, because the `notifyScreenRemoved` code is
run on JS thread BEFORE transaction execution is scheduled on UI thread.

This code works only under assumption, that the transaction is scheduled
in a SERIAL, no prioritized manner.

TOOD: We need to confirm that this is a case on Android.
@kkafar
Copy link
Member

kkafar commented Jul 17, 2025

@maciekstosio Do you remember, what was the conclusion for this PR? Were there any issues? I have something in the back of my head, that there was an issue, but can not recall exactly.

@mensonones
Copy link

This problem would not be solved with?

fun startRemovalTransition() {
        if (!isBeingRemoved) {
            isBeingRemoved = true
            post { startTransitionRecursive(this) }
        }
    }

@kkafar kkafar self-requested a review October 13, 2025 14:07
Copy link
Member

@kkafar kkafar left a comment

Choose a reason for hiding this comment

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

I've tested the runtime & it seems to work fine.
This PR will be also helpful in landing of #3250, therefore I decide to proceed here.

@kkafar kkafar merged commit 1b3d3f9 into main Oct 14, 2025
5 checks passed
@kkafar kkafar deleted the @maciekstosio/Fix-crash-from-#2810 branch October 14, 2025 10:03
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.

Going back in native stack cause crash on Android 7 if screen contains FlatList with refresh props (React Navigation related)

3 participants