TextPainter should dispatch creation and disposal events. #137416

ksokolovskyi · 2023-10-27T14:06:57Z

Description

Adds TextPainter creation and disposal dispatching for memory leak tracking as part of Clean up memory leaks in Flutter Framework, phase 2. #137311.

Tests

Updates text_painter_test.dart to test TextPainter object creation and disposal dispatching;
Fixes memory leak in _SnackBarState;
Fixes memory leak in _SwitchPainter;
Fixes memory leak in _FlutterLogoPainter;
Fixes memory leak in RenderDecoratedBox;
Fixes memory leak in BannerPainter.

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], including [Features we expect every widget to implement].
I signed the [CLA].
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is [test-exempt].
All existing and new tests are passing.

ksokolovskyi · 2023-10-27T14:46:57Z

cc @polina-c

navaronbracke · 2023-10-27T19:26:12Z

packages/flutter/lib/src/painting/flutter_logo.dart

@@ -219,6 +219,12 @@ class _FlutterLogoPainter extends BoxPainter {
  late TextPainter _textPainter;
  late Rect _textBoundingRect;

+  @override
+  void dispose() {
+    _textPainter.dispose();


Now the TODO on line 218 is redundant?

navaronbracke · 2023-10-27T19:31:54Z

packages/flutter/test/painting/text_painter_test.dart

+  test('TextPainter dispatches memory events', () async {
+    await expectLater(
+      await memoryEvents(() => TextPainter().dispose(), TextPainter),
+      areCreateAndDispose,


@polina-c Probably an unfortunate typo in leak_tracker_flutter_testing but shouldn't this be named as areCreatedAndDisposed ?

nope
the name is about events
the name states that the events are about creation and disposal, not that they are created and disposed

I see. That indeed makes more sense.

navaronbracke

Just skimming through (I tend to keep an eye on leak tracker things :) )

polina-c · 2023-10-27T19:35:57Z

packages/flutter/lib/src/painting/text_painter.dart

@@ -498,7 +500,15 @@ class TextPainter {
       _locale = locale,
       _strutStyle = strutStyle,
       _textWidthBasis = textWidthBasis,
-       _textHeightBehavior = textHeightBehavior;
+       _textHeightBehavior = textHeightBehavior {
+    if (kFlutterMemoryAllocationsEnabled) {


Can you add TODO here: #137435

ksokolovskyi · 2023-10-27T19:47:04Z

@polina-c, @navaronbracke thanks for the review.
I made some adjustments according to the latest comments. Could you please take a look again?

…lutter/flutter#137416)

flutter/flutter@5907c97...a4ec627 2023-10-28 engine-flutter-autoroll@skia.org Roll Flutter Engine from f5fbd9cd60c6 to 84dcb4fb9301 (1 revision) (flutter/flutter#137468) 2023-10-28 engine-flutter-autoroll@skia.org Roll Flutter Engine from 03de8a41995b to f5fbd9cd60c6 (2 revisions) (flutter/flutter#137467) 2023-10-28 polinach@google.com Instrument more disposables. (flutter/flutter#137309) 2023-10-28 sokolovskyi.konstantin@gmail.com TextPainter should dispatch creation and disposal events. (flutter/flutter#137416) 2023-10-28 engine-flutter-autoroll@skia.org Roll Flutter Engine from a76821199d9d to 03de8a41995b (2 revisions) (flutter/flutter#137464) 2023-10-28 engine-flutter-autoroll@skia.org Roll Flutter Engine from f1e30b4b9f27 to a76821199d9d (3 revisions) (flutter/flutter#137462) 2023-10-28 engine-flutter-autoroll@skia.org Roll Flutter Engine from 7e2aa68b2f27 to f1e30b4b9f27 (2 revisions) (flutter/flutter#137461) 2023-10-27 engine-flutter-autoroll@skia.org Roll Flutter Engine from 513e007ed682 to 7e2aa68b2f27 (1 revision) (flutter/flutter#137460) 2023-10-27 engine-flutter-autoroll@skia.org Roll Flutter Engine from 32bb5b057c86 to 513e007ed682 (3 revisions) (flutter/flutter#137457) 2023-10-27 engine-flutter-autoroll@skia.org Roll Flutter Engine from f2ec263cebf9 to 32bb5b057c86 (1 revision) (flutter/flutter#137452) 2023-10-27 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 2.22.4 to 2.22.5 (flutter/flutter#137450) 2023-10-27 engine-flutter-autoroll@skia.org Roll Flutter Engine from 453a04dbf891 to f2ec263cebf9 (2 revisions) (flutter/flutter#137449) 2023-10-27 ditman@gmail.com [web] Add 'nonce' prop to flutter.js loadEntrypoint (flutter/flutter#137204) 2023-10-27 engine-flutter-autoroll@skia.org Roll Flutter Engine from 1e66c0ae7bda to 453a04dbf891 (1 revision) (flutter/flutter#137446) 2023-10-27 goderbauer@google.com Provide exception for listing an issue. (flutter/flutter#137092) 2023-10-27 engine-flutter-autoroll@skia.org Roll Flutter Engine from 0bba9eeb8f5d to 1e66c0ae7bda (1 revision) (flutter/flutter#137442) 2023-10-27 engine-flutter-autoroll@skia.org Roll Flutter Engine from a198ad4e740d to 0bba9eeb8f5d (1 revision) (flutter/flutter#137437) 2023-10-27 katelovett@google.com Bump goldctl in .ci.yaml (flutter/flutter#137441) 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 camillesimon@google.com,rmistry@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#5255) flutter/flutter@5907c97...a4ec627 2023-10-28 engine-flutter-autoroll@skia.org Roll Flutter Engine from f5fbd9cd60c6 to 84dcb4fb9301 (1 revision) (flutter/flutter#137468) 2023-10-28 engine-flutter-autoroll@skia.org Roll Flutter Engine from 03de8a41995b to f5fbd9cd60c6 (2 revisions) (flutter/flutter#137467) 2023-10-28 polinach@google.com Instrument more disposables. (flutter/flutter#137309) 2023-10-28 sokolovskyi.konstantin@gmail.com TextPainter should dispatch creation and disposal events. (flutter/flutter#137416) 2023-10-28 engine-flutter-autoroll@skia.org Roll Flutter Engine from a76821199d9d to 03de8a41995b (2 revisions) (flutter/flutter#137464) 2023-10-28 engine-flutter-autoroll@skia.org Roll Flutter Engine from f1e30b4b9f27 to a76821199d9d (3 revisions) (flutter/flutter#137462) 2023-10-28 engine-flutter-autoroll@skia.org Roll Flutter Engine from 7e2aa68b2f27 to f1e30b4b9f27 (2 revisions) (flutter/flutter#137461) 2023-10-27 engine-flutter-autoroll@skia.org Roll Flutter Engine from 513e007ed682 to 7e2aa68b2f27 (1 revision) (flutter/flutter#137460) 2023-10-27 engine-flutter-autoroll@skia.org Roll Flutter Engine from 32bb5b057c86 to 513e007ed682 (3 revisions) (flutter/flutter#137457) 2023-10-27 engine-flutter-autoroll@skia.org Roll Flutter Engine from f2ec263cebf9 to 32bb5b057c86 (1 revision) (flutter/flutter#137452) 2023-10-27 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 2.22.4 to 2.22.5 (flutter/flutter#137450) 2023-10-27 engine-flutter-autoroll@skia.org Roll Flutter Engine from 453a04dbf891 to f2ec263cebf9 (2 revisions) (flutter/flutter#137449) 2023-10-27 ditman@gmail.com [web] Add 'nonce' prop to flutter.js loadEntrypoint (flutter/flutter#137204) 2023-10-27 engine-flutter-autoroll@skia.org Roll Flutter Engine from 1e66c0ae7bda to 453a04dbf891 (1 revision) (flutter/flutter#137446) 2023-10-27 goderbauer@google.com Provide exception for listing an issue. (flutter/flutter#137092) 2023-10-27 engine-flutter-autoroll@skia.org Roll Flutter Engine from 0bba9eeb8f5d to 1e66c0ae7bda (1 revision) (flutter/flutter#137442) 2023-10-27 engine-flutter-autoroll@skia.org Roll Flutter Engine from a198ad4e740d to 0bba9eeb8f5d (1 revision) (flutter/flutter#137437) 2023-10-27 katelovett@google.com Bump goldctl in .ci.yaml (flutter/flutter#137441) 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 camillesimon@google.com,rmistry@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

…lutter/flutter#137416)

Instrumented TextPainter for leak tracking

25907de

github-actions bot added a: text input Entering text in a text field or keyboard related problems framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Oct 27, 2023

ksokolovskyi changed the title ~~Instrumented TextPainter for leak tracking~~ TextPainter should dispatch creation and disposal events. Oct 27, 2023

navaronbracke reviewed Oct 27, 2023

View reviewed changes

polina-c reviewed Oct 27, 2023

View reviewed changes

Fixes

ae89ef1

ksokolovskyi requested a review from polina-c October 27, 2023 19:47

polina-c approved these changes Oct 27, 2023

View reviewed changes

polina-c merged commit fea5613 into flutter:master Oct 28, 2023
65 checks passed

engine-flutter-autoroll mentioned this pull request Oct 28, 2023

Roll Flutter from 5907c9757e3f to a4ec6278b1a4 (18 revisions) flutter/packages#5253

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 28, 2023

fea561301 TextPainter should dispatch creation and disposal events. (f…

c1d35e1

…lutter/flutter#137416)

engine-flutter-autoroll mentioned this pull request Oct 28, 2023

Roll Flutter from 5907c9757e3f to a4ec6278b1a4 (18 revisions) flutter/packages#5255

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 28, 2023

fea561301 TextPainter should dispatch creation and disposal events. (f…

70cc327

…lutter/flutter#137416)

HansMuller mentioned this pull request Nov 1, 2023

Flutter Framework Team: Notable Commits #121415

Open

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 16, 2024

fea561301 TextPainter should dispatch creation and disposal events. (f…

97528fd

…lutter/flutter#137416)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TextPainter should dispatch creation and disposal events. #137416

TextPainter should dispatch creation and disposal events. #137416

ksokolovskyi commented Oct 27, 2023

ksokolovskyi commented Oct 27, 2023

navaronbracke Oct 27, 2023

navaronbracke Oct 27, 2023

polina-c Oct 27, 2023

navaronbracke Oct 28, 2023

navaronbracke left a comment

polina-c Oct 27, 2023

ksokolovskyi commented Oct 27, 2023

TextPainter should dispatch creation and disposal events. #137416

TextPainter should dispatch creation and disposal events. #137416

Conversation

ksokolovskyi commented Oct 27, 2023

Description

Tests

Pre-launch Checklist

ksokolovskyi commented Oct 27, 2023

navaronbracke Oct 27, 2023

Choose a reason for hiding this comment

navaronbracke Oct 27, 2023

Choose a reason for hiding this comment

polina-c Oct 27, 2023

Choose a reason for hiding this comment

navaronbracke Oct 28, 2023

Choose a reason for hiding this comment

navaronbracke left a comment

Choose a reason for hiding this comment

polina-c Oct 27, 2023

Choose a reason for hiding this comment

ksokolovskyi commented Oct 27, 2023