Fix stuck predictive back platform channel calls #133368

justinmc · 2023-08-25T23:24:39Z

Sometimes, platform channel calls to setFrameworkHandlesBack, were getting stuck when made when the app was shutting down. This was uncovered by Google tests that were flaking. This PR avoids making those calls and fixes the tests.

b/296900781

chunhtai

LGTM, just some nit picks

chunhtai · 2023-08-25T23:28:06Z

packages/flutter/lib/src/widgets/app.dart

    _updateRouting();
    _locale = _resolveLocales(WidgetsBinding.instance.platformDispatcher.locales, widget.supportedLocales);
    WidgetsBinding.instance.addObserver(this);
+    if (WidgetsBinding.instance.lifecycleState != null) {


Is this check needed?

Good call, I'll change the variable to non-nullable and keep this check.

Actually, I removed the check and kept it non-nullable, as you suggested. Otherwise there's a chance it could still be uninitialized.

chunhtai · 2023-08-25T23:29:36Z

packages/flutter/test/widgets/app_test.dart

+    tearDown(() {
+      TestDefaultBinaryMessengerBinding.instance.defaultBinaryMessenger
+          .setMockMethodCallHandler(SystemChannels.platform, null);
+      SystemNavigator.setFrameworkHandlesBack(true);


maybe add comment when setting it to true?

Your comment makes me realize that this setting/resetting of this isn't necessary anymore. It used to be tracked in a static bool that would keep state between tests, but now it's not. I'll remove it here and elsewhere.

justinmc · 2023-08-25T23:30:26Z

packages/flutter/lib/src/widgets/app.dart

+    if (WidgetsBinding.instance.lifecycleState != null) {
+      _appLifecycleState = WidgetsBinding.instance.lifecycleState;
+    }


This initialization is given in the example in the docs for WidgetsBindingObserver. I simply missed it in my last PR where I relanded predictive back to fix a similar test. I confirmed that this variable was incorrectly null for awhile in the failing tests, and after this change it gets set correctly.

However, the test continued to flake occasionally after this change, hence my other change above.

justinmc · 2023-08-25T23:31:04Z

packages/flutter/lib/src/widgets/app.dart

+    switch(_appLifecycleState) {
+      case null:
+      case AppLifecycleState.detached:
+      case AppLifecycleState.inactive:


I then observed in the failing tests that the problematic calls were only made when the lifecycle was inactive.

should this be handle on the engine side? it seems hard for the caller of SystemNavigator.setFrameworkHandlesBack to know this.

In my experimentation, the failure happens even if the engine doesn't handle the call at all. I've been super confused about how those tests are set up that this kind of timeout error is even possible. I would definitely be up for revisiting this code if someone finds a better way to avoid the failure.

…ys initialized, especially in tests

…ic bool?

…state so it's not null

…tter#133368)

flutter/flutter@cd3aeef...ec387a4 2023-08-28 [email protected] Revert "ShortcutManager should dispatch creation in constructor." (flutter/flutter#133472) 2023-08-28 [email protected] Roll Flutter Engine from 9ab2603db24f to 91522a188bda (2 revisions) (flutter/flutter#133462) 2023-08-28 [email protected] FocusNode and FocusManager should dispatch creation in constructor. (flutter/flutter#133352) 2023-08-28 [email protected] ShortcutManager should dispatch creation in constructor. (flutter/flutter#133356) 2023-08-28 [email protected] Fix stuck predictive back platform channel calls (flutter/flutter#133368) 2023-08-28 [email protected] added option to change color of heading row(flutter#132428) (flutter/flutter#132728) 2023-08-28 [email protected] PlatformRouteInformationProvider should dispatch creation in constructor. (flutter/flutter#133353) 2023-08-28 [email protected] Roll Flutter Engine from 4924cf453398 to 9ab2603db24f (1 revision) (flutter/flutter#133449) 2023-08-28 [email protected] Roll Flutter Engine from d89824ab018f to 4924cf453398 (1 revision) (flutter/flutter#133447) 2023-08-28 [email protected] Roll Flutter Engine from b26e2da130ab to d89824ab018f (2 revisions) (flutter/flutter#133444) 2023-08-28 [email protected] Roll Flutter Engine from 78c26aeff3ee to b26e2da130ab (2 revisions) (flutter/flutter#133432) 2023-08-28 [email protected] Roll Flutter Engine from f8a171aa173f to 78c26aeff3ee (1 revision) (flutter/flutter#133428) 2023-08-28 [email protected] Roll Flutter Engine from 0593c0455fd6 to f8a171aa173f (1 revision) (flutter/flutter#133425) 2023-08-27 [email protected] Roll Flutter Engine from fc592728a7d0 to 0593c0455fd6 (4 revisions) (flutter/flutter#133419) 2023-08-27 [email protected] Roll Flutter Engine from b4e2758d6d43 to fc592728a7d0 (1 revision) (flutter/flutter#133410) 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 [email protected],[email protected] 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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…tter#133368)

justinmc added 2 commits August 25, 2023 15:04

AppLifecycleState should be initialized

22d9bd8

Also do nothing on inactive, and test

78f5c3a

justinmc requested a review from chunhtai August 25, 2023 23:24

justinmc self-assigned this Aug 25, 2023

github-actions bot added the framework flutter/packages/flutter repository. See also f: labels. label Aug 25, 2023

chunhtai approved these changes Aug 25, 2023

View reviewed changes

justinmc commented Aug 25, 2023

View reviewed changes

justinmc added 4 commits August 25, 2023 16:37

Review: Make variable nonnullable

a9283b3

analyzer

5fd5d81

Actually should be nullable and if case removed. Not necessarily alwa…

5112ff2

…ys initialized, especially in tests

No need to set/unset setFrameworkHandlesBack now that there's no stat…

9add899

…ic bool?

github-actions bot added f: cupertino flutter/packages/flutter/cupertino repository f: routes Navigator, Router, and related APIs. labels Aug 25, 2023

Yes no need to set frameworkHandlesBack, but need to set applifecycle…

74c6a3d

…state so it's not null

justinmc marked this pull request as ready for review August 26, 2023 03:20

justinmc added 2 commits August 25, 2023 20:54

Finish fixing app_test.dart

86a819d

This test is only native Android, whoops

3977d15

justinmc merged commit dfd4147 into flutter:master Aug 28, 2023

justinmc deleted the predictive-back-app-lifecycle-fix branch August 28, 2023 16:23

engine-flutter-autoroll mentioned this pull request Aug 28, 2023

Roll Flutter from cd3aeef3a826 to dfd4147ceaf0 (11 revisions) flutter/packages#4803

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 28, 2023

dfd4147 Fix stuck predictive back platform channel calls (flutter/flu…

590891a

…tter#133368)

engine-flutter-autoroll mentioned this pull request Aug 28, 2023

Roll Flutter from cd3aeef3a826 to 05259ca938c9 (14 revisions) flutter/packages#4805

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 28, 2023

dfd4147 Fix stuck predictive back platform channel calls (flutter/flu…

c93859a

…tter#133368)

engine-flutter-autoroll mentioned this pull request Aug 28, 2023

Roll Flutter from cd3aeef3a826 to ec387a467a68 (15 revisions) flutter/packages#4807

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 28, 2023

dfd4147 Fix stuck predictive back platform channel calls (flutter/flu…

a754060

…tter#133368)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 15, 2023

dfd4147 Fix stuck predictive back platform channel calls (flutter/flu…

2fe65bf

…tter#133368)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix stuck predictive back platform channel calls #133368

Fix stuck predictive back platform channel calls #133368

Uh oh!

justinmc commented Aug 25, 2023 •

edited

Loading

Uh oh!

chunhtai left a comment

Uh oh!

chunhtai Aug 25, 2023

Uh oh!

justinmc Aug 25, 2023

Uh oh!

justinmc Aug 26, 2023

Uh oh!

chunhtai Aug 25, 2023

Uh oh!

justinmc Aug 25, 2023

Uh oh!

justinmc Aug 25, 2023

Uh oh!

justinmc Aug 25, 2023

Uh oh!

chunhtai Aug 28, 2023

Uh oh!

justinmc Aug 28, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Fix stuck predictive back platform channel calls #133368

Fix stuck predictive back platform channel calls #133368

Uh oh!

Conversation

justinmc commented Aug 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chunhtai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

justinmc commented Aug 25, 2023 •

edited

Loading