maps icon indicating copy to clipboard operation
maps copied to clipboard

[Bug]: Exception in ViewTagResolver after app reloading

Open orca-nazar opened this issue 2 years ago • 23 comments

Mapbox Implementation

Mapbox

Mapbox Version

10.16.2

Platform

Android

@rnmapbox/maps version

#main

Standalone component to reproduce

// I could not reproduce it in the Example app 
export default () => {};

Observed behavior and steps to reproduce

The issue occurs after hot reloading or reloading via terminal in debug mode. To avoid it only helps full(force) closing the app

Expected behavior

No issues

Notes / preliminary analysis

Looks like making the manager optional and adding guards when extracting it, solves the problem

private val manager : UIManager?
        get() =
            if (BuildConfig.IS_NEW_ARCHITECTURE_ENABLED) {
                UIManagerHelper.getUIManager(context, UIManagerType.FABRIC)
            } else {
                UIManagerHelper.getUIManager(context, UIManagerType.DEFAULT)
            }

Additional links and references

screencap-2023-12-06T073217 785Z

orca-nazar avatar Dec 06 '23 08:12 orca-nazar

Looks like making the manager optional and adding guards when extracting it, solves the problem

Not solves just hides the issue. Without view tag resolver we will not be able to call methods on the components like MapView#getCenter.

mfazekas avatar Dec 06 '23 11:12 mfazekas

Looks like making the manager optional and adding guards when extracting it, solves the problem

Not solves just hides the issue. Without view tag resolver we will not be able to call methods on the components like MapView#getCenter.

Yes, I see. Maybe there is a way to avoid red screens, when exception occurs. I guess it should be handled by try/catch block, right? @mfazekas

orca-nazar avatar Dec 06 '23 11:12 orca-nazar

@mfazekas If we move out viewWaiters from catch and handle it in try block. Do you think it might work for all cases?

    fun <V>withViewResolved(viewTag: Int, reject: Promise? = null, fn: (V) -> Unit) {
        context.runOnUiQueueThread() {
            try {
                val view = manager?.resolveView(viewTag)

                if (view == null && !createdViews.contains(viewTag)) {
                    viewWaiters.getOrPut(viewTag) { mutableListOf<ViewTagWaiter<View>>() }.add(ViewTagWaiter<View>({ view -> fn(view as V) }, reject))
                } else if (view == null && createdViews.contains(viewTag)) {
                    reject?.reject(Exception("No view"))
                } else {
                    fn(view as V)
                }
            } catch (err: IllegalViewOperationException) {
                if (!createdViews.contains(viewTag)) {
                    viewWaiters.getOrPut(viewTag) { mutableListOf<ViewTagWaiter<View>>() }.add(ViewTagWaiter<View>({ view -> fn(view as V) }, reject))
                } else {
                    reject?.reject(err)
                }
            }
        }
    }

Updated: I see, that catch should be handled in an original way as IllegalViewOperationException still occurs

orca-nazar avatar Dec 06 '23 15:12 orca-nazar

So the crash is coming from

                UIManagerHelper.getUIManager(context, UIManagerType.DEFAULT)!!

And we need that UI manager for finding the native views for viewTag. (JS side passes methods with viewTags and we need to find the native view using the viewTag.)

Does it say anything int the logcat? Which RN version do you use?

mfazekas avatar Dec 06 '23 18:12 mfazekas

RN: 0.71.8 On initial loading UI manager is available, however IllegalViewOperationException is thrown (it's covered by catch block). After reloading either via metro bundler or RN context menu, the manager becomes null and createdViews are not empty. So it goes to this condition: reject?.reject(Exception("No view")). Actually, it should be another message here

orca-nazar avatar Dec 07 '23 18:12 orca-nazar

IIRC for some reason after refresh, the catalyst instance is not present and this check fails: https://github.com/facebook/react-native/blob/31005b7fd9a9579920b55f921c477a90fb3f6d10/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/bridge/ReactContext.java#L240 when calling UIManagerHelper.getUIManager(context, UIManagerType.FABRIC)!! (at least on new arch). It would be nice to find out why it happens and if we are doing something wrong.

WoLewicki avatar Dec 08 '23 11:12 WoLewicki

@WoLewicki do you see it on new arch? In 0.73?

mfazekas avatar Dec 08 '23 12:12 mfazekas

Yes

WoLewicki avatar Dec 08 '23 13:12 WoLewicki

Yep I see it on our example app. But for me react-navigation also doesn't works after a refresh. Does refresh works if @rnmapbox/maps not installed?

@orca-nazar do you also have the new arch enabled?

2023-12-08 17:35:28.365 19202-19202 unknown:Vi...rtyUpdater com.rnmapboxglexample                W  Could not find generated setter for class com.facebook.react.uimanager.RootViewManager
2023-12-08 17:35:28.370 19202-19202 unknown:co...agerHelper com.rnmapboxglexample                E  Unhandled SoftException
                                                                                                    com.facebook.react.bridge.ReactNoCrashSoftException: Cannot get UIManager because the context doesn't contain an active CatalystInstance.
                                                                                                    	at com.facebook.react.uimanager.UIManagerHelper.getUIManager(UIManagerHelper.java:77)
                                                                                                    	at com.facebook.react.uimanager.UIManagerHelper.getEventDispatcher(UIManagerHelper.java:128)
                                                                                                    	at com.facebook.react.uimanager.UIManagerHelper.getEventDispatcherForReactTag(UIManagerHelper.java:106)
                                                                                                    	at com.th3rdwave.safeareacontext.SafeAreaProviderManagerKt.handleOnInsetsChange(SafeAreaProviderManager.kt:39)
                                                                                                    	at com.th3rdwave.safeareacontext.SafeAreaProviderManagerKt.access$handleOnInsetsChange(SafeAreaProviderManager.kt:1)
                                                                                                    	at com.th3rdwave.safeareacontext.SafeAreaProviderManager$addEventEmitters$1.invoke(SafeAreaProviderManager.kt:28)
                                                                                                    	at com.th3rdwave.safeareacontext.SafeAreaProviderManager$addEventEmitters$1.invoke(SafeAreaProviderManager.kt:28)
                                                                                                    	at com.th3rdwave.safeareacontext.SafeAreaProvider.maybeUpdateInsets(SafeAreaProvider.kt:21)
                                                                                        

mfazekas avatar Dec 08 '23 16:12 mfazekas

Nope, the error happens here UIManagerHelper.getUIManager(context, UIManagerType.DEFAULT)

orca-nazar avatar Dec 08 '23 20:12 orca-nazar

FWIW For the Fabric issues on reload went away for me since I've update reps in our example app. (I've upgraded react-native-safe-area-context and react-native-screens and removed react-native-svg https://github.com/rnmapbox/maps/pull/3257/files) But I'm not 100% if that this is the solution of the issue for the fabric case.

mfazekas avatar Dec 09 '23 20:12 mfazekas

Hmm I am afraid it is something we would want to fix upstream in the RN core, since we use methods from the UIManagerHelper and they fail. cc @cortinico are you aware of such issue, the catalyst instance being not present after resfresh of the app? Or maybe we are using it in the wrong way?

WoLewicki avatar Dec 11 '23 15:12 WoLewicki

cc @cortinico are you aware of such issue, the catalyst instance being not present after resfresh of the app? Or maybe we are using it in the wrong way?

Sorry but it's really hard to say as I lack context on what is the issue

cortinico avatar Dec 12 '23 16:12 cortinico

@mfazekas could we make a simple example where the bug is easily reproducible?

WoLewicki avatar Dec 12 '23 16:12 WoLewicki

I'm seeing this as well. First load works OK, but if you put the app into the background and then bring it back active I get an Exception in Native call from JS in ViewTagResolver.kt:51.

For info, I am NOT using new arch and I see this on both Mapbox 10 and 11.

"react-native": "^0.73.1",
"@rnmapbox/maps": "^10.1.3",
"react-native-safe-area-context": "^4.8.2",
"react-native-screens": "^3.29.0",

Screenshot 2023-12-24 at 12 25 56

dpyeates avatar Dec 24 '23 12:12 dpyeates

Any update on this? I know its a quiet time of year with Christmas and New Year but I'm getting multiple daily emails from users saying my application runs fine the first time after installation but every subsequent start the app crashes straight away.

Big thanks to all and Happy New Year everyone.

dpyeates avatar Dec 28 '23 17:12 dpyeates

@dpyeates no I had not looked into this issue nor do I plan to. I cannot reproduce the issue in development (with or without fabric), so it would be very hard for me to do so.

What I suggest you to do. Assumption: the reload in debug issue relates to production, and you can repro the issue in your debug build. 1.) Simplify your MapView usage to see if you can still repro the issue (ideally you just replace your App.js with a simple <MapView component>) 2.) Try to reproduce the issue on a blank rn project (created with npx [email protected] init AwesomeProject --version 0.71.8 If you cannot, then add packages you're using to the blank project.

mfazekas avatar Dec 28 '23 18:12 mfazekas