[iOSTextInput] fix potential dangling pointer access #26547
Conversation
| CGRect _cachedFirstRect; | ||
| // The view has reached end of life, and is no longer | ||
| // allowed to access its textInputDelegate. | ||
| BOOL _decommissioned; |
There was a problem hiding this comment.
Why do we need this extra state, wouldn't removal of the FlutterTextInputView be enough to say it's decommissioned?
There was a problem hiding this comment.
It seems like the crux of your problem was the reuse of the views, right? Which you removed elsewhere.
There was a problem hiding this comment.
The crux of the problem is UIKit actually holds onto "decommissioned" views (e.g. UITextInteraction may keep a view alive after it's removed from its superview) and attempts to change the selection in the view (as a result the view will try to access the engine and send the update to the framework).
There was a problem hiding this comment.
I see, do we know who is holding onto the view? It might just be a matter of clearing out it's state as well. _textInterfaction.view = nil?
There was a problem hiding this comment.
(Sorry had to context switch). I think I tried that but didn't work. UITextInteraction.textInput is a weak ref: https://developer.apple.com/documentation/uikit/uitextinteraction/3255084-textinput?language=objc, UIKit seems to be retaining it somewhere else and removing the weak ref doesn't seem to do anything other than setting it to nil.
There was a problem hiding this comment.
I'm not sure there's a public API that allows us to release the reference. I think technically it's reasonable for uikit to retain a strong ref to a UITextInput instance until it's not needed.
There was a problem hiding this comment.
regarding setting _textInputDelegate instead (the first comment), I thought about that but slightly prefer having an additional lifecycle tracking variable that's irreversible. Then I'll be able to add asserts to verify that decommissioned views are not gonna get reused. But no strong preference.
There was a problem hiding this comment.
Okay, it's kind of weird but sounds like you've looking into all the alternatives so I can go along with this. Thanks for looking into it.
| - (void)cleanUpViewHierarchy:(BOOL)includeActiveView clearText:(BOOL)clearText { | ||
| - (void)cleanUpViewHierarchy:(BOOL)includeActiveView | ||
| clearText:(BOOL)clearText | ||
| decommisionOnly:(BOOL)decommisionOnly { |
There was a problem hiding this comment.
The method would be more clear if the argument you are adding talks about "removeFromSuperview" instead of thinking about the "only" modifier on the word "decomission".
gaaclarke
left a comment
gaaclarke
left a comment
There was a problem hiding this comment.
It's a little funky but I don't have alternatives and I think we asked all the right questions before hand, LGTM.
LongCatIsLooong
added
the
waiting for tree to go green
3 participants
Also fixes flutter/flutter#79031
Fixes the problem discovered in #26486.
Pre-launch Checklist
writing and running engine tests.
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.