_DefaultTabControllerState should dispose all created TabContoller instances.

[ksokolovskyi]

Description

• Fixes Memory Leak: _DefaultTabControllerState doesn't dispose created TabController. #136607

Tests

• Updates material/tabs_test.dart to use testWidgetsWithLeakTracking.

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].
• All existing and new tests are passing.

[ksokolovskyi]

cc @polina-c

[polina-c]

Is this true? May be it is up to invoker if the instance can be used or not?

[ksokolovskyi]

To be honest, usage of the TabController after the removal of AnimationController from it (after the call of _copyWith) is impossible because we have several “bang operators” which will throw:

flutter/packages/flutter/lib/src/material/tab_controller.dart

Line 152 in 0b035e0

_animationController!.value = index.toDouble();

flutter/packages/flutter/lib/src/material/tab_controller.dart

Line 200 in 0b035e0

_animationController!

flutter/packages/flutter/lib/src/material/tab_controller.dart

Line 210 in 0b035e0

_animationController!.value = _index.toDouble();

flutter/packages/flutter/lib/src/material/tab_controller.dart

Line 263 in 0b035e0

double get offset => _animationController!.value - _index.toDouble();

flutter/packages/flutter/lib/src/material/tab_controller.dart

Line 270 in 0b035e0

_animationController!.value = value + _index.toDouble();

[polina-c]

This method does not do anything with the original instance, that makes it unusable, so it should not have opinion about what to do with it.

I suggest:

1. Move _animationController = null; out of this method to the invokers, because the methods name does not assume any updates like this.
2. Revert update do the comment.

How does it sound?

[ksokolovskyi]

@polina-c, thanks for the suggestions. According to what you suggested, here is the new code:

@override
void didUpdateWidget(DefaultTabController oldWidget) {
  super.didUpdateWidget(oldWidget);
  if (oldWidget.length != widget.length) {
    ...
    final TabController newController = _controller._copyWith...
    _controller._animationController = null;
    _controller.dispose();
    _controller = newController;
  }

  if (oldWidget.animationDuration != widget.animationDuration) {
    final TabController newController = _controller._copyWith...
    _controller._animationController = null;
    _controller.dispose();
    _controller = newController;
  }
}

What do you think?
Does explicit nulling of the private member look good to you?

[polina-c]

How about making it more readable:

TabController newController = _controller;

if (oldWidget.length != widget.length) {
  newController = newController._copyWith...
}

if (oldWidget.animationDuration != widget.animationDuration) {
  newController = newController._copyWith...
}

if (newController != _controller) {
  _controller.dispose();
  _controller = newController;
  _animationController = null;
}

[ksokolovskyi]

@polina-c, thanks a lot for your suggestion, it looks much cleaner. But if we do it in such a way, then we will lose the reference to the first new TabController if the length and animationDuration are changed at the same time.

[polina-c]

Really? Both _copyWith are invoked for newController, not for _controller. so, nothing is lost.

[ksokolovskyi]

Maybe I am missing something, but here is how I see the flow in case both length and animationDuration was updated:

// newController = TabController0
TabController newController = _controller;

if (oldWidget.length != widget.length) {
  // newController = TabController1
  newController = newController._copyWith...
}

if (oldWidget.animationDuration != widget.animationDuration) {
  // newController = TabController2
  newController = newController._copyWith...
}

if (newController != _controller) {
  _controller._animationController = null;
  // TabController0.dispose()
  _controller.dispose();
  // _controller = TabController2
  _controller = newController;

 // TabController1 is lost
}

[polina-c]

Oh, I see now.
You are right.
Thanks for explanation.

Then the only question is about _animationController = null;:

1. Why we need it? Old controller is not referenced and is disposed, so what's the point to null _animationController?
2. If it is needed, (1) it does not seem _copyWith is right method to do it and (2) there should be comment that explains why it is needed.

[ksokolovskyi]

Thanks for the review.

Under the hood, _copyWith transfers AnimationController from one TabController to another.

  /// Creates a new [TabController] with `index`, `previousIndex`, `length`, and
  /// `animationDuration` if they are non-null.
  ///
  /// This method is used by [DefaultTabController].
  ///
  /// When [DefaultTabController.length] is updated, this method is called to
  /// create a new [TabController] without creating a new [AnimationController].
  TabController _copyWith({
  ...
  }) {
   ...
    return TabController._(
      ...
      animationController: _animationController,
      ....
    );
  }

In dispose method of the TabController we have disposal of the AnimationController.

  @override
  void dispose() {
    _animationController?.dispose();
    _animationController = null;
    super.dispose();
  }

We can't hold the same AnimationController in both TabControllers because it will cause double disposal in the future. So, to not dispose recently injected AnimationController in the old TabController, we need to null it. It will be disposed in the new TabController in the future.

This explains why I added nulling of the _animationController in the _copyWith method and added this comment:

  /// This instance of [TabController] must not be used anymore and has to be
  /// disposed since [AnimationController] was injected into the new
  /// [TabController].

What can you suggest to improve or change?

[polina-c]

Got it. Thanks.

Then _copyWith is now doing way more than just copying.
How about renaming _copyWith to _copyWithAndDispose, and making it fully disposing the original instance?

[ksokolovskyi]

Great suggestion, thanks!
Pushed the new version.

[ksokolovskyi]

@polina-c, thanks for help and review!