Skip to content

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

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

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

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a3019bf
Fix Alert leaving behind a window and view controller (issue 32304)
paddlefish Sep 30, 2021
d1cd0fe
Simply hiding the window is enough to resolve the issue
paddlefish Oct 1, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions React/CoreModules/RCTAlertController.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ - (void)show:(BOOL)animated completion:(void (^)(void))completion

- (void)hide
{
[_alertWindow setHidden: YES];
Copy link
Contributor

@philIip philIip Jan 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@asafkorem asafkorem Jan 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

@asafkorem asafkorem Jan 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@asafkorem asafkorem Jan 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

@philIip philIip Jan 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

@asafkorem asafkorem Jan 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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).

Copy link
Contributor

@philIip philIip Jan 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

@asafkorem asafkorem Jan 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

_alertWindow = nil;
}

Expand Down