[iOS] Eliminate strong retain cycle from VSyncClient#187164
Conversation
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a DisplayLinkRelay helper class in VSyncClient.swift to prevent a strong retain cycle between VSyncClient and CADisplayLink, and adds a corresponding unit test to verify deallocation. The reviewer points out that if VSyncClient is deallocated without explicit invalidation, the CADisplayLink will still leak on the run loop, and suggests invalidating the display link inside the relay if the target is nil.
| func onDisplayLink(_ link: CADisplayLink) { | ||
| target?.onDisplayLink(link) | ||
| } |
There was a problem hiding this comment.
If VSyncClient is deallocated without invalidate() being explicitly called, the CADisplayLink remains registered on the run loop. Because the run loop strongly retains the active CADisplayLink, and the display link strongly retains the DisplayLinkRelay, both will be leaked and the display link will continue to fire on every frame, wasting CPU and battery resources.
To prevent this, we can make the relay self-healing: if target is nil (meaning VSyncClient has been deallocated), we should invalidate the display link immediately. This will remove it from the run loop and clean up all associated resources.
func onDisplayLink(_ link: CADisplayLink) {
guard let target = target else {
link.invalidate()
return
}
target.onDisplayLink(link)
}There was a problem hiding this comment.
Bad bot. That's incorrect:
-
The retain cycle is gone because
DisplayLinkRelaynow holds a weak ref back to theVSyncClient, sodeinit()will be called if whoever holds a strong reference toVSyncClientreleases it. That's literally the whole point of this change. (The bot doesn't disagree with me on this point) -
We DO call
invalidate()explicitly indeinit(), which callsCADisplayLink.invalidate(), which ensures theCADisplayLinkis deregistered from the runloop and is released. (The bot seems to have missed this).
The proposed fix is nonsense though. It presumes that onDisplayLink is called, and that's not a safe assumption since the display link can be paused. But again, it doesn't remotely matter because the premise of this comment is incorrect.
There was a problem hiding this comment.
That said... the bot got me thinking about the invalidate() call in deinit().
Having the invalidate() call there is definitely the right approach. It guarantees we don't leak if someone misses an explicit deinit(). But the thing with deinit() is we have no idea what thread it'll run on.
Unlike Timer.invalidate() it's supposedly safe to call CADisplayLink.invalidate() on any thread. Apple's docs for CADisplayLink.invalidate() say "This method is thread safe, so you can call it from a thread separate to the one in which the display link runs."
So CADisplayLink.invalidate() promises to manage shunting the invalidation work over to the correct runloop, but I think it's probably better for us to just manually ensure we're invoking it on the correct thread ourselves. We have the task runner when we create this thing so we can use it to check if we're on the same thread and if so synchronously invalidate, otherwise post a task to the correct task runner, guaranteeing this gets deterministically enqueued behind any other tasks we've posted to the task runner, just like we do elsewhere we need thread-specific behaviour both in the engine and the embedders. I think it's also a good idea to be explicit about it just to help readers understand what's going on. Maybe I'm being overly paranoid, but we use TaskRunner as our abstraction for this, so we shouldn't bypass it, even implicitly.
Made that change and added a test for it.
So I dunno... good bot... sort of? It barked up the wrong tree but the tree still had something interesting in it.
14df5bd to
afc8ba8
Compare
| super.init() | ||
|
|
||
| let link = CADisplayLink(target: self, selector: #selector(onDisplayLink(_:))) | ||
| let relay = DisplayLinkRelay(target: self) |
There was a problem hiding this comment.
should it be called vsync client relay? (since it's wrapping vsync client)? I don't know 🤷
There was a problem hiding this comment.
Ah good catch! Yes -- FlutterBinaryMessengerRelay and FlutterTextureRegistryRelay are named after the things they act as a stand-in for rather than the thing that is meant to hold them (those classes at least could be held by lots of things).
Fixed!
| guard let link = displayLink else { return } | ||
| displayLink = nil | ||
|
|
||
| if taskRunner.runsTasksOnCurrentThread() { |
There was a problem hiding this comment.
this check is new. why are we adding it?
There was a problem hiding this comment.
See this comment: #187164 (comment)
Historically in the engine/embedders we're always very explicit about thread hops. CADisplayLink.invalidate() will automatically move the task to the correct runloop if it's called on another thread, but that sort of does an end run around the TaskRunner interface that we use everywhere else. Since we know it has to happen on the thread we created it on, we should explicitly guard against deinit() being called on the wrong thread and move the call to the right one explicitly.
Technically it's unnecessary, but it's clearer and more deterministic (depending on TaskRunner implementation).
afc8ba8 to
a1d2aa1
Compare
`CADisplayLink` strongly retains its target. Because `VSyncClient`
strongly retains the CADisplayLink instance, this created a strong
retain cycle where deinit was never called and the client leaked unless
`invalidate()` was manually invoked.
While the class was previously manually invalidated at all callsites,
this design is fragile and prone to leaks if a callsite fails to call
`invalidate()`.
This patch adds a `VSyncClientRelay` class to serve as a weak proxy
target for the `CADisplayLink`. The relay maintains a weak reference to
`VSyncClient`, breaking the retain cycle and allowing the client to
deallocate automatically when released. We've already got an
`invalidate()` call in `deinit()` which is now called once the last
reference ot `VSyncClient` is gone.
While we no longer leak if we fail to call `invalidate()` it's still
advisable to call it manually since we typically want to be very
deterministic about when we shut the link down.
The second important change that this patch introduces is that we now
guarantee `invalidate()` will be called from the same thread that
registered it. All callsites still do invalidate manually on the correct
thread but now that deinit() is a potential callsite for invalidation,
we should be extra careful, do a thread check, and force ourselves onto
the right thread. To do that, we now keep a reference to the task runner
that we used to register the display link and use that to deregister.
I've also added a unit test to verify that `VSyncClient` is successfully
deallocated without requiring an explicit call to `invalidate()`.
### Retain Graphs
#### Before
```mermaid
classDiagram
class Owner {
+FlutterVSyncClient client_
}
class VSyncClient {
+CADisplayLink displayLink
+onDisplayLink()
}
class CADisplayLink {
+id target
+SEL selector
}
class NSRunLoop {
+DisplayLink activeLinks
}
Owner --> VSyncClient : strong
VSyncClient --> CADisplayLink : strong (displayLink)
CADisplayLink --> VSyncClient : strong (target)
NSRunLoop --> CADisplayLink : strong (when active)
```
#### After
```mermaid
classDiagram
class Owner {
+VSyncClient client_
}
class VSyncClient {
+CADisplayLink displayLink
+onDisplayLink()
}
class CADisplayLink {
+id target
}
class VSyncClientRelay {
+weak VSyncClient target
+onDisplayLink()
}
class NSRunLoop {
+DisplayLink activeLinks
}
Owner --> VSyncClient : strong
VSyncClient --> CADisplayLink : strong (displayLink)
CADisplayLink --> VSyncClientRelay : strong (target)
VSyncClientRelay ..> VSyncClient : "weak (target)"
NSRunLoop --> CADisplayLink : strong (when active)
```
### Background
This circular reference has been around since pre-Flutter 1.0, going
back to the original C++ implementation:
1. Commit 54ee61e (Oct 2016) — "Migrate vsync away from Mojo
services": The strong retain cycle was introduced back in the
MRC days when `VSyncClient` was first created in
`vsync_waiter_ios.mm`. The client retained `CADisplayLink`, which
strongly retained the client as its target without any invalidation,
resulting in the memory leak.
2. Commit 5f95d3b (Aug 2024) — "Migrate vsync_waiter_ios to ARC":
The vsync waiter code was migrated to ARC. The strong retain cycle
and the leak remained.
3. Commit 6eea11a (Apr 2026) — "[iOS] Extract FlutterVSyncClient
from vsync_waiter_ios": `FlutterVSyncClient` was extracted to
`FlutterVSyncClient.mm`. I added a manual `-[FlutterVSyncClient
invalidate]` method was to break the retain cycle by unregistering
the display link from the run loop at the callsites.
4. Commit 5bcd096 (May 2026) — "[iOS] Migrate VSyncClient and
DisplayLinkManager to Swift": I migrated `FlutterVSyncClient` to
Swift 1:1. The manual invalidation calls were preserved but we
deferred adding a more future-proof fix to this patch.
5. This Commit (May 2026) Eliminates the strong retain cycle entirely
using a weak `VSyncClientRelay` proxy, so we no longer need to rely
on manual invalidation calls. They're still advisable though since
typically we want to be really deterministic about when we invalidate
the vsync connection.
a1d2aa1 to
68de83f
Compare
|
An existing Git SHA, To re-trigger presubmits after closing or re-opeing a PR, or pushing a HEAD commit (i.e. with |
|
An existing Git SHA, To re-trigger presubmits after closing or re-opeing a PR, or pushing a HEAD commit (i.e. with |
|
Detected the If you add the The "Merge" button is also unlocked. To bypass presubmits as well as the tree status, press the GitHub "Add to Merge Queue". |
|
Landing to see if we can get the tree green. This one has passed presubmits 3+ times now. The tree is red on a SHA-specific benchmark lockfile that all the windows builders are timing out on. See discussion at https://discord.com/channels/608014603317936148/1290464157765865552/1509495831571726407. Landing anything will move us to a new SHA which should prevent the issue. Build failures in question:
|
flutter/flutter@c8f2f16...e70534d 2026-05-28 30870216+gaaclarke@users.noreply.github.com Linux glyph gamma correction (flutter/flutter#187122) 2026-05-28 chris@bracken.jp [iOS] Eliminate strong retain cycle from VSyncClient (flutter/flutter#187164) 2026-05-28 katelovett@google.com Revert "[pubspec] Bump Dart SDK constraint to ^3.13.0 (#186957)" (flutter/flutter#187209) 2026-05-28 engine-flutter-autoroll@skia.org Roll Skia from f1b8ba877c07 to 32acea791248 (3 revisions) (flutter/flutter#187220) 2026-05-27 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from k9EizfEGJO4WwQN9-... to SBpmmPxqx3lAvGojE... (flutter/flutter#187211) 2026-05-27 bdero@google.com [Impeller] Add block-compressed texture format support (BC, ETC2, ASTC) (flutter/flutter#187077) 2026-05-27 katelovett@google.com Disable analyzer (flutter/flutter#187205) 2026-05-27 bdero@google.com [Flutter GPU] Add explicit draw counts (reland) (flutter/flutter#187192) 2026-05-27 bdero@google.com [Flutter GPU] Inject per-backend defines into shader bundle compilation (flutter/flutter#187081) 2026-05-27 engine-flutter-autoroll@skia.org Roll Skia from fa944af10f91 to f1b8ba877c07 (13 revisions) (flutter/flutter#187194) 2026-05-27 47866232+chunhtai@users.noreply.github.com Fixes bug when changing accessibilityFocusBlockType doesn't update ch… (flutter/flutter#186596) 2026-05-27 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#187191) 2026-05-27 kevmoo@users.noreply.github.com [web, tool] Support recompiling shaders and unify asset processing (2nd try) (flutter/flutter#186902) 2026-05-27 robert.ancell@canonical.com Fix crash if FlView is destroyed during a draw. (flutter/flutter#186848) 2026-05-27 engine-flutter-autoroll@skia.org Roll Packages from fc795e5 to 4b424d7 (4 revisions) (flutter/flutter#187174) 2026-05-27 15619084+vashworth@users.noreply.github.com Stop prefetching Swift packages in pub get (flutter/flutter#187113) 2026-05-27 jesswon@google.com Update CI with newer android sdk package (flutter/flutter#187143) 2026-05-27 jason-simmons@users.noreply.github.com Add a tag to the Linux platform properties in .ci.yaml that specifies x64 CPUs (flutter/flutter#187124) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC boetger@google.com,stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
…r#11799) flutter/flutter@c8f2f16...e70534d 2026-05-28 30870216+gaaclarke@users.noreply.github.com Linux glyph gamma correction (flutter/flutter#187122) 2026-05-28 chris@bracken.jp [iOS] Eliminate strong retain cycle from VSyncClient (flutter/flutter#187164) 2026-05-28 katelovett@google.com Revert "[pubspec] Bump Dart SDK constraint to ^3.13.0 (#186957)" (flutter/flutter#187209) 2026-05-28 engine-flutter-autoroll@skia.org Roll Skia from f1b8ba877c07 to 32acea791248 (3 revisions) (flutter/flutter#187220) 2026-05-27 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from k9EizfEGJO4WwQN9-... to SBpmmPxqx3lAvGojE... (flutter/flutter#187211) 2026-05-27 bdero@google.com [Impeller] Add block-compressed texture format support (BC, ETC2, ASTC) (flutter/flutter#187077) 2026-05-27 katelovett@google.com Disable analyzer (flutter/flutter#187205) 2026-05-27 bdero@google.com [Flutter GPU] Add explicit draw counts (reland) (flutter/flutter#187192) 2026-05-27 bdero@google.com [Flutter GPU] Inject per-backend defines into shader bundle compilation (flutter/flutter#187081) 2026-05-27 engine-flutter-autoroll@skia.org Roll Skia from fa944af10f91 to f1b8ba877c07 (13 revisions) (flutter/flutter#187194) 2026-05-27 47866232+chunhtai@users.noreply.github.com Fixes bug when changing accessibilityFocusBlockType doesn't update ch… (flutter/flutter#186596) 2026-05-27 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#187191) 2026-05-27 kevmoo@users.noreply.github.com [web, tool] Support recompiling shaders and unify asset processing (2nd try) (flutter/flutter#186902) 2026-05-27 robert.ancell@canonical.com Fix crash if FlView is destroyed during a draw. (flutter/flutter#186848) 2026-05-27 engine-flutter-autoroll@skia.org Roll Packages from fc795e5 to 4b424d7 (4 revisions) (flutter/flutter#187174) 2026-05-27 15619084+vashworth@users.noreply.github.com Stop prefetching Swift packages in pub get (flutter/flutter#187113) 2026-05-27 jesswon@google.com Update CI with newer android sdk package (flutter/flutter#187143) 2026-05-27 jason-simmons@users.noreply.github.com Add a tag to the Linux platform properties in .ci.yaml that specifies x64 CPUs (flutter/flutter#187124) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC boetger@google.com,stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
|
@cbracken Glad to see that this issue is gone! While working on an unrelated iOS 26 fix (my MR is still open for months), I noticed Your solution looks clean and correct. Thanks for tracking this down! |
|
@cbracken I dived more into it - on iOS 27.0 I face with leaking
It is not retained by our code now, but test fails as it covers more that practically necessary.
|
[iOS] Eliminate strong retain cycle from VSyncClient
`CADisplayLink` strongly retains its target. Because `VSyncClient`
strongly retains the CADisplayLink instance, this created a strong
retain cycle where deinit was never called and the client leaked unless
`invalidate()` was manually invoked.
While the class was previously manually invalidated at all callsites,
this design is fragile and prone to leaks if a callsite fails to call
`invalidate()`.
This patch adds a `VSyncClientRelay` class to serve as a weak proxy
target for the `CADisplayLink`. The relay maintains a weak reference to
`VSyncClient`, breaking the retain cycle and allowing the client to
deallocate automatically when released. We've already got an
`invalidate()` call in `deinit()` which is now called once the last
reference ot `VSyncClient` is gone.
While we no longer leak if we fail to call `invalidate()` it's still
advisable to call it manually since we typically want to be very
deterministic about when we shut the link down.
The second important change that this patch introduces is that we now
guarantee `invalidate()` will be called from the same thread that
registered it. All callsites still do invalidate manually on the correct
thread but now that deinit() is a potential callsite for invalidation,
we should be extra careful, do a thread check, and force ourselves onto
the right thread. To do that, we now keep a reference to the task runner
that we used to register the display link and use that to deregister.
I've also added a unit test to verify that `VSyncClient` is successfully
deallocated without requiring an explicit call to `invalidate()`.
### Retain Graphs
#### Before
```mermaid
classDiagram
class Owner {
+FlutterVSyncClient client_
}
class VSyncClient {
+CADisplayLink displayLink
+onDisplayLink()
}
class CADisplayLink {
+id target
+SEL selector
}
class NSRunLoop {
+DisplayLink activeLinks
}
Owner --> VSyncClient : strong
VSyncClient --> CADisplayLink : strong (displayLink)
CADisplayLink --> VSyncClient : strong (target)
NSRunLoop --> CADisplayLink : strong (when active)
```
#### After
```mermaid
classDiagram
class Owner {
+VSyncClient client_
}
class VSyncClient {
+CADisplayLink displayLink
+onDisplayLink()
}
class CADisplayLink {
+id target
}
class VSyncClientRelay {
+weak VSyncClient target
+onDisplayLink()
}
class NSRunLoop {
+DisplayLink activeLinks
}
Owner --> VSyncClient : strong
VSyncClient --> CADisplayLink : strong (displayLink)
CADisplayLink --> VSyncClientRelay : strong (target)
VSyncClientRelay ..> VSyncClient : "weak (target)"
NSRunLoop --> CADisplayLink : strong (when active)
```
### Background
This circular reference has been around since pre-Flutter 1.0, going
back to the original C++ implementation:
1. Commit 54ee61e (Oct 2016) — "Migrate vsync away from Mojo
services": The strong retain cycle was introduced back in the
MRC days when `VSyncClient` was first created in
`vsync_waiter_ios.mm`. The client retained `CADisplayLink`, which
strongly retained the client as its target without any invalidation,
resulting in the memory leak.
2. Commit 5f95d3b (Aug 2024) — "Migrate vsync_waiter_ios to ARC":
The vsync waiter code was migrated to ARC. The strong retain cycle
and the leak remained.
3. Commit 6eea11a (Apr 2026) — "[iOS] Extract FlutterVSyncClient
from vsync_waiter_ios": `FlutterVSyncClient` was extracted to
`FlutterVSyncClient.mm`. I added a manual `-[FlutterVSyncClient
invalidate]` method was to break the retain cycle by unregistering
the display link from the run loop at the callsites.
4. Commit 5bcd096 (May 2026) — "[iOS] Migrate VSyncClient and
DisplayLinkManager to Swift": I migrated `FlutterVSyncClient` to
Swift 1:1. The manual invalidation calls were preserved but we
deferred adding a more future-proof fix to this patch.
5. This Commit (May 2026) Eliminates the strong retain cycle entirely
using a weak `VSyncClientRelay` proxy, so we no longer need to rely
on manual invalidation calls. They're still advisable though since
typically we want to be really deterministic about when we invalidate
the vsync connection.



[iOS] Eliminate strong retain cycle from VSyncClient
CADisplayLinkstrongly retains its target. BecauseVSyncClientstrongly retains the CADisplayLink instance, this created a strong
retain cycle where deinit was never called and the client leaked unless
invalidate()was manually invoked.While the class was previously manually invalidated at all callsites,
this design is fragile and prone to leaks if a callsite fails to call
invalidate().This patch adds a
VSyncClientRelayclass to serve as a weak proxytarget for the
CADisplayLink. The relay maintains a weak reference toVSyncClient, breaking the retain cycle and allowing the client todeallocate automatically when released. We've already got an
invalidate()call indeinit()which is now called once the lastreference ot
VSyncClientis gone.While we no longer leak if we fail to call
invalidate()it's stilladvisable to call it manually since we typically want to be very
deterministic about when we shut the link down.
The second important change that this patch introduces is that we now
guarantee
invalidate()will be called from the same thread thatregistered it. All callsites still do invalidate manually on the correct
thread but now that deinit() is a potential callsite for invalidation,
we should be extra careful, do a thread check, and force ourselves onto
the right thread. To do that, we now keep a reference to the task runner
that we used to register the display link and use that to deregister.
I've also added a unit test to verify that
VSyncClientis successfullydeallocated without requiring an explicit call to
invalidate().Retain Graphs
Before
classDiagram class Owner { +FlutterVSyncClient client_ } class VSyncClient { +CADisplayLink displayLink +onDisplayLink() } class CADisplayLink { +id target +SEL selector } class NSRunLoop { +DisplayLink activeLinks } Owner --> VSyncClient : strong VSyncClient --> CADisplayLink : strong (displayLink) CADisplayLink --> VSyncClient : strong (target) NSRunLoop --> CADisplayLink : strong (when active)After
classDiagram class Owner { +VSyncClient client_ } class VSyncClient { +CADisplayLink displayLink +onDisplayLink() } class CADisplayLink { +id target } class VSyncClientRelay { +weak VSyncClient target +onDisplayLink() } class NSRunLoop { +DisplayLink activeLinks } Owner --> VSyncClient : strong VSyncClient --> CADisplayLink : strong (displayLink) CADisplayLink --> VSyncClientRelay : strong (target) VSyncClientRelay ..> VSyncClient : "weak (target)" NSRunLoop --> CADisplayLink : strong (when active)Background
This circular reference has been around since pre-Flutter 1.0, going
back to the original C++ implementation:
services": The strong retain cycle was introduced back in the
MRC days when
VSyncClientwas first created invsync_waiter_ios.mm. The client retainedCADisplayLink, whichstrongly retained the client as its target without any invalidation,
resulting in the memory leak.
The vsync waiter code was migrated to ARC. The strong retain cycle
and the leak remained.
from vsync_waiter_ios":
FlutterVSyncClientwas extracted toFlutterVSyncClient.mm. I added a manual-[FlutterVSyncClient invalidate]method was to break the retain cycle by unregisteringthe display link from the run loop at the callsites.
DisplayLinkManager to Swift": I migrated
FlutterVSyncClienttoSwift 1:1. The manual invalidation calls were preserved but we
deferred adding a more future-proof fix to this patch.
using a weak
VSyncClientRelayproxy, so we no longer need to relyon manual invalidation calls. They're still advisable though since
typically we want to be really deterministic about when we invalidate
the vsync connection.