Android crash - @ReactMethod setup - promise.resolve(this)

[crherman7]

analytics-react-native/packages/core/android/src/main/java/com/segment/analytics/reactnative/core/RNAnalyticsModule.kt

Line 115 in acd0f2f

return promise.resolve(this)

If a case scenario presents itself where the code hits this conditional the promise.resolve(this) causes the following crash:

E/unknown:ReactNative: Exception in native call
    java.lang.RuntimeException: Cannot convert argument of type class com.segment.analytics.reactnative.core.RNAnalyticsModule
        at com.facebook.react.bridge.Arguments.fromJavaArgs(Arguments.java:181)
        at com.facebook.react.bridge.CallbackImpl.invoke(CallbackImpl.java:30)
        at com.facebook.react.bridge.PromiseImpl.resolve(PromiseImpl.java:55)
        at com.segment.analytics.reactnative.core.RNAnalyticsModule.setup(RNAnalyticsModule.kt:116)
        at java.lang.reflect.Method.invoke(Native Method)
        at com.facebook.react.bridge.JavaMethodWrapper.invoke(JavaMethodWrapper.java:371)
        at com.facebook.react.bridge.JavaModuleWrapper.invoke(JavaModuleWrapper.java:150)
        at com.facebook.react.bridge.queue.NativeRunnable.run(Native Method)
        at android.os.Handler.handleCallback(Handler.java:789)
        at android.os.Handler.dispatchMessage(Handler.java:98)
        at com.facebook.react.bridge.queue.MessageQueueThreadHandler.dispatchMessage(MessageQueueThreadHandler.java:26)
        at android.os.Looper.loop(Looper.java:164)
        at com.facebook.react.bridge.queue.MessageQueueThreadImpl$4.run(MessageQueueThreadImpl.java:225)
        at java.lang.Thread.run(Thread.java:764)

I don't believe it should be resolving the class instance this - as this is not readable by the react-native bridge. Any idea why it would be resolving promise.resolve(this)?

[efstathiosntonas]

any updates on this one?

[crherman7]

@efstathiosntonas I haven't see anything updated or heard anything from their team on this. I ended up just creating a patch script that changed this to promise.resolve(null) as resolving a class instance across the bridge is not appropriate. IMO after looking through some of their PR merges they are not being meticulous nor testing these changes.

I'm not sure the use case of what they are trying to resolve, but I can surely put up a PR for this if the team believes this should be promise.resolve(null).

[efstathiosntonas]

thanks for sharing the patch, it seems that when recordScreenViews: false the error gets raised. If this is true then the issue goes away

[crherman7]

@efstathiosntonas oh nice find, that aligns with my implementation as we have our set to false! If their team doesn't respond soon maybe I will just throw up a PR to see what they say since they aren't responding here.

[alanjcharles]

Hello @crherman7 and @efstathiosntonas apologies for the delay on our end. I'm looking into this, can I ask what version you are on? Did the issue present itself when you updated versions? Any additional context will be super helpful as well. Thanks!

[crherman7]

@alanjcharles this is currently on master,

analytics-react-native/packages/core/android/src/main/java/com/segment/analytics/reactnative/core/RNAnalyticsModule.kt

Line 115 in 226a9e4

return promise.resolve(this)

, so I presume the latest. I can't remember the context of when it started breaking as I opened this issue in September. But you cannot resolve a class instance over the react-native bridge. So the only thing you have to do is understand what exactly is needed to be resolved, other than the class instance. I'm not sure what the debug conditional is necessary for.

[alanjcharles]

@crherman7 I ask because we have released new versions since September. https://github.com/segmentio/analytics-react-native/blob/master/CHANGELOG.md#132-2020-10-23

It's more efficient if we can replicate your setup as closely as possible when creating new bug requests for our engineering team, even if it's not relevant (or unchanged in new versions). Not including the relevant versions will only delay the process further here. Thanks for understanding.

[crherman7]

This is currently on master and 1.3.2.

if (BuildConfig.DEBUG) {
  return promise.resolve(this)
}

[crherman7]

Here is the PR it was introduced in: #162 with an active bug because of a misspelling. Here: #168 the misspelling was fixed. Whether that debug conditional is necessary or not, I have not dug in deep enough to know - but this is besides the point that you cannot resolve a promise over the bridge; rn bridge are reserved for primitive json types.

[alanjcharles]

@crherman7 Thank you. Apologies for any confusion. I am asking which version you are currently using and which version you were on when it was introduced. If you have updated since September, that's great. I understand that it is still present in 1.3.2 and master, but it is important for our team to understand which version(s) customers are experiencing this issue so we can understand which versions are affected and which customers might be potentially impacted. Since new versions have been released since you created this issue, it's important to make sure everyone is on the same page to make this as efficient as possible moving forward. I think I have what I need. I'll let you know when I have an update for you.

[crherman7]

@alanjcharles yeah makes sense - we have had other issues because of unpinned podfiles so we ended up just going back down to 1.2.1 but this specific issue would still reside in that version. Wether or not it's crashing someone's app is one issue but it's overall a code-quality issue that should be fixed in-line with the next release. It's not blocking as anybody can patch a file in node_modules but these patches are always hacky-scripts that are not dependable.

[bsneed]

The underlying issue is multiple instances of a singleton are being created. I'm not sure what the original author intended with the debug check, but this library doesn't support multiple instances of analytics so I'd investigate on the client side why multiple instances are being instantiated.

[crherman7]

@bsneed whether or not the multiple instances of a singleton instance are being created which would be a complete anti-pattern - the core issue here that is directly crashing the app is very clear. Is this being fixed?

[bsneed]

I'm not sure what you're saying. Yes, instantiating a singleton multiple times is an anti-pattern I guess, so stop doing it, or we need to know the use case that makes this necessary. I already changed it to null to prevent the crash in debug mode, but I don't know that it gets anyone much.