[iOS][test] Fix VSyncClient display link deallocation test on iOS 27#188627
Conversation
Follow-up to flutter#187164, which introduced this test to verify the retain cycle fix. The test passes on iOS 18 and 26 but fails on iOS 27, where QuartzCore holds a _CADisplayLinkAssertion on any registered CADisplayLink until an internal display server event fires. A never-unpaused link (as created in this test) may therefore outlive VSyncClient itself - this is not a retain cycle, just deferred OS-level cleanup. Rename the test to testDeallocatesAfterRegistrationCompletes and remove the weakDisplayLink assertion, which was testing iOS-internal cleanup timing rather than the no-retain-cycle guarantee the test is meant to provide. The weakClient assertion is retained and continues to cover the actual invariant.
github-actions
Bot
added
platform-ios
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates VSyncClientTest.swift by replacing the testDisplayLinkIsDeallocatedOnTaskRunnerThread test with testDeallocatesAfterRegistrationCompletes to handle iOS 27+ display link behavior, simplifying the test structure. Feedback was provided to correct a comment stating that invalidate() executes synchronously inline, as the invalidation task is actually posted asynchronously to the task runner thread.
|
Here are memgraphs related to investigation. To gather them I've created a |
That's the memory graph for iOS 27 when `TestViewController` and `_CADisplayLinkAssertion` both hold strong references to it
so when `TestViewController` initiates the display link disposal (exactly the same code as in test) it is still held by `_CADisplayLinkAssertion`.
Compare with iOS 26 memory graph: 
Here Verdict: in test we checked not only things that we can control, but also the non-existence of internal strong reference holders. This assumption was right before iSO 27.0, but now it is wrong - so test should check only thing that is under our control. |
hellohuanlin
left a comment
hellohuanlin
left a comment
There was a problem hiding this comment.
LGTM!
CC @cbracken who may find this interesting.
|
@Akhrameev Thanks so much for fixing this! You're 100% right; the test was over-testing. This lgtm! |
cbracken
added
the
autosubmit
auto-submit
Bot
removed
the
autosubmit
|
autosubmit label was removed for flutter/flutter/188627, because - The status or check suite Linux web_canvaskit_tests_2 has failed. Please fix the issues identified (or deflake) before re-applying this label. |
cbracken
added
the
autosubmit
flutter-dashboard
Bot
removed
the
autosubmit
flutter/flutter@dc2a870...f7b66f3 2026-07-10 [email protected] [iOS][test] Fix VSyncClient display link deallocation test on iOS 27 (flutter/flutter#188627) 2026-07-10 [email protected] Fix broken listener code in WindowScope (flutter/flutter#189208) 2026-07-10 [email protected] [web] Preserve text field focus across tab switches (flutter/flutter#188738) 2026-07-10 [email protected] [Impeller] Recycle HostBuffer arena entries only after GPU completion (flutter/flutter#188965) 2026-07-10 [email protected] Assert TextStyle height is not NaN (flutter/flutter#186617) 2026-07-10 [email protected] Roll Fuchsia Test Scripts from dFkTCiDxEtPxYK5Nn... to wLST_A-xfOeGT_5mj... (flutter/flutter#189259) 2026-07-10 [email protected] Roll Dart SDK from a11fb7ed40a5 to 0fc1668c4af4 (5 revisions) (flutter/flutter#189257) 2026-07-10 [email protected] Adds a missing `await` to a `FutureOr` (flutter/flutter#189198) 2026-07-10 [email protected] [web] Repair RenderCanvas CSS size drift (flutter/flutter#188797) 2026-07-10 [email protected] Roll Fuchsia Test Scripts from s5_gZFJ8De9AJalTw... to dFkTCiDxEtPxYK5Nn... (flutter/flutter#189215) 2026-07-10 [email protected] Roll Fuchsia Linux SDK from QcRFUtvCw2EobfJ8s... to czpzDg9ABY2oKLAOY... (flutter/flutter#189222) 2026-07-10 [email protected] Update Depdencies Used By `flutter_engine_group_performance` (flutter/flutter#189229) 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 [email protected],[email protected] 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
3 participants
Follow-up to #187164, which fixed a retain cycle where
CADisplayLinkwas directly targetingVSyncClientby introducingVSyncClientRelayas a weak-proxy target. The accompanying test verified the fix by checking bothweakClient == nil(no retain cycle) andweakDisplayLink == nil(display link fully released) after deallocation. The test passes on iOS 18 and 26 but fails reproducibly on iOS 27. This PR renames and adjusts the test to reflect what it actually verifies: the absence of a retain cycle, not iOS-internal display server cleanup timing.Investigation
On iOS 27,
weakClient == nilpasses:VSyncClientis correctly released. HoweverweakDisplayLinkremains non-nil indefinitely, causing the test to time out._CADisplayLinkAssertionis aQuartzCore-internal object. It is created whenCADisplayLinkis added to a run loop and registered with the display server. On iOS 26 and earlier, this assertion is released synchronously whenlink.invalidate()is called. On iOS 27, the assertion persists: its release appears to be deferred to an internal display server event that never fires for a link that was never unpaused.Approaches ruled out
remove + invalidate on task runner thread(original) -> PERMANENT HOLDremove + invalidate on main run loop-> PERMANENT HOLDisPaused = false + remove + invalidate in same synchronous block-> PERMANENT HOLDThe display server is out-of-process. No synchronous call sequence from the app side is sufficient to force it to release
_CADisplayLinkAssertionfor a link that was never activated.Fix
The
weakDisplayLinkassertion was testing iOS-internal display server cleanup timing rather than the retain cycle the test was designed to catch. SinceVSyncClientitself is correctly released (weakClient == nil) on all iOS versions, there is no leak in Flutter's object graph.The test is renamed to
testDeallocatesAfterRegistrationCompletesand theweakDisplayLinkcheck is removed. TheweakClientassertion is retained and continues to verify the no-retain-cycle invariant.Test Results
| --- | --- | --- |
| iOS | Before | Now |
| 18.6 | Pass | Pass |
| 26.0 | Pass | Pass |
| 26.0.1 | Pass | Pass |
| 26.1 | Pass | Pass |
| 27.0 | Fail | Pass |
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.