Started making the accessibility bridge retain its view. #12225
Conversation
|
This platform view thing already created a bunch of conceptual mismatches. Let's rename it in flutter/flutter#40283 (tangential to this bug). |
|
I somehow was sure though that I saw some PRs previously which did the reverse to fix some other bug. I don't think that other bug had a test added. Perhaps @amirh or @mklim might remember what the context was. Also https://docs.google.com/document/d/1F5BrSpcpctKAnXvf5Vr3twmHcyIipU1zKH_AJ3Mkuao/ Matt intended to refactor this accessibility bridge ownership too. |
@cyanglaz and @goderbauer has been working on this recently and may have more context. |
| weak_factory_(this), | ||
| previous_route_id_(0), | ||
| previous_routes_({}) { | ||
| [view_ retain]; |
There was a problem hiding this comment.
The UIView probably has a reference back to the VC too right? Wouldn't this produce a leak after a VC is dismissed? Since this object is probably never destroyed per engine.
There was a problem hiding this comment.
Yes, sorta. The UIView would live until a new view controller is presented. This isn't ideal so I'm working on an alternative PR right now.
There was a problem hiding this comment.
I've authored #12232. That will solve the original issue. You know what though, we might still want this PR. This one turns a crash into a leak and that one eliminates both. This might be helpful to protect against future bugs.
|
I've authored #12232 as a better fix for the original problem. We might want to keep this though as a safety net since it turns a crash into a memory leak. It could protect us in the future. |
|
This isn't necessary considering the more complete solution presented in the other PR, going to drop it. |
4 participants
Related issue: flutter/flutter#33173
The referenced issue mentions this crash, but I believe that the originally stated bug is something different since this is all related to iOS.
Why should we retain here? Because the AccessibilityBridge can live longer than the view. Imagine you have a UINavigationController and you push and pop a FlutterViewController. There is no way for AccessibilityBridge to know that the view has been deleted and he is still referencing it.
This potentially means that the FlutterView can end up living in memory longer than we expected, the last one will be retained until a new one is presented to reset the AccessibilityBridge. A potentially better relationship would to make FlutterView have an AccessibilityBridge instead of making PlatformViewIOS have one. That would allow it to be deleted in lock-step with the FlutterView. That would require a much more complicated reworking of the code though.