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

[unimodules-app-loader] Fixes Class Name Cache #8292

Conversation

thorbenprimke
Copy link
Contributor

@thorbenprimke thorbenprimke commented May 13, 2020

Why

When migrating from managed to bare workflow, we encountered an instant app crash after upgrading the Android app from the Expo version to the bare version. Our app has a task defined and after inspecting the native code, we found that the class name is cached and the managed and bare classnames do not match.

This happens when installing a bare build over a previously installed managed build.

Not sure if the cause is the same but it may have been reported via #7949 .

How

Adds additional condition to the app loader registered name check that ensures the cached and current classname for the task match.

After update With clean install
Screen Shot 2020-05-13 at 11 16 26 AM Screen Shot 2020-05-13 at 11 32 05 AM

Stacktrace:

05-13 10:43:44.114 12768 12813 E Expo    : Cannot initialize app loader. host.exp.exponent.t.c
05-13 10:43:44.115 12768 12813 W System.err: java.lang.ClassNotFoundException: host.exp.exponent.t.c
05-13 10:43:44.115 12768 12813 W System.err: 	at java.lang.Class.classForName(Native Method)
05-13 10:43:44.115 12768 12813 W System.err: 	at java.lang.Class.forName(Class.java:454)
05-13 10:43:44.115 12768 12813 W System.err: 	at java.lang.Class.forName(Class.java:379)
05-13 10:43:44.115 12768 12813 W System.err: 	at org.unimodules.apploader.AppLoaderProvider.createLoader(AppLoaderProvider.java:52)
05-13 10:43:44.115 12768 12813 W System.err: 	at org.unimodules.apploader.AppLoaderProvider.getLoader(AppLoaderProvider.java:32)
05-13 10:43:44.115 12768 12813 W System.err: 	at expo.modules.taskManager.TaskService.getAppLoader(TaskService.java:438)
05-13 10:43:44.115 12768 12813 W System.err: 	at expo.modules.taskManager.TaskService.isStartedByHeadlessLoader(TaskService.java:273)
05-13 10:43:44.115 12768 12813 W System.err: 	at expo.modules.taskManager.TaskService.setTaskManager(TaskService.java:247)
05-13 10:43:44.115 12768 12813 W System.err: 	at expo.modules.taskManager.TaskManagerInternalModule.onCreate(TaskManagerInternalModule.java:52)
05-13 10:43:44.115 12768 12813 W System.err: 	at org.unimodules.core.ModuleRegistry.initialize(ModuleRegistry.java:149)
05-13 10:43:44.115 12768 12813 W System.err: 	at org.unimodules.core.ModuleRegistry.ensureIsInitialized(ModuleRegistry.java:131)
05-13 10:43:44.115 12768 12813 W System.err: 	at org.unimodules.adapters.react.ModuleRegistryReadyNotifier.initialize(ModuleRegistryReadyNotifier.java:28)
05-13 10:43:44.115 12768 12813 W System.err: 	at com.facebook.react.bridge.ModuleHolder.doInitialize(ModuleHolder.java:222)
05-13 10:43:44.115 12768 12813 W System.err: 	at com.facebook.react.bridge.ModuleHolder.markInitializable(ModuleHolder.java:97)
05-13 10:43:44.115 12768 12813 W System.err: 	at com.facebook.react.bridge.NativeModuleRegistry.notifyJSInstanceInitialized(NativeModuleRegistry.java:102)
05-13 10:43:44.115 12768 12813 W System.err: 	at com.facebook.react.bridge.CatalystInstanceImpl$2.run(CatalystInstanceImpl.java:441)
05-13 10:43:44.115 12768 12813 W System.err: 	at android.os.Handler.handleCallback(Handler.java:883)
05-13 10:43:44.115 12768 12813 W System.err: 	at android.os.Handler.dispatchMessage(Handler.java:100)
05-13 10:43:44.115 12768 12813 W System.err: 	at com.facebook.react.bridge.queue.MessageQueueThreadHandler.dispatchMessage(MessageQueueThreadHandler.java:26)
05-13 10:43:44.115 12768 12813 W System.err: 	at android.os.Looper.loop(Looper.java:214)
05-13 10:43:44.115 12768 12813 W System.err: 	at com.facebook.react.bridge.queue.MessageQueueThreadImpl$4.run(MessageQueueThreadImpl.java:225)
05-13 10:43:44.115 12768 12813 W System.err: 	at java.lang.Thread.run(Thread.java:919)
05-13 10:43:44.115 12768 12813 W System.err: Caused by: java.lang.ClassNotFoundException: host.exp.exponent.t.c
05-13 10:43:44.115 12768 12813 W System.err: 	... 22 more
05-13 10:43:44.115 12768 12813 E AndroidRuntime: FATAL EXCEPTION: mqt_native_modules
05-13 10:43:44.115 12768 12813 E AndroidRuntime: Process: org.howwefeel, PID: 12768
05-13 10:43:44.115 12768 12813 E AndroidRuntime: java.lang.NullPointerException: Attempt to invoke interface method 'boolean org.unimodules.apploader.HeadlessAppLoader.isRunning(java.lang.String)' on a null object reference
05-13 10:43:44.115 12768 12813 E AndroidRuntime: 	at expo.modules.taskManager.TaskService.isStartedByHeadlessLoader(TaskService.java:273)
05-13 10:43:44.115 12768 12813 E AndroidRuntime: 	at expo.modules.taskManager.TaskService.setTaskManager(TaskService.java:247)
05-13 10:43:44.115 12768 12813 E AndroidRuntime: 	at expo.modules.taskManager.TaskManagerInternalModule.onCreate(TaskManagerInternalModule.java:52)
05-13 10:43:44.115 12768 12813 E AndroidRuntime: 	at org.unimodules.core.ModuleRegistry.initialize(ModuleRegistry.java:149)
05-13 10:43:44.115 12768 12813 E AndroidRuntime: 	at org.unimodules.core.ModuleRegistry.ensureIsInitialized(ModuleRegistry.java:131)
05-13 10:43:44.115 12768 12813 E AndroidRuntime: 	at org.unimodules.adapters.react.ModuleRegistryReadyNotifier.initialize(ModuleRegistryReadyNotifier.java:28)
05-13 10:43:44.115 12768 12813 E AndroidRuntime: 	at com.facebook.react.bridge.ModuleHolder.doInitialize(ModuleHolder.java:222)
05-13 10:43:44.115 12768 12813 E AndroidRuntime: 	at com.facebook.react.bridge.ModuleHolder.markInitializable(ModuleHolder.java:97)
05-13 10:43:44.115 12768 12813 E AndroidRuntime: 	at com.facebook.react.bridge.NativeModuleRegistry.notifyJSInstanceInitialized(NativeModuleRegistry.java:102)
05-13 10:43:44.115 12768 12813 E AndroidRuntime: 	at com.facebook.react.bridge.CatalystInstanceImpl$2.run(CatalystInstanceImpl.java:441)
05-13 10:43:44.115 12768 12813 E AndroidRuntime: 	at android.os.Handler.handleCallback(Handler.java:883)
05-13 10:43:44.115 12768 12813 E AndroidRuntime: 	at android.os.Handler.dispatchMessage(Handler.java:100)
05-13 10:43:44.115 12768 12813 E AndroidRuntime: 	at com.facebook.react.bridge.queue.MessageQueueThreadHandler.dispatchMessage(MessageQueueThreadHandler.java:26)
05-13 10:43:44.115 12768 12813 E AndroidRuntime: 	at android.os.Looper.loop(Looper.java:214)
05-13 10:43:44.115 12768 12813 E AndroidRuntime: 	at com.facebook.react.bridge.queue.MessageQueueThreadImpl$4.run(MessageQueueThreadImpl.java:225)
05-13 10:43:44.115 12768 12813 E AndroidRuntime: 	at java.lang.Thread.run(Thread.java:919)

Test Plan

Verified that app no longer has instant crasher and launches as expected.

@thorbenprimke thorbenprimke force-pushed the thorbenprimke-expo-unimodules-app-loader-class-fix branch from b514cb5 to 8223f9b Compare May 13, 2020 19:40
@sjchmiela sjchmiela requested a review from mczernek May 14, 2020 07:13
Copy link
Member

@tsapeta tsapeta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thank you for this fix! 🙇‍♂️
I guess it might have something to do with ProGuard - in managed workflow you use Expo client for production so the class names are minified but once you eject to bare, you run a debug app where ProGuard is turned off. I think we can also force ProGuard to keep original class name for app loaders and that would make your registered tasks in managed still registered after migrating to bare 🤔 I noted this on my todo list 🙂

@tsapeta tsapeta merged commit 3bc821c into expo:master May 14, 2020
@thorbenprimke
Copy link
Contributor Author

thorbenprimke commented May 19, 2020

@tsapeta - The case that the PR fixed is for a crash on app start. However after releasing our new binary, we noticed another one happens when the app is not started but the task is executed. In this case, the className has not been updated and it fails due to an NPE from not finding the class.

In regards to your ProGuard comment about keeping the original class name in managed. Our app was previously managed and it looks like the class name was obfuscated. However it seems that the package has changed from host.exp.exponent to org.unimodules.adapters.react.apploader. Were you saying that one should / could use -applymapping option with Proguard to carry forward the previously obfuscated name? Even for the managed flow, I think, if you don't use applymapping, the app loader class name could not be valid between two binary releases.

In our case, we don't have ProGuard enabled for the release build. It's on my list but I didn't get around to setting it up (it didn't work out of the box).
Anyhow, to unbreak the production release, I went with this dirty hack.

Any thoughts on how this could be solved better for the future? Maybe the class names for tasks should be specified in the manifest and the app loader classes should be excluded from obfuscation?

Also maybe this is better suited for an issue - happy to move it there to continue the discussion.

@mczernek
Copy link
Contributor

mczernek commented May 20, 2020

I am afraid the issue is not caused by proguard. Root cause is the fact, that task manegers works differently in bare and managed workflow. Specifically, they use different loaders to run tasks in background.

It is general design error I made when preparing support for task manager in bare. I guess I haven't thought of your scenario.

There is no easy and non-dirty hack for that I can think of. However, I am on it and will try to figure out some way to fix it 🤞

@mczernek
Copy link
Contributor

This is a wild guess for now, but...
@thorbenprimke would you test if such change works for you?

@thorbenprimke
Copy link
Contributor Author

thorbenprimke commented May 20, 2020

@mczernek - I don't think that would fix it because after a new version of the app is installed, it's starts with a fresh process and thus loaderClasses should be empty. I do agree that it makes that check stronger though.

From my previous testing (and this PR), the code path is hit when the app is launched and the task is registered. However if the task is triggered after a new app install without the app ever having been launched, the task is not registered again and thus the classname is not updated.

This is the stacktrace that we were seeing in production. My hack to fix it was based on this stacktrace (and seems to work) but I didn't actually verify it (besides not seeing it in the latest binary release).

java.lang.RuntimeException java.lang.NullPointerException: Attempt to invoke interface method 'void org.unimodules.apploader.HeadlessAppLoader.loadApp(android.content.Context, org.unimodules.apploader.HeadlessAppLoader$Params, java.lang.Runnable, org.unimodules.core.interfaces.Consumer)' on a null object reference 
    JobServiceEngine.java:112 android.app.job.JobServiceEngine$JobHandler.handleMessage
    Handler.java:107 android.os.Handler.dispatchMessage
    Looper.java:237 android.os.Looper.loop
    ActivityThread.java:7811 android.app.ActivityThread.main
    Method.java:-2 java.lang.reflect.Method.invoke
    RuntimeInit.java:493 com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run
    ZygoteInit.java:1076 com.android.internal.os.ZygoteInit.main

Caused by: java.lang.NullPointerException Attempt to invoke interface method 'void org.unimodules.apploader.HeadlessAppLoader.loadApp(android.content.Context, org.unimodules.apploader.HeadlessAppLoader$Params, java.lang.Runnable, org.unimodules.core.interfaces.Consumer)' on a null object reference 
    TaskService.java:411 expo.modules.taskManager.TaskService.executeTask
    Task.java:58 expo.modules.taskManager.Task.execute
    BackgroundFetchTaskConsumer.java:87 expo.modules.backgroundfetch.BackgroundFetchTaskConsumer.didExecuteJob
    TaskService.java:334 expo.modules.taskManager.TaskService.handleJob
    TaskJobService.java:13 expo.modules.taskManager.TaskJobService.onStartJob
    JobService.java:62 android.app.job.JobService$1.onStartJob
    JobServiceEngine.java:108 android.app.job.JobServiceEngine$JobHandler.handleMessage
    Handler.java:107 android.os.Handler.dispatchMessage
    Looper.java:237 android.os.Looper.loop
    ActivityThread.java:7811 android.app.ActivityThread.main
    Method.java:-2 java.lang.reflect.Method.invoke
    RuntimeInit.java:493 com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run
    ZygoteInit.java:1076 com.android.internal.os.ZygoteInit.main

I traced it to getLoader which catches the ClassNotFoundException that I had previously encountered (this PR) and returns null but the subsequent consuming code does not handle the null case - which I suppose is good and bad in the sense that if it just handled it gracefully, we would have not been alerted to this issue.

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

Successfully merging this pull request may close these issues.

None yet

3 participants