th3rdwave / react-native-safe-area-context

A flexible way to handle safe area insets in JS. Also works on Android and Web!
MIT License
2.14k stars 197 forks source link

[Android] Slow screen opening after react-native update #448

Open OrlovMaks opened 11 months ago

OrlovMaks commented 11 months ago

Hey, everybody! After updating react-native (and all libraries) from version 0.70.11 to 0.72.6, I got a problem that screens with SafeAreaView open slowly and without animation. I couldn't find absolutely no information on how to fix it.

In the video the first screen "Security" is in SafeAreaView, the second screen "Your rank" is just View. https://github.com/th3rdwave/react-native-safe-area-context/assets/93343100/45623a6f-a038-4372-837a-abfa7466b8e8

My App component is wrapped with (I tried with and without the initialMetrics parameter).

My package.json { "react": "18.2.0", "react-native": "0.72.6", "react-native-safe-area-context": "4.7.4", "react-native-screens": "3.27.0", "react-native-reanimated": "3.5.4", "@react-navigation/bottom-tabs": "6.5.11", "@react-navigation/native": "6.1.9", "@react-navigation/stack": "6.3.20",

efstathiosntonas commented 11 months ago

Maybe a long shot but, do you by any chance have reduce motion enabled on the device/simulator?

17Amir17 commented 11 months ago

I see this as-well something like a 500ms delay on rendering whatever is inside safe view RN 0.72.4 looks like only on Android - and Reduce Motion is not enabled My other App on RN 0.71.6 works fine

jacobp100 commented 11 months ago

There's not really anything to go off here. If you manage to make a minimal reproduction - or better yet, find the actual issue - let us know.

OrlovMaks commented 11 months ago

@jacobp100 Tomorrow I will make a copy of the application to the public repository and provide a link to it. Unfortunately, I myself could not solve this problem.

OrlovMaks commented 11 months ago

@jacobp100 @17Amir17 I prepared a repository with the project from which I removed everything except the structure of my navigation: https://github.com/OrlovMaks/sample_safeAreaView_issue

While I was preparing this project I found a problem (check the screenshot): I have detachInactiveScreens={false} everywhere and I make navigation from WalletStackNavigator to general StackNavigator if I remove detachInactiveScreens from this general stack everything works. https://github.com/th3rdwave/react-native-safe-area-context/assets/93343100/4225fb1f-275d-4252-aefc-ac4d0ad50316

jacobp100 commented 11 months ago

Right so is it a problem with this library or another?

17Amir17 commented 11 months ago

I have this issue with BottomSheetModal. I created https://github.com/17Amir17/SafeAreaContext to reproduce. From the comments in my codebase the reason we have a SafeArea in the modal is because of a rare cutoff of some android device.

https://github.com/th3rdwave/react-native-safe-area-context/assets/36531255/56a39589-92a9-4971-9a2b-d9967909dc22

I'm not really sure how to debug this further but would love to help @jacobp100 if you have any directions

mgerdes commented 11 months ago

@jacobp100 I'm seeing this issue too. It seems like it's because is we are hitting this timeout in SafeAreaView and that freezes the UI for 500ms

https://github.com/th3rdwave/react-native-safe-area-context/blob/b086b0ef5043836b72c6430cca1641641694fbce/android/src/main/java/com/th3rdwave/safeareacontext/SafeAreaView.kt#L97-L99

I debugged a little bit and it seems like the code in runOnNativeModulesQueueThread below is never able to acquire the lock, because the lock is being held the entire time we are waiting in the while loop right below, so it doesn't complete until the timeout ends.

https://github.com/th3rdwave/react-native-safe-area-context/blob/b086b0ef5043836b72c6430cca1641641694fbce/android/src/main/java/com/th3rdwave/safeareacontext/SafeAreaView.kt#L77-L95

I'm not sure what the best fix here is though.

jacobp100 commented 11 months ago

Good catch on that. If you're able to get some flame graph and figure out what exactly is blocking the main thread, that would be helpful. Is this something that shows up only in development - or production too?

mgerdes commented 11 months ago

Good catch on that. If you're able to get some flame graph and figure out what exactly is blocking the main thread, that would be helpful. Is this something that shows up only in development - or production too?

I think the waitForReactLayout in SafeAreaView.kt is what's blocking the main thread. I added these log statements to try to see what's happening

Screenshot 2023-11-01 at 1 18 43 PM

and this was the result:

2023-11-01 13:03:54.865 runOnNativeModulesQueueThread 0
2023-11-01 13:03:54.865 main thread 0
2023-11-01 13:03:54.866 awaitNanos 0
2023-11-01 13:03:55.367 awaitNanos 1
2023-11-01 13:03:55.367 main thread 1
2023-11-01 13:03:55.367 Timed out waiting for layout.
2023-11-01 13:03:55.377 native thread 0
2023-11-01 13:03:55.377 native thread 1

So if I'm looking at this right it seems like the code in runOnNativeModulesQueueThread that's supposed to run on the native thread isn't run while the main thread is blocked and that makes waitForReactLayout block the main thread the entire time.

jacobp100 commented 11 months ago

If you increase the max time nanos to something much longer (say 10s), does it finish before? Or do we have a deadlock?

mgerdes commented 11 months ago

If you increase the max time nanos to something much longer (say 10s), does it finish before? Or do we have a deadlock?

Yea, I just upped it to 10s and the whole app freezes for those 10 seconds it's waiting.

This is what I was logging:

2023-11-01 14:22:29.130  runOnNativeModulesQueueThread 0
2023-11-01 14:22:29.130  main thread 0
2023-11-01 14:22:29.130  awaitNanos 0
2023-11-01 14:22:39.131  awaitNanos 1
2023-11-01 14:22:39.131  main thread 1
2023-11-01 14:22:39.131  Timed out waiting for layout.
2023-11-01 14:22:39.140  native thread 0
2023-11-01 14:22:39.140  native thread 1

So yea, it looks like something isn't letting the code on the native thread run until the main thread times out waiting.

jacobp100 commented 11 months ago

What’s your setup? Version of react native? Are you using fabric?

Also could you check the value of getReactContext(this).isOnNativeModulesQueueThread() (at the start of the function - outside the runOnNativeModulesQueueThread)

mgerdes commented 11 months ago

We are on react-native 0.72.5. No we are not using fabric.

I printed the value of getReactContext(this).isOnNativeModulesQueueThread at the start of the function - outside of runOnNativeModulesQueueThread, and it returns false.

jacobp100 commented 11 months ago

Try adding a printStackTrace in the runOnNativeModulesQueueThread function in the react native library code and see if there’s something else that runs around that time. It sounds like this and another task are in deadlock

mgerdes commented 11 months ago

Here's what the stacktrace looks like in runOnNativeModulesQueueThread

java.lang.Exception: Stack trace
    at java.lang.Thread.dumpStack(Thread.java:1615)
    at com.th3rdwave.safeareacontext.SafeAreaView.waitForReactLayout(SafeAreaView.kt:71)
    at com.th3rdwave.safeareacontext.SafeAreaView.updateInsets(SafeAreaView.kt:52)
    at com.th3rdwave.safeareacontext.SafeAreaView.maybeUpdateInsets(SafeAreaView.kt:113)
    at com.th3rdwave.safeareacontext.SafeAreaView.onAttachedToWindow(SafeAreaView.kt:134)
    at android.view.View.dispatchAttachedToWindow(View.java:21357)
    at android.view.ViewGroup.dispatchAttachedToWindow(ViewGroup.java:3491)
    at android.view.ViewGroup.dispatchAttachedToWindow(ViewGroup.java:3498)
    at android.view.ViewGroup.dispatchAttachedToWindow(ViewGroup.java:3498)
    at android.view.ViewGroup.addViewInner(ViewGroup.java:5291)
    at android.view.ViewGroup.addView(ViewGroup.java:5077)
    at com.facebook.react.views.view.ReactViewGroup.addView(ReactViewGroup.java:516)
    at android.view.ViewGroup.addView(ViewGroup.java:5017)
    at com.facebook.react.uimanager.ViewGroupManager.addView(ViewGroupManager.java:38)
    at com.facebook.react.uimanager.NativeViewHierarchyManager.manageChildren(NativeViewHierarchyManager.java:533)
    at com.swmansion.reanimated.layoutReanimation.ReanimatedNativeHierarchyManager.manageChildren(ReanimatedNativeHierarchyManager.java:300)
    at com.facebook.react.uimanager.UIViewOperationQueue$ManageChildrenOperation.execute(UIViewOperationQueue.java:217)
    at com.facebook.react.uimanager.UIViewOperationQueue$1.run(UIViewOperationQueue.java:915)
    at com.facebook.react.uimanager.UIViewOperationQueue.flushPendingBatches(UIViewOperationQueue.java:1026)
    at com.facebook.react.uimanager.UIViewOperationQueue.-$$Nest$mflushPendingBatches(Unknown Source:0)
    at com.facebook.react.uimanager.UIViewOperationQueue$DispatchUIFrameCallback.doFrameGuarded(UIViewOperationQueue.java:1086)
    at com.facebook.react.uimanager.GuardedFrameCallback.doFrame(GuardedFrameCallback.java:29)
    at com.facebook.react.modules.core.ReactChoreographer$ReactChoreographerDispatcher.doFrame(ReactChoreographer.java:175)
    at com.facebook.react.modules.core.ChoreographerCompat$FrameCallback$1.doFrame(ChoreographerCompat.java:85)
    at android.view.Choreographer$CallbackRecord.run(Choreographer.java:1229)
    at android.view.Choreographer$CallbackRecord.run(Choreographer.java:1239)
    at android.view.Choreographer.doCallbacks(Choreographer.java:899)
    at android.view.Choreographer.doFrame(Choreographer.java:827)
    at android.view.Choreographer$FrameDisplayEventReceiver.run(Choreographer.java:1214)
    at android.os.Handler.handleCallback(Handler.java:942)
    at android.os.Handler.dispatchMessage(Handler.java:99)
    at android.os.Looper.loopOnce(Looper.java:201)
    at android.os.Looper.loop(Looper.java:288)
    at android.app.ActivityThread.main(ActivityThread.java:7918)
    at java.lang.reflect.Method.invoke(Native Method)
    at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:548)
    at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:936)
jacobp100 commented 11 months ago

Do you have a few traces before and after that?

mgerdes commented 11 months ago

I'm not sure what you mean? Before and after it's inside runOnNativeModulesQueueThread? Or still inside of runOnNativeModulesQueueThread?

jacobp100 commented 11 months ago

I think some other functions - presumably outside of this library - will be calling runOnNativeModulesQueueThread before or after this call. If there are, what are they?

smokfyz commented 11 months ago

Hi. I got the same issue on iOS. Temporary fixed it using useSafeAreaInsets and paddings instead of SafeAreaView.

enahum commented 10 months ago

facing the same problem after updating to RN 0.72+ including 0.73-alpha(s).. this is in combination with react-native-navigation from wix.

danilobuerger commented 9 months ago

Experiencing the same issue after upgrading to RN 0.72 (from 0.68). @jacobp100 is there anything I can do to help you debug this?

jacobp100 commented 9 months ago

Somebody needs to look into it. There’s only two people that actively maintain this, and I don’t think either of us will investigate this - so if you want this fixed, do take a look for yourself. We are of course on hand to answer any questions etc.

danilobuerger commented 9 months ago

So what I figured out is that

uiManager.uiImplementation.dispatchViewUpdates(-1)

scheduled on the native module queue is called after the lock times out. I would assume that this means that waitForReactLayout blocks the execution of anything on the native modules queue.

danilobuerger commented 9 months ago

@jacobp100 I found the offending code 🥳

In SafeAreaView we call

uiManager.setViewLocalData(id, localData)

before

waitForReactLayout()

This runs

mUIImplementation.setViewLocalData(tag, data);

on the native modules thread.

This in turn calls

dispatchViewUpdatesIfNeeded() -> dispatchViewUpdates()

Since rn 72 (https://github.com/facebook/react-native/commit/bc766ec7f8b18ddc0ff72a2fff5783eeeff24857) we have

if (getRootViewNum() <= 0) {
  // If there are no RootViews registered, there will be no View updates to dispatch.
  // This is a hack to prevent this from being called when Fabric is used everywhere.
  // This should no longer be necessary in Bridgeless Mode.
  return;
}

guarding the entry and this in turn waits for the main thread where we are already waiting for this thread. Deadlock.

jacobp100 commented 9 months ago

🎉 do you know what the fix is?

danilobuerger commented 9 months ago

Sadly no idea yet. The problem is that waitForReactLayout is called inside NativeViewHierarchyManager.manageChildren (see stack trace above) and that synchronizes on the same object as getRootViewNum so it will always deadlock. Maybe @feiyin0719 or @kelset from the original PR introducing this know more 🤷‍♂️

jacobp100 commented 9 months ago

My experience with core dev is they’re too busy to get back to you. If you find a work around though, we here are good at merging and releasing PRs. It sounds like you’re really close!

jacobp100 commented 9 months ago

https://github.com/th3rdwave/react-native-safe-area-context/blob/f7e7e20b930c1e414ea34ede5d7aa1c406edc36a/android/src/main/java/com/th3rdwave/safeareacontext/SafeAreaView.kt#L53-L60

Sadly there doesn't seem to be a way to properly dirty a yoga node from java

I'm not certain this is the case - see this:

https://github.com/facebook/react-native/blob/b5e08e80d90b6d03d1f49f0674c01f03ee300c46/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactTextShadowNode.java#L318-L319C14

I think it has to happen in the shadow node, so might need a slight refactor. But maybe this could avoid the deadlock?

feiyin0719 commented 9 months ago

@jacobp100 I found the offending code 🥳

In SafeAreaView we call

uiManager.setViewLocalData(id, localData)

before

waitForReactLayout()

This runs

mUIImplementation.setViewLocalData(tag, data);

on the native modules thread.

This in turn calls

dispatchViewUpdatesIfNeeded() -> dispatchViewUpdates()

Since rn 72 (facebook/react-native@bc766ec) we have

if (getRootViewNum() <= 0) {
  // If there are no RootViews registered, there will be no View updates to dispatch.
  // This is a hack to prevent this from being called when Fabric is used everywhere.
  // This should no longer be necessary in Bridgeless Mode.
  return;
}

guarding the entry and this in turn waits for the main thread where we are already waiting for this thread. Deadlock.

maybe we can move getRootViewNum to UIManager.onBatchComplete to let it run as before

danilobuerger commented 9 months ago

@feiyin0719 Thanks for getting back to me! I just tested your suggestion and it does work in this case, we no longer deadlock. I am unsure if it still fixes your memory leak. Should I push a branch so you can validate?

kelset commented 9 months ago

hey folks - I'm currently not on the release crew and just got back from my holiday break; do you still need me to help somehow? Looks like a commit cherry picked in a 0.72 patch was the reason why this problem started happening, am I following correctly?

Svarto commented 8 months ago

@danilobuerger I have the same issue in my app, so a fix would be fantastic. Not sure what about the memory leak problem you refer to - can't find that discussed yet in this thread...

jacobp100 commented 8 months ago

@kelset yes, but I don't have enough context about the change

kelset commented 8 months ago

if that's the case I'd suggest to flag up this problem in the latest open discussion on 0.72 patch and ask for the cherry pick to be reverted: https://github.com/reactwg/react-native-releases/discussions/97

Bowlerr commented 8 months ago

Hey, is there any patch for this temporarily?

vlanemcev commented 8 months ago

@danilobuerger Hi!

Were you able to fix the issue by making some patch-package or anything else?

vlanemcev commented 8 months ago

To everyone who is needed temporary fix. (@enahum @OrlovMaks )

In our project we have an version for IOS with SafeAreaView usage:

import React, { memo, useMemo } from 'react';
import {
  Edge,
  NativeSafeAreaViewProps,
  SafeAreaView as SafeAreaContextView,
} from 'react-native-safe-area-context';
import { useHeaderHeight } from '@react-navigation/elements';

import { commonFlex } from 'src/app/theme/constants/commonStyles';

import { useIsNetworkConnected } from 'src/shared/components/NetworkConnection';
import { isProduction } from 'src/shared/constants/environment';

type SafeAreaViewProps = {
  /**
   * Should we add top inset which will be respect Header height
   * Read more about this specific behaviour:
   * https://reactnavigation.org/docs/nesting-navigators#parent-navigators-ui-is-rendered-on-top-of-child-navigator
   * https://reactnavigation.org/docs/elements#headertransparent
   */
  shouldAddHeaderHeightInset?: boolean;
} & NativeSafeAreaViewProps;

const SafeAreaView = ({
  shouldAddHeaderHeightInset,
  edges = [],
  style,
  children,
  ...rest
}: SafeAreaViewProps) => {
  const parentScreenHeaderHeight = useHeaderHeight();

  const isNetworkConnected = useIsNetworkConnected();

  const isAnyAppStatuesShown = !isProduction || !isNetworkConnected;

  // Remove `bottom` edge from SafeAreaContextView if AppMetaInfoBar or NetworkConnectionBar is displayed.
  const computedEdges = useMemo(
    () =>
      (edges as Edge[]).filter((edge) => {
        return isAnyAppStatuesShown ? edge !== 'bottom' : edge;
      }),
    [edges, isAnyAppStatuesShown]
  );

  return (
    <SafeAreaContextView
      edges={['left', 'right', ...computedEdges]}
      style={[
        commonFlex.flexContainer,
        shouldAddHeaderHeightInset
          ? { paddingTop: parentScreenHeaderHeight }
          : {},
        style,
      ]}
      {...rest}
    >
      {children}
    </SafeAreaContextView>
  );
};

export default memo(SafeAreaView);

and dedicated version for Android (to mitigate this issue), which useSafeAreaInsets usage:

import React, { memo, useMemo } from 'react';
import { View, ViewProps } from 'react-native';
import { useSafeAreaInsets } from 'react-native-safe-area-context';
import { useHeaderHeight } from '@react-navigation/elements';

import { commonFlex } from 'src/app/theme/constants/commonStyles';

import { useIsNetworkConnected } from 'src/shared/components/NetworkConnection';
import { isProduction } from 'src/shared/constants/environment';

// TODO: watch this issue: https://github.com/th3rdwave/react-native-safe-area-context/issues/448
// to come back to the previous realisation of SafeAreaView (as for IOS)

interface SafeAreaViewProps extends ViewProps {
  edges?: ('top' | 'right' | 'bottom' | 'left')[]; // edges as an array of strings
  children?: React.ReactNode;

  /**
   * Should we add top inset which will be respect Header height
   * Read more about this specific behaviour:
   * https://reactnavigation.org/docs/nesting-navigators#parent-navigators-ui-is-rendered-on-top-of-child-navigator
   * https://reactnavigation.org/docs/elements#headertransparent
   */
  shouldAddHeaderHeightInset?: boolean;
}

const SafeAreaView = ({
  shouldAddHeaderHeightInset,
  edges = [],
  style,
  children,
  ...rest
}: SafeAreaViewProps) => {
  const safeAreaInsets = useSafeAreaInsets();
  const parentScreenHeaderHeight = useHeaderHeight();

  const isNetworkConnected = useIsNetworkConnected();
  const isAnyAppStatuesShown = !isProduction || !isNetworkConnected;

  const computedEdges = useMemo(() => {
    // Include 'left' and 'right' by default
    let finalEdges = ['left', 'right', ...edges];

    if (isAnyAppStatuesShown) {
      // Remove `bottom` edge from SafeAreaContextView if AppMetaInfoBar or NetworkConnectionBar is displayed.

      finalEdges = finalEdges.filter((edge) => edge !== 'bottom');
    }

    return finalEdges;
  }, [edges, isAnyAppStatuesShown]);

  const computedPadding = useMemo<{
    paddingTop?: number;
    paddingBottom?: number;
    paddingLeft?: number;
    paddingRight?: number;
  }>(() => {
    let padding = {};

    if (computedEdges.includes('top')) {
      padding = { ...padding, paddingTop: safeAreaInsets.top };
    }

    if (computedEdges.includes('bottom')) {
      padding = { ...padding, paddingBottom: safeAreaInsets.bottom };
    }

    if (computedEdges.includes('left')) {
      padding = { ...padding, paddingLeft: safeAreaInsets.left };
    }

    if (computedEdges.includes('right')) {
      padding = { ...padding, paddingRight: safeAreaInsets.right };
    }

    return padding;
  }, [computedEdges, safeAreaInsets]);

  // Calculate the total top padding
  const topPadding = shouldAddHeaderHeightInset
    ? (computedPadding?.paddingTop ?? 0) + parentScreenHeaderHeight
    : computedPadding?.paddingTop;

  return (
    <View
      style={[
        commonFlex.flexContainer,
        { ...computedPadding, paddingTop: topPadding },
        style,
      ]}
      {...rest}
    >
      {children}
    </View>
  );
};

export default memo(SafeAreaView);
chaadow commented 8 months ago

I think I found a universal solution ( and it worked for my case) . I followed this documentation and it recommends to use useSafeAreaInsets instead of SafeAreaView.

image

All I did was apply the recommandation ( by using a regular View with paddings applied and no more jumpy or freezing for me!

Here is an example

function Demo() {
  const insets = useSafeAreaInsets();

  return (
    <View
      style={{
        // Paddings to handle safe area
        paddingTop: insets.top,
        paddingBottom: insets.bottom,
        paddingLeft: insets.left,
        paddingRight: insets.right,
      }}
    >
      <Text>Some text.</Text>
    </View>
  );
}
yayvery commented 7 months ago

The previous two "fixes" suggesting to use useSafeAreaInsets instead of SafeAreaView are missing the point of why SafeAreaView exists.

chaadow commented 7 months ago

@yayvery I'm simply following the official documentation recommendation and it did fix the issue for me ( I've linked it + provided a screenshot of the relevant section)

If we're both missing the point, please enlighten us all as well as the official maintainers of the package how to use SafeAreaView in this context.

yayvery commented 7 months ago

If it works for you that's great, but it's misleading to call it a "universal solution" when it doesn't at all address the actual issue that has been identified (https://github.com/th3rdwave/react-native-safe-area-context/issues/448#issuecomment-1871155936)

The point of SafeAreaView is clearly explained in the README.

EvertEt commented 6 months ago

@feiyin0719 Thanks for getting back to me! I just tested your suggestion and it does work in this case, we no longer deadlock. I am unsure if it still fixes your memory leak. Should I push a branch so you can validate?

@feiyin0719 Is there any way we can support testing this change to get it merged back? See https://github.com/discord/react-native/commit/5c0d4da399050da583cd31bf2620fea9a2a30cf1 for a version of the proposed change.

It looks like this issue affects several apps. Thanks in advance

patrickkempff commented 6 months ago

We have the same problem, we are trying to apply the patch from https://github.com/discord/react-native/commit/5c0d4da399050da583cd31bf2620fea9a2a30cf1 to see if it fixes our issue.

vanHoi commented 6 months ago

We also have this issue in our app. When applying the patch from discord/react-native@5c0d4da and building React Native from source, it fixes the issue.

yayvery commented 6 months ago

thanks for confirming it fixes the issue @vanHoi ! I'll try to get this reviewed & merged into react-native (need to figure out the CLA situation tho)

EvertEt commented 6 months ago

Thanks @yayvery , let us know if you need help or want someone else to open the PR.

It would be great to ask (like @kelset suggested) to have this fix cherry picked to 0.72.

vanHoi commented 6 months ago

Thanks @yayvery for the solution! Just to clarify, we have applied this patch to React Native 0.73.6, the issues still persists there.

EvertEt commented 6 months ago

I can also confirm the patch works on 0.72.12.

jackylu0124 commented 6 months ago

@yayvery @EvertEt @vanHoi hey all, I have a quick question, is this fix incorporated into the React Native 0.72.12 version? If not, is there a plan to incorporate this fix into the next 0.72 patch version? Thanks a lot in advance!

I think this issue (https://github.com/th3rdwave/react-native-safe-area-context/issues/219#issuecomment-1913104995) is also related to one discussed above.