ReportTiming callback should record the sendFrameToEngine when it was scheduled #144212
Conversation
github-actions
bot
added
the
framework
goderbauer
left a comment
goderbauer
left a comment
There was a problem hiding this comment.
Overall this seems like the right fix, the assert makes incorrect assumptions since we offer API to reset sendFramesToEngine.
| // This test needs LiveTestWidgetsFlutterBinding for multiple reasons. | ||
| // First, this was the environment that this bug was discovered. | ||
| // Second, LiveTestWidgetsFlutterBinding doesn't override | ||
| // scheduleWarmUpFrame, so that the test can start with no frames rendered | ||
| // allowing deferFirstFrame to work (see below). |
There was a problem hiding this comment.
I don't understand this paragraph. In my observation, the problem is that the warm-up frame triggered internally by testWidgets before the test body executes is actually send to the engine, but after that resetFirstFrame is called, which now puts engine and framework in an inconsistent state: The framework now things no frame has been sent (which means deferFirstFrame does have an effect) while the engine things the first frame has been sent.
There was a problem hiding this comment.
Kind of. The _runTestBody has:
runApp(Container(key: UniqueKey(), child: _preTestMessage)); // Reset the tree to a known state.
await pump();so a frame is pumped to the engine anyway, warm-up or not.
And your understanding on the inconsistency is absolutely correct. Now there's a reportTiming callback pending, which is supposed to only happen after rendered frames, while the framework thinks no frames have been rendered, allowing deferFirstFrame to take effect. This combined situation of LiveTestWidgetsFlutterBinding, runApp, and deferFirstFrame is probably the only case that can trigger this bug.
| // Note that the `runApp` call also schedules a warm up frame, which would | ||
| // have directly rendered the frame if the test had used | ||
| // `AutomatedTestWidgetsFlutterBinding`. |
There was a problem hiding this comment.
This doesn't seem super relevant and adds more confusion to what is actually going on here. Can we just drop this? Also note that "note that" is banned: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#avoid-empty-prose
dkwingsmt
force-pushed
the
record-send-frames
branch
from
cc285ba to
b17215a
Compare
|
@goderbauer I've addressed your feedbacks and rewrote some paragraphs. Can you take another look? (I'm filing an issue to record the problem, which I'll link to the test file. ) |
| // run. If the reportTiming callback were to assume that | ||
| // `sendFramesToEngine` is true, the callback would crash. | ||
| await tester.binding.waitUntilFirstFrameRasterized; | ||
| expect(find.text('First frame'), findsOne); |
There was a problem hiding this comment.
From the test failures: It looks like the test needs a pump call before this expect, which makes sense. Sorry to not include that in my previous suggestion.
There was a problem hiding this comment.
What do you think if I just revert to await tester.pump(const Duration(milliseconds: 1));?
There was a problem hiding this comment.
I ended up including both.
dkwingsmt
added
the
autosubmit
|
I also wrote an issue to describe the problem #144261 and linked it to the test case. This PR will close the issue. |
auto-submit
bot
removed
the
autosubmit
|
auto label is removed for flutter/flutter/144212, due to - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label. |
dkwingsmt
added
the
autosubmit
auto-submit
bot
removed
the
autosubmit
|
auto label is removed for flutter/flutter/144212, due to - The status or check suite Linux analyze has failed. Please fix the issues identified (or deflake) before re-applying this label. |
dkwingsmt
added
the
autosubmit
|
Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change). If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
…n it was scheduled (flutter/flutter#144212)
flutter/flutter@d00bfe8...e92bca3 2024-02-29 [email protected] [flutter_tools] Update external link in Android manifest template (flutter/flutter#144302) 2024-02-29 [email protected] Roll Flutter Engine from 232217f39c3c to d068d980f952 (1 revision) (flutter/flutter#144369) 2024-02-29 [email protected] Roll Flutter Engine from 9e1876141be8 to 232217f39c3c (1 revision) (flutter/flutter#144366) 2024-02-29 [email protected] Roll Flutter Engine from 61510db94a1c to 9e1876141be8 (1 revision) (flutter/flutter#144362) 2024-02-29 [email protected] Roll Flutter Engine from 10331db8f748 to 61510db94a1c (5 revisions) (flutter/flutter#144355) 2024-02-29 [email protected] Docs on the interaction between Shortcuts and text input (flutter/flutter#144328) 2024-02-29 [email protected] Use robolectric/AndroidJUnit4 for integration test tests (flutter/flutter#144348) 2024-02-29 [email protected] Reland "Cache FocusNode.enclosingScope, clean up descendantsAreFocusable (#144207)" (flutter/flutter#144330) 2024-02-29 [email protected] Roll Flutter Engine from 455c814fe5de to 10331db8f748 (7 revisions) (flutter/flutter#144345) 2024-02-29 [email protected] ReportTiming callback should record the sendFrameToEngine when it was scheduled (flutter/flutter#144212) 2024-02-29 [email protected] Disable flaky golden file test (flutter/flutter#144351) 2024-02-28 [email protected] Mention SelectionArea in SelectableText docs (flutter/flutter#143784) 2024-02-28 49699333+dependabot[bot]@users.noreply.github.com Bump peter-evans/create-pull-request from 6.0.0 to 6.0.1 (flutter/flutter#144344) 2024-02-28 [email protected] Reland "Reland - Introduce tone-based surfaces and accent color add-ons - Part 2" (flutter/flutter#144273) 2024-02-28 [email protected] Remove irrelevant comment in TextPainter (flutter/flutter#144308) 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],[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
flutter/flutter@d00bfe8...e92bca3 2024-02-29 [email protected] [flutter_tools] Update external link in Android manifest template (flutter/flutter#144302) 2024-02-29 [email protected] Roll Flutter Engine from 232217f39c3c to d068d980f952 (1 revision) (flutter/flutter#144369) 2024-02-29 [email protected] Roll Flutter Engine from 9e1876141be8 to 232217f39c3c (1 revision) (flutter/flutter#144366) 2024-02-29 [email protected] Roll Flutter Engine from 61510db94a1c to 9e1876141be8 (1 revision) (flutter/flutter#144362) 2024-02-29 [email protected] Roll Flutter Engine from 10331db8f748 to 61510db94a1c (5 revisions) (flutter/flutter#144355) 2024-02-29 [email protected] Docs on the interaction between Shortcuts and text input (flutter/flutter#144328) 2024-02-29 [email protected] Use robolectric/AndroidJUnit4 for integration test tests (flutter/flutter#144348) 2024-02-29 [email protected] Reland "Cache FocusNode.enclosingScope, clean up descendantsAreFocusable (#144207)" (flutter/flutter#144330) 2024-02-29 [email protected] Roll Flutter Engine from 455c814fe5de to 10331db8f748 (7 revisions) (flutter/flutter#144345) 2024-02-29 [email protected] ReportTiming callback should record the sendFrameToEngine when it was scheduled (flutter/flutter#144212) 2024-02-29 [email protected] Disable flaky golden file test (flutter/flutter#144351) 2024-02-28 [email protected] Mention SelectionArea in SelectableText docs (flutter/flutter#143784) 2024-02-28 49699333+dependabot[bot]@users.noreply.github.com Bump peter-evans/create-pull-request from 6.0.0 to 6.0.1 (flutter/flutter#144344) 2024-02-28 [email protected] Reland "Reland - Introduce tone-based surfaces and accent color add-ons - Part 2" (flutter/flutter#144273) 2024-02-28 [email protected] Remove irrelevant comment in TextPainter (flutter/flutter#144308) 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],[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
This relands #50931. The crash that caused the 4th revert has been fixed by flutter/flutter#144212. [There has been discussion](#51019) on why the benchmark in previous attempts show significant drop in build time. This PR addresses it using option a as described in [this comment](#51186 (comment)). This PR also addresses flutter/flutter#144584 with option 1. A test is added. [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
…n it was scheduled (flutter/flutter#144212)
2 participants
Fixes #144261
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.