Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
1 task done
iballan opened this issue May 17, 2023 · 16 comments
Closed
1 task done

Comments

@iballan
Copy link

iballan commented May 17, 2023

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

  • I agree to follow this project's Code of Conduct
@iballan
Copy link
Author

iballan commented May 17, 2023

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

@iballan
Copy link
Author

iballan commented May 18, 2023

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
Copy link
Contributor

@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
Copy link

sgjesse commented May 22, 2023

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
Copy link
Author

iballan commented May 22, 2023

@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
Copy link

sgjesse commented May 22, 2023

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<com.mypackage.app.NotificationServiceExtension> 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
Copy link
Author

iballan commented May 22, 2023

@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
Copy link

sgjesse commented May 22, 2023

@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
Copy link
Author

iballan commented May 22, 2023

@sgjesse yeah thats why I reported this issue!

@sgjesse
Copy link

sgjesse commented May 23, 2023

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
Copy link

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

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

@tajchert
Copy link

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
Copy link

sgjesse commented Aug 21, 2023

@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
Copy link

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
Copy link

jt-gilkeson commented Nov 19, 2023

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
Copy link
Contributor

nan-li commented Oct 25, 2024

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.

@nan-li nan-li closed this as completed Oct 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants