Skip to content

[iOS] Eliminate strong retain cycle from VSyncClient#187164

Merged
cbracken merged 2 commits into
flutter:masterfrom
cbracken:vsync-weak-relay
May 28, 2026
Merged

[iOS] Eliminate strong retain cycle from VSyncClient#187164
cbracken merged 2 commits into
flutter:masterfrom
cbracken:vsync-weak-relay

Conversation

@cbracken

@cbracken cbracken commented May 27, 2026

Copy link
Copy Markdown
Member

[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

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

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

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.

@flutter-dashboard flutter-dashboard Bot added the CICD Run CI/CD label May 27, 2026
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Warning

Gemini encountered an error creating the review. You can try again by commenting /gemini review.

@cbracken cbracken requested a review from hellohuanlin May 27, 2026 11:45
@github-actions github-actions Bot added platform-ios iOS applications specifically engine flutter/engine related. See also e: labels. team-ios Owned by iOS platform team labels May 27, 2026
@cbracken cbracken requested a review from vashworth May 27, 2026 11:46
@flutter flutter deleted a comment from gemini-code-assist Bot May 27, 2026
@cbracken

Copy link
Copy Markdown
Member Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Comment on lines +204 to +206
func onDisplayLink(_ link: CADisplayLink) {
target?.onDisplayLink(link)
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

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

@cbracken cbracken May 27, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Bad bot. That's incorrect:

  • The retain cycle is gone because DisplayLinkRelay now holds a weak ref back to the VSyncClient, so deinit() will be called if whoever holds a strong reference to VSyncClient releases 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 in deinit(), which calls CADisplayLink.invalidate(), which ensures the CADisplayLink is 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.

@cbracken cbracken May 27, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

@cbracken cbracken force-pushed the vsync-weak-relay branch from 14df5bd to afc8ba8 Compare May 27, 2026 14:57
@cbracken cbracken requested a review from a team as a code owner May 27, 2026 14:57
@github-actions github-actions Bot removed the CICD Run CI/CD label May 27, 2026
@cbracken cbracken added the CICD Run CI/CD label May 27, 2026
hellohuanlin
hellohuanlin previously approved these changes May 27, 2026
super.init()

let link = CADisplayLink(target: self, selector: #selector(onDisplayLink(_:)))
let relay = DisplayLinkRelay(target: self)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should it be called vsync client relay? (since it's wrapping vsync client)? I don't know 🤷

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this check is new. why are we adding it?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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

`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.
@cbracken cbracken force-pushed the vsync-weak-relay branch from a1d2aa1 to 68de83f Compare May 27, 2026 23:00
@flutter-dashboard flutter-dashboard Bot added the CICD Run CI/CD label May 27, 2026
@cbracken cbracken added CICD Run CI/CD and removed CICD Run CI/CD labels May 28, 2026
@fluttergithubbot

Copy link
Copy Markdown
Contributor

An existing Git SHA, 68de83f1e126f87c0e5d19206a76ce66fd30532f, was detected, and no actions were taken.

To re-trigger presubmits after closing or re-opeing a PR, or pushing a HEAD commit (i.e. with --force) that already was pushed before, push a blank commit (git commit --allow-empty -m "Trigger Build") or rebase to continue.

@cbracken cbracken enabled auto-merge May 28, 2026 04:44
@cbracken cbracken added CICD Run CI/CD and removed CICD Run CI/CD labels May 28, 2026
@fluttergithubbot

Copy link
Copy Markdown
Contributor

An existing Git SHA, 68de83f1e126f87c0e5d19206a76ce66fd30532f, was detected, and no actions were taken.

To re-trigger presubmits after closing or re-opeing a PR, or pushing a HEAD commit (i.e. with --force) that already was pushed before, push a blank commit (git commit --allow-empty -m "Trigger Build") or rebase to continue.

@github-actions github-actions Bot removed the CICD Run CI/CD label May 28, 2026
@cbracken cbracken added CICD Run CI/CD emergency Jump the queue; land PR in front of all others; only use for emergencies labels May 28, 2026
@flutter-dashboard

Copy link
Copy Markdown

Detected the emergency label.

If you add the autosubmit label, the bot will wait until all presubmits pass but ignore the tree status, allowing fixes for tree breakages while still validating that they don't break any existing presubmits.

The "Merge" button is also unlocked. To bypass presubmits as well as the tree status, press the GitHub "Add to Merge Queue".

@cbracken

cbracken commented May 28, 2026

Copy link
Copy Markdown
Member Author

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:

@cbracken cbracken disabled auto-merge May 28, 2026 11:24
@cbracken cbracken enabled auto-merge May 28, 2026 11:24
@cbracken cbracken added this pull request to the merge queue May 28, 2026
Merged via the queue into flutter:master with commit 621f035 May 28, 2026
203 of 204 checks passed
@cbracken cbracken deleted the vsync-weak-relay branch May 28, 2026 13:29
albertoblue87-netizen

This comment was marked as off-topic.

auto-submit Bot pushed a commit to flutter/packages that referenced this pull request May 28, 2026
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
creatorpiyush pushed a commit to creatorpiyush/packages that referenced this pull request Jun 10, 2026
…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
@Akhrameev

Akhrameev commented Jun 23, 2026

Copy link
Copy Markdown

@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 testDisplayLinkIsDeallocatedOnTaskRunnerThread was consistently failing on the iOS 26.x simulator (but passing on iOS 18.6). I logged it to investigate later, since it reproduced on clean master with no other changes.

Your solution looks clean and correct. Thanks for tracking this down!

@Akhrameev

Copy link
Copy Markdown

@cbracken I dived more into it - on iOS 27.0 I face with leaking CADisplayLink

Screenshot 2026-06-25 at 22 51 08

It is not retained by our code now, but test fails as it covers more that practically necessary.

_CADisplisplayLinkAssertion holds the CADisplayLink without our control and it is not an issue in current code - just in test.

@Akhrameev

Copy link
Copy Markdown

follow-up: on iOS 26.4.1 memory graph is more simple.

Compare iOS 26.4.1 version (I have a TestViewController to retain the link here):

Screenshot 2026-06-25 at 22 47 55

iOS 27.0 version (with TestViewController as well):

Screenshot 2026-06-25 at 22 50 21

iOS 27.0 has a new strong reference to CADisplayLink via _CADisplisplayLinkAssertion. So when we remove our strong reference, CADisplayLink does not get freed in test.

Note: TestViewController had buttons to launch the test code to access memory graph to understand what happens on iOS 27.0 comparing to earlier versions.

via-guy pushed a commit to via-guy/flutter that referenced this pull request Jun 26, 2026
[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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CICD Run CI/CD emergency Jump the queue; land PR in front of all others; only use for emergencies engine flutter/engine related. See also e: labels. platform-ios iOS applications specifically team-ios Owned by iOS platform team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants