Fix memory leaks in `SnackBar`

ValentinVignal commented 3 weeks ago

Part of https://github.com/flutter/flutter/issues/141198

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

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

ValentinVignal commented 3 weeks ago

cc @polina-c

polina-c commented 3 weeks ago

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 commented 3 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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

polina-c commented 2 weeks ago

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 commented 2 weeks ago

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 commented 1 week ago

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 commented 1 week ago

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

flutter-dashboard[bot] commented 1 week ago

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 commented 1 week ago

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

polina-c commented 1 week ago

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 commented 1 week ago

🤔 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 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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)

ValentinVignal commented 6 days ago

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

ValentinVignal commented 2 days ago

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

flutter / flutter

Fix memory leaks in `SnackBar` #147212

Pre-launch Checklist