OneSignal / OneSignal-Android-SDK

OneSignal is a free push notification service for mobile apps. This plugin makes it easy to integrate your native Android or Amazon app with OneSignal. https://onesignal.com
Other
605 stars 369 forks source link

[Bug]: has no zero argument constructor while creating class in reflection - R8 #1779

Closed iballan closed 3 weeks ago

iballan commented 1 year ago

What happened?

Error while initializing OneSignal with custom Notification Service Extension

W  java.lang.InstantiationException: java.lang.Class<com.mypackage.app.NotificationServiceExtension> has no zero argument constructor
W   at java.lang.Class.newInstance(Native Method)
W   at com.onesignal.OSNotificationController.setupNotificationServiceExtension(OSNotificationController.java:191)
W   at com.onesignal.OneSignal.init(OneSignal.java:819)
W   at com.onesignal.OneSignal.setAppId(OneSignal.java:737)
W   at com.mypackage.app.MBApp.initOnce(MBApp.kt:115)
W   at com.mypackage.app.activities.landing.SplashActivity.onCreate(SplashActivity.kt:36)
W   at android.app.Activity.performCreate(Activity.java:8214)
W   at android.app.Activity.performCreate(Activity.java:8202)
W   at android.app.Instrumentation.callActivityOnCreate(Instrumentation.java:1320)
W   at android.app.ActivityThread.performLaunchActivity(ActivityThread.java:4033)
W   at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:4247)
W   at android.app.servertransaction.LaunchActivityItem.execute(LaunchActivityItem.java:91)
W   at android.app.servertransaction.TransactionExecutor.executeCallbacks(TransactionExecutor.java:149)
W   at android.app.servertransaction.TransactionExecutor.execute(TransactionExecutor.java:103)
W   at android.app.ActivityThread$H.handleMessage(ActivityThread.java:2613)
W   at android.os.Handler.dispatchMessage(Handler.java:110)
W   at android.os.Looper.loop(Looper.java:219)
W   at android.app.ActivityThread.main(ActivityThread.java:8668)
W   at java.lang.reflect.Method.invoke(Native Method)
W   at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:513)
W   at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1109)

This crash took place after upgrading gradle version and the default is now R8 enabled

@Keep
open class NotificationServiceExtension : OneSignal.OSRemoteNotificationReceivedHandler {
    override fun remoteNotificationReceived(context: Context, notificationReceivedEvent: OSNotificationReceivedEvent) {
        val isSilent = false
        val notification = notificationReceivedEvent.notification
        val mutableNotification = notification.mutableCopy()
        notificationReceivedEvent.complete(if (isSilent) null else mutableNotification)
    }
}

Steps to reproduce?

1. oneSignalVersion = "4.8.6"
2. Use custom service extension
3. initialize onesignal

What did you expect to happen?

While initializing onesignal, it is trying to create instance of the service extension. there it crashes. The extension class is written in Kotlin, that can be the reason with R8 it has no constructor maybe

OneSignal Android SDK version

Release 4.8.6

Android version

12, 11, 10

Specific Android models

No response

Relevant log output

No response

Code of Conduct

iballan commented 1 year ago

Converted the code of the NotificationServiceExtension to Java and still getting this error

iballan commented 1 year ago

I can confirm that it is bug related in the R8! Since the service extension is an unused class, i think it is removed or shrinked by proguard! The workaround was adding this

Timber.i("fix ${NotificationServiceExtension()}")  // or just creating instance of the class that will be created by reflection could work

before calling OneSignal.init fixed for me! Maybe that is preventing R8 from destroying the class or removing its constructors

jennantilla commented 1 year ago

@iballan thank you for the additional details and following up with what works for you! We'll look into potentially adding some documentation around this. Let us know if you have any additional questions or concerns!

sgjesse commented 1 year ago

From the stack trace it seems like com.onesignal.OSNotificationController.setupNotificationServiceExtension is using reflection to create an instance of a class (through java.lang.Class.newInstance). For any use of reflection in code where R8 is applied there should be sufficient keep rules to ensure that the items reflected upon is still present. See https://developer.android.com/build/shrink-code#shrink-code and https://www.guardsquare.com/manual/configuration/usage.

iballan commented 1 year ago

@sgjesse I agree with you, OneSignal developers agree with you too, That's why they added this rule:

# OSRemoteNotificationReceivedHandler is an interface designed to be extend then referenced in the
#    app's AndroidManifest.xml as a meta-data tag.
# This doesn't count as a hard reference so this entry is required.
-keep class ** implements com.onesignal.OneSignal$OSRemoteNotificationReceivedHandler {
   void remoteNotificationReceived(android.content.Context, com.onesignal.OSNotificationReceivedEvent);
}

in their proguard. I also added a rule to keep my class that I extend from OSRemoteNotificationReceivedHandler specifically. The issue is that all this was not enough because the extended class is not used in the code anywhere, And R8 removed it no matter what rule i added. Adding this:

Timber.i("fix ${NotificationServiceExtension()}") // this indicate that the class is used for R8

was the only solution for me for now. I didn't wanna add a rule to keep all unused class, because that would be against using R8 at all

sgjesse commented 1 year ago

Isn't it then just the keep rule missing keeping the no-args constructor?

-keep class ** implements com.onesignal.OneSignal$OSRemoteNotificationReceivedHandler {
   <init>();
   void remoteNotificationReceived(android.content.Context, com.onesignal.OSNotificationReceivedEvent);
}

The exception java.lang.InstantiationException: java.lang.Class has no zero argument constructor indicate that the class has not been removed.

R8 should not remove anything which has an unconditional keep rukle.

iballan commented 1 year ago

@sgjesse However, I am not sure if we need the rule already when we have the @keep annotation on the class. I've added @keep on the top of the class and a rule to keep the whole class in proguard file:

-keep class my.package.NotificationServiceExtension { *; }
-keep classmembers my.package.NotificationServiceExtension { *; }
-keep public class my.package.NotificationServiceExtension  { public protected *; }

Unfortunately nothing worked! still R8 removed it's constructor or removed it completely since it is not used or referenced anywhere in the code

sgjesse commented 1 year ago

@iballan

If you have the keep rule

-keep class my.package.NotificationServiceExtension { *; }

then the class my.package.NotificationServiceExtension and all its members should be kept. If that does not happen then maybe the configuration file is not picked up by the compilation. Try to add some garbage into it to see if the compiler complaints about the syntax.

Same for the @Keep annotation - it should cause the class and all members to remain untouched.

iballan commented 1 year ago

@sgjesse yeah thats why I reported this issue!

sgjesse commented 1 year ago

That sounds like the configuration is not correctly passed to the compiler. Could you try to add the following rule:

-keep class * { *; }

That should work, as that keeps everything in the application. If that does not work then the configuration is not picked up. If it works, when the configuration is picked up, but maybe the class name for my.package.NotificationServiceExtension generated by Kotlin is not exactly what you expect. Have you tried to look at the class file generated from open class NotificationServiceExtension using javap?

StuStirling commented 1 year ago

Adding this to my modules consumer-rules.pro fixed the issue

-keep class my.package.NotificationServiceExtension { *; }

tajchert commented 1 year ago

Fix in my case was to add:

-keep class com.onesignal.** { *; }

to my proguard-rules.pro to turn off obfuscation on OneSignal classes, referencing explicit classes from OneSignal SDK didn't work for me well (I was still getting crashes in release build) so I turn off whole obfuscation of OneSignal SDK just to be on the safe side.

sgjesse commented 1 year ago

@tajchert, are you doing reflection into classes in the OneSignal SDK?

If not then library has consumer keep rules defined in OneSignalSDK/onesignal/consumer-proguard-rules.pro - please check if these are included by your build setup. If they are, and you still have issues, please open an issue on this project so these consumer keep rules can be updated.

michael-winkler commented 1 year ago

Should this:


-keep class ** implements com.onesignal.OneSignal$OSRemoteNotificationReceivedHandler {
   void remoteNotificationReceived(android.content.Context, com.onesignal.OSNotificationReceivedEvent);
}

not be changed to this:

-keep class ** implements com.onesignal.notifications.INotificationServiceExtension{
   void onNotificationReceived(com.onesignal.notifications.INotificationReceivedEvent);
}

for 'com.onesignal:OneSignal:5.0.0' ? see https://github.com/OneSignal/OneSignal-Android-SDK/blob/2cbb251031a26ef61984b9cb7d728ef9c5277e92/OneSignalSDK/onesignal/consumer-proguard-rules.pro#L83C1-L85C2

jt-gilkeson commented 12 months ago

Same issue. In 5.0.x we have to manually keep (the @sgjesse workaround) our implementation of the INotificationServiceExtension or R8 removes it and we get the trace about the no zero argument constructor.

Suggestion: internally OneSignal should create a sample app that overrides INotificationServiceExtension, INotificationClickListener, and IPushSubscriptionObserver that you build with r8 and verify as part of automated testing for your library.

nan-li commented 3 weeks ago

Thanks for your suggestion, we have had no further reports of this so I will close out this issue. If this is still a problem, please open a new report with updated information.