mParticle / react-native-mparticle

React Native module for mParticle
https://www.mparticle.com
Other
13 stars 27 forks source link

logEvent calls crash because of update in mparticle-android-sdk #10

Closed jordanhaven closed 4 years ago

jordanhaven commented 4 years ago

The dependency on the android SDK is currently set to com.mparticle:android-core:[5.9.3, ).

Version 5.10.2 of that SDK was just released on 10/7, and includes (apparently) breaking changes to (at least) the logEvent method (see: https://github.com/mParticle/mparticle-android-sdk/commit/74a34d7c1bd52e01f3bee68903de4c42c6450c39#diff-402a8f1a5d5e1043d9b3f6c5b83af0c5, lines 346-394)

Any fresh builds using this plugin will crash if you try to log an event.

Sample stacktrace:

2019-10-08 16:20:57.182 20665-20975/[my redacted package name] E/unknown:ReactNative: Exception in native call
    java.lang.RuntimeException: Could not invoke MParticle.logEvent
        at com.facebook.react.bridge.JavaMethodWrapper.invoke(JavaMethodWrapper.java:383)
        at com.facebook.react.bridge.JavaModuleWrapper.invoke(JavaModuleWrapper.java:158)
        at com.facebook.react.bridge.queue.NativeRunnable.run(Native Method)
        at android.os.Handler.handleCallback(Handler.java:751)
        at android.os.Handler.dispatchMessage(Handler.java:95)
        at com.facebook.react.bridge.queue.MessageQueueThreadHandler.dispatchMessage(MessageQueueThreadHandler.java:29)
        at android.os.Looper.loop(Looper.java:154)
        at com.facebook.react.bridge.queue.MessageQueueThreadImpl$4.run(MessageQueueThreadImpl.java:232)
        at java.lang.Thread.run(Thread.java:762)
     Caused by: java.lang.reflect.InvocationTargetException
        at java.lang.reflect.Method.invoke(Native Method)
        at com.facebook.react.bridge.JavaMethodWrapper.invoke(JavaMethodWrapper.java:372)
        at com.facebook.react.bridge.JavaModuleWrapper.invoke(JavaModuleWrapper.java:158) 
        at com.facebook.react.bridge.queue.NativeRunnable.run(Native Method) 
        at android.os.Handler.handleCallback(Handler.java:751) 
        at android.os.Handler.dispatchMessage(Handler.java:95) 
        at com.facebook.react.bridge.queue.MessageQueueThreadHandler.dispatchMessage(MessageQueueThreadHandler.java:29) 
        at android.os.Looper.loop(Looper.java:154) 
        at com.facebook.react.bridge.queue.MessageQueueThreadImpl$4.run(MessageQueueThreadImpl.java:232) 
        at java.lang.Thread.run(Thread.java:762) 
     Caused by: java.lang.NoSuchMethodError: No virtual method logEvent(Lcom/mparticle/BaseEvent;)V in class Lcom/mparticle/MParticle; or its super classes (declaration of 'com.mparticle.MParticle' appears in /data/app/[my redacted package name]-2/base.apk:classes5.dex)
        at com.mparticle.react.MParticleModule.logEvent(MParticleModule.java:62)
        at java.lang.reflect.Method.invoke(Native Method) 
        at com.facebook.react.bridge.JavaMethodWrapper.invoke(JavaMethodWrapper.java:372) 
        at com.facebook.react.bridge.JavaModuleWrapper.invoke(JavaModuleWrapper.java:158) 
        at com.facebook.react.bridge.queue.NativeRunnable.run(Native Method) 
        at android.os.Handler.handleCallback(Handler.java:751) 
        at android.os.Handler.dispatchMessage(Handler.java:95) 
        at com.facebook.react.bridge.queue.MessageQueueThreadHandler.dispatchMessage(MessageQueueThreadHandler.java:29) 
        at android.os.Looper.loop(Looper.java:154) 
        at com.facebook.react.bridge.queue.MessageQueueThreadImpl$4.run(MessageQueueThreadImpl.java:232) 
        at java.lang.Thread.run(Thread.java:762)

Can you please change the dependencies to use static, pinned versions instead of open-ended dynamic versions?

samdozor commented 4 years ago

hey @jordanhaven - thanks for the report. We can definitely change this to a static dependency. That said:

jordanhaven commented 4 years ago

Hey @samdozor we did try a full clean/rebuild. Nuked node_modules, android/.idea, and android/.gradle, ran invalidate cache, clean, and build. I also just tried it on a completely fresh checkout, and it is still throwing.

We already have it pinned to 5.9.3 in our android/app/build.gradle, but that doesn't seem to make a difference. It was pinned with a compile configuration, and I tried changing that to implementation, but still no dice.

When I look at all the dependencies being compiled, I'm seeing both 5.9.3 and 5.10.2:

Screen Shot 2019-10-09 at 12 11 29 Screen Shot 2019-10-09 at 12 12 14
willpassidomo commented 4 years ago

Hey @jordanhaven you can add the following snippet in your app level build.gradle file to pin the mparticle version to 5.9.3

configurations.all {
    resolutionStrategy {
        force 'com.mparticle:android-core:5.9.3'
    }
}

Hope this helps!

jefferyjxn commented 4 years ago

I literally just started experiencing this issue a few minutes ago. Great timing. That snippet resolved it for me. Thanks @willpassidomo!

jordanhaven commented 4 years ago

@willpassidomo works perfectly, thanks!

jordanhaven commented 4 years ago

Actually, nevermind, it appeared to be working for me (I think because I had modified the mParticle build.gradle to be static to unblock myself and hadn't changed it back), but fresh builds are still failing :(

ETA: it seems like on an initial clean/rebuild, the dependencies look correct, but once we build and run on device, we see 5.10.3 show up.

Initial open in AS/gradle sync:

Screen Shot 2019-10-11 at 12 27 15

After running on device:

Screen Shot 2019-10-11 at 12 29 43
willpassidomo commented 4 years ago

hey @jordanhaven would mind rerunning this with our latest React release, 2.3.3? We put out a fix yesterday for the underlying issue, so it should make the downgrade to 5.9.3 no longer necessary.

The issue we experienced recently had to do with recent versions of Gradle Build Tools (3.4+) starting using R8 by default in its build chain, which seems to cause issues similar to the ones you experienced when the dependency is compiled into a project which is not using R8.

The problem played itself out in the Android module of our React Native project which, until yesterday's release, was still using an old version of Gradle Build Tools. The latest version should resolve this issue

jordanhaven commented 4 years ago

Nope :/

I've tried:

In all cases, I get the same issue.

The only time I got a different error was when upgrading this plugin to 2.3.3 and changing our implementation 'com.mparticle:android-core:5.9.3' to implementation 'com.mparticle:android-core:5.10.3', thinking, hey, if the core versions all match, it should be bueno, right? But then it wouldn't build and I got errors related to r8 (last relevant cause in stacktrace: Caused by: com.android.tools.r8.utils.AbortException: Error: Class content provided for type descriptor com.mparticle.internal.Q actually defines class com.mparticle.internal.q). And it's the same error regardless of whether or not android.enableR8=true is set in gradle.properties.

I've also done a lot of cache invalidation/restarts (both from the UI and from going nuclear and emptying out ~/.gradle/caches).

willpassidomo commented 4 years ago

@jordanhaven I'm sorry to hear that. What version of Gradle Build Tools do you have in your Android project?

if it is not the latest (3.5.1) I would highly recommend upgrading to that level, there seem a high number of issues related to R8 in earlier versions

willpassidomo commented 4 years ago

@jordanhaven @arcyn1c We just released version 5.11.0, which rolled back the Gradle Build Tool upgrades we previously made. This version will be pulled in automatically by the React SDK, so you will be able to go back to your original code (sans the fixes we went over here). I'm going to close this for now, but if any issues like this pop back up, please do open up a new issue! Thanks for the report!

markgibaud commented 4 years ago

@willpassidomo we experienced this issue as well, and made a change our own gradle file to effectively roll forward the Android SDK to fix our builds:

dependencies {
    implementation 'com.mparticle:android-core:5.10.2'
}

However this doesn't fix the underlying "problem" of something you reference in your last comment - the "auto upgrade" facility of the RN sdk. Correct me if I'm wrong but the range specifier in the gradle file of the RN package, of the Android SDK dependency, is "take latest" which means our builds are not deterministic. We'd prefer a fixed version so that our builds are deterministic and we can control SDK upgrades, whether they are the RN SDK's or the underlying native SDK's. Right now it still seems like any developer of the Android SDK can release a new (even major) version with a breaking change (like the baseEvent one above) and it would get pulled into the next builds of anybody using the RN sdk and potentially break live apps.

willpassidomo commented 4 years ago

@markgibaud Hey Mark, thanks for reaching out!

The Android SDK dependency iscompileOnly means it will only be used during Compile time. The version you specify in your application build.gradle is the version that will be used at Runtime. The version you specify in your RN project, using implementation is the version that will be used at Runtime. As long as the dependency in your RN project is locked, you will achieve deterministic builds.

https://developer.android.com/studio/build/dependencies#dependency_configurations

We agree that it is not perfect to even have an open-ended compileOnly dependency, but we are extremely conscientious of maintaining API compatibility, so it is not likely to ever be an issue. We will update this behavior in the near future, but it doesn't rise to the level warranting a hotfix

Just to clarify, the issue originally cited in this issue was not a result of any API changes, rather an issue with the update of the Gradle Build Tools past version 3.4.1, which caused multidexing problems in some dependent applications. Essentially, when the app was compiled, our SDK was split amongst different ".dex" files, and for some reason was not being mapped properly. We suspect that the solution would be resolved if clients also upgraded their Gradle Build Tool, but we decided to instead roll back the upgrade. The specific issue with that particular Gradle Build Tools version, 3.4.1, was that in that version the new code shrinker "R8" was enabled by default and it was not playing nice with build chains still using Progaurd.

The rollback occurred in our Android SDK in version 5.11.0, so using any version equal or greater than that should alleviate the multidexing issue

I hope you found this answer satisfactory, and please reach back out if you find my explanation lacking :)

markgibaud commented 4 years ago

Hi @willpassidomo thanks for the quick reply and apologies for not getting back sooner.

I'm not an expert at gradle and how compileOnly and implementation at different levels interact, but it was definitely such that without updating any source on our side whatsoever, the new update of the mParticle Android SDK was pulled in at build time and because it contained that breaking change we couldn't release our Android app. We proved this multiple ways on different machines before figuring out that that was the problem (the problem was of course that although everything compiled correctly, at runtime the logEvent called failed) Agree it's not worth a hotfix but going forward we'd definitely want to reach a stage where from any given commit we would be able to produce a deterministic build.

Anyway thanks for looking into it and we're satisfied it's unlikely to happen again before fixing the versions in the RN package 👍