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
[unimodules-app-loader] Fixes Class Name Cache #8292
Conversation
b514cb5
to
8223f9b
Compare
There was a problem hiding this 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 - 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 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). 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. |
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 🤞 |
This is a wild guess for now, but... |
@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 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).
I traced it to getLoader which catches the ClassNotFoundException that I had previously encountered (this PR) and returns |
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.
Stacktrace:
Test Plan
Verified that app no longer has instant crasher and launches as expected.