fix(Android): do not require an activity to register sheet insets observer - #4531
Open
Bram-dc wants to merge 1 commit into
Open
fix(Android): do not require an activity to register sheet insets observer#4531Bram-dc wants to merge 1 commit into
Bram-dc wants to merge 1 commit into
Conversation
…erver SheetDelegate resolves the decor view through checkNotNull(currentActivity) in the host fragment's ON_START. The fragment can reach ON_START while React holds no current activity: the sheet is open, the activity hosting it is stopped, another ReactActivity is destroyed as the current one, and the user then returns to the app. Fragment ON_START is dispatched before ReactActivity.onResume calls onHostResume, so currentActivity is still null at that point and every such return became a fatal IllegalStateException. Resolve the decor view leniently and retry the registration in ON_RESUME, which runs after onHostResume has restored the activity. registerOnView is idempotent, and preserveBackgroundFocus in the same class already treats a missing current activity as an ordinary condition.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
SheetDelegateresolves the decor view throughcheckNotNull(screen.reactContext.currentActivity)in the host fragment'sON_START:The fragment can legitimately reach
ON_STARTwhile React holds no current activity, and then this crashes the app:The ordering that produces it:
SheetDelegateis observing the host fragment's lifecycleReactActivityis destroyed while it is the current activity, soReactContext.currentActivityis clearedReactActivity.onResumeis what callsonHostResumeand restorescurrentActivity, but fragmentON_STARTis dispatched beforeonResume. So atON_STARTthere is reliably no current activity, andcheckNotNullturns an ordinary return-to-app into a fatal exception.Apps with a second
ReactActivityare the ones that hit this, but nothing in the sheet actually needs the decor view that early, andpreserveBackgroundFocusa few lines below already treats a missing current activity as an ordinary condition:So the delegate is already inconsistent with itself about whether a missing activity is fatal.
Changes
requireDecorView()with a nullabledecorViewOrNull.registerInsetsObserver(): it registers when a decor view is available, otherwise logs a warning and sets a pending flag.ON_RESUMEretries the registration when it is pending. That is the right place becauseonHostResumehas run by then.InsetsObserverProxy.registerOnViewjust callsViewCompat.setOnApplyWindowInsetsListenerand overwrites its weak reference, so re-entering it is safe. The normal path is unchanged: when an activity is present atON_START, registration happens exactly as before and theON_RESUMEbranch never runs.Before & after - visual documentation
No visual changes. The insets observer ends up registered on the same decor view either way; it just no longer throws when it has to wait one lifecycle step for it.
Test plan
There are no Kotlin unit tests in the repo, so this was verified by hand.
To reproduce you need a second
ReactActivityin the app, since that is what clearscurrentActivityindependently of the one hosting the sheet:presentation: 'formSheet'ReactActivitycome up and then be destroyed while it is the current activityOn
mainstep 4 crashes with theIllegalStateExceptionabove every time. With this change the app returns to the sheet, and if the activity is not yet available you get:followed by normal registration a moment later, with insets and keyboard handling behaving as usual.
I have been running this patched into 4.26.2 in production for several weeks and the crash is gone. In our case the second activity is an overlay screen driven by an AccessibilityService, which is what makes the ordering reproducible rather than rare.
I have not attached a standalone reproducer repository, because reproducing it needs a second
ReactActivitybeing destroyed at the right moment rather than anything expressible in a Snack. The lifecycle ordering argument stands on its own from the code, and I am happy to build a minimal two-activity sample if that would help review.Formatting checked with
ktlint 1.3.0(the versionandroid/spotless.gradlepins) against the changed file.Checklist