Repository navigation
fix(android): soft-fail missing view in SurfaceMountingManager mount ops - #59773
VimukthiShohan wants to merge 2 commits into
Conversation
…ePadding A ViewState can exist without an Android View (createViewUnsafe only creates one for layoutable nodes, and updateEventEmitter registers bare placeholders). updateOverflowInset and updatePadding then hit checkNotNull and throw "Unable to find View for tag", tearing down the whole surface. Log a ReactNoCrashSoftException and return instead, matching the missing-viewState branch right above each assert. Adds Robolectric tests for both methods.
|
Hi @VimukthiShohan! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
Same ViewState-without-View case as updateOverflowInset and updatePadding.
removeViewAt hit checkNotNull on the parent view ("Unable to find parentView
for tag") and updateState on the ViewManager, both throwing and tearing down
the surface. Log a ReactNoCrashSoftException and return instead.
updateState keeps storing the new state wrapper and still destroys the
previous one on the early return, so the C++ State is freed immediately.
Adds Robolectric tests for both methods, plus one that verifies the replaced
state wrapper is destroyed on the updateState early return.
VimukthiShohan
changed the title
meta-cla
Bot
added
the
CLA Signed
facebook-github-tools
Bot
added
the
Shared with Meta
|
We should not be suppressing this error, but identifying the root cause. Have you got a reliable repro for this? Non-layoutable nodes should never receive children, the path where createView does not allocate one is only used for text nodes iirc. |
|
No reliable repro, unfortunately. It is a production-only crash: 4 users / 18 events on one release (RN 0.87.1, Galaxy S23 Ultra(Android 17) and Samsung A56(Android 16)), always ~20 s after launch when a login screen mounts with the keyboard open. Three Maestro/manual variants on an Android 17 emulator produced zero Fabric soft exceptions. Agreed on non-layoutable nodes: I re-read the dispatcher and I can't yet prove that from the field: the crash message carries only the tag. Two options, your call:
Happy to do either. |
|
Let's come back to this once we have more info. |
Summary:
A Fabric
ViewStatecan exist without an AndroidView:createViewUnsafecreates one only for layoutablenodes, and
updateEventEmitterregisters a bare placeholder for a tag it has not seen (see #58526).When the mount instruction stream then sends
updateOverflowInset,updatePadding,removeViewAtorupdateStatefor such a tag, each method hits acheckNotNulland throws, which tears down the whole surface:We see this crash in production on Android (Play Store builds, RN 0.87.1). All four methods already handle a
missing
ViewStateone branch above the assert by logging aReactNoCrashSoftExceptionunderSURFACE_MOUNTING_MANAGER_MISSING_VIEWSTATEand returning. This PR uses the same pattern when theViewStateexists but has no view. #58527 makes the same change foraddViewAt.removeViewAt: Fix IllegalStateException in SurfaceMountingManager.removeViewAt (#57860) #57860 (landed as 3f553d7) soft-failed the case where the parent view is not aViewGroup,but left the
checkNotNull(parentView)just above it. A parentViewStatewith no view still throwsUnable to find parentView for tag. This PR soft-fails that case too.updateState: the method stores the new state wrapper before the asserts. On the new early return it keepsdoing so and still calls
prevStateWrapper?.destroyState(), so the previous C++ State is freed right awayinstead of waiting for GC.
Could this be picked into
0.87-stable? Apps on prebuilt AARs cannot patch it locally.Changelog:
[ANDROID] [FIXED] - Log a soft exception instead of crashing when
updateOverflowInset,updatePadding,removeViewAtorupdateStatetargets a ViewState with no viewTest Plan:
Four Robolectric tests were added to
SurfaceMountingManagerTest:updateOverflowInset_doesNotThrowWhenViewStateHasNoView,updatePadding_doesNotThrowWhenViewStateHasNoView,removeViewAt_doesNotThrowWhenParentViewStateHasNoViewandupdateState_doesNotThrowWhenViewStateHasNoView.Each one preallocates tag 42 with
isLayoutable = false, so the ViewState is registered but has no view andno view manager, and then calls the method (
removeViewAt(43, 42, 0)uses 42 as the parent).A fifth test,
updateState_destroysPreviousStateWhenViewStateHasNoView, callsupdateStatetwice on thesame tag with mock
StateWrappers and verifies that the first one is destroyed on the early return.Each fix was checked test-first. Every test failed at its own assert before its fix and passed after it.
updateOverflowInsetandupdatePadding(first commit), tests only:removeViewAtandupdateState(second commit), tests only:With only
removeViewAtfixed, theupdateStatetest still fails at its own assert:With the
prevStateWrapper?.destroyState()line removed from theupdateStateearly return, the leak testfails at its
verify:With all four fixes:
ktfmt --dry-run --set-exit-if-changedpasses on both changed files.