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

Clean up 'not disposed' memory leaks in FF, phase 3. #141198

Open
8 of 9 tasks
polina-c opened this issue Jan 9, 2024 · 11 comments
Open
8 of 9 tasks

Clean up 'not disposed' memory leaks in FF, phase 3. #141198

polina-c opened this issue Jan 9, 2024 · 11 comments
Assignees
Labels
a: leak tracking Issues and PRs related to memory leaks detected by leak_tracker c: new feature Nothing broken; request for a new capability P1 High-priority issues at the top of the work list

Comments

@polina-c
Copy link
Contributor

polina-c commented Jan 9, 2024

Looking for volunteers to help with this!!! 🚀
Bigger issue: #134787

Goal

Clean up remaining memory leaks (~25) in order to switch to leak monitoring state.

Steps

In general:

  1. Leak tracking is disabled by default, but enabled just for three non-blocking windows bots. To enable leak tracking for tests locally, search for if (isLeakTrackingEnabled()) { in the code and temporary replace it with if (true).
  2. If you create an issue or PR for cleanups, cc @polina-c
  3. Update everybody on your plans by commenting on this issue, so that we do not duplicate effort.
  4. If your PR stops ignoring leaks for some tests, trigger leak verifying bots for your PR: find them in code by _leak_tracking and temporarily comment out bringup: true.
  5. Ping me on GitHub or twitter (PolinaCC) if a PR is waiting for something more than one business day
  6. If your PR or issue is closed, and you noticed I forgot to add label a: leak tracking, remind me, please

Fix a memory leak:

  1. Identify a leak from one of the sources:

    a. flutter dashboard
    Look for 3 windows icons on the top left with tooltip ending "leak_tracking".
    If one of them have the last execution failing, open its test stdout and search for contains leaks:.

    b. Known not-investigated leaky test
    Search for [leaks-to-clean] in flutter/test

    c. Not assigned known issue
    Assign the issue to yourself.

  2. Investigate the leak.

    if there is a known issue for the leak and it is assigned
    then: add/update TODO for the leak (see template below)
    else: (fix the leak) or (create new issue and add the leak TODO to the test)

Or instrument a disposable (example):

Templates

Leak tracking opt-out TODO:

testWidgets('<test name>',
// TODO(< polina-c | your alias >): <explanation and link to issue> [leaks-to-clean]
experimentalLeakTesting: LeakTesting.settings.withIgnoredAll(),
(WidgetTester tester) async { ...

Leak tracking opt-in TODO (when tracking is opted out in flutter_test_config.dart or in the entire test file):

// TODO(polina-c): remove when fixed https://github.com/flutter/flutter/issues/145600 [leak-tracking-opt-in]
experimentalLeakTesting: LeakTesting.settings.withTracked(classes: <String>['CurvedAnimation']),
(WidgetTester tester) async { ...

Steps to repro (to be copied to issues):

To repro the leak:

  1. Copy link to the issue
  2. Search for the copied link in Flutter source code. You will find number of TODOs marked [leaks-to-clean].
  3. Remove the opting out line, one of the TODOs
  4. Run the test with the TODO, or, if it is global TODO, run all tests, with added --dart-define LEAK_TRACKING=true
  5. See the failure.

References

Issues: all, open, known leaks.
PRs: all, open.

Discord

@polina-c polina-c self-assigned this Jan 9, 2024
@polina-c polina-c added the a: leak tracking Issues and PRs related to memory leaks detected by leak_tracker label Jan 9, 2024
@polina-c polina-c added the c: new feature Nothing broken; request for a new capability label Jan 11, 2024
@polina-c
Copy link
Contributor Author

polina-c commented Jan 11, 2024

CC those who participated in other leak tracking activities.

@BlackLeg15, @bivens-dev, @blaugold, @CoderBuck, @cmkweber, @davidmigloz, @Dean-Spotec, @droidbg, @felipecastrosales, @gustav3d, @Harishwarrior, @iapicca, @jayjah, @jogboms, @JohnnyRainbow81, @ksokolovskyi, @Levi-Lesches, @LucasXu0, @Markzipan, @mono0926, @Michal-MK, @navaronbracke, @NobodyForNothing, @noga-dev, @normidar, @p-mazhnik, @philos3, @PiN73, @rei-codes, @resfandiari, @sullenel, @tgucio,@victoreronmosele, @WillianSalceda

I am searching for volunteers to help on this issue.

@polina-c polina-c added the P1 High-priority issues at the top of the work list label Jan 11, 2024
@polina-c
Copy link
Contributor Author

polina-c commented Jan 12, 2024

Working to:

  • - cleanup failures on flutter dashboard
  • - instrument ImageInfo
  • - update documentation for leak tracker
  • - fix performance issue

@ksokolovskyi
Copy link
Contributor

@polina-c ScrollDragController, SelectionOverlay, TextSelectionOverlay, and SnapshotPainter are already instrumented, could you please add checkmarks for them?

@polina-c
Copy link
Contributor Author

polina-c commented Jan 12, 2024

@polina-c ScrollDragController, SelectionOverlay, TextSelectionOverlay, and SnapshotPainter are already instrumented, could you please add checkmarks for them?

@ksokolovskyi
Done. Thank you.

auto-submit bot added a commit that referenced this issue Jan 15, 2024
…141545)

Reverts #141526
Initiated by: CaseyHillers
This change reverts the following previous change:
Original Description:
### Description
- Adds `BoxPainter` creation and disposal events dispatching for memory leak tracking as part of #141198

### Tests
- Updates `decoration_test.dart` to test `BoxPainter` object creation and disposal events dispatching.
@polina-c
Copy link
Contributor Author

Progress check: ~50 laking tests or test files.

@polina-c
Copy link
Contributor Author

polina-c commented Jan 30, 2024

Tagged remaining leaks with [leaks-to-clean].
There are 38.

auto-submit bot pushed a commit that referenced this issue Feb 22, 2024
Contributes to #141198

### Description
- Adds `CurvedAnimation` disposals to `material/chip.dart`, `material/input_decorator.dart`, `material/toggleable.dart`, `widgets/animated_switcher.dart`, `widgets/overscroll_indicator.dart`.
@polina-c
Copy link
Contributor Author

CurvedAnimation is leaking a lot. I need to recount leaks after this instrumentation :)

@polina-c
Copy link
Contributor Author

polina-c commented Mar 10, 2024

To repro a leak:

  1. Copy link to the issue
  2. Search for the copied link in Flutter source code. You will find number of TODOs marked [leaks-to-clean].
  3. Remove the opting out line, one of the TODOs
  4. Run the test with the TODO, or, if it is global TODO, run all tests, with added --dart-define LEAK_TRACKING=true
  5. See the failure.

@polina-c
Copy link
Contributor Author

polina-c commented May 1, 2024

See remaining leaks of CurvedAnimation here.

auto-submit bot added a commit that referenced this issue May 1, 2024
Reverts: #147403
Initiated by: chingjun
Reason for reverting: Causing an internal test to fail, see b/338159496 for details
Original PR Author: ValentinVignal

Reviewed By: {polina-c}

This change reverts the following previous change:
Part of #141198
auto-submit bot pushed a commit that referenced this issue May 7, 2024
part of #141198 

- Fixes memory leak on `RawDialogRoute`
- Adds opt-in test
auto-submit bot pushed a commit that referenced this issue May 8, 2024
Part of #141198
*Fixes memory leak on `RangeSlider`*
@ismailcaakir
Copy link

Unfortunately I solved the problem by downgrading to 3.19.0 on the stable channel.

@polina-c
Copy link
Contributor Author

polina-c commented Jun 3, 2024

Unfortunately I solved the problem by downgrading to 3.19.0 on the stable channel.

@ismailcaakir , can you give more details? Which exact problem do you mean?

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 c: new feature Nothing broken; request for a new capability P1 High-priority issues at the top of the work list
Projects
None yet
Development

No branches or pull requests

3 participants