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

App crashing with NPE for some classes when Lazy Class Key is used #4309

Closed
nitinsethi86 opened this issue May 15, 2024 · 7 comments
Closed

Comments

@nitinsethi86
Copy link

nitinsethi86 commented May 15, 2024

What is the issue?
NPE when the class used in LazyClassKey is removed by R8.

R8 has removed that type from the final APK. Can we provide examples of R8 rules that should accompany with usage of LazyClassKey?

From the documentation, it's unclear if the R8 rules ship with the library or we need to write one on our own.

Example module that uses LazyClassKey

@Module
public abstract class AppDataFactoryModule {

    @Binds
    @IntoMap
    @LazyClassKey(ILogger.class)
    abstract Object bindILogger(ILogger logger);
}

Dagger version
2.51.1

@wanyingd1996
Copy link

Hi, dagger ships an r8 rule -identifiernamestring that comes with the usage of @LazyClassKey, you shouldn't need to do anything else to make that work. What is the version of R8 you are using? Maybe identifiernamestring isn't included in an earlier version of R8. If version update doesn't help, can you provide a repro for the problem. I copied your example into our sample r8 app, and it build and runs successfully.

@nitinsethi86
Copy link
Author

nitinsethi86 commented May 21, 2024

Can you point me to the exact rule you are talking about?
I use the R8 version 8.2.47 that ships with AGP 8.2.2.

I came up with a solution to make all Class used as class key implement a marker interface called LazyClassKeyable. Once we do that, the below R8 rule will work for all keys.

# Keep rule for supporting LazyClassKey
-keep class * implements com.microsoft.teams.injection.LazyClassKeyable { *; }

With a unit test, we can force future additions to implement the marker interface LazyClassKeyable. I was thinking of writing a unit tests that gets holds of all keys and assert that they can be assigned to LazyClassKeyable.

    @Test
    fun appDataFactory_mapKeys_shouldBeLazyClassKeyable() {
        val app = context as StubbedSkypeTeamsApplication
        val userDataFactory : UserDataFactory = app.getUserDataFactory(TEST_USER_OBJECT_ID)
        assertEquals("Note: New key added to multibinding UserDataFactoryMap needs to implement LazyClassKeyable.",251, userDataFactory.mProviderMap.size)
        val values = userDataFactory.mProviderMap.values
        values.forEach {
            val classObject = it.get()
            assertTrue("Key $classObject should implement LazyClassKeyable", classObject is LazyClassKeyable)
        }
    }

@wanyingd1996
Copy link

wanyingd1996 commented May 22, 2024

Hi, the r8 rule we included in the library is here. -keep the class works, but may result in your class name not being obfuscated? I still want to figure out why the class gets removed, I expect with usage of -identifiernamesstring rule, the class shouldn't have been removed. Is it ok to provide a repro of the problem? Or provide more detail about what's special about the class being removed? Also, please share the NPE stack trace. Thanks!

@nitinsethi86
Copy link
Author

nitinsethi86 commented May 29, 2024

Please find the attached reproducer project. Do I have to add the annotation in the class being used for LazyClassKey?

Ideally I would be using the Interface to inject. However, for the sake of reproduction, I had to avoid using the interface to ensure that R8 removed it from the final APK which helps in reproducing the issue.

MultiModuleApp.zip

The stacktrace is as below:

2024-05-29 15:15:24.224 11205-11205 AndroidRuntime pid-11205 E FATAL EXCEPTION: main
Process: com.ms.multimoduleapp, PID: 11205
java.lang.RuntimeException: Unable to create application com.ms.multimoduleapp.MultiModuleApp: java.lang.IllegalArgumentException: Unknown class class G0.a
at android.app.ActivityThread.handleBindApplication(ActivityThread.java:7003)
at android.app.ActivityThread.-$$Nest$mhandleBindApplication(Unknown Source:0)
at android.app.ActivityThread$H.handleMessage(ActivityThread.java:2236)
at android.os.Handler.dispatchMessage(Handler.java:106)
at android.os.Looper.loopOnce(Looper.java:205)
at android.os.Looper.loop(Looper.java:294)
at android.app.ActivityThread.main(ActivityThread.java:8177)
at java.lang.reflect.Method.invoke(Native Method)
at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:552)
at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:971)
Caused by: java.lang.IllegalArgumentException: Unknown class class G0.a
at com.ms.multimoduleapp.MultiModuleApp.onCreate(Unknown Source:110)
at android.app.Instrumentation.callApplicationOnCreate(Instrumentation.java:1316)
at android.app.ActivityThread.handleBindApplication(ActivityThread.java:6998)
at android.app.ActivityThread.-$$Nest$mhandleBindApplication(Unknown Source:0) 
at android.app.ActivityThread$H.handleMessage(ActivityThread.java:2236) 
at android.os.Handler.dispatchMessage(Handler.java:106) 
at android.os.Looper.loopOnce(Looper.java:205) 
at android.os.Looper.loop(Looper.java:294) 
at android.app.ActivityThread.main(ActivityThread.java:8177) 
at java.lang.reflect.Method.invoke(Native Method) 
at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:552) 
at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:971) 

@wanyingd1996
Copy link

Thanks, I was able to repro, and found that there is something changed between agp 8.3.0 and 8.4.0, causing the resulting obfuscated code to change.

with 8.3.0:
Screenshot 2024-06-03 at 1 58 23 PM
The identifier name string rule is working properly, with the map key obfuscated.

with 8.4.0
Screenshot 2024-06-03 at 1 53 34 PM

Our identifier name string rule fails to take effect with 8.4.0, and the name of the class ILogger was not obfuscated, therefore causing the inconsistency when trying to read the map passing in the obfuscated class name.

As you mentioned in an earlier comment:

I use the R8 version 8.2.47 that ships with AGP 8.2.2.

I tried changing classpath 'com.android.tools.build:gradle:8.4.0' to classpath 'com.android.tools.build:gradle:8.2.2', this runs successfully, so I'm not sure where does the error coming from for that one.

In summary, the problem is our identifier name string rules fails to take effect in the newest AGP. In this case, you may add the following rule:

-keepclasseswithmembers,includedescriptorclasses class * {
   @dagger.internal.KeepFieldType <fields>;
}

to your proguard.pro, this will keep all @LazyClassKey referenced class names, so that you won't need to mark them individually.

At the meantime, I will try to figure out why the rule fails to take effect, and probably follow up with AGP team about the issue. Thanks!

@nitinsethi86
Copy link
Author

nitinsethi86 commented Jun 5, 2024

Thanks @wanyingd1996. This seems to be working. I will test it more and update here.

@wanyingd1996
Copy link

Hi, AGP team has fixed the issue in in R8 version 8.4.37 and 8.5.23, so the alternative solution is override the r8 version in settings.gradle.

pluginManagement {
    buildscript {
        repositories {
            mavenCentral()
            maven {
                url = uri("https://storage.googleapis.com/r8-releases/raw")
            }
        }
        dependencies {
            classpath("com.android.tools:r8: 8.4.37")
        }
    }
}

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

2 participants