Should there be a check to see if the controller has been disposed like the comment said? Or was that comment incorrect?

From my understanding, the comment I removed states that _end should not be called at all but I agree that it could be read as _end will have no effect if the _controller is disposed.

Looking at the Git history this code had very few changes since its introduction (#9575).
But looking more closely it looks like at this time, DrivenScrollActivity._end was implemented this way:

  void _end() {
    delegate?.goBallistic(0.0);
  }

And the DrivenScrollActivity.dispose method was the same as now:

  @override
  void dispose() {
    _completer.complete();
    _controller.dispose();
    super.dispose();
  }

But the ActivityScroll.dispose method was:

  void dispose() {
    _delegate = null;
  }

Which means _end had no effect once dispose was called.

So it looks like a bug was introduced during the null safety migration, see #64672.
It changed ActivityScroll.dispose to:

  void dispose() { }

So to answer your question:

Should there be a check to see if the controller has been disposed like the comment said? Or was that comment incorrect?

Yes there should be a check, but AFAIK, there is no standard way to check if the animation controller was disposed so I chose to rely on the completer state.

I plan to update the PR to replace the check on completer with a check on an ‘_isDisposed’ instance variable, that will make the code more self explanatory. I will also add a narrowed test.

I investigated this further:

• The comment (.whenComplete(_end); // won't trigger if we dispose _controller first) is right, it is expected that the whenComplete parameter is not called when a running animation controller is disposed.
• But in our case, the animation controller completes just before being disposed (this has to be on the exact same frame). So at the time the _end function is called the animation is disposed.
• It seems the issue surfaced because of Futures resolution order : _controller.whenComplete is executed before the new activity start but the Future it returned is executed after the start of the new activity. I tried hard to write a more narrowed test (one not relying on TabBarController) but I was not able to figure out one exposing the same Future ordering.

@Piinks I updated the PR with an _isDisposed flag and some comments.

Looks like we have the same condition in BallisticScrollActivity.

I think having the _isDisposed flag makes sense, it's not dissimilar from the mounted check that is available elsewhere.

Can you move the isDisposed flag to the super class - ScrollActivity - so we can use it the same way in _end for BallisticScrollActivity and DrivenScrollActivity?

Great!
I pushed an update where _isDisposed is moved to ScrollActivity.