Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix test that leaks images. #148494

Merged
merged 14 commits into from
May 21, 2024
Merged

Fix test that leaks images. #148494

merged 14 commits into from
May 21, 2024

Conversation

polina-c
Copy link
Contributor

@polina-c polina-c commented May 16, 2024

Contributes to #145599

Repro:
flutter test test/cupertino/tab_scaffold_test.dart --dart-define LEAK_TRACKING=true --plain-name "Adding new tabs does not crash the app"

What is going on:

  1. ImageCache.putIfAbsent, in case the image already existed in the cache, invokes ImageCache._trackLiveImage, that creates _LiveImage and passes the image's completer as parameter completer
  2. _LiveImage constructor invokes super constructor (of _CachedImageBase) that initializes the member handle
  3. handle is disposed using scheduler, and disposal does not happen in time of test completion.

Adding delay to the test increases number of not disposed objects from 30 to 120.
Should we force schedule at the end of the widget tests somehow to make scheduler switched to right state?

Creation call stack:

#9______new_ImageStreamCompleterHandle.__(package:flutter/src/painting/image_stream.dart:465:41)
#10_____ImageStreamCompleter.keepAlive_(package:flutter/src/painting/image_stream.dart:655:39)
#11_____new__CachedImageBase_(package:flutter/src/painting/image_cache.dart:609:27)
#12_____new__LiveImage_(package:flutter/src/painting/image_cache.dart:647:9)
#13_____ImageCache._trackLiveImage.<anonymous_closure>_(package:flutter/src/painting/image_cache.dart:302:14)
#14______LinkedHashMapMixin.putIfAbsent_(dart:collection-patch/compact_hash.dart:543:23)
#15_____ImageCache._trackLiveImage_(package:flutter/src/painting/image_cache.dart:296:17)
#16_____ImageCache.putIfAbsent_(package:flutter/src/painting/image_cache.dart:378:7)
#17_____ImageProvider.resolveStreamForKey_(package:flutter/src/painting/image_provider.dart:517:81)
                      #18_____ScrollAwareImageProvider.resolveStreamForKey_(package:flutter/src/widgets/scroll_aware_image_provider.dart:104:19)
#19_____ImageProvider.resolve.<anonymous_closure>_(package:flutter/src/painting/image_provider.dart:366:9)
#20_____ImageProvider._createErrorHandlerAndKey.<anonymous_closure>_(package:flutter/src/painting/image_provider.dart:479:24)
#21_____SynchronousFuture.then_(package:flutter/src/foundation/synchronous_future.dart:43:39)
                      #22_____ImageProvider._createErrorHandlerAndKey_(package:flutter/src/painting/image_provider.dart:476:9)
#23_____ImageProvider.resolve_(package:flutter/src/painting/image_provider.dart:363:5)
#24______ImageState._resolveImage_(package:flutter/src/widgets/image.dart:1111:16)
#25______ImageState.didChangeDependencies_(package:flutter/src/widgets/image.dart:1061:5)
#26_____StatefulElement._firstBuild_(package:flutter/src/widgets/framework.dart:5630:11)
#27_____ComponentElement.mount_(package:flutter/src/widgets/framework.dart:5457:5)
#28_____Element.inflateWidget_(package:flutter/src/widgets/framework.dart:4334:16)
#29_____Element.updateChild_(package:flutter/src/widgets/framework.dart:3843:18)
#30_____SingleChildRenderObjectElement.mount_(package:flutter/src/widgets/framework.dart:6763:14)

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: cupertino flutter/packages/flutter/cupertino repository labels May 16, 2024
@polina-c polina-c added the a: leak tracking Issues and PRs related to memory leaks detected by leak_tracker label May 19, 2024
@@ -685,6 +688,8 @@ void main() {

// Tapping the tabs should still work.
expect(tabsPainted, const <int>[0, 0, 18]);

await tester.runAsync(() async =>Future<void>.delayed(const Duration(milliseconds: 10)));
Copy link
Contributor

Choose a reason for hiding this comment

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

replacing this line with imageCache.clear(); fixes all the leaks in this test for me.

@polina-c
Copy link
Contributor Author

polina-c commented May 21, 2024

On my mac with imageCache.clear(); there are less leaks with this change and they are different:

  Expected: leak free
    Actual: <Instance of 'Leaks'>
     Which: contains leaks:
            # The text is generated by leak_tracker.
            # For leak troubleshooting tips open:
            # https://github.com/dart-lang/leak_tracker/blob/main/doc/TROUBLESHOOT.md
            notDisposed:
              total: 6
              objects:
                StatefulElement:
                  test: Adding new tabs does not crash the app
                  identityHashCode: 823867307
                _MediaQueryFromViewState:
                  test: Adding new tabs does not crash the app
                  identityHashCode: 701933900
                LeafRenderObjectElement:
                  test: Adding new tabs does not crash the app
                  identityHashCode: 637995193
                RenderErrorBox:
                  test: Adding new tabs does not crash the app
                  identityHashCode: 761576866
                PictureLayer:
                  test: Adding new tabs does not crash the app
                  identityHashCode: 1042115368
                _NativePicture:
                  test: Adding new tabs does not crash the app
                  identityHashCode: 515013832

But bots run on Windows, and if on Windows there are no leaks, it is good fix for now. Eventually we will add bots for other platforms.

However there is a question:
There are many tests that deal with animation. Why they are not leaking? Why we need to clean cash just for this specific test? I think we need explanation to guide developers when it is needed.

@Hixie
Copy link
Contributor

Hixie commented May 21, 2024

This test loads images. The images get cached. The cache is shared across tests (intentionally). My theory about it being related to animated GIFs was mistaken.

@polina-c polina-c marked this pull request as ready for review May 21, 2024 20:15
@polina-c polina-c changed the title Investigation for https://github.com/flutter/flutter/issues/145599 Fix test that leak images. May 21, 2024
@Hixie
Copy link
Contributor

Hixie commented May 21, 2024

We should document somewhere that if you use cached images, they'll get flagged as leaked unless you manually clear the cache.

Comment on lines 50 to 52
tearDownAll(() {
imageCache.clear();
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Per https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#make-each-test-entirely-self-contained it's generally better to put this in each test, that way you can easily copy tests to other files and generally easily tell what exactly each test is doing.

Copy link
Contributor Author

@polina-c polina-c May 21, 2024

Choose a reason for hiding this comment

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

Clean up for each test will improve test isolation.
Clean up just at the end will improve test performance.

I guess one of the reasons image cashing exists is exactly performance of testing.

Existing code uses tearDown. So, switched.

@Hixie
Copy link
Contributor

Hixie commented May 21, 2024

cc @jonahwilliams who is probably the person most familiar with the image cache?
cc @goderbauer and @Piinks who may have opinions too

@polina-c
Copy link
Contributor Author

We should document somewhere that if you use cached images, they'll get flagged as leaked unless you manually clear the cache.

Added the guidance to the page that is linked in leak related exceptions: dart-lang/leak_tracker#233

@polina-c polina-c changed the title Fix test that leak images. Fix test that leaks images. May 21, 2024
Copy link
Member

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

LGTM

@polina-c polina-c added the autosubmit Merge PR when tree becomes green via auto submit App label May 21, 2024
@auto-submit auto-submit bot merged commit 87aa842 into flutter:master May 21, 2024
73 checks passed
@polina-c polina-c deleted the repro branch May 21, 2024 21:46
hello-coder-xu added a commit to hello-coder-xu/flutter that referenced this pull request May 22, 2024
…pdate

* master: (92 commits)
  Add tests for actions.0.dart API example. (flutter#148678)
  Introduce `WidgetStateBorderSide.lerp` (flutter#148122)
  add `default-flavor` field to flutter pubspec, which will be used as the flavor in `flutter build/run` if `--flavor` is not provided (flutter#147968)
  [wiki migration] Pages under docs/postmortems/ (flutter#148798)
  Roll Flutter Engine from e5a73e520e89 to c89defa55801 (2 revisions) (flutter#148812)
  Make hover tests functional and cleanup mouse pointers in Material toggleables (flutter#148808)
  Fix two dimensional viewport unexpected null exception when no child is laid out (flutter#148256)
  Roll Flutter Engine from bc1345b6b50a to e5a73e520e89 (3 revisions) (flutter#148807)
  Add test for undo_history_controller.0.dart (flutter#148205)
  Roll Flutter Engine from a8872c8915a2 to bc1345b6b50a (6 revisions) (flutter#148802)
  Fix test that leaks images. (flutter#148494)
  Fix warnings in `dependency_version_checker.gradle.kts` (flutter#148699)
  [wiki migration] Android team pages (flutter#148585)
  Fix leaky test. (flutter#148788)
  Add DropdownButton.menuWidth (flutter#148125)
  Add test for focus example 2 (flutter#147624)
  Add a migrator to remove `FlutterMultiDexApplication.java` (flutter#148515)
  [wiki migration] Infra team pages (flutter#148718)
  Roll Flutter Engine from 8a352f01e503 to a8872c8915a2 (1 revision) (flutter#148776)
  Fix the output of the CDN test. (flutter#148730)
  ...
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 22, 2024
hello-coder-xu added a commit to hello-coder-xu/flutter that referenced this pull request May 23, 2024
* master: (29 commits)
  Add tests for actions.0.dart API example. (flutter#148678)
  Introduce `WidgetStateBorderSide.lerp` (flutter#148122)
  add `default-flavor` field to flutter pubspec, which will be used as the flavor in `flutter build/run` if `--flavor` is not provided (flutter#147968)
  [wiki migration] Pages under docs/postmortems/ (flutter#148798)
  Roll Flutter Engine from e5a73e520e89 to c89defa55801 (2 revisions) (flutter#148812)
  Make hover tests functional and cleanup mouse pointers in Material toggleables (flutter#148808)
  Fix two dimensional viewport unexpected null exception when no child is laid out (flutter#148256)
  Roll Flutter Engine from bc1345b6b50a to e5a73e520e89 (3 revisions) (flutter#148807)
  Add test for undo_history_controller.0.dart (flutter#148205)
  Roll Flutter Engine from a8872c8915a2 to bc1345b6b50a (6 revisions) (flutter#148802)
  Fix test that leaks images. (flutter#148494)
  Fix warnings in `dependency_version_checker.gradle.kts` (flutter#148699)
  [wiki migration] Android team pages (flutter#148585)
  Fix leaky test. (flutter#148788)
  Add DropdownButton.menuWidth (flutter#148125)
  Add test for focus example 2 (flutter#147624)
  Add a migrator to remove `FlutterMultiDexApplication.java` (flutter#148515)
  [wiki migration] Infra team pages (flutter#148718)
  Roll Flutter Engine from 8a352f01e503 to a8872c8915a2 (1 revision) (flutter#148776)
  Fix the output of the CDN test. (flutter#148730)
  ...
hello-coder-xu added a commit to hello-coder-xu/flutter that referenced this pull request May 23, 2024
* master: (92 commits)
  Add tests for actions.0.dart API example. (flutter#148678)
  Introduce `WidgetStateBorderSide.lerp` (flutter#148122)
  add `default-flavor` field to flutter pubspec, which will be used as the flavor in `flutter build/run` if `--flavor` is not provided (flutter#147968)
  [wiki migration] Pages under docs/postmortems/ (flutter#148798)
  Roll Flutter Engine from e5a73e520e89 to c89defa55801 (2 revisions) (flutter#148812)
  Make hover tests functional and cleanup mouse pointers in Material toggleables (flutter#148808)
  Fix two dimensional viewport unexpected null exception when no child is laid out (flutter#148256)
  Roll Flutter Engine from bc1345b6b50a to e5a73e520e89 (3 revisions) (flutter#148807)
  Add test for undo_history_controller.0.dart (flutter#148205)
  Roll Flutter Engine from a8872c8915a2 to bc1345b6b50a (6 revisions) (flutter#148802)
  Fix test that leaks images. (flutter#148494)
  Fix warnings in `dependency_version_checker.gradle.kts` (flutter#148699)
  [wiki migration] Android team pages (flutter#148585)
  Fix leaky test. (flutter#148788)
  Add DropdownButton.menuWidth (flutter#148125)
  Add test for focus example 2 (flutter#147624)
  Add a migrator to remove `FlutterMultiDexApplication.java` (flutter#148515)
  [wiki migration] Infra team pages (flutter#148718)
  Roll Flutter Engine from 8a352f01e503 to a8872c8915a2 (1 revision) (flutter#148776)
  Fix the output of the CDN test. (flutter#148730)
  ...
auto-submit bot pushed a commit to flutter/packages that referenced this pull request May 23, 2024
flutter/flutter@d02292d...73bf206

2024-05-22 102401667+Dimilkalathiya@users.noreply.github.com `CupertinoDialogRoute` leak fix (flutter/flutter#148774)
2024-05-22 fluttergithubbot@gmail.com Marks Windows plugin_test to be flaky (flutter/flutter#148835)
2024-05-22 sokolovskyi.konstantin@gmail.com Add tests for actions.0.dart API example. (flutter/flutter#148678)
2024-05-22 tessertaha@gmail.com Introduce `WidgetStateBorderSide.lerp` (flutter/flutter#148122)
2024-05-22 holzgeist@users.noreply.github.com add `default-flavor` field to flutter pubspec, which will be used as the flavor in `flutter build/run` if `--flavor` is not provided (flutter/flutter#147968)
2024-05-22 katelovett@google.com [wiki migration] Pages under docs/postmortems/ (flutter/flutter#148798)
2024-05-22 engine-flutter-autoroll@skia.org Roll Flutter Engine from e5a73e520e89 to c89defa55801 (2 revisions) (flutter/flutter#148812)
2024-05-22 victorsanniay@gmail.com Make hover tests functional and cleanup mouse pointers in Material toggleables (flutter/flutter#148808)
2024-05-21 amirpanahandeh@yahoo.com Fix two dimensional viewport unexpected null exception when no child is laid out (flutter/flutter#148256)
2024-05-21 engine-flutter-autoroll@skia.org Roll Flutter Engine from bc1345b6b50a to e5a73e520e89 (3 revisions) (flutter/flutter#148807)
2024-05-21 32538273+ValentinVignal@users.noreply.github.com Add test for undo_history_controller.0.dart (flutter/flutter#148205)
2024-05-21 engine-flutter-autoroll@skia.org Roll Flutter Engine from a8872c8915a2 to bc1345b6b50a (6 revisions) (flutter/flutter#148802)
2024-05-21 polinach@google.com Fix test that leaks images. (flutter/flutter#148494)
2024-05-21 34871572+gmackall@users.noreply.github.com Fix warnings in `dependency_version_checker.gradle.kts` (flutter/flutter#148699)
2024-05-21 katelovett@google.com [wiki migration] Android team pages (flutter/flutter#148585)
2024-05-21 polinach@google.com Fix leaky test. (flutter/flutter#148788)
2024-05-21 bruno.leroux@gmail.com Add DropdownButton.menuWidth (flutter/flutter#148125)
2024-05-21 82763757+NobodyForNothing@users.noreply.github.com Add test for focus example 2 (flutter/flutter#147624)
2024-05-21 34871572+gmackall@users.noreply.github.com Add a migrator to remove `FlutterMultiDexApplication.java` (flutter/flutter#148515)
2024-05-21 katelovett@google.com [wiki migration] Infra team pages (flutter/flutter#148718)
2024-05-21 engine-flutter-autoroll@skia.org Roll Flutter Engine from 8a352f01e503 to a8872c8915a2 (1 revision) (flutter/flutter#148776)
2024-05-21 jacksongardner@google.com Fix the output of the CDN test. (flutter/flutter#148730)
2024-05-21 katelovett@google.com [wiki migration] Release team pages (flutter/flutter#148723)
2024-05-21 ian@hixie.ch Remove hidden dependencies on LocalProcessManager (flutter/flutter#148096)
2024-05-21 pateltirth454@gmail.com Adds Missing `onHover` & `onFocusChange` for `OutlinedButton.icon` (flutter/flutter#144374)
2024-05-21 82763757+NobodyForNothing@users.noreply.github.com Adds tests to NestedScrollView examples (flutter/flutter#148170)
2024-05-21 engine-flutter-autoroll@skia.org Roll Flutter Engine from c2ef01f6f1ab to 8a352f01e503 (18 revisions) (flutter/flutter#148766)
2024-05-21 nate.w5687@gmail.com `switch` expressions: finale (flutter/flutter#148711)
2024-05-21 matej.knopp@gmail.com [iOS] specify minimum os version for native asset frameworks (flutter/flutter#148504)
2024-05-21 59215665+davidhicks980@users.noreply.github.com Removed brand references from MenuAnchor.dart (flutter/flutter#148760)
2024-05-21 zanderso@users.noreply.github.com Skip flaky test in expression_evaluation_test.dart (flutter/flutter#148737)

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 rmistry@google.com,stuartmorgan@google.com,tarrinneal@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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: leak tracking Issues and PRs related to memory leaks detected by leak_tracker autosubmit Merge PR when tree becomes green via auto submit App f: cupertino flutter/packages/flutter/cupertino repository framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants