Fix _scheduleSystemFontsUpdate assertion during non-idle scheduler phase#184332
Conversation
There was a problem hiding this comment.
Code Review
This pull request modifies the RelayoutWhenSystemFontsChangeMixin to allow system font updates to be scheduled during any scheduler phase. When the scheduler is not idle, the update is now deferred using a post-frame callback to avoid assertion failures. A regression test was added to verify this behavior, and the AUTHORS file was updated. Feedback was provided to improve code conciseness by refactoring the local scheduleUpdate function into a tear-off.
| void scheduleUpdate() { | ||
| SchedulerBinding.instance.scheduleFrameCallback((Duration timeStamp) { | ||
| assert(_hasPendingSystemFontsDidChangeCallBack); | ||
| _hasPendingSystemFontsDidChangeCallBack = false; | ||
| assert( | ||
| attached || (debugDisposed ?? true), | ||
| '$this is detached during ${SchedulerBinding.instance.schedulerPhase} but is not disposed.', | ||
| ); | ||
| if (attached) { | ||
| systemFontsDidChange(); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| if (SchedulerBinding.instance.schedulerPhase == SchedulerPhase.idle) { | ||
| scheduleUpdate(); | ||
| } else { | ||
| // Not idle — defer to after the current frame completes. | ||
| SchedulerBinding.instance.addPostFrameCallback((_) { | ||
| scheduleUpdate(); | ||
| }); | ||
| } |
There was a problem hiding this comment.
For slightly better conciseness, you can adjust the signature of scheduleUpdate to accept an optional Duration argument. This allows you to pass scheduleUpdate directly to addPostFrameCallback as a tear-off, removing the need for a wrapping closure.
void scheduleUpdate([Duration? _]) {
SchedulerBinding.instance.scheduleFrameCallback((Duration timeStamp) {
assert(_hasPendingSystemFontsDidChangeCallBack);
_hasPendingSystemFontsDidChangeCallBack = false;
assert(
attached || (debugDisposed ?? true),
'$this is detached during ${SchedulerBinding.instance.schedulerPhase} but is not disposed.',
);
if (attached) {
systemFontsDidChange();
}
});
}
if (SchedulerBinding.instance.schedulerPhase == SchedulerPhase.idle) {
scheduleUpdate();
} else {
// Not idle — defer to after the current frame completes.
SchedulerBinding.instance.addPostFrameCallback(scheduleUpdate);
}There was a problem hiding this comment.
Good suggestion — applied. Using a tear-off with void scheduleUpdate([Duration? _]) is cleaner. Updated.
10e8050 to
5bd1a91
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds a new entry to the AUTHORS file and modifies the RelayoutWhenSystemFontsChangeMixin to handle system font updates that occur during non-idle scheduler phases, preventing assertion failures. A regression test has been included to verify the fix. Feedback suggests simplifying the logic in _scheduleSystemFontsUpdate by removing the conditional check for SchedulerPhase.idle and the use of addPostFrameCallback, as scheduleFrameCallback is safe to call during any phase and will naturally defer execution to the next frame.
| if (_hasPendingSystemFontsDidChangeCallBack) { | ||
| return; | ||
| } | ||
| _hasPendingSystemFontsDidChangeCallBack = true; | ||
| SchedulerBinding.instance.scheduleFrameCallback((Duration timeStamp) { | ||
| assert(_hasPendingSystemFontsDidChangeCallBack); | ||
| _hasPendingSystemFontsDidChangeCallBack = false; | ||
| assert( | ||
| attached || (debugDisposed ?? true), | ||
| '$this is detached during ${SchedulerBinding.instance.schedulerPhase} but is not disposed.', | ||
| ); | ||
| if (attached) { | ||
| systemFontsDidChange(); | ||
| } | ||
| }); | ||
|
|
||
| // The system fonts notification can arrive during any scheduler phase | ||
| // (e.g. midFrameMicrotasks on web when fonts load asynchronously). | ||
| // If we're not idle, defer scheduling to the next frame via a | ||
| // post-frame callback. If idle, schedule directly. | ||
| void scheduleUpdate([Duration? _]) { | ||
| SchedulerBinding.instance.scheduleFrameCallback((Duration timeStamp) { | ||
| assert(_hasPendingSystemFontsDidChangeCallBack); | ||
| _hasPendingSystemFontsDidChangeCallBack = false; | ||
| assert( | ||
| attached || (debugDisposed ?? true), | ||
| '$this is detached during ${SchedulerBinding.instance.schedulerPhase} but is not disposed.', | ||
| ); | ||
| if (attached) { | ||
| systemFontsDidChange(); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| if (SchedulerBinding.instance.schedulerPhase == SchedulerPhase.idle) { | ||
| scheduleUpdate(); | ||
| } else { | ||
| // Not idle — defer to after the current frame completes. | ||
| SchedulerBinding.instance.addPostFrameCallback(scheduleUpdate); | ||
| } |
There was a problem hiding this comment.
The logic to check for SchedulerPhase.idle and use addPostFrameCallback as an intermediary step is likely unnecessary. SchedulerBinding.instance.scheduleFrameCallback is safe to call during any scheduler phase; if called during a frame, it simply schedules the callback for the next frame's transient callback phase.
By removing the assertion and calling scheduleFrameCallback directly, you achieve the same result (executing in the next frame) with simpler code and without the potential two-frame delay that can occur if _scheduleSystemFontsUpdate is called during the postFrameCallbacks phase.
Also, the local function scheduleUpdate can be removed to keep the implementation concise.
if (_hasPendingSystemFontsDidChangeCallBack) {
return;
}
_hasPendingSystemFontsDidChangeCallBack = true;
// The system fonts notification can arrive during any scheduler phase
// (e.g. midFrameMicrotasks on web when fonts load asynchronously).
// We schedule a frame callback to ensure the update happens in the
// transient callback phase of the next frame.
SchedulerBinding.instance.scheduleFrameCallback((Duration timeStamp) {
assert(_hasPendingSystemFontsDidChangeCallBack);
_hasPendingSystemFontsDidChangeCallBack = false;
assert(
attached || (debugDisposed ?? true),
'$this is detached during ${SchedulerBinding.instance.schedulerPhase} but is not disposed.',
);
if (attached) {
systemFontsDidChange();
}
});References
- Optimize for readability: Code is read more often than it is written. (link)
There was a problem hiding this comment.
Hmm, I loathe when gemini code assist contradicts itself. 😆 What do you think @solvejet-team?
There was a problem hiding this comment.
Haha yeah, gotta love it, This is the "future" we are worried about 😄 But the second suggestion is actually right — I dug into scheduleFrameCallback and it has no phase checks at all, it just appends to _transientCallbacks. And handleBeginFrame snapshots that map before iterating, so anything added mid-frame just runs next frame. The addPostFrameCallback detour was unnecessary overhead. Simplified!
There was a problem hiding this comment.
Sweet! I was like oh gosh, I think Gemini is contradicting itself. 🤣
5bd1a91 to
5af0e99
Compare
Piinks
left a comment
There was a problem hiding this comment.
LGTM! I would suggest updating the PR description to reflect the updates here. This will need a second review to land, so that will be helpful to the next reviewer. :)
5af0e99 to
0350281
Compare
Renzo-Olivares
left a comment
There was a problem hiding this comment.
LGTM, the analyzer issues need to be addressed. It looks like the added test is not formatted correctly. Also confirmed the scheduleFrameCallback behavior discussed.
0350281 to
6f0350c
Compare
|
Fixed the formatting issues and updated the PR description. Ready for another look! 🙂 |
|
autosubmit label was removed for flutter/flutter/184332, because The base commit of the PR is older than 7 days and can not be merged. Please merge the latest changes from the main into this branch and resubmit the PR. |
The system fonts notification can arrive during any scheduler phase (e.g. midFrameMicrotasks on web when fonts load asynchronously via google_fonts). The previous implementation asserted that the scheduler was idle, which is not guaranteed for async platform messages. This change removes the idle-phase assertion and defers scheduling to a post-frame callback when the scheduler is not idle, ensuring the frame callback is only scheduled when it is safe to do so. Fixes flutter#151873
6f0350c to
d096096
Compare
flutter/flutter@31f1802...8e8a194 2026-04-17 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Fix ImageInfo.isCloneOf to correctly compare scale values (fixes #184626) (#184643)" (flutter/flutter#185203) 2026-04-17 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Roll Fuchsia Test Scripts from R2EllDf4DgBXVNuiN... to dQ4PjIJB5kZFU8Y32... (#185192)" (flutter/flutter#185199) 2026-04-17 chris@bracken.jp [iOS] Migrate FlutterLaunchEngine to Swift (flutter/flutter#185151) 2026-04-17 116356835+AbdeMohlbi@users.noreply.github.com Remove unused `FlutterRunArguments.java` file (flutter/flutter#184160) 2026-04-17 engine-flutter-autoroll@skia.org Roll Fuchsia Test Scripts from R2EllDf4DgBXVNuiN... to dQ4PjIJB5kZFU8Y32... (flutter/flutter#185192) 2026-04-17 pewpewstol@gmail.com [Impeller] Fix morphology filter asymmetric dilation/erosion (flutter/flutter#184913) 2026-04-17 nate.w5687@gmail.com `AnimationStyle` methods (flutter/flutter#182333) 2026-04-17 karan@solvejet.net Fix _scheduleSystemFontsUpdate assertion during non-idle scheduler phase (flutter/flutter#184332) 2026-04-17 154381524+flutteractionsbot@users.noreply.github.com Revert "Unpin sdk package dependencies" (flutter/flutter#185186) 2026-04-16 116356835+AbdeMohlbi@users.noreply.github.com Clarify why the `child` is nullable in `AnimatedTransitionBuilder` (flutter/flutter#182995) 2026-04-16 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from IdBT8fSMYrYSip65j... to di3JdYrdE9OFu8Iyl... (flutter/flutter#185173) 2026-04-16 1063596+reidbaker@users.noreply.github.com Add dart_skills_lint to dev/tool with test and update readme instructions for new skills authors (flutter/flutter#185033) 2026-04-16 flar@google.com [Impeller] Provide std::optional getter for stroke property (flutter/flutter#185112) 2026-04-16 katelovett@google.com Validate itemExtent with geometry in RenderSliverFixedExtentBoxAdaptor (flutter/flutter#185159) 2026-04-16 152433210+mozammal-hossain@users.noreply.github.com [iOS] Clarify provisioning profile error instructions (flutter/flutter#184051) 2026-04-16 bkonyi@google.com [Widget Preview] Fix flaky integration test timeout during flutter clean (flutter/flutter#184991) 2026-04-16 engine-flutter-autoroll@skia.org Roll Dart SDK from fbddcbe0cd96 to 7c2564c18770 (2 revisions) (flutter/flutter#185143) 2026-04-16 sigurdm@google.com Unpin sdk package dependencies (flutter/flutter#184821) 2026-04-16 68919043+Istiak-Ahmed78@users.noreply.github.com Fix ImageInfo.isCloneOf to correctly compare scale values (fixes #184626) (flutter/flutter#184643) 2026-04-16 engine-flutter-autoroll@skia.org Roll Skia from 391cdbe3ffe9 to d8415c5d7b91 (2 revisions) (flutter/flutter#185141) 2026-04-16 154381524+flutteractionsbot@users.noreply.github.com Sync CHANGELOG.md from stable (flutter/flutter#185166) 2026-04-16 154381524+flutteractionsbot@users.noreply.github.com Revert "Move widget_preview_scaffold into pub workspace" (flutter/flutter#185164) 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#11523) flutter/flutter@31f1802...8e8a194 2026-04-17 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Fix ImageInfo.isCloneOf to correctly compare scale values (fixes #184626) (#184643)" (flutter/flutter#185203) 2026-04-17 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Roll Fuchsia Test Scripts from R2EllDf4DgBXVNuiN... to dQ4PjIJB5kZFU8Y32... (#185192)" (flutter/flutter#185199) 2026-04-17 chris@bracken.jp [iOS] Migrate FlutterLaunchEngine to Swift (flutter/flutter#185151) 2026-04-17 116356835+AbdeMohlbi@users.noreply.github.com Remove unused `FlutterRunArguments.java` file (flutter/flutter#184160) 2026-04-17 engine-flutter-autoroll@skia.org Roll Fuchsia Test Scripts from R2EllDf4DgBXVNuiN... to dQ4PjIJB5kZFU8Y32... (flutter/flutter#185192) 2026-04-17 pewpewstol@gmail.com [Impeller] Fix morphology filter asymmetric dilation/erosion (flutter/flutter#184913) 2026-04-17 nate.w5687@gmail.com `AnimationStyle` methods (flutter/flutter#182333) 2026-04-17 karan@solvejet.net Fix _scheduleSystemFontsUpdate assertion during non-idle scheduler phase (flutter/flutter#184332) 2026-04-17 154381524+flutteractionsbot@users.noreply.github.com Revert "Unpin sdk package dependencies" (flutter/flutter#185186) 2026-04-16 116356835+AbdeMohlbi@users.noreply.github.com Clarify why the `child` is nullable in `AnimatedTransitionBuilder` (flutter/flutter#182995) 2026-04-16 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from IdBT8fSMYrYSip65j... to di3JdYrdE9OFu8Iyl... (flutter/flutter#185173) 2026-04-16 1063596+reidbaker@users.noreply.github.com Add dart_skills_lint to dev/tool with test and update readme instructions for new skills authors (flutter/flutter#185033) 2026-04-16 flar@google.com [Impeller] Provide std::optional getter for stroke property (flutter/flutter#185112) 2026-04-16 katelovett@google.com Validate itemExtent with geometry in RenderSliverFixedExtentBoxAdaptor (flutter/flutter#185159) 2026-04-16 152433210+mozammal-hossain@users.noreply.github.com [iOS] Clarify provisioning profile error instructions (flutter/flutter#184051) 2026-04-16 bkonyi@google.com [Widget Preview] Fix flaky integration test timeout during flutter clean (flutter/flutter#184991) 2026-04-16 engine-flutter-autoroll@skia.org Roll Dart SDK from fbddcbe0cd96 to 7c2564c18770 (2 revisions) (flutter/flutter#185143) 2026-04-16 sigurdm@google.com Unpin sdk package dependencies (flutter/flutter#184821) 2026-04-16 68919043+Istiak-Ahmed78@users.noreply.github.com Fix ImageInfo.isCloneOf to correctly compare scale values (fixes #184626) (flutter/flutter#184643) 2026-04-16 engine-flutter-autoroll@skia.org Roll Skia from 391cdbe3ffe9 to d8415c5d7b91 (2 revisions) (flutter/flutter#185141) 2026-04-16 154381524+flutteractionsbot@users.noreply.github.com Sync CHANGELOG.md from stable (flutter/flutter#185166) 2026-04-16 154381524+flutteractionsbot@users.noreply.github.com Revert "Move widget_preview_scaffold into pub workspace" (flutter/flutter#185164) 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
Description
Fixes the assertion failure in RelayoutWhenSystemFontsChangeMixin._scheduleSystemFontsUpdate() when the system fonts notification arrives during a non-idle scheduler phase.
Problem
On web, when fonts load asynchronously (e.g. via google_fonts), the platform sends a fontsChange system message that can arrive during SchedulerPhase.midFrameMicrotasks. The _scheduleSystemFontsUpdate method asserted schedulerPhase == idle, causing:
Failed assertion: SchedulerBinding.instance.schedulerPhase == SchedulerPhase.idleFix
Remove the idle-phase assertion
scheduleFrameCallback is safe to call during any phase — if called mid-frame, the callback naturally runs in the next frame's transient callback phase (since handleBeginFrame snapshots and replaces _transientCallbacks before iterating)
The _hasPendingSystemFontsDidChangeCallBack flag prevents duplicate scheduling
Test
Added regression test that triggers a fontsChange platform message during transientCallbacks phase.
Fixes #151873