Fix memory leaks in `SnackBar` [prod-leak-fix] #147212

ValentinVignal · 2024-04-23T04:46:42Z

Part of #141198

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.
I followed the breaking change policy and added Data Driven Fixes where supported.
All existing and new tests are passing.

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

ValentinVignal · 2024-04-23T04:46:55Z

cc @polina-c

polina-c · 2024-04-23T20:20:39Z

We have an internal 'Google testing' test failed. This means one of two:

Public test coverage is not good enough and should be extended to cover the failure
The internal usage of the feature is wrong and should be fixed

Putting the test details here hoping they will help you to choose between the two.
Let me know if you need code of SnackBarContainer or you can repro the failure without it.

Test code:

      testWidgets(
          'should remove snack bar when popping back from other scaffold, '
          'if the cross-scaffold min duration has expired.', (tester) async {
        await pumpScaffoldRoutes(tester);
        store.updateState(ApplicationState((b) => b
          ..view.messaging.snackBar = SnackBarState(
            'message',
            shownAt: now,
            persistAcrossScaffolds: persistAcrossScaffolds,
          ).toBuilder()));
        await tester.pumpAndSettle();

        // Verify snack bar shown on route #1.
        expect(find.byKey(Key('route1')), findsOneWidget);
        expectSnackBar('message');

        // Navigate to route #2.
        now = now.add(Duration(milliseconds: 500));
        await tester.tap(find.byKey(Key('navigate.button')));
        await tester.pumpAndSettle();
        expect(find.byKey(Key('route2')), findsOneWidget);

        // If persistent, expect the snack bar to transfer across.
        if (persistAcrossScaffolds) {
          expectSnackBar('message');
        } else {
          expectNoSnackBar();
        }

        // Pop back to route #1.
        now = now.add(defaultSnackBarDuration);
        await tester.pageBack();
        await tester.pumpAndSettle();
        expect(find.byKey(Key('route1')), findsOneWidget);

        // Expect the snack bar to not linger on route #1.
        expectNoSnackBar();
      });

Error:

	Null check operator used on a null value
		The relevant error-causing widget was:
		AnimatedBuilder
		AnimatedBuilder:third_party/dart/flutter/lib/src/material/snack_bar.dart:859:28
		When the exception was thrown, this was the stack:
		#0      _SnackBarState.build.<anonymous closure> (third_party/dart/flutter/lib/src/material/snack_bar.dart:864:43) ...

ValentinVignal · 2024-04-24T02:08:09Z

That is a weird bug, it looks like an animation is not set when running the build, but they are supposed to be since it is done in the initState and didUpdateWidget 🤔 I wasn't able to reproduce the failure, would you have access to pumpScaffoldRoutes, store, SnackBarState, ApplicationState ?

polina-c · 2024-04-26T20:31:20Z

It is challenging to share internal code in a way that does not leak internal sensitive information.

Let's try other approach.
Can you please add some debug time (in asserts like here) protection for usage after disposal?
To the animation itself and _SnackBarState.
So that we can see what caused the failure clearer.

Let's see how many tests will fail in Flutter. If not many and they are easy to fix, we may want to keep it. And it may help us to figure out the internal failure.

ValentinVignal · 2024-04-29T15:31:31Z

Sure, I added the asserts in perf: Add debugDispose to snackbar and curved animation. Let's see what comes out of it

polina-c · 2024-04-30T15:48:16Z

packages/flutter/lib/src/animation/animations.dart

    return reverseCurve == null || (_curveDirection ?? parent.status) != AnimationStatus.reverse;
  }

+  bool _debugDisposed = false;


I think we can use isDisposed defined on 425.

polina-c · 2024-05-01T14:55:48Z

It seems states are used after disposal. Can it be that Null check operator used on a null value that was raised in G3 is caused by usage after disposal?

If yes, can we make the classes more resilient to usage after disposal and some test coverage for the resilience?

ValentinVignal · 2024-05-02T14:33:55Z

perf: Add debugDispose to snackbar and curved animation

Sure, I guess I should also remove what was done in perf: Add debugDispose to snackbar and curved animation (the debug time protection in asserts for usage after disposal?) ?

polina-c · 2024-05-02T22:42:20Z

perf: Add debugDispose to snackbar and curved animation

Sure, I guess I should also remove what was done in perf: Add debugDispose to snackbar and curved animation (the debug time protection in asserts for usage after disposal?) ?

For now you may want to replace debug asserts with debug prints in case of violation, so that we see if usage after disposal is causing failures in google testing, in case they are still here.

polina-c · 2024-05-03T16:29:36Z

Converted the PR to draft, as it is not intended for merge at the moment.

flutter-dashboard · 2024-05-03T16:31:09Z

This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

ValentinVignal · 2024-05-05T14:49:55Z

@polina-c ci: Add debug prints to investigate failing ci adds some logs, do they show something in the google internal tests?

polina-c · 2024-05-05T16:36:39Z

I see print:

CurvedAnimation.value is used after being disposed. AnimationController#540df(▶ 0.400; for SnackBar)➩Interval(0.72⋯1.0)➩Cubic(0.40, 0.00, 0.20, 1.00)ₒₙ/Threshold

The print is followed by the error:

The following _TypeError was thrown building AnimatedBuilder(listenable: AnimationController#178cc(▶
		0.400; for SnackBar)➩Cubic(0.40, 0.00, 0.20, 1.00), dirty, state: _AnimatedState#23f3b):
		Null check operator used on a null value
		The relevant error-causing widget was:
		AnimatedBuilder
		AnimatedBuilder:third_party/dart/flutter/lib/src/material/snack_bar.dart:874:28
		When the exception was thrown, this was the stack:
		#0      _SnackBarState.build.<anonymous closure> (third_party/dart/flutter/lib/src/material/snack_bar.dart:879:43)
		#1      ListenableBuilder.build (third_party/dart/flutter/lib/src/widgets/transitions.dart:1127:48)
		#2      _AnimatedState.build (third_party/dart/flutter/lib/src/widgets/transitions.dart:138:48)
		#3      StatefulElement.build (third_party/dart/flutter/lib/src/widgets/framework.dart:5593:27)
		#4      ComponentElement.performRebuild (third_party/dart/flutter/lib/src/widgets/framework.dart:5481:15)
		#5      StatefulElement.performRebuild (third_party/dart/flutter/lib/src/widgets/framework.dart:5644:11)
		#6      Element.rebuild (third_party/dart/flutter/lib/src/widgets/framework.dart:5197:7)
		#7      BuildOwner.buildScope (third_party/dart/flutter/lib/src/widgets/framework.dart:2905:19)
		#8      AutomatedTestWidgetsFlutterBinding.drawFrame (third_party/dart/flutter_test/lib/src/binding.dart:1418:19)
		#9      RendererBinding._handlePersistentFrameCallback (third_party/dart/flutter/lib/src/rendering/binding.dart:468:5)
		#10     SchedulerBinding._invokeFrameCallback (third_party/dart/flutter/lib/src/scheduler/binding.dart:1392:15)
		#11     SchedulerBinding.handleDrawFrame (third_party/dart/flutter/lib/src/scheduler/binding.dart:1313:9)
		#12     AutomatedTestWidgetsFlutterBinding.pump.<anonymous closure> (third_party/dart/flutter_test/lib/src/binding.dart:1273:9)
		#15     TestAsyncUtils.guard (third_party/dart/flutter_test/lib/src/test_async_utils.dart:71:41)
		#16     AutomatedTestWidgetsFlutterBinding.pump (third_party/dart/flutter_test/lib/src/binding.dart:1260:27)

Does it help?

ValentinVignal · 2024-05-06T02:46:08Z

🤔 I'm still confused with it haha.
I believe the snack bar is getting disposed and for some reason the AnimatedBuilder using _heightAnimation rebuilds.

I can try to add more prints.

Alternatively, I could do a check like that:

     if (_heightAnimation != null) {
      snackBarTransition = AnimatedBuilder(
        animation: _heightAnimation!,
        builder: (BuildContext context, Widget? child) {
          return Align(
            alignment: AlignmentDirectional.topStart,
            heightFactor: _heightAnimation!.value,
            child: child,
          );
        },
        child: snackBar,
      );
    } else {
      snackBarTransition = child;
    }

but that looks wrong to me

polina-c · 2024-05-06T16:44:14Z

I do not know what is right thing to do here. I am open for experiments in different directions directions. Feel free to start tackling them in parallel by creating number of draft PRs that are intended to solve the same problem, to check how tests will react. :)

ValentinVignal · 2024-05-07T06:09:34Z

Sure I can do that. Can you help me check if there are some useful logs from the commit debug: Add more prints ?

polina-c · 2024-05-07T17:43:44Z

Checking.
For debug prints it would be helpful to prefix all them with something outstanding like !!!!, to make search easier.
And, you may want to add identityHachCode(animation) to see which prints go from which instances.

polina-c · 2024-05-07T17:45:34Z

Snackbar.dispose called. _SnackBarState#e1f97
		Snackbar.dispose called. _SnackBarState#e5677
		00:01 �[32m+1�[0m: SnackBarContainer with persistAcrossScaffolds=true should provide a SnackBar action when configured�[0m
		Snackbar.dispose called. _SnackBarState#cfaa6
		00:01 �[32m+2�[0m: SnackBarContainer with persistAcrossScaffolds=true should hide SnackBar action when allowSnackbarAction=false�[0m
		Snackbar.dispose called. _SnackBarState#06f1c
20:30:31.000	U	00:01 �[32m+3�[0m: SnackBarContainer with persistAcrossScaffolds=true should only transfer persistent snack bar across scaffolds�[0m
		Snackbar.dispose called. _SnackBarState#352da
		Snackbar.dispose called. _SnackBarState#938f5
		CurvedAnimation.value is used after being disposed. AnimationController#8c4d9(▶ 0.400; for SnackBar)➩Interval(0.72⋯1.0)➩Cubic(0.40, 0.00, 0.20, 1.00)ₒₙ/Threshold
		SnackBar._heightAnimation in AnimatedBuilder. _SnackBarState#938f5(lifecycle state: defunct, not mounted)
		SnackBar is disposed in AnimatedBuilder. _SnackBarState#938f5(lifecycle state: defunct, not mounted)
		SnackBar._heightAnimation in AnimatedBuilder. _SnackBarState#938f5(lifecycle state: defunct, not mounted)
		SnackBar is disposed in AnimatedBuilder. _SnackBarState#938f5(lifecycle state: defunct, not mounted)
		══╡ EXCEPTION CAUGHT BY WIDGETS LIBRARY ╞═══════════════════════════════════════════════════════════
		The following _TypeError was thrown building AnimatedBuilder(listenable: AnimationController#8c4d9(▶
		0.400; for SnackBar)➩Cubic(0.40, 0.00, 0.20, 1.00), dirty, state: _AnimatedState#fa9e2):
		Null check operator used on a null value
		The relevant error-causing widget was:
		AnimatedBuilder
		AnimatedBuilder:third_party/dart/flutter/lib/src/material/snack_bar.dart:875:28
		When the exception was thrown, this was the stack:
		#0      _SnackBarState.build.<anonymous closure> (third_party/dart/flutter/lib/src/material/snack_bar.dart:889:43)
		#1      ListenableBuilder.build (third_party/dart/flutter/lib/src/widgets/transitions.dart:1127:48)
		#2      _AnimatedState.build (third_party/dart/flutter/lib/src/widgets/transitions.dart:138:48)
		#3      StatefulElement.build (third_party/dart/flutter/lib/src/widgets/framework.dart:5593:27)
		#4      ComponentElement.performRebuild (third_party/dart/flutter/lib/src/widgets/framework.dart:5481:15)

# Conflicts: # packages/flutter/lib/src/material/snack_bar.dart

ValentinVignal · 2024-05-09T16:38:38Z

@polina-c It looks like fix: Use ValueListenableBuilder did the trick 😃

ValentinVignal · 2024-05-14T01:49:23Z

@polina-c Is there something else you want me to do on this?

flutter/flutter@d2da1b2...39651e8 2024-05-15 [email protected] Roll Flutter Engine from d35a1a603c80 to bf1c6da0dd31 (1 revision) (flutter/flutter#148369) 2024-05-15 [email protected] Roll Flutter Engine from 55c62ff82c7e to d35a1a603c80 (4 revisions) (flutter/flutter#148367) 2024-05-15 [email protected] Roll Flutter Engine from a1d930a3a84d to 55c62ff82c7e (3 revisions) (flutter/flutter#148365) 2024-05-14 [email protected] Roll Flutter Engine from dfb5871260a6 to a1d930a3a84d (2 revisions) (flutter/flutter#148360) 2024-05-14 [email protected] Roll Flutter Engine from 1b508a071c96 to dfb5871260a6 (1 revision) (flutter/flutter#148356) 2024-05-14 49699333+dependabot[bot]@users.noreply.github.com Bump codecov/codecov-action from 4.3.1 to 4.4.0 (flutter/flutter#148355) 2024-05-14 [email protected] Fix memory leaks in `SnackBar` (flutter/flutter#147212) 2024-05-14 [email protected] Fix memory leaks in open upwards page transition (flutter/flutter#148046) 2024-05-14 [email protected] Fixes semantics ordering when there are multiple TextFields with prefâ�¦ (flutter/flutter#148267) 2024-05-14 [email protected] Roll Flutter Engine from ae9ff69a0840 to 1b508a071c96 (2 revisions) (flutter/flutter#148351) 2024-05-14 [email protected] Add missing InputDecorator.hintText tests (flutter/flutter#148113) 2024-05-14 [email protected] Fix abi key for local golden file testing (flutter/flutter#148072) 2024-05-14 [email protected] Maintain the same layout constraints for item in the ReorderableList during dragging as before dragging. (flutter/flutter#147863) 2024-05-14 [email protected] test material text field example (flutter/flutter#147864) 2024-05-14 [email protected] Move toggleable to widget layer (flutter/flutter#148272) 2024-05-14 [email protected] Roll Flutter Engine from 08b44d906fab to ae9ff69a0840 (1 revision) (flutter/flutter#148341) 2024-05-14 [email protected] add another print trace to runInView (flutter/flutter#148337) 2024-05-14 [email protected] Roll Flutter Engine from 7bf865774d06 to 08b44d906fab (4 revisions) (flutter/flutter#148338) 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] 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

ValentinVignal added 2 commits April 23, 2024 12:46

fix: Fix leak memories in snack bar

ae36e79

test: Update the tests

767be78

github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Apr 23, 2024

polina-c approved these changes Apr 23, 2024

View reviewed changes

polina-c added the a: leak tracking Issues and PRs related to memory leaks detected by leak_tracker label Apr 23, 2024

perf: Add debugDispose to snackbar and curved animation

0a845a8

github-actions bot added the a: animation Animation APIs label Apr 29, 2024

Merge branch 'master' into flutter/fix-leak-memories-in-snack-bar

52e1526

polina-c reviewed Apr 30, 2024

View reviewed changes

refactor: Use existing isDisposed

ceaebbe

ci: Add debug prints to investigate failing ci

1d0766e

polina-c marked this pull request as draft May 3, 2024 16:28

debug: Add more prints

e9763ce

ValentinVignal added 2 commits May 9, 2024 23:13

fix: Use ValueListenableBuilder

c79ce63

Merge branch 'main' into flutter/fix-leak-memories-in-snack-bar

5224016

# Conflicts: # packages/flutter/lib/src/material/snack_bar.dart

revert: Remove debut prints

61153c9

github-actions bot removed the a: animation Animation APIs label May 9, 2024

ValentinVignal marked this pull request as ready for review May 9, 2024 16:42

ValentinVignal requested a review from polina-c May 13, 2024 06:20

polina-c merged commit f335145 into flutter:master May 14, 2024

engine-flutter-autoroll mentioned this pull request May 15, 2024

Roll Flutter from d2da1b280b51 to 39651e84ea12 (18 revisions) flutter/packages#6738

Merged

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

f335145 Fix memory leaks in SnackBar (flutter/flutter#147212)

c921499

Piinks mentioned this pull request May 17, 2024

Flutter Framework Team: Notable Commits #121415

Closed

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

f335145 Fix memory leaks in SnackBar (flutter/flutter#147212)

9f93321

polina-c changed the title ~~Fix memory leaks in SnackBar~~ Fix memory leaks in SnackBar [prod-leak-fix] Aug 29, 2024

Fix memory leaks in SnackBar [prod-leak-fix] #147212

Fix memory leaks in SnackBar [prod-leak-fix] #147212

Uh oh!

Conversation

ValentinVignal commented Apr 23, 2024

Pre-launch Checklist

Uh oh!

ValentinVignal commented Apr 23, 2024

Uh oh!

polina-c commented Apr 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ValentinVignal commented Apr 24, 2024

Uh oh!

polina-c commented Apr 26, 2024

Uh oh!

ValentinVignal commented Apr 29, 2024

Uh oh!

polina-c Apr 30, 2024

Choose a reason for hiding this comment

Uh oh!

polina-c commented May 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ValentinVignal commented May 2, 2024

Uh oh!

polina-c commented May 2, 2024

Uh oh!

polina-c commented May 3, 2024

Uh oh!

flutter-dashboard bot commented May 3, 2024

Uh oh!

ValentinVignal commented May 5, 2024

Uh oh!

polina-c commented May 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ValentinVignal commented May 6, 2024

Uh oh!

polina-c commented May 6, 2024

Uh oh!

ValentinVignal commented May 7, 2024

Uh oh!

polina-c commented May 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

polina-c commented May 7, 2024

Uh oh!

ValentinVignal commented May 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ValentinVignal commented May 14, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Fix memory leaks in `SnackBar` [prod-leak-fix] #147212

Fix memory leaks in `SnackBar` [prod-leak-fix] #147212

polina-c commented Apr 23, 2024 •

edited

Loading

polina-c commented May 1, 2024 •

edited

Loading

polina-c commented May 5, 2024 •

edited

Loading

polina-c commented May 7, 2024 •

edited

Loading

ValentinVignal commented May 9, 2024 •

edited

Loading