-
Notifications
You must be signed in to change notification settings - Fork 6k
[iOS] fix Flutter Metal Layer retain cycle. #54097
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
alternatively we could say that the devicelab tests not immediately crashing demonstrates that this works :) |
Check out: engine/shell/platform/darwin/ios/framework/Source/VsyncWaiterIosTest.mm Lines 130 to 144 in 74785a7
engine/shell/platform/darwin/ios/framework/Source/vsync_waiter_ios.mm Lines 42 to 46 in 74785a7
engine/shell/platform/darwin/ios/framework/Source/vsync_waiter_ios.mm Lines 137 to 140 in 74785a7
|
|
Another way to fix this: #54119 |
| IOSSurfaceMetalImpeller::~IOSSurfaceMetalImpeller() = default; | ||
| IOSSurfaceMetalImpeller::~IOSSurfaceMetalImpeller() { | ||
| if ([layer_ isKindOfClass:[FlutterMetalLayer class]]) { | ||
| [(FlutterMetalLayer*)layer_.get() invalidate]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a preference for @knopp s patch. It really brittle for IOSSurfaceMetalImpeller to know to check if the layers is of a specific type to call a method on it.
|
Let's do the other one |
The display link and/or notification observer was keeping the FlutterMetalLayer alive. Add an invalidate method and call it in the IOSSurface dtor.
How do we test this?