fixes DialogRoute memory leak

[Dimilkalathiya]

part of #141198

• Fixes memory leak on DialogRoute
• Adds opt-in test

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]

Is it possible that buildTransitions is called a 2nd time with an animation parameter that is not the same as the one when it was first called? If yes, the CurvedAnimation _fadeCurve needs to be replaced with a new one to have the correct animation

[Dimilkalathiya]

Thanks for pointing it out, i have looked into abstract class ModalRoute and abstract class TransitionRoute and it initialises animation only once and it does not get replaced so using above code snippet is safe :)

[Dimilkalathiya]

cc: @polina-c

[polina-c]

@chingjun , can you review to trigger google testing on this?

[auto-submit]

auto label is removed for flutter/flutter/147816, due to This PR has not met approval requirements for merging. The PR author is not a member of flutter-hackers and needs 2 more review(s) in order to merge this PR.

• Merge guidelines: A PR needs at least one approved review if the author is already part of flutter-hackers or two member reviews if the author is not a flutter-hacker before re-applying the autosubmit label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

[chingjun]

@polina-c when you approve the PR on github, does it trigger Google Testing?

Nope. Google Testing is triggered by autosubmit, that requires two reviews from maintainers. So, I need a second one.
It is ok not to trigger it for simple PRs. For this one I want it to be triggered.

[chingjun]

RSLGTM to trigger Google Testing

[auto-submit]

auto label is removed for flutter/flutter/147816, due to - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label.

[Dimilkalathiya]

@polina-c it seems the Google test is failing can you provide a brief idea on what is Google test? And how should I be able to fix the test failing?

I saw logs and it says "Please contact a Google developer to investigate"

[polina-c]

Google testing is testing of the change for internal Google projects. Its failure means one of two:

1. The change introduced a bug to Flutter, and public test coverage is not good enough to catch it. So, we need to fix the bug and maybe add some test coverage.
2. The internal project is using Flutter in a wrong way and should be fixed.

I checked logs. There is no failures in Flutter code, but there is change of behavior of an application: widget that is expected to be visible, is not.
Unfortunately, I cannot share more details.

Is it possible to fix the leak with less changes to the original code? Without overriding buildTransitions?

[Dimilkalathiya]

I see, unfortunately we need to override for disposing CurvedAnimation properly

The previous widget function was outside of class and the curved animation was getting used inside directly so there was no way to dispose it properly

But I'll try to come up with a better solution, and test-out other options

Also if I push code with changes will Google test now re-run itself? Or we have to manually trigger it again?

[polina-c]

I see, unfortunately we need to override for disposing CurvedAnimation properly

The previous widget function was outside of class and the curved animation was getting used inside directly so there was no way to dispose it properly

But I'll try to come up with a better solution, and test-out other options

Also if I push code with changes will Google test now re-run itself? Or we have to manually trigger it again?

Thanks for explanation.
To be clear, I am not confident that your change is wrong. It may be that application is doing something wrong. We just do not know.

Can you explain why override is needed? Why the methid cannot be just moved inside and the animation stored as a new member variable?

I think google testing will re-run, but I can be wrong.

[Dimilkalathiya]

Previous implementation was passing the widget function into the super constructor so handling variable initialisation and disposal was not possible since we can't initialise them while not constructed.

So the best solution i thought was to override it.

[polina-c]

How about

1. Putting the builder into separate class like _DialogRouteAnimation, that would also hold the instance of the CurvedAnimation.

2. Creating private constructor like

DialogRoute._(this._animation, {required super.pageBuilder, super.barrierLabel, super.transitionDuration,  super.transitionBuilder, ... });

3. Converting existing constructor to factory constructor:

factory DialogRoute({
...
  }) {
    final animation = _DialogRouteAnimation();
    return DialogRoute._(animation,
         transitionBuilder: animation.buildMaterialDialogTransitions,
...

Then you will be able to dispose the animation:

  @override
  void dispose() {
    _animation.dispose();
    super.dispose();
  }

I hope this will preserve existing functionality better, and app behavior will not change.

[Dimilkalathiya]

Seems like an awesome idea, I'll try this

[Dimilkalathiya]

@polina-c seems like it's not related to override logic as the test is still failing.

I am still not sure, I think it's most likely a new animation handling snippet.

Since now the google test will initiate automatically, I'll test-out some ideas I have.

Revoke approval to prevent Google Testing from being triggered automatically.

[chingjun]

Revoking approval on "Google Testing" since changes are supposed to be reviewed by Googlers before Google Testing can be initiated. It wouldn't be useful without a way to see the actual result anyway.

I just looked at the test result, I think your latest change has fixed the original issue. The test that used to fail has now passed.

But it is introducing new error for classes that extends DialogRoute<T>, now that the constructor has become a factory.

Something that looks like:

class SubDialogRoute<T> extends DialogRoute<T> {
  SubDialogRoute({
    required BuildContext context,
    required WidgetBuilder builder,
    CapturedThemes? themes,
    Color? barrierColor,
    bool barrierDismissible = true,
    String? barrierLabel,
    bool useSafeArea = true,
    RouteSettings? settings,
  }) : super(
          context: context,
          builder: builder,
          themes: themes,
          barrierColor: barrierColor,
          barrierDismissible: barrierDismissible,
          barrierLabel: barrierLabel,
          useSafeArea: useSafeArea,
          settings: settings,
        );
}

Will now trigger a compiler error:

ERROR: xxx.dart:xx
The generative constructor 'DialogRoute<T> DialogRoute({required BuildContext context, required Widget Function(BuildContext) builder, CapturedThemes? themes, Color? barrierColor = Colors.black54, bool barrierDismissible = true, String? barrierLabel, bool useSafeArea = true, RouteSettings? settings, Offset? anchorPoint, TraversalEdgeBehavior? traversalEdgeBehavior})' is expected, but a factory was found. #non_generative_constructor

  }) : super(
       ^^^^^^


xxx.dart:xxx: Error: Superclass has no constructor named 'DialogRoute'.
  }) : super(
       ^^^^^

[Dimilkalathiya]

@chingjun @polina-c I see it's bit tricky, if we override buildTransitions it does not display a widget that should be visible, Adding factory constructor fixed this issue but that mean it will break any other class that extends to DialogRoute

How about if we make override less invasive?

previously we were overriding entire widget which may have been cause of missing widget?

instead of:

  @override
  Widget buildTransitions(BuildContext context, Animation<double> animation, Animation<double> secondaryAnimation, Widget child) {
    _fadeCurve ??= CurvedAnimation(
      parent: animation,
      curve: Curves.easeOut,
    );
   return FadeTransition(
      opacity: _fadeCurve!,
      child: child,
    );
  }

we could try this:

  @override
  Widget buildTransitions(BuildContext context, Animation<double> animation, Animation<double> secondaryAnimation, Widget child) {
    _fadeCurve ??= CurvedAnimation(
      parent: animation,
      curve: Curves.easeOut,
    );
   return FadeTransition(
      opacity: _fadeCurve!,
      child: super.buildTransitions(context, animation, secondaryAnimation, child),
    );
  }

Let me know if we have better way to handle it or if above mentioned solution worth try. :)

[polina-c]

Can you move this class to the place of the method? It will make code review easier.

[polina-c]

Indentation issues.

[polina-c]

What if previously created _fadeCurve used different animation. Should we check the equality somehow and recreate _fadeCurve, if it is different?

[polina-c]

@chingjun , it is good learning that change of type of constructor is a breaking change. Thanks.

Okay, as we got failure, my previous comments are not relevant.
We can give a try to what you, @Dimilkalathiya , suggest. Let's see how it works.

[Dimilkalathiya]

@chingjun @polina-c Google test is passing now