UnsupportedOperationException at iterator.remove() after change in SentryOptions.java

[sdeff]

The version of sentry-android:
2.1.0 and above

---

Hi,

in our RN-Android/iOS project we initialize both, the Sentry Android and the Sentry RN SDK.

As we want to track Android Crashes with the Android SDK, we disable native crash handling in React Native:

Sentry.init({
  ...
  enableNativeCrashHandling: false,
  ...
});

With the following change in Version 2.1.0 inside SentryOptions.java

(Before 2.1.0)

private final @NotNull List<EventProcessor> eventProcessors = new ArrayList<>();

->

(After 2.1.0)

private final @NotNull List<EventProcessor> eventProcessors = new CopyOnWriteArrayList<>();

the RNSentryModule.java crashes at:

for (Iterator<Integration> iterator = options.getIntegrations().iterator(); iterator.hasNext(); ) {
            Integration integration = iterator.next();
                if (rnOptions.hasKey("enableNativeCrashHandling") &&
                        !rnOptions.getBoolean("enableNativeCrashHandling")) {
                    if (integration instanceof UncaughtExceptionHandlerIntegration ||
                            integration instanceof AnrIntegration ||
                            integration instanceof NdkIntegration) {
                            iterator.remove();
                    }
                }
}

because iterator.remove() is an unsupported operation on CopyOnWriteArrayList.

Best regards,
Steffen

[marandaneto]

hey @sdeff thanks for raising this issue.

       final List<Integration> integrations = options.getIntegrations();
       for (final Integration integration : integrations) {
         if (integration instanceof UncaughtExceptionHandlerIntegration ||
           integration instanceof AnrIntegration ||
           integration instanceof NdkIntegration) {
           integrations.remove(integration);
         }
       }

woud solve the issue.

[bruno-garcia]

Thanks for raising this and @marandaneto for pointing out the fix.

One question though. are you using enableNative:

  /**
   * Enables native transport + device info + offline caching.
   * Be careful, disabling this also breaks automatic release setting.
   * This means you have to manage setting the release yourself.
   * Defaults to `true`.
   */
  enableNative?: boolean;

[sdeff]

Thanks for your quick response! Yes, we've set enableNative to true, is this a problem?

[sdeff]

Independently of this bug, I wonder if removing the integrations with the sentry-react-native SDK like this:

       final List<Integration> integrations = options.getIntegrations();
       for (final Integration integration : integrations) {
         if (integration instanceof UncaughtExceptionHandlerIntegration ||
           integration instanceof AnrIntegration ||
           integration instanceof NdkIntegration) {
           integrations.remove(integration);
         }
       }

Will also remove the UncaughtExceptionHandlerIntegration from the previously initialized sentry-android SDK and therefore result in no native crash handling at all?

Because right now with enableNativeCrashHandling: true we have the problem that we receive two crash events for each native crash (one from each sentry-android and sentry-react-native SDK).

[marandaneto]

hey @sdeff so sentry-react-native already bundles sentry-android underneath and all you need is to set enableNativeCrashHandling=true

you don't need to init. sentry-android manually or even to bother removing/adding integrations, I guess I've missed this detail while reading this issue for the first time.

actually, you can't init sentry-android manually as sentry-react-native already did it automatically, initing sentry-android` 2 times goes against our guidelines and it'd behave badly.

I hope this clarifies your question :)

[marandaneto]

we have the problem that we receive two crash events for each native crash (one from each sentry-android and sentry-react-native SDK).

this is odd, can you elaborate a bit more on that or even raising a new issue? not sure if it's your setup or a bug.

[sdeff]

Thanks again for your quick reply @marandaneto!

this is odd, can you elaborate a bit more on that or even raising a new issue? not sure if it's your setup or a bug.

I guess it's our setup which initializes both, the sentry-android and the sentry-react-native SDK.

actually, you can't init sentry-android manually as sentry-react-native already did it automatically, initing sentry-android` 2 times goes against our guidelines and it'd behave badly.

I understand that, but what if I want to receive crash events for (native Android) crashes which could happen before the RN initialization?

Wouldn't that result in a "blind spot" where our app could crash without us even noticing?

[marandaneto]

@sdeff that's a very valid point, but it really depends on the order of initing your dependencies, basically, sentry-react-native should be the 1st one.

in order to init the sentry-android SDK, you need a Context, and to get a Context on a react-native App, you need to extend ReactContextBaseJavaModule and at this point, the react-native FW is already in place, so a blind spot indeed exists, we can only try to reduce this to a minimum, initing sentry-react-native and sentry-android synchronously in the main thread.

There might be still improvements to be made, but this gap is known :(
If you don't mind, raise a new issue for that, if this gets more attraction, we'd definitely look into other approaches.

[marandaneto]

https://github.com/getsentry/sentry-react-native/releases/tag/1.4.0