Fix janks and memory leaks in `CupertinoPageTransition` and `CupertinoFullscreenDialogTransition` #146999

ValentinVignal · 2024-04-18T16:33:05Z

Initially, part of #141198 to fix leak memories in CupertinoPageTransition.
But it was overlapping with #147133 which got merged first.
During my work, I noticed the CupertinoPageTransition and CupertinoFullscreenDialogTransition's animations states were lost when a push was interrupted with a pop. When that is the case, it should use the forward curve for the reverse animation to avoid discontinuities. This PR fixes it

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-18T16:33:13Z

cc @polina-c

polina-c · 2024-04-18T16:55:35Z

packages/flutter/lib/src/cupertino/route.dart

+  Animation<Offset>  get _secondaryPositionAnimation => (_backingSecondaryPositionAnimation ?? widget.secondaryRouteAnimation).drive(_kMiddleLeftTween);
+
+  CurvedAnimation? _backingPrimaryShadowAnimation;
+  // When this page is coming in to cover another page.


_primaryShadowAnimation did not have this comment in original code

polina-c · 2024-04-18T16:56:01Z

packages/flutter/lib/src/cupertino/route.dart

+
+  CurvedAnimation? _backingPrimaryPositionAnimation;
+  // When this page is coming in to cover another page.
+  Animation<Offset>  get _primaryPositionAnimation => (_backingPrimaryPositionAnimation ?? widget.primaryRouteAnimation).drive(_kRightMiddleTween);


// -> ///

Here and in other comment.

polina-c · 2024-04-18T16:57:20Z

packages/flutter/lib/src/cupertino/route.dart

+
+  CurvedAnimation? _backingSecondaryPositionAnimation;
+  // When this page is coming in to cover another page.
+  Animation<Offset>  get _secondaryPositionAnimation => (_backingSecondaryPositionAnimation ?? widget.secondaryRouteAnimation).drive(_kMiddleLeftTween);


Do we really want to invoke method drive on every invocation of the getter?

May be _setBackingCurveAnimations is right place to invoke it?

packages/flutter/lib/src/cupertino/route.dart

polina-c · 2024-04-19T18:19:05Z

Looks puzzling.
Maybe usage after disposal?

ValentinVignal · 2024-04-22T12:05:56Z

packages/flutter/test/cupertino/route_test.dart

+    // Give time to the animation to finish and update its status to
+    // AnimationState.completed, so the reverse curved can be used in the next
+    // step.
+    await tester.pump(const Duration(milliseconds: 1));


The issue was that the parent of the curved animation needs to change its status from forward to completed so it doesn't ignore when it the parents updates it to reverse

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

Lines 427 to 431 in 1a905d5

void _updateCurveDirection(AnimationStatus status) {

_curveDirection = switch (status) {

AnimationStatus.dismissed || AnimationStatus.completed => null,

AnimationStatus.forward || AnimationStatus.reverse => _curveDirection ?? status,

};

This is how I managed to do it, I don't know if there is a better way to do that?

tester.pump() alone doesn't work. tester.pumpAndSettle() works but I believe it is because it has a default duration of 100ms.

Would you have a better approach?

I think .pumpAndSettle() works for what you are trying to do. It will repeatedly call pump until any active animations are done, so that would fit. The 100ms default duration is for each individual pump, so you can lower that if you want, and would fit this case.

Sure I can do that. I don't mind the 100ms but I felt it was a bit "hidden"

ValentinVignal · 2024-04-22T12:19:27Z

@polina-c I managed to "fix" the tests in route_test.dart, I tried to explain them, tell me if something is obscure

ValentinVignal · 2024-04-22T12:11:31Z

packages/flutter/test/cupertino/route_test.dart

+    // The whole transition is 500ms based on
+    // CupertinoPageRoute.transitionDuration. Break it up into 10 small chunks.
+    //
+    // The screen width is 800.
+    // The top left corner of the placeholder will go from 0 to -800 / 3 = -266.67.

-    await tester.pump(const Duration(milliseconds: 40));
-    expect(tester.getTopLeft(find.byType(Placeholder)).dx, moreOrLessEquals(-111.0, epsilon: 1.0));
+    await tester.pump(const Duration(milliseconds: 50));
+    expect(tester.getTopLeft(find.byType(Placeholder)).dx, moreOrLessEquals(-69.0, epsilon: 1.0));

-    await tester.pump(const Duration(milliseconds: 40));
-    expect(tester.getTopLeft(find.byType(Placeholder)).dx, moreOrLessEquals(-161.0, epsilon: 1.0));
+    await tester.pump(const Duration(milliseconds: 50));
+    expect(tester.getTopLeft(find.byType(Placeholder)).dx, moreOrLessEquals(-136.0, epsilon: 1.0));


I believe this test was outdated, the total duration of the page is 500ms, so it makes more sense to update with 10 chunks of 50ms instead of 40ms.

During the forward (push), the value "didn't change" (meaning the test was passing). I only updated them because I changed the duration in the pump()s.

Can you create a new test rather than changing the existing one? We want to preserve the original test to prevent regressions.

ValentinVignal · 2024-04-22T12:18:08Z

packages/flutter/test/cupertino/route_test.dart

    // Exit animation
    await tester.tap(find.text('Close'));
    await tester.pump();

-    await tester.pump(const Duration(milliseconds: 40));
-    expect(tester.getTopLeft(find.byType(Placeholder)).dx, moreOrLessEquals(-83.0, epsilon: 1.0));
+    await tester.pump(const Duration(milliseconds: 50));
+    expect(tester.getTopLeft(find.byType(Placeholder)).dx, moreOrLessEquals(-197.0, epsilon: 1.0));

-    await tester.pump(const Duration(milliseconds: 360));
+    await tester.pump(const Duration(milliseconds: 450));
    expect(tester.getTopLeft(find.byType(Placeholder)).dx, moreOrLessEquals(-0.0, epsilon: 1.0));


Here the values did changes (meaning the test was falling), but from the code, it looks like they were wrong:

The reverse animation is Curves.easeInToLinear and the Placeholder goes from -800 / 3 = -266.67 px to 0px (the screen is 800px wide and _kMiddleLeftTween only uses 1/3 rd of the space).

The duration of the animation is 500ms:

- (800 / 3) * Curves.easeInToLinear.transform(450 / 500) is around 197

polina-c · 2024-04-22T21:49:57Z

packages/flutter/test/cupertino/route_test.dart

-    await tester.pump(const Duration(milliseconds: 40));
-    expect(tester.getTopLeft(find.byType(Placeholder)).dx, moreOrLessEquals(-83.0, epsilon: 1.0));
+    await tester.pump(const Duration(milliseconds: 50));
+    expect(tester.getTopLeft(find.byType(Placeholder)).dx, moreOrLessEquals(-197.0, epsilon: 1.0));


This change from -83 to -197 looks alerting to me.
The test passes at flutter master with -83. And it stops passing with -83 after your change. That means your change is not just improvement in memory management, it modified functionality, that should not happen.

I agree, I was trying to "fix" those changes in the tests. However, with my changes the behavior changes and I will have to update those values.

Before, a new CurvedAnimation was recreated at every build (in the constructor of the stateless widget CupertinoPageTransition). Now, it is reusing the same one. This leads to a change in the functionality (which I believe is correct).

As a reminder, here are some useful values:

Curves.easeInToLinear.transform(0.72); // 0.3149519026279449 - (800 / 3) * Curves.easeInToLinear.transform(0.72); // -83.98717403411865 Curves.linearToEaseOut.transform(0.72); // 0.979008914809674 - (800 / 3) * Curves.linearToEaseOut.transform(0.72); // -261.0690439492464

And the secondary animation is

_secondPositionCurvedAnimation = CurvedAnimation( parent: widget.secondaryRouteAnimation, curve: Curves.linearToEaseOut, reverseCurve: Curves.easeInToLinear, );

Before, the test was animating the push of the page from 0ms to 400ms (the entire duration of the push animation is 500ms) and then popping. So the push animation was not completed when being reversed. During the forward animation, the curve used is Curves.linearToEaseOut (which is correct).
During the pop/reverse animation, because the CurvedAnimation is recreated, it was using the reversedAnimation: Curves.easeInToLinear which led to this -83 value.

But this shouldn't happen, if the animation is not complete, it should use its forward curve during the reverse animation. I guess this is to prevent jumps and janks.

This is what those lines are doing:

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

Lines 427 to 432 in 89a4ffa

void _updateCurveDirection(AnimationStatus status) {

_curveDirection = switch (status) {

AnimationStatus.dismissed || AnimationStatus.completed => null,

AnimationStatus.forward || AnimationStatus.reverse => _curveDirection ?? status,

};

}

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

Lines 434 to 436 in 89a4ffa

bool get _useForwardCurve {

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

}

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

Lines 450 to 457 in 89a4ffa

double get value {

final Curve? activeCurve = _useForwardCurve ? curve : reverseCurve;

final double t = parent.value;

if (activeCurve == null) {

return t;

}

if (t == 0.0 || t == 1.0) {

Now that the CurvedAnimation is stored in the state and reused, it keeps using the forward curve and doesn't "jump" to the reversedCurve, which I believe fixes a jank/jump that currently exists on the master channel.

For the test, I decided the modify it so the push animation finishes and the pop uses the reversedCurved by changing the durations from 40ms to 50ms. But I can revert those duration changes and make the pop use the forward curve instead.

Rather than changing an existing test, can you add a new one? This feel like a potential regression.

+1 on leaving this test alone as much as possible. It does look like logically the animation wouldn't finish by the time the original test tries to go back, so the jump makes sense. I'd rather the test was left as it was to check for regression, even with the jump, and probably renamed. A new test that correctly waits for the animation to finish, then goes back would be preferable.

Does the original test still fail, and how?

Those were the minimum changes I found to "let this test alone as much as possible".

What was happening in this test:

Push using the forward animation (500ms)

The push is interrupted with a pop at 400ms

The pop uses the reversed animation (but should not because it leads to a discontinuity).

After my changes without any modification on the test:

Push using the forward animation (500ms)

The push is interrupted with a pop at 400ms

The pop uses the forward animation for the reverse animation (there is no discontinuity)

The test fails at the line

await tester.pump(const Duration(milliseconds: 40)); expect(tester.getTopLeft(find.byType(Placeholder)).dx, moreOrLessEquals(-83.0, epsilon: 1.0));

Expected: -83
Actual value: -261

(Comment for reference: https://github.com/flutter/flutter/pull/146999/files#r1575569345)

To make this test pass, I have 2 options:

a. I let the push being interrupted and I need to change the expected values to match the use of the forward curve instead of the reverse curved.
b. I let the push animation complete with more pumps so the reverse animation for the pop uses the reversed curve and I don't need to change the values in the expects.

I have implemented both options. The one linked to the comment is a.. And a bit below, there is the group('Interrupted push', () { ... }); that implements b.

Is that what you are expecting?

@MitchellGoodwin , does this help?

Thank you for the explanation. That all makes sense and the two tests LGTM. Thank you for adjusting the old on and adding a new one.

ValentinVignal · 2024-04-23T04:14:38Z

@polina-c This was the same as #147133 but I believe there are some flows in the PR that this one fixes. I think we can keep it open

polina-c · 2024-04-23T18:58:50Z

@Piinks , Valentin is suggesting functional fix in the animation, as side effect of the leak fix. Can you help to review it or suggest someone who has enough expertise?

polina-c · 2024-04-23T19:09:25Z

@polina-c This was the same as #147133 but I believe there are some flows in the PR that this one fixes. I think we can keep it open

Nope, if the change is good, we want to push it forward. You believe it is good, right?
It just will take more time to explain it to others :)

ValentinVignal · 2024-04-24T01:08:29Z

You believe it is good, right?

I've at least started to convince myself it is good haha 😅 It for sure needs a confirmation from someone with expertise

Piinks

@ValentinVignal it looks like there are a lot of changes here unrelated to fixing the leak, some of the changes are identical code, just moved around. I can't actually tell what has changed to fix the leak. Can you revert those unrelated changes?

Piinks · 2024-04-24T19:51:47Z

packages/flutter/lib/src/cupertino/route.dart

-
-  ///  * `primaryRouteAnimation` is a linear route animation from 0.0 to 1.0
-  ///    when this screen is being pushed.
+  /// A linear route animation from 0.0 to 1.0 when this screen is being pushed.


Is this only linear based on linearTransition?

Piinks · 2024-04-24T19:51:57Z

packages/flutter/lib/src/cupertino/route.dart

  final Animation<double> primaryRouteAnimation;

-  ///  * `secondaryRouteAnimation` is a linear route animation from 0.0 to 1.0
-  ///    when another screen is being pushed on top of this one.
+  /// A linear route animation from 0.0 to 1.0 when another screen is being


Same here, is this only linear based on linearTransition?

Piinks · 2024-04-24T19:52:31Z

packages/flutter/lib/src/cupertino/route.dart

+  /// When this page is coming in to cover another page.
  late Animation<Offset> _primaryPositionAnimation;
-  // When this page is becoming covered by another page.
+
+  CurvedAnimation? _secondaryPositionCurvedAnimation;
+
+  /// When this page is becoming covered by another page.


Since these are private, the /// public docs do not need to be added. For the others, can you add the comments back?

Piinks · 2024-04-24T19:53:05Z

packages/flutter/lib/src/cupertino/route.dart


  @override
-  void initState() {
+  void initState()  {


Suggested change

void initState() {

void initState() {

Piinks · 2024-04-24T19:54:55Z

packages/flutter/lib/src/cupertino/route.dart

-    || oldWidget.child != widget.child || oldWidget.linearTransition != widget.linearTransition) {
-    _disposeCurve();
-    _setupAnimation();
+    if (


If child changes, why would be not update anymore?

_setUpAnination() doesn't use widget.child so there is no reason to recompute it when child changes.

Moreover, SizedBox() != SizedBox() is true, so that means that oldWidget.child != widget.child will most likely be always true

ValentinVignal · 2024-04-26T01:49:59Z

@ValentinVignal it looks like there are a lot of changes here unrelated to fixing the leak, some of the changes are identical code, just moved around. I can't actually tell what has changed to fix the leak. Can you revert those unrelated changes?

Sorry about that, it's because we had 2 PRs with conflicts. I can try to start everything again

ValentinVignal · 2024-04-29T13:33:13Z

@Piinks @polina-c The changes should have been cleaned now

packages/flutter/test/cupertino/route_test.dart

polina-c · 2024-05-05T17:09:52Z

packages/flutter/test/cupertino/route_test.dart

@@ -2433,7 +2620,7 @@ void main() {
    });
  });

-  testWidgets(
+ testWidgets(


extra space?

…e-transition

polina-c · 2024-05-06T17:27:29Z

@MitchellGoodwin , may I get your formal approval to kick off google testing?

MitchellGoodwin

LGTM

auto-submit · 2024-05-06T20:32:22Z

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

polina-c · 2024-05-06T21:24:42Z

Google testing is marked as red, but the tests on CL actually passed. Marking as infra failure.

… `CupertinoFullscreenDialogTransition` (flutter/flutter#146999)

flutter/flutter@04424e1...7920a52 2024-05-07 sokolovskyi.konstantin@gmail.com Add tests for callback_shortcuts.0.dart API example. (flutter/flutter#147536) 2024-05-07 barpac02@gmail.com Improve Android SDK and NDK mistmatch warning message (flutter/flutter#147809) 2024-05-07 sokolovskyi.konstantin@gmail.com Add tests for shortcuts.dart API examples. (flutter/flutter#147433) 2024-05-07 sosamaalirizvi@gmail.com Added missing tests for ButtonStyle example (flutter/flutter#147457) 2024-05-07 engine-flutter-autoroll@skia.org Roll Flutter Engine from ece1d686e3ba to 150f694a7816 (1 revision) (flutter/flutter#147912) 2024-05-07 leroux_bruno@yahoo.fr DropdownMenu cleanup (flutter/flutter#147860) 2024-05-07 engine-flutter-autoroll@skia.org Roll Flutter Engine from 80470584e1e1 to ece1d686e3ba (1 revision) (flutter/flutter#147910) 2024-05-07 engine-flutter-autoroll@skia.org Roll Flutter Engine from b7bfd94af743 to 80470584e1e1 (1 revision) (flutter/flutter#147906) 2024-05-07 engine-flutter-autoroll@skia.org Roll Flutter Engine from b64e2300bcd0 to b7bfd94af743 (1 revision) (flutter/flutter#147905) 2024-05-07 zeqinjie@qq.com fix MenuItemButton if child is null (flutter/flutter#147485) 2024-05-07 gspencergoog@users.noreply.github.com Fix document generation, eliminate template support from snippets tool. (flutter/flutter#147893) 2024-05-07 engine-flutter-autoroll@skia.org Roll Flutter Engine from 4cb9e02c06ce to b64e2300bcd0 (1 revision) (flutter/flutter#147904) 2024-05-07 82763757+NobodyForNothing@users.noreply.github.com test focus example 0 (flutter/flutter#147564) 2024-05-07 engine-flutter-autoroll@skia.org Roll Flutter Engine from 422f92b992c5 to 4cb9e02c06ce (1 revision) (flutter/flutter#147899) 2024-05-06 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#147896) 2024-05-06 engine-flutter-autoroll@skia.org Roll Flutter Engine from 463ff7d2d4d5 to 422f92b992c5 (5 revisions) (flutter/flutter#147895) 2024-05-06 59215665+davidhicks980@users.noreply.github.com MultiSelectableSelectionContainerDelegate documentation fixes. (flutter/flutter#147843) 2024-05-06 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#147891) 2024-05-06 49699333+dependabot[bot]@users.noreply.github.com Bump actions/checkout from 4.1.4 to 4.1.5 (flutter/flutter#147888) 2024-05-06 32538273+ValentinVignal@users.noreply.github.com Fix janks and memory leaks in `CupertinoPageTransition` and `CupertinoFullscreenDialogTransition` (flutter/flutter#146999) 2024-05-06 engine-flutter-autoroll@skia.org Roll Flutter Engine from 960a0c8fecbe to 463ff7d2d4d5 (1 revision) (flutter/flutter#147886) 2024-05-06 engine-flutter-autoroll@skia.org Roll Flutter Engine from 70cc300f9ad8 to 960a0c8fecbe (1 revision) (flutter/flutter#147884) 2024-05-06 jonahwilliams@google.com [new gallery] Reisze gallery images (flutter/flutter#147882) 2024-05-06 engine-flutter-autoroll@skia.org Roll Flutter Engine from d0f99b35eac6 to 70cc300f9ad8 (1 revision) (flutter/flutter#147880) 2024-05-06 polinach@google.com Fix leak in a test. (flutter/flutter#147846) 2024-05-06 engine-flutter-autoroll@skia.org Roll Flutter Engine from a30ae7729c95 to d0f99b35eac6 (3 revisions) (flutter/flutter#147878) 2024-05-06 engine-flutter-autoroll@skia.org Roll Packages from f4719ca to 2dfe645 (3 revisions) (flutter/flutter#147866) 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 bmparr@google.com,rmistry@google.com,stuartmorgan@google.com 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

…r#6683) flutter/flutter@04424e1...7920a52 2024-05-07 sokolovskyi.konstantin@gmail.com Add tests for callback_shortcuts.0.dart API example. (flutter/flutter#147536) 2024-05-07 barpac02@gmail.com Improve Android SDK and NDK mistmatch warning message (flutter/flutter#147809) 2024-05-07 sokolovskyi.konstantin@gmail.com Add tests for shortcuts.dart API examples. (flutter/flutter#147433) 2024-05-07 sosamaalirizvi@gmail.com Added missing tests for ButtonStyle example (flutter/flutter#147457) 2024-05-07 engine-flutter-autoroll@skia.org Roll Flutter Engine from ece1d686e3ba to 150f694a7816 (1 revision) (flutter/flutter#147912) 2024-05-07 leroux_bruno@yahoo.fr DropdownMenu cleanup (flutter/flutter#147860) 2024-05-07 engine-flutter-autoroll@skia.org Roll Flutter Engine from 80470584e1e1 to ece1d686e3ba (1 revision) (flutter/flutter#147910) 2024-05-07 engine-flutter-autoroll@skia.org Roll Flutter Engine from b7bfd94af743 to 80470584e1e1 (1 revision) (flutter/flutter#147906) 2024-05-07 engine-flutter-autoroll@skia.org Roll Flutter Engine from b64e2300bcd0 to b7bfd94af743 (1 revision) (flutter/flutter#147905) 2024-05-07 zeqinjie@qq.com fix MenuItemButton if child is null (flutter/flutter#147485) 2024-05-07 gspencergoog@users.noreply.github.com Fix document generation, eliminate template support from snippets tool. (flutter/flutter#147893) 2024-05-07 engine-flutter-autoroll@skia.org Roll Flutter Engine from 4cb9e02c06ce to b64e2300bcd0 (1 revision) (flutter/flutter#147904) 2024-05-07 82763757+NobodyForNothing@users.noreply.github.com test focus example 0 (flutter/flutter#147564) 2024-05-07 engine-flutter-autoroll@skia.org Roll Flutter Engine from 422f92b992c5 to 4cb9e02c06ce (1 revision) (flutter/flutter#147899) 2024-05-06 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#147896) 2024-05-06 engine-flutter-autoroll@skia.org Roll Flutter Engine from 463ff7d2d4d5 to 422f92b992c5 (5 revisions) (flutter/flutter#147895) 2024-05-06 59215665+davidhicks980@users.noreply.github.com MultiSelectableSelectionContainerDelegate documentation fixes. (flutter/flutter#147843) 2024-05-06 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#147891) 2024-05-06 49699333+dependabot[bot]@users.noreply.github.com Bump actions/checkout from 4.1.4 to 4.1.5 (flutter/flutter#147888) 2024-05-06 32538273+ValentinVignal@users.noreply.github.com Fix janks and memory leaks in `CupertinoPageTransition` and `CupertinoFullscreenDialogTransition` (flutter/flutter#146999) 2024-05-06 engine-flutter-autoroll@skia.org Roll Flutter Engine from 960a0c8fecbe to 463ff7d2d4d5 (1 revision) (flutter/flutter#147886) 2024-05-06 engine-flutter-autoroll@skia.org Roll Flutter Engine from 70cc300f9ad8 to 960a0c8fecbe (1 revision) (flutter/flutter#147884) 2024-05-06 jonahwilliams@google.com [new gallery] Reisze gallery images (flutter/flutter#147882) 2024-05-06 engine-flutter-autoroll@skia.org Roll Flutter Engine from d0f99b35eac6 to 70cc300f9ad8 (1 revision) (flutter/flutter#147880) 2024-05-06 polinach@google.com Fix leak in a test. (flutter/flutter#147846) 2024-05-06 engine-flutter-autoroll@skia.org Roll Flutter Engine from a30ae7729c95 to d0f99b35eac6 (3 revisions) (flutter/flutter#147878) 2024-05-06 engine-flutter-autoroll@skia.org Roll Packages from f4719ca to 2dfe645 (3 revisions) (flutter/flutter#147866) 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 bmparr@google.com,rmistry@google.com,stuartmorgan@google.com 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

github-actions bot added a: text input Entering text in a text field or keyboard related problems framework flutter/packages/flutter repository. See also f: labels. f: cupertino flutter/packages/flutter/cupertino repository f: routes Navigator, Router, and related APIs. labels Apr 18, 2024

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

polina-c reviewed Apr 18, 2024

View reviewed changes

ValentinVignal commented Apr 19, 2024

View reviewed changes

packages/flutter/lib/src/cupertino/route.dart Outdated Show resolved Hide resolved

ValentinVignal commented Apr 22, 2024

View reviewed changes

ValentinVignal requested a review from polina-c April 22, 2024 15:27

polina-c reviewed Apr 22, 2024

View reviewed changes

ValentinVignal mentioned this pull request Apr 23, 2024

fixes cupertino page transition leak #147133

Merged

9 tasks

polina-c requested a review from Piinks April 23, 2024 19:00

ValentinVignal mentioned this pull request Apr 24, 2024

Disable leak tracking for selection text area #147273

Merged

9 tasks

Piinks reviewed Apr 24, 2024

View reviewed changes

ValentinVignal added 2 commits April 29, 2024 21:31

fix: Avoid janks when push is interrupted

939d8db

test: Update the tests

a2758a1

ValentinVignal force-pushed the flutter/fix-memory-leaks-in-curpertino-page-transition branch from 06c94d6 to a2758a1 Compare April 29, 2024 13:32

github-actions bot removed the a: text input Entering text in a text field or keyboard related problems label Apr 29, 2024

Piinks requested a review from MitchellGoodwin April 29, 2024 22:21

refactor: Use pumpAndSettle to finish animations

72a5113

ValentinVignal changed the title ~~Fix memory leaks in CupertinoPageTransition~~ Fix janks CupertinoPageTransition May 2, 2024

ValentinVignal requested a review from Piinks May 2, 2024 01:17

ValentinVignal changed the title ~~Fix janks CupertinoPageTransition~~ Fix janks in CupertinoPageTransition and CupertinoFullscreenDialogTransition May 2, 2024

This was referenced May 4, 2024

Remaining leaks of CurvedAnimation. #147594

Draft

Fix memory leaks in context menu #147822

Merged

polina-c reviewed May 5, 2024

View reviewed changes

packages/flutter/test/cupertino/route_test.dart Outdated Show resolved Hide resolved

ValentinVignal changed the title ~~Fix janks in CupertinoPageTransition and CupertinoFullscreenDialogTransition~~ Fix janks and memory leaks in CupertinoPageTransition and CupertinoFullscreenDialogTransition May 5, 2024

ValentinVignal requested a review from polina-c May 5, 2024 08:30

polina-c reviewed May 5, 2024

View reviewed changes

test: Add back missing test

8f3c106

ValentinVignal force-pushed the flutter/fix-memory-leaks-in-curpertino-page-transition branch from 7a2129a to 8f3c106 Compare May 6, 2024 01:44

ValentinVignal requested a review from polina-c May 6, 2024 01:45

polina-c approved these changes May 6, 2024

View reviewed changes

Merge branch 'master' into flutter/fix-memory-leaks-in-curpertino-pag…

67f3775

…e-transition

MitchellGoodwin approved these changes May 6, 2024

View reviewed changes

polina-c added the autosubmit Merge PR when tree becomes green via auto submit App label May 6, 2024

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label May 6, 2024

polina-c merged commit f3978c7 into flutter:master May 6, 2024
72 checks passed

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

Roll Flutter from 04424e10fe28 to 28e65b781105 (25 revisions) flutter/packages#6681

Closed

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

f3978c7a4 Fix janks and memory leaks in CupertinoPageTransition and…

d898e26

… `CupertinoFullscreenDialogTransition` (flutter/flutter#146999)

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

Roll Flutter from 04424e10fe28 to 7920a52dd568 (27 revisions) flutter/packages#6683

Merged

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

f3978c7a4 Fix janks and memory leaks in CupertinoPageTransition and…

b597737

… `CupertinoFullscreenDialogTransition` (flutter/flutter#146999)

Piinks mentioned this pull request May 10, 2024

Flutter Framework Team: Notable Commits #121415

Open

	void _updateCurveDirection(AnimationStatus status) {
	_curveDirection = switch (status) {
	AnimationStatus.dismissed \|\| AnimationStatus.completed => null,
	AnimationStatus.forward \|\| AnimationStatus.reverse => _curveDirection ?? status,
	};

	bool get _useForwardCurve {
	return reverseCurve == null \|\| (_curveDirection ?? parent.status) != AnimationStatus.reverse;
	}

	double get value {
	final Curve? activeCurve = _useForwardCurve ? curve : reverseCurve;

	final double t = parent.value;
	if (activeCurve == null) {
	return t;
	}
	if (t == 0.0 \|\| t == 1.0) {

Fix janks and memory leaks in CupertinoPageTransition and CupertinoFullscreenDialogTransition #146999

Fix janks and memory leaks in CupertinoPageTransition and CupertinoFullscreenDialogTransition #146999

Conversation

ValentinVignal commented Apr 18, 2024 • edited

Pre-launch Checklist

ValentinVignal commented Apr 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

polina-c commented Apr 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ValentinVignal commented Apr 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ValentinVignal Apr 22, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MitchellGoodwin Apr 29, 2024 • edited

Choose a reason for hiding this comment

ValentinVignal Apr 30, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ValentinVignal commented Apr 23, 2024

polina-c commented Apr 23, 2024

polina-c commented Apr 23, 2024

ValentinVignal commented Apr 24, 2024

Piinks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ValentinVignal commented Apr 26, 2024 • edited

ValentinVignal commented Apr 29, 2024

Choose a reason for hiding this comment

polina-c commented May 6, 2024

MitchellGoodwin left a comment

Choose a reason for hiding this comment

auto-submit bot commented May 6, 2024

polina-c commented May 6, 2024

Fix janks and memory leaks in `CupertinoPageTransition` and `CupertinoFullscreenDialogTransition` #146999

Fix janks and memory leaks in `CupertinoPageTransition` and `CupertinoFullscreenDialogTransition` #146999

ValentinVignal commented Apr 18, 2024 •

edited

ValentinVignal Apr 22, 2024 •

edited

MitchellGoodwin Apr 29, 2024 •

edited

ValentinVignal Apr 30, 2024 •

edited

ValentinVignal commented Apr 26, 2024 •

edited