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(iOS): Crash while pushing n different screens at the same time #2249

Merged
merged 10 commits into from
Jul 26, 2024
85 changes: 85 additions & 0 deletions apps/src/tests/Test2235.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
import React from 'react';
import { View, StyleSheet } from 'react-native';
import {
createNativeStackNavigator,
NativeStackNavigationProp,
} from '@react-navigation/native-stack';
import { Button } from '../shared';
import { NavigationContainer } from '@react-navigation/native';

type StackParamList = {
Main: undefined;
Detail: undefined;
Detail2: undefined;
};

interface MainScreenProps {
navigation: NativeStackNavigationProp<StackParamList, 'Main'>;
}

const MainScreen = ({ navigation }: MainScreenProps): React.JSX.Element => (
<View style={{ ...styles.container, backgroundColor: 'moccasin' }}>
<Button
testID="simple-native-stack-go-to-detail"
title="Go to detail"
onPress={() => {
navigation.navigate('Detail');
navigation.navigate('Detail2');
}}
/>
</View>
);

interface DetailScreenProps {
navigation: NativeStackNavigationProp<StackParamList, 'Detail'>;
}

const DetailScreen = ({ navigation }: DetailScreenProps): React.JSX.Element => (
<View style={{ ...styles.container, backgroundColor: 'thistle' }}>
<Button
title="Go back"
onPress={() => navigation.goBack()}
testID="simple-native-stack-detail-go-back"
/>
</View>
);

const DetailScreen2 = ({
navigation,
}: DetailScreenProps): React.JSX.Element => (
<View style={{ ...styles.container, backgroundColor: 'yellow' }}>
<Button
title="Go back"
onPress={() => navigation.goBack()}
testID="simple-native-stack-detail-go-back"
/>
</View>
);

const Stack = createNativeStackNavigator<StackParamList>();

const App = (): React.JSX.Element => (
<NavigationContainer>
<Stack.Navigator
screenOptions={{
headerBackVisible: false,
}}>
<Stack.Screen
name="Main"
component={MainScreen}
options={{ title: 'Simple Native Stack' }}
/>
<Stack.Screen name="Detail" component={DetailScreen} />
<Stack.Screen name="Detail2" component={DetailScreen2} />
</Stack.Navigator>
</NavigationContainer>
);

const styles = StyleSheet.create({
container: {
flex: 1,
paddingTop: 10,
},
});

export default App;
1 change: 1 addition & 0 deletions apps/src/tests/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -104,5 +104,6 @@ export { default as Test2184 } from './Test2184';
export { default as Test2223 } from './Test2223';
export { default as Test2227 } from './Test2227';
export { default as Test2229 } from './Test2229';
export { default as Test2235 } from './Test2235';
export { default as TestScreenAnimation } from './TestScreenAnimation';
export { default as TestHeader } from './TestHeader';
26 changes: 20 additions & 6 deletions ios/RNSScreenStack.mm
Original file line number Diff line number Diff line change
Expand Up @@ -1117,9 +1117,8 @@ - (void)mountChildComponentView:(UIView<RCTComponentViewProtocol> *)childCompone

[_reactSubviews insertObject:(RNSScreenView *)childComponentView atIndex:index];
((RNSScreenView *)childComponentView).reactSuperview = self;
dispatch_async(dispatch_get_main_queue(), ^{
[self maybeAddToParentAndUpdateContainer];
});
// Child update operation is performed in the mountingTransactionDidMount method to prevent error, while
// navigating to `n` different screens at the same time.
Copy link
Member

Choose a reason for hiding this comment

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

I think this comment isn't very helpful outside the scope of this PR since the deleted lines won't be visible. WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

I just made it for the sake of questions like "why we cannot move this update method to the mount and unmount methods, if they're doing the same?" (especially, that removing this method won't be visible in git blame), but if you think this comment is unnecessary, I can remove it.

tboba marked this conversation as resolved.
Show resolved Hide resolved
}

- (void)unmountChildComponentView:(UIView<RCTComponentViewProtocol> *)childComponentView index:(NSInteger)index
Expand Down Expand Up @@ -1152,9 +1151,6 @@ - (void)unmountChildComponentView:(UIView<RCTComponentViewProtocol> *)childCompo
screenChildComponent.reactSuperview = nil;
[_reactSubviews removeObject:screenChildComponent];
[screenChildComponent removeFromSuperview];
dispatch_async(dispatch_get_main_queue(), ^{
[self maybeAddToParentAndUpdateContainer];
});
}

- (void)takeSnapshot
Expand All @@ -1178,6 +1174,24 @@ - (void)mountingTransactionWillMount:(react::MountingTransaction const &)transac
}
}

- (void)mountingTransactionDidMount:(const facebook::react::MountingTransaction &)transaction
withSurfaceTelemetry:(const facebook::react::SurfaceTelemetry &)surfaceTelemetry
{
for (auto &mutation : transaction.getMutations()) {
tboba marked this conversation as resolved.
Show resolved Hide resolved
if (mutation.parentShadowView.componentName != nil &&
strcmp(mutation.parentShadowView.componentName, "RNSScreenStack") == 0 &&
(mutation.type == react::ShadowViewMutation::Type::Update ||
mutation.type == react::ShadowViewMutation::Type::Remove)) {
// we need to wait until children have their layout set. At this point they don't have the layout
// set yet, however the layout call is already enqueued on ui thread. Enqueuing update call on the
// ui queue will guarantee that the update will run after layout.
dispatch_async(dispatch_get_main_queue(), ^{
[self maybeAddToParentAndUpdateContainer];
});
}
tboba marked this conversation as resolved.
Show resolved Hide resolved
}
}

- (void)prepareForRecycle
{
[super prepareForRecycle];
Expand Down
Loading