-
Notifications
You must be signed in to change notification settings - Fork 24.3k
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
Fix - TextInput Drawable to avoid Null Pointer Exception RuntimeError #17530 #29452
Conversation
The AppCompat Theme Theme.AppCompat.Light.NoActionBar provides a default Drawable resource for AppCompatEditText. The resource is located in @drawable/abc_edit_text_material.xml https://chromium.googlesource.com/android_tools/+/7200281446186c7192cb02f54dc2b38e02d705e5/sdk/extras/android/support/v7/appcompat/res/drawable/abc_edit_text_material.xml A Runtime Error is triggered in a scenario with the following conditions: 1) Rendering a large number of TextInputs in the screen with a key prop 2) Triggering re-render with setInterval The scenario is also experienced with FlatList and ONLY using TextInput component. The following Runtime Error is triggered: NullPointerException:tempt to invoke virtual method 'android.graphics.drawable.Drawable android.graphics.drawable.Drawable$ConstantState.newDrawable(android.content.res.Resources) It is caused from the following line from abc_edit_text_material.xml <item android:state_pressed="false" android:state_focused="false" android:drawable="@drawable/abc_textfield_default_mtrl_alpha"/> I posted a Minimal Reproducible Example at facebook#17530 (comment) This commit simply changes RNTester to use a custom drawable resource for TextInput named @drawable/edit_text.
A Minimum Reproducible Example that triggers the Null Pointer Exception Runtime Error NullPointerException:tempt to invoke virtual method 'android.graphics.drawable.Drawable android.graphics.drawable.Drawable$ConstantState.newDrawable(android.content.res.Resources)' The following are the conditions that trigger the runtime error: 1) a large list (>100) TextInputs with key prop 2) with FlatList passing prop data=[{key: 1, key:2,.., key:5000]] 3) trigger re-render to ensure the NPE Runtime Error is triggered The example is built using hooks useEffect similar to componentDidMount. Original post is here facebook#17530 (comment)
Adding the edit_text.xml custom drawable to the android template to fix Null Pointer Exception Runtime Error from facebook#17530 The template will include the following settings in new reactnative applications, more info on the issue, the cause of the error and the solution in commit 0858d41
Base commit: b4ac211 |
Base commit: b4ac211 |
Thank you @fabriziobertoglio1987 for your awesome work. I really admire your great patience and endeavor on this haunting issue. The solution looks good to me, but still the question is not answered throughly: What is the real cause. What is wrong with the drawable? Why remove And why removing key props could also prevent the exception? Is it because key props will try to reuse the component and hence cached some dirty state? I know little to dig it like you did, but it keeps me wondering if there is something wrong with react native that managing the ui state. Anyway, well done! |
The Runtime is triggered in DrawableContainer.getChild() because ConstantState variable cs is The Background abc_edit_text_material.xml used for AppCompatEditText defines StateListDrawable which will change the background image of the TextInput depending on the state. The lines below are trying to replace the old Background Image with a new one, but the process fails because final ConstantState cs = mDrawableFutures.valueAt(keyIndex);
final Drawable prepared = prepareDrawable(cs.newDrawable(mSourceRes)); I am still trying to understand why
I try to answer your questions with my current knowledge of the problem, sorry if they are not satisfing answers. Tomorrow I will keep working on this and try to find a better solution.
The value of
I believe removing this Drawable is just a workaround to solve the real issue. The real issue is how
Maybe because a |
@fabriziobertoglio1987 I'd like to hire you if I am a boss 👍 Anyway do not let this issue take too much of your time. You do not have to understand it 100% to make it a ready pr and tag some core members to review. If this change prevent the crash without regressions it's already a great valuable contribution to the community. |
Thanks a lot @sunnylqm . I don't think that I could have done this Pull Request alone. I believe the interaction was very important to find a solution, the issue was really challenging and I am very happy that we could solve it together. I wish you a good evening. |
@fabriziobertoglio1987 any chance I can apply this fix in an active project w/o waiting for this PR to be merged? |
This comment has been minimized.
This comment has been minimized.
Hi guys, can we merge this PR now? |
When will this be ready PR? |
which react-native version would be containing the fix of this issue? |
Hello, I believe it will be after 0.63.3 version, because there are people with this version experiencing the same issue. |
Steps to land a fix:
And now we are at stage 0 |
@fabriziobertoglio1987 @lunaleaps Hi! Any predictions for this pull request to be approved? |
@lunaleaps has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
This pull request was successfully merged by @fabriziobertoglio1987 in 254493e. When will my fix make it into a release? | Upcoming Releases |
Is this PR released? I have not found any changelog for it. |
Hello, You can check here if this fix is planned for a future release. |
Is this already merged? I'm using version |
hey guys i have been able to cut off this crash on JS side, do you think it is reliable? i am on RN 0.62 |
Summary: Fix the TextInput npe in #29452 on react native side because if it is just avoided on App side by changing themes may cause side effects. ## Changelog: - [ANDROID] [FIXED] - Fix TextInput NPE. <!-- Help reviewers and the release process by writing your own changelog entry. Pick one each for the category and type tags: [ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message For more details, see: https://reactnative.dev/contributing/changelogs-in-pull-requests Pull Request resolved: #37302 Test Plan: Thanks to Fabriziobertoglio1987's wok, the problem in #29452 can be tested easily. This fixing works fine and causes no side effects. The following video shows the test result. And the number 101 has been changed to 1001 in RNTester/TextInputKeyProp.js when testing. https://user-images.githubusercontent.com/23273745/236796702-e61a6fa9-9935-4179-9c5f-e9370d543657.mp4 Reviewed By: javache Differential Revision: D45688987 Pulled By: cipolleschi fbshipit-source-id: 4e13c19c10ed53cfcead79e66ab2e232369317e0
facebook#37302) Summary: Fix the TextInput npe in facebook#29452 on react native side because if it is just avoided on App side by changing themes may cause side effects. - [ANDROID] [FIXED] - Fix TextInput NPE. <!-- Help reviewers and the release process by writing your own changelog entry. Pick one each for the category and type tags: [ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message For more details, see: https://reactnative.dev/contributing/changelogs-in-pull-requests Pull Request resolved: facebook#37302 Test Plan: Thanks to Fabriziobertoglio1987's wok, the problem in facebook#29452 can be tested easily. This fixing works fine and causes no side effects. The following video shows the test result. And the number 101 has been changed to 1001 in RNTester/TextInputKeyProp.js when testing. https://user-images.githubusercontent.com/23273745/236796702-e61a6fa9-9935-4179-9c5f-e9370d543657.mp4 Reviewed By: javache Differential Revision: D45688987 Pulled By: cipolleschi fbshipit-source-id: 4e13c19c10ed53cfcead79e66ab2e232369317e0
I found that thing again in our Crashlytics..
Our versions from package.json |
Summary
This issue fixes #17530 fixes expo/expo#9905 with the help of @sunnylqm https://github.com/sunnylqm
Re-rendering a large number of TextInputs with key prop on the screen will trigger the below Null Pointer Exception Runtime Error
NullPointerException:tempt to invoke virtual method 'android.graphics.drawable.Drawable android.graphics.drawable.Drawable$ConstantState.newDrawable(android.content.res.Resources)'
The error is caused by null.newDrawable(mSourceRes) at
https://github.com/aosp-mirror/platform_frameworks_base/blob/20b012282e0c3d94b5c0aa799cdda065f2df06db/graphics/java/android/graphics/drawable/DrawableContainer.java#L919
More info #29452 (comment) #17530 (comment)
The Theme Theme.AppCompat.Light.NoActionBar defines the Drawables for AppCompatEditText in @drawable/abc_edit_text_material.xml
https://chromium.googlesource.com/android_tools/+/7200281446186c7192cb02f54dc2b38e02d705e5/sdk/extras/android/support/v7/appcompat/res/drawable/abc_edit_text_material.xml
Removing the following line from the above xml file @drawable/abc_edit_text_material.xml fixes the error #17530 (comment)
<item android:state_pressed="false" android:state_focused="false" android:drawable="@drawable/abc_textfield_default_mtrl_alpha"/>
The Theme default EditText background is replaced with a custom background, which is a copy of the original background without the above item which triggers the Runtime Error. The changes are implemented in RNTester with commit (more info in the commit) 0858d41. The new custom drawable used as default background for the TextInput is named edit_text.
<item name="android:editTextBackground">@drawable/edit_text</item>
The same changes have been added to react-native default template for creating new applications with commit (more info) f349308, lean core moved the cli tools to https://github.com/react-native-community/cli, but the default template for creating a new application is stored in facebook/react-native/template.
New applications will be generated with this configurations and will not experience the error, existing react-native applications will fix the error by upgrading with the upgrade-helper.
A Minimum Reproducible Example to reproduce this error is included in commit (more info in the commit) fabOnReact@4a414e2 and #17530 (comment)
Changelog
[Android] [Fixed] - TextInput Drawable to avoid Null Pointer Exception RuntimeError #17530
Test Plan
Works in all scenarios on Android.
CLICK TO OPEN TESTS RESULTS - React Native
Test Results from the Testing in RNTester.
Minimum Reproducible Example added with commit (more info in the commit) fabOnReact@4a414e2
The example included in commit fabOnReact@4a414e2 will cause a NPE Runtime Error on Master Branch, while no error is experienced in the feature branch. The links are video hosted on s3 of this tests (playable by google chrome).
The below screenshots were taken to detect any issues with the EditText Background. There is no difference between master and feature branch.
CLICK TO OPEN TESTS RESULTS - React Native Cli
As lean core move cli tools to https://github.com/react-native-community/cli, I tested the changes to the template in a separate repository https://github.com/fabriziobertoglio1987/react-native-template and generated the template with the following command
The generated app did not experience any issues and includes all the changes in rn_edit_text_material.xml and styles.xml