Fix Alert leaving behind a window and view controller (issue 32304) #32305

paddlefish · 2021-09-30T22:01:42Z

In hide, the window is explicitly removed from the screen and hidden and the root view controller is set to nil to prevent any possible retain cycle between them

This fixes issue 32304 #32304

Summary

Restores ability to display an alert, even on iOS 15, without the user losing all ability to interact with the screen.

Changelog

[iOS] [Fixed] - Fix Alert.alert causing user interaction to be blocked on iOS 15 (issues/32304)

Test Plan

Build and run a react-native app on iOS 15. Display an alert and dismiss it.

Now try to interact with the app some more.

Expected: Buttons, scrollbars, etc.. in the main view of the app still function.

Wiithout this fix: It's not possible to interact with those components, as an invisible window is covering the entire screen, blocking all user interaction.

In hide, the window is explicitly removed from the screen and hidden and the root view controller is set to nil to prevent any possible retain cycle between them

paddlefish · 2021-09-30T22:05:03Z

The circlcie failure of test_docker_android must be unrelated -- this change only affects one .m file that is iOS only.

analysis-bot · 2021-09-30T22:30:39Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	7,714,509	+3
android	hermes	armeabi-v7a	7,243,229	+0
android	hermes	x86	8,134,234	+8
android	hermes	x86_64	8,099,586	+6
android	jsc	arm64-v8a	9,633,763	-29
android	jsc	armeabi-v7a	8,550,004	-25
android	jsc	x86	9,647,766	-34
android	jsc	x86_64	10,256,809	-26

Base commit: 34b7d22

danibonilha

I was facing the same issue, and come up with the _alertWindow.hidden = YES solution, then found this PR, but it's just a suggestion anyway, not sure about the implications of one vs other besides the apple recommendation.

danibonilha · 2021-10-01T16:32:12Z

React/CoreModules/RCTAlertController.m

@@ -35,6 +35,9 @@ - (void)show:(BOOL)animated completion:(void (^)(void))completion

 - (void)hide
 {
+  [_alertWindow resignKeyWindow];


There's a recommendation that we shouldn't call this directly on Apple docs

Ugh, my bad. I guess I made an invalid assumption that UIWindow was like NSWindow

danibonilha · 2021-10-01T16:33:52Z

React/CoreModules/RCTAlertController.m

+  [_alertWindow setHidden: YES];
+  _alertWindow.rootViewController = nil;


Wondering if _alertWindow.hidden = YES is enough to get the expected behavior

Good catch, let me experiment

Maybe this will work

if (@available(iOS 13.0, *)) { _alertWindow.windowScene = nil; }

https://developer.apple.com/documentation/uikit/uiwindow/3197962-windowscene?language=objc

@danibonilha thanks for your help. setting hidden was enough. I confirmed using Xcode debug view hierarchy

(If it hadn't worked you'd see another UIWindow hanging out in the scene)

For reference, here you can see the orphaned window (prior to applying this fix)

resignKeyWindow should not be called directly

asafkorem · 2021-12-28T16:57:02Z

@danibonilha @paddlefish Thank you for the investigation and the fix!
That's a serious issue, especially when using Detox for UI tests automation.. 😞

I can confirm that this bug happens to me with both iOS 15 and iOS 14.5.

facebook-github-bot · 2022-01-04T19:27:12Z

@lunaleaps has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

philIip · 2022-01-04T22:31:37Z

React/CoreModules/RCTAlertController.m

@@ -35,6 +35,7 @@ - (void)show:(BOOL)animated completion:(void (^)(void))completion

 - (void)hide
 {
+  [_alertWindow setHidden: YES];


[_alertWindow removeFromSuperview]; feels more correct to me - have you tried that?

From removeFromSuperview docs:

If the view’s superview is not nil, the superview releases the view.

AFAIK there shouldn't be any superview for _alertWindow (UIWindow).

The correct way to remove a UIWindow in iOS is to set its hidden property to YES.
Also, needs to remove all references to the window and ARC will automatically free this UIWindow.

The line after this change, set the _alertWindow reference to nil, but the window is already associated with a scene (see the screenshots from this PR). So we also need to remove the windowScene from that window, as recommended by Apple: https://developer.apple.com/documentation/uikit/uiwindowscene/3198091-windows.

To remove the window from the current scene, or move it to a different scene, change the value of the window's windowScene property.

@paddlefish since I can't push this change to your branch, I created another PR based on your changes and my comment and co-authorized you. @philIip & @lunaleaps you are welcome to take a look: #32833.

@asafkorem why wouldn't the alertWindow have a superview?

also, i'm not convinced that setting it to hidden is really related to this core issue. the issue is that setting _alertWindow to nil is not removing it from the view hierarchy (its retain count is not hitting 0), thus it is consuming touches. setting it to hidden unblocks the issue but doesn't fix the actual problem.

the latter point about the windowScene makes more sense in terms of the window lingering in memory somewhere. my suggestion with removing from the superview is potentially this UIWindow is still someone's subview and needs to be removed explicitly for the superview to clean it up.

@asafkorem why wouldn't the alertWindow have a superview?

@philIip the _alertWindow is a UIWindow, which doesn't have any superview by default, and there isn't any call where we define it as a subview of another view. Therefore, calling removeFromSuperview will do nothing, as the doc says (#32305 (comment)).

i'm not convinced that setting it to hidden is really related to this core issue

Setting the alert's window to be hidden from the hide method ensures that this UI operation is handled correctly, regardless of the memory management.

Removing any strong reference (made by this class) to this object is done to ensure that this window will be released from memory once no other object is pointing at it.

Theoretically, this window could be retained from another place, for example some object that kept a strong reference to the topmost visible window ATM this window was presented, or some object that keeps a strong reference to the generated alert view-controller (that has a strong reference to this window). In these examples, the window will still be stuck after calling hide until its reference count will be zero (same as the original bug).

@asafkorem I understand, where i'm coming from is that it is not impossible for UIWindow to have a superview (it is a UIView after all) since we can't control or know what UIKit does under the hood with the UIWindow management during runtime, and that could be a lingering strong reference. It's unlikely, but it's a possibility as well since the API allows it.

@philIip that's right, it is possible by the API, but this should not happen in this case, since this class present and manage this window, and it does not do that.
If that happens, for any reason, it might indicate on some other bug that must be addressed, and I strongly recommend not to obscure this potential bug from this method.

paddlefish · 2022-01-06T18:16:58Z

We can close this now. See #32833

Summary: Resolves this issue: #32304. **NOTE:** This PR is based on a prior PR for this fix: #32305, I've co-authorized its creator for this change (paddlefish). Without this change, calling to hide an alert, leaves a `UIWindow` that blocks user interactions with the screen. The correct way to remove a `UIWindow` in iOS is to set its hidden property to `YES`. Also, it is required to remove all references to the window (the associated `windowScene` for example) and ARC will automatically free this `UIWindow`. The line after this change, set the `_alertWindow` reference to `nil`, but the window is already associated with a scene (see the screenshots from [this PR](#32305 (comment))). So we also need to remove the `windowScene` from that window, as recommended by Apple: https://developer.apple.com/documentation/uikit/uiwindowscene/3198091-windows. >To remove the window from the current scene, or move it to a different scene, change the value of the window's windowScene property. ## Changelog  [iOS] [Fixed] - remove alert's window when call to `hide`. Pull Request resolved: #32833 Test Plan: See #32305 Reviewed By: hramos Differential Revision: D33460430 Pulled By: lunaleaps fbshipit-source-id: b13c2c7ee6404f1e1c787265bc4af8a31005bcf1

Fix Alert leaving behind a window and view controller (issue 32304)

a3019bf

In hide, the window is explicitly removed from the screen and hidden and the root view controller is set to nil to prevent any possible retain cycle between them

facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Sep 30, 2021

danibonilha reviewed Oct 1, 2021

View reviewed changes

react-native-bot added Bug Platform: iOS iOS applications. labels Oct 1, 2021

Simply hiding the window is enough to resolve the issue

d1cd0fe

resignKeyWindow should not be called directly

philIip reviewed Jan 4, 2022

View reviewed changes

asafkorem mentioned this pull request Jan 5, 2022

fix(iOS): remove alert's window when call to hide. #32833

Closed

paddlefish closed this Jan 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Alert leaving behind a window and view controller (issue 32304) #32305

Fix Alert leaving behind a window and view controller (issue 32304) #32305

paddlefish commented Sep 30, 2021

paddlefish commented Sep 30, 2021

analysis-bot commented Sep 30, 2021 •

edited

Loading

danibonilha left a comment •

edited

Loading

danibonilha Oct 1, 2021

paddlefish Oct 1, 2021

danibonilha Oct 1, 2021

paddlefish Oct 1, 2021

paddlefish Oct 1, 2021

paddlefish Oct 1, 2021

paddlefish Oct 1, 2021

paddlefish Oct 1, 2021

asafkorem commented Dec 28, 2021

facebook-github-bot commented Jan 4, 2022

philIip Jan 4, 2022

asafkorem Jan 5, 2022 •

edited

Loading

asafkorem Jan 5, 2022

asafkorem Jan 5, 2022

philIip Jan 7, 2022 •

edited

Loading

asafkorem Jan 7, 2022 •

edited

Loading

philIip Jan 7, 2022 •

edited

Loading

asafkorem Jan 7, 2022 •

edited

Loading

paddlefish commented Jan 6, 2022

		[_alertWindow setHidden: YES];
		_alertWindow.rootViewController = nil;

Fix Alert leaving behind a window and view controller (issue 32304) #32305

Fix Alert leaving behind a window and view controller (issue 32304) #32305

Conversation

paddlefish commented Sep 30, 2021

Summary

Changelog

Test Plan

paddlefish commented Sep 30, 2021

analysis-bot commented Sep 30, 2021 • edited Loading

danibonilha left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asafkorem commented Dec 28, 2021

facebook-github-bot commented Jan 4, 2022

Choose a reason for hiding this comment

asafkorem Jan 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philIip Jan 7, 2022 • edited Loading

Choose a reason for hiding this comment

asafkorem Jan 7, 2022 • edited Loading

Choose a reason for hiding this comment

philIip Jan 7, 2022 • edited Loading

Choose a reason for hiding this comment

asafkorem Jan 7, 2022 • edited Loading

Choose a reason for hiding this comment

paddlefish commented Jan 6, 2022

analysis-bot commented Sep 30, 2021 •

edited

Loading

danibonilha left a comment •

edited

Loading

asafkorem Jan 5, 2022 •

edited

Loading

philIip Jan 7, 2022 •

edited

Loading

asafkorem Jan 7, 2022 •

edited

Loading

philIip Jan 7, 2022 •

edited

Loading

asafkorem Jan 7, 2022 •

edited

Loading