Proguard -assumenosideeffects Compatibility

[jkasten2]

Description

One Line Summary

Fix compatibility issue with apps that use -assumenosideeffects with java.lang.Class.getName() with Proguard or Dexguard. (No issue with R8.)

Details

Motivation

To fix issue #1423 which cause a crash on some devices when aggressive minification setting -assumenosideeffects with java.lang.Class.getName() is used.

Scope

This effects code paths that check if a Java class exists for feature detection. No logic was changed, just a proxy class was added to prevent Proguard from striping code.

How this prevents the issue

Addressed this by adding a new private opaqueHasClass method and utilized it in all existing class exist helper methods.
This method has the "Keep" annotation on it so proguard will never change it's code and will not make a wrong assumptions about side effects.

Before code change

Here in the smali output you can see the try-catch was removed by Proguard from hasHMSAvailabilityLibrary, even when the rule -keep class com.onesignal.OSUtils** {*;} or @Keep is added to try to prevent this modification.

.method private static n()Z
    .registers 1
    const-class v0, Lcom/huawei/hms/api/HuaweiApiAvailability;
    const/4 v0, 0x1
    return v0
.end method

After code change

After you see opaqueHasClass is kept as-is. hasHMSAvailabilityLibrary is renamed which is ok, the important thing is that the try-catch is kept which is required.

.method private static opaqueHasClass(Ljava/lang/Class;)Z
    .registers 1
    const/4 p0, 0x1
    return p0
.end method

// hasHMSAvailabilityLibrary
.method private static o()Z
    .registers 1

    :try_start_0
        const-class v0, Lcom/huawei/hms/api/HuaweiApiAvailability;
        invoke-static {v0}, Lcom/onesignal/OSUtils;->opaqueHasClass(Ljava/lang/Class;)Z
        move-result v0
    :try_end_6
        .catch Ljava/lang/NoClassDefFoundError; {:try_start_0 .. :try_end_6} :catch_7
        return v0
    :catch_7
        move-exception v0

    const/4 v0, 0x0
    return v0
.end method

Testing

Unit testing

No unit tests where added since this is a build issue and it has to run on a device to verify.

Manual testing

Tested on a FireOS tablet and a Android 12 emulator without the FCM library.

Disabled R8 in gradle.properties, this forces the Android Gradle Plugin to use Proguard.

android.enableR8 = false
android.enableR8.libraries = false

Tested with classpath 'net.sf.proguard:proguard-gradle:6.1.1' and with the following proguard-rules.pro file contents:

-ignorewarnings
-optimizationpasses 1
-dontskipnonpubliclibraryclasses
-optimizations !field/*,!class/merging/*

-assumenosideeffects public class java.lang.Object {
    public final java.lang.Class getClass();
}
-assumenosideeffects public final class java.lang.Class {
    public java.lang.String getName();
}

Affected code checklist

• Notifications
  • Display
  • Open
  • Push Processing
  • Confirm Deliveries
• Outcomes
• Sessions
• In-App Messaging
• REST API requests
• Public API changes
• Push Token Registration

Checklist

Overview

• I have filled out all REQUIRED sections above
• PR does one thing
• Any Public API changes are explained in the PR details and conform to existing APIs

Testing

• I have included test coverage for these changes, or explained why they are not needed
• All automated tests pass, or I explained why that is not possible
• I have personally tested this on my device, or explained why that is not possible

Final pass

• Code is as readable as possible.
• I have reviewed this PR myself, ensuring it meets each checklist item

---

This change is 

[adamschlesinger]

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @Jeasmine, @jkasten2, @nan-li, and @rgomezp)

---

OneSignalSDK/onesignal/src/main/java/com/onesignal/OSUtils.java, line 161 at r1 (raw file):

   static boolean hasFCMLibrary() {
      try {
         return hasClass(com.google.firebase.messaging.FirebaseMessaging.class);

Isn't this (and the additional below) try/catch redundant now?

[jkasten2]

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @adamschlesinger, @Jeasmine, @nan-li, and @rgomezp)

---

OneSignalSDK/onesignal/src/main/java/com/onesignal/OSUtils.java, line 161 at r1 (raw file):

Previously, adamschlesinger (Adam Schlesinger) wrote…

Isn't this (and the additional below) try/catch redundant now?

I simplified the code in hasClass (renamed to opaqueHasClass) so the 2nd catch isn't needed. I also updated my PR description to show the smali code differences and the configs I used to test with.

[adamschlesinger]

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Jeasmine, @nan-li, and @rgomezp)