Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[web] Enforce onDrawFrame/onBeginFrame render rule #49214

Merged
merged 6 commits into from
Dec 19, 2023

Conversation

harryterkelsen
Copy link
Contributor

Also update tests to conform to the render rule.

The render rule is that the call to render should only occur in the scope of an onDrawFrame or onBeginFrame call. As a side effect, this change also fixes a memory leak which occurs in tests using FakeAsync in the flutter_test environment (because they don't render anymore).

Fixes flutter/flutter#137073
Fixes flutter/flutter#137669

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

Also update tests to conform to the render rule.
@github-actions github-actions bot added the platform-web Code specifically for the web engine label Dec 19, 2023
@@ -80,7 +80,7 @@ abstract class PlatformDispatcher {

void scheduleFrame();

void render(Scene scene, [FlutterView view]);
Future<void> render(Scene scene, [FlutterView view]);
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like the mobile version no longer has this method.

Copy link
Member

Choose a reason for hiding this comment

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

Agh shoot, forgot to remove it from our interface 🤦

Copy link
Contributor

@yjbanov yjbanov left a comment

Choose a reason for hiding this comment

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

lgtm

@harryterkelsen harryterkelsen added the autosubmit Merge PR when tree becomes green via auto submit App label Dec 19, 2023
@auto-submit auto-submit bot merged commit d9cc207 into flutter:main Dec 19, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 20, 2023
harryterkelsen added a commit to flutter/flutter that referenced this pull request Dec 20, 2023
The test was flaky before due to overflowing GPU memory during the test.
The memory leak was fixed here
flutter/engine#49214

Fixes #137669

As a side effect of the fix, this test also runs much faster, from about
3 minutes on my Macbook down to about 25 seconds.

## Pre-launch Checklist

- [x] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [x] I read the [Tree Hygiene] wiki page, which explains my
responsibilities.
- [x] I read and followed the [Flutter Style Guide], including [Features
we expect every widget to implement].
- [x] I signed the [CLA].
- [x] I listed at least one issue that this PR fixes in the description
above.
- [x] I updated/added relevant documentation (doc comments with `///`).
- [x] I added new tests to check the change I am making, or this PR is
[test-exempt].
- [x] All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel
on [Discord].

<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#overview
[Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene
[test-exempt]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#tests
[Flutter Style Guide]:
https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo
[Features we expect every widget to implement]:
https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#features-we-expect-every-widget-to-implement
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[breaking change policy]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes
[Discord]: https://github.com/flutter/flutter/wiki/Chat
auto-submit bot added a commit to flutter/flutter that referenced this pull request Dec 21, 2023
…140515)

Reverts #140462
Initiated by: cbracken
This change reverts the following previous change:
Original Description:
The test was flaky before due to overflowing GPU memory during the test. The memory leak was fixed here flutter/engine#49214

Fixes #137669

As a side effect of the fix, this test also runs much faster, from about 3 minutes on my Macbook down to about 25 seconds.
@cbracken cbracken added the revert Label used to revert changes in a closed and merged pull request. label Dec 21, 2023
Copy link
Contributor

auto-submit bot commented Dec 21, 2023

Time to revert pull request flutter/engine/49214 has elapsed.
You need to open the revert manually and process as a regular pull request.

@auto-submit auto-submit bot removed the revert Label used to revert changes in a closed and merged pull request. label Dec 21, 2023
cbracken added a commit to cbracken/flutter_engine that referenced this pull request Dec 21, 2023
@cbracken cbracken mentioned this pull request Dec 21, 2023
8 tasks
cbracken added a commit that referenced this pull request Dec 21, 2023
This reverts #49214 and
#48758.

#49214 appears to cause a Flutter
Gold diff during the engine roll. See
flutter/flutter#140434 for an example. It's
unclear to me whether this is an expected diff and should be approved or
unexpected and should be reverting, so reverting to the last previous
rollable state.

## Pre-launch Checklist

- [ ] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [ ] I read the [Tree Hygiene] wiki page, which explains my
responsibilities.
- [ ] I read and followed the [Flutter Style Guide] and the [C++,
Objective-C, Java style guides].
- [ ] I listed at least one issue that this PR fixes in the description
above.
- [ ] I added new tests to check the change I am making or feature I am
adding, or the PR is [test-exempt]. See [testing the engine] for
instructions on writing and running engine tests.
- [ ] I updated/added relevant documentation (doc comments with `///`).
- [ ] I signed the [CLA].
- [ ] All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel
on [Discord].

<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#overview
[Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene
[test-exempt]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#tests
[Flutter Style Guide]:
https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo
[C++, Objective-C, Java style guides]:
https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
[testing the engine]:
https://github.com/flutter/flutter/wiki/Testing-the-engine
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[breaking change policy]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes
[Discord]: https://github.com/flutter/flutter/wiki/Chat
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 21, 2023
harryterkelsen added a commit that referenced this pull request Dec 21, 2023
Enforces the render rule on the web except in the HTML renderer, which
must still render to the DOM even outside of `onDrawFrame` or
`onBegineFrame` scopes in order for golden tests to continue to work in
the framework.

This is a reland of #49214 with
one change (checking for `renderer.rendererTag == 'html'` in the
`render` method).

## Pre-launch Checklist

- [x] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [x] I read the [Tree Hygiene] wiki page, which explains my
responsibilities.
- [x] I read and followed the [Flutter Style Guide] and the [C++,
Objective-C, Java style guides].
- [x] I listed at least one issue that this PR fixes in the description
above.
- [x] I added new tests to check the change I am making or feature I am
adding, or the PR is [test-exempt]. See [testing the engine] for
instructions on writing and running engine tests.
- [x] I updated/added relevant documentation (doc comments with `///`).
- [x] I signed the [CLA].
- [x] All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel
on [Discord].

<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#overview
[Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene
[test-exempt]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#tests
[Flutter Style Guide]:
https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo
[C++, Objective-C, Java style guides]:
https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
[testing the engine]:
https://github.com/flutter/flutter/wiki/Testing-the-engine
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[breaking change policy]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes
[Discord]: https://github.com/flutter/flutter/wiki/Chat
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Dec 21, 2023
…140519)

Roll Flutter Engine from c70f0a495ace to 1b1b2a12a597 (32 revisions)

flutter/engine@c70f0a4...1b1b2a1

2023-12-21 skia-flutter-autoroll@skia.org Roll Dart SDK from 4a10533a4dc8 to bcf68d22f0fa (1 revision) (flutter/engine#49329)
2023-12-21 skia-flutter-autoroll@skia.org Roll Dart SDK from 1d0c3ecd1349 to 4a10533a4dc8 (1 revision) (flutter/engine#49326)
2023-12-21 flar@google.com Ensure sorted rects in ui.Canvas for legacy compatibility (flutter/engine#49309)
2023-12-21 skia-flutter-autoroll@skia.org Roll Dart SDK from 63a452b45026 to 1d0c3ecd1349 (1 revision) (flutter/engine#49318)
2023-12-21 dnfield@google.com [Impeller] Make IPLR files multi-platform (flutter/engine#49253)
2023-12-21 ditman@gmail.com [web] Defer injection of platform views until needed. (flutter/engine#48960)
2023-12-21 skia-flutter-autoroll@skia.org Roll Dart SDK from 1aef027ec953 to 63a452b45026 (2 revisions) (flutter/engine#49311)
2023-12-21 skia-flutter-autoroll@skia.org Roll Skia from 29917d8c97ca to 4b16117e94b2 (4 revisions) (flutter/engine#49310)
2023-12-21 737941+loic-sharma@users.noreply.github.com Reland "[Windows] Move to FlutterCompositor for rendering" (flutter/engine#49262)
2023-12-21 30870216+gaaclarke@users.noreply.github.com Reland `[Impeller] new blur: refactored math and fixed expanded padding size` (flutter/engine#49302)
2023-12-20 dkwingsmt@users.noreply.github.com Multiview pipeline Pt. 1: Skip illegal render calls (flutter/engine#49266)
2023-12-20 barpac02@gmail.com SemanticsUpdateBuilder: make all args non-null (flutter/engine#49148)
2023-12-20 30870216+gaaclarke@users.noreply.github.com [Impeller] fixed Rect::Contains (flutter/engine#49294)
2023-12-20 skia-flutter-autoroll@skia.org Roll Dart SDK from 97c3b7e1885a to 1aef027ec953 (1 revision) (flutter/engine#49295)
2023-12-20 30870216+gaaclarke@users.noreply.github.com Revert "[Impeller] new blur: refactored math and fixed expanded padding size" (flutter/engine#49298)
2023-12-20 30870216+gaaclarke@users.noreply.github.com [Impeller] new blur: refactored math and fixed expanded padding size (flutter/engine#49206)
2023-12-20 dkwingsmt@users.noreply.github.com Multi-view pointer event (flutter/engine#46213)
2023-12-20 1961493+harryterkelsen@users.noreply.github.com [web:multiview] Only call `Renderer.clearFragmentProgramCache` on hot restart (flutter/engine#48758)
2023-12-20 skia-flutter-autoroll@skia.org Roll Skia from 9cb1bb1164ea to 29917d8c97ca (1 revision) (flutter/engine#49289)
2023-12-20 bdero@google.com [Impeller] Add interactive Blur+Clip AiksTest. (flutter/engine#49283)
2023-12-20 sergiy.dubovik@supercell.com [macos] FlutterKeyboardManager memory leak fix (flutter/engine#48824)
2023-12-20 zanderso@users.noreply.github.com Don't guard Windows arm64 Dart SDK download on the release candidate flag (flutter/engine#49244)
2023-12-20 15619084+vashworth@users.noreply.github.com Fix testAppExtensionLaunching for Xcode 15/iOS 17 (flutter/engine#49242)
2023-12-20 skia-flutter-autoroll@skia.org Roll Skia from 8060d6b36066 to 9cb1bb1164ea (2 revisions) (flutter/engine#49288)
2023-12-20 skia-flutter-autoroll@skia.org Roll Dart SDK from ed415d966d8a to 97c3b7e1885a (1 revision) (flutter/engine#49287)
2023-12-20 skia-flutter-autoroll@skia.org Roll Skia from d0f09ad481f7 to 8060d6b36066 (1 revision) (flutter/engine#49285)
2023-12-20 kevinjchisholm@gmail.com [release] Update release config (flutter/engine#49254)
2023-12-20 skia-flutter-autoroll@skia.org Roll Dart SDK from 1732c4c92ccd to ed415d966d8a (1 revision) (flutter/engine#49274)
2023-12-20 skia-flutter-autoroll@skia.org Roll Dart SDK from 4c59838945d9 to 1732c4c92ccd (1 revision) (flutter/engine#49269)
2023-12-20 goderbauer@google.com Sync lints with flutter/flutter (flutter/engine#49192)
2023-12-19 1961493+harryterkelsen@users.noreply.github.com [web] Enforce onDrawFrame/onBeginFrame render rule (flutter/engine#49214)
2023-12-19 barpac02@gmail.com [Docs] Add more info about running tests on iOS (flutter/engine#48859)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC jimgraham@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: 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
...
CoderDake pushed a commit to CoderDake/flutter that referenced this pull request Dec 28, 2023
…40462)

The test was flaky before due to overflowing GPU memory during the test.
The memory leak was fixed here
flutter/engine#49214

Fixes flutter#137669

As a side effect of the fix, this test also runs much faster, from about
3 minutes on my Macbook down to about 25 seconds.

## Pre-launch Checklist

- [x] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [x] I read the [Tree Hygiene] wiki page, which explains my
responsibilities.
- [x] I read and followed the [Flutter Style Guide], including [Features
we expect every widget to implement].
- [x] I signed the [CLA].
- [x] I listed at least one issue that this PR fixes in the description
above.
- [x] I updated/added relevant documentation (doc comments with `///`).
- [x] I added new tests to check the change I am making, or this PR is
[test-exempt].
- [x] All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel
on [Discord].

<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#overview
[Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene
[test-exempt]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#tests
[Flutter Style Guide]:
https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo
[Features we expect every widget to implement]:
https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#features-we-expect-every-widget-to-implement
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[breaking change policy]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes
[Discord]: https://github.com/flutter/flutter/wiki/Chat
CoderDake pushed a commit to CoderDake/flutter that referenced this pull request Dec 28, 2023
…lutter#140515)

Reverts flutter#140462
Initiated by: cbracken
This change reverts the following previous change:
Original Description:
The test was flaky before due to overflowing GPU memory during the test. The memory leak was fixed here flutter/engine#49214

Fixes flutter#137669

As a side effect of the fix, this test also runs much faster, from about 3 minutes on my Macbook down to about 25 seconds.
CoderDake pushed a commit to CoderDake/flutter that referenced this pull request Dec 28, 2023
…lutter#140519)

Roll Flutter Engine from c70f0a495ace to 1b1b2a12a597 (32 revisions)

flutter/engine@c70f0a4...1b1b2a1

2023-12-21 skia-flutter-autoroll@skia.org Roll Dart SDK from 4a10533a4dc8 to bcf68d22f0fa (1 revision) (flutter/engine#49329)
2023-12-21 skia-flutter-autoroll@skia.org Roll Dart SDK from 1d0c3ecd1349 to 4a10533a4dc8 (1 revision) (flutter/engine#49326)
2023-12-21 flar@google.com Ensure sorted rects in ui.Canvas for legacy compatibility (flutter/engine#49309)
2023-12-21 skia-flutter-autoroll@skia.org Roll Dart SDK from 63a452b45026 to 1d0c3ecd1349 (1 revision) (flutter/engine#49318)
2023-12-21 dnfield@google.com [Impeller] Make IPLR files multi-platform (flutter/engine#49253)
2023-12-21 ditman@gmail.com [web] Defer injection of platform views until needed. (flutter/engine#48960)
2023-12-21 skia-flutter-autoroll@skia.org Roll Dart SDK from 1aef027ec953 to 63a452b45026 (2 revisions) (flutter/engine#49311)
2023-12-21 skia-flutter-autoroll@skia.org Roll Skia from 29917d8c97ca to 4b16117e94b2 (4 revisions) (flutter/engine#49310)
2023-12-21 737941+loic-sharma@users.noreply.github.com Reland "[Windows] Move to FlutterCompositor for rendering" (flutter/engine#49262)
2023-12-21 30870216+gaaclarke@users.noreply.github.com Reland `[Impeller] new blur: refactored math and fixed expanded padding size` (flutter/engine#49302)
2023-12-20 dkwingsmt@users.noreply.github.com Multiview pipeline Pt. 1: Skip illegal render calls (flutter/engine#49266)
2023-12-20 barpac02@gmail.com SemanticsUpdateBuilder: make all args non-null (flutter/engine#49148)
2023-12-20 30870216+gaaclarke@users.noreply.github.com [Impeller] fixed Rect::Contains (flutter/engine#49294)
2023-12-20 skia-flutter-autoroll@skia.org Roll Dart SDK from 97c3b7e1885a to 1aef027ec953 (1 revision) (flutter/engine#49295)
2023-12-20 30870216+gaaclarke@users.noreply.github.com Revert "[Impeller] new blur: refactored math and fixed expanded padding size" (flutter/engine#49298)
2023-12-20 30870216+gaaclarke@users.noreply.github.com [Impeller] new blur: refactored math and fixed expanded padding size (flutter/engine#49206)
2023-12-20 dkwingsmt@users.noreply.github.com Multi-view pointer event (flutter/engine#46213)
2023-12-20 1961493+harryterkelsen@users.noreply.github.com [web:multiview] Only call `Renderer.clearFragmentProgramCache` on hot restart (flutter/engine#48758)
2023-12-20 skia-flutter-autoroll@skia.org Roll Skia from 9cb1bb1164ea to 29917d8c97ca (1 revision) (flutter/engine#49289)
2023-12-20 bdero@google.com [Impeller] Add interactive Blur+Clip AiksTest. (flutter/engine#49283)
2023-12-20 sergiy.dubovik@supercell.com [macos] FlutterKeyboardManager memory leak fix (flutter/engine#48824)
2023-12-20 zanderso@users.noreply.github.com Don't guard Windows arm64 Dart SDK download on the release candidate flag (flutter/engine#49244)
2023-12-20 15619084+vashworth@users.noreply.github.com Fix testAppExtensionLaunching for Xcode 15/iOS 17 (flutter/engine#49242)
2023-12-20 skia-flutter-autoroll@skia.org Roll Skia from 8060d6b36066 to 9cb1bb1164ea (2 revisions) (flutter/engine#49288)
2023-12-20 skia-flutter-autoroll@skia.org Roll Dart SDK from ed415d966d8a to 97c3b7e1885a (1 revision) (flutter/engine#49287)
2023-12-20 skia-flutter-autoroll@skia.org Roll Skia from d0f09ad481f7 to 8060d6b36066 (1 revision) (flutter/engine#49285)
2023-12-20 kevinjchisholm@gmail.com [release] Update release config (flutter/engine#49254)
2023-12-20 skia-flutter-autoroll@skia.org Roll Dart SDK from 1732c4c92ccd to ed415d966d8a (1 revision) (flutter/engine#49274)
2023-12-20 skia-flutter-autoroll@skia.org Roll Dart SDK from 4c59838945d9 to 1732c4c92ccd (1 revision) (flutter/engine#49269)
2023-12-20 goderbauer@google.com Sync lints with flutter/flutter (flutter/engine#49192)
2023-12-19 1961493+harryterkelsen@users.noreply.github.com [web] Enforce onDrawFrame/onBeginFrame render rule (flutter/engine#49214)
2023-12-19 barpac02@gmail.com [Docs] Add more info about running tests on iOS (flutter/engine#48859)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC jimgraham@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: 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
...
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
autosubmit Merge PR when tree becomes green via auto submit App platform-web Code specifically for the web engine
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enforce the rendering rule Linux web_canvaskit_tests_4 is 5.05% flaky
4 participants