[Text] Set LayoutParams in ReactTextView to fix crash #7011
Conversation
|
By analyzing the blame information on this pull request, we identified @dmmiller, @mkonicek and @axelander to be potential reviewers. |
facebook-github-bot
added
GH Review: review-needed
CLA Signed
|
cc @kmagiera |
|
@ide Will you look if this is related to #6805 and the subsequent fix (expo@1a99efb) I put in? I think what happens is that when removeClippedSubviews is true we do some operations in a slightly different order such that we can set text before the view is actually attached to a window. We thought there might be other issues that manifested like this but weren't sure. Can you see if fixing in the same way or looking at that has any leads? |
|
|
||
| // Android's TextView crashes when it tries to relayout if LayoutParams are | ||
| // null; explicitly set the LayoutParams to prevent this crash. See: | ||
| // https://github.com/facebook/react-native/pulls/7011 |
There was a problem hiding this comment.
looks like there is a typo in the link (should be /pull/7011 perhaps)
|
@dmmiller sure -- are you referring to the technique of overriding |
| // null; explicitly set the LayoutParams to prevent this crash. See: | ||
| // https://github.com/facebook/react-native/pulls/7011 | ||
| setLayoutParams(new ViewGroup.LayoutParams( | ||
| ViewGroup.LayoutParams.WRAP_CONTENT, |
There was a problem hiding this comment.
I'd feel much safer if we were using LayoutParams(0, 0) instead of WRAP_CONTENT
Also layout param object can be created once as a static variable and then reused instead of allocating new object every time we create new component
|
@ide updated the pull request. |
|
@ide yes. I was referring to the diff where we do stuff in onAttachedToWindow. |
ide
force-pushed
the
layoutparams-fix
branch
2 times, most recently
from
6cdceab
to
154dd3e
Compare
|
@ide updated the pull request. |
|
Trying a new approach by setting the layout params from |
|
@ide updated the pull request. |
|
The crash still happens when using |
|
@ide updated the pull request. |
|
@dmmiller would you mind taking a look at this pull request? It's been a while since the last commit was reviewed. |
|
+1 |
|
why not merge to master, newest version is crash, too |
|
If it's not the right fix or simple to maintain we don't want to merge it. Instead you should fork RN and cherry-pick the fixes you need. |
|
@facebook-github-bot shipit |
ReactTextView occasionally crashes when `setText` is called. Doing some cursory research, it looks like this could be a bug in Android. We also suspect it might be related to removeClippedSubviews though. The LayoutParams don't actually matter because RN controls layout, but on occasion (haven't narrowed down what this is...) `mLayout` is non-null and triggers relayout during `setText`, which fails because the LayoutParams are null. Stack trace: http://pastebin.com/P8VbxvPz Test Plan: Run an Android app that was sporadically crashing with the stack trace linked above. With this patch the crashes appear to have disappeared and the app is performing fine.
|
@facebook-github-bot shipit |
ghost
added
GH Review: accepted
Import Started
|
Thanks for importing. If you are an FB employee go to Phabricator to review. |
Summary: ReactTextView occasionally crashes when `setText` is called. Doing some cursory research, it looks like this could be a bug in Android. We also suspect it might be related to removeClippedSubviews though. The LayoutParams don't actually matter because RN controls layout, but on occasion (haven't narrowed down what this is...) `mLayout` is non-null and triggers relayout during `setText`, which fails because the LayoutParams are null. jesseruder came up with this fix and it appears to be working well. Stack trace: http://pastebin.com/P8VbxvPz Closes facebook#7011 Differential Revision: D3508097 fbshipit-source-id: 12b4aa11e42112c8ba19a1af325e3ee9a232d08f
Summary: ReactTextView occasionally crashes when `setText` is called. Doing some cursory research, it looks like this could be a bug in Android. We also suspect it might be related to removeClippedSubviews though. The LayoutParams don't actually matter because RN controls layout, but on occasion (haven't narrowed down what this is...) `mLayout` is non-null and triggers relayout during `setText`, which fails because the LayoutParams are null. jesseruder came up with this fix and it appears to be working well. Stack trace: http://pastebin.com/P8VbxvPz Closes facebook#7011 Differential Revision: D3508097 fbshipit-source-id: 12b4aa11e42112c8ba19a1af325e3ee9a232d08f
Summary: ## Issue: Sometimes ReactTextView are vertically displayed as one column with bridgeless. ## Root cause: After debugging, I found out this is caused by a workaround in 2016 to fix a crash caused by mLayout occasionally being non-null and triggers relayout during setText. #7011 ## Fix Revert previous hack, if the crash happens again I'll try to fix it. Changelog: [Android][Fixed] - Fix text in ReactTextView sometimes being vertically displayed Reviewed By: mdvacca Differential Revision: D26581756 fbshipit-source-id: a373d84dc1ab3d787bda7ec82f2d0865a354cf60
ReactTextView occasionally crashes when
setTextis called. Doing some cursory research, it looks like this could be a bug in Android. We also suspect it might be related to removeClippedSubviews though.The LayoutParams don't actually matter because RN controls layout, but on occasion (haven't narrowed down what this is...)
mLayoutis non-null and triggers relayout duringsetText, which fails because the LayoutParams are null. @jesseruder came up with this fix and it appears to be working well.Stack trace: http://pastebin.com/P8VbxvPz
Test Plan: Run an Android app that was sporadically crashing with the stack trace linked above. With this patch the crashes appear to have disappeared and the app is performing fine.