null check added to avoid NPE while calling FlutterView.detachFromFlutterEngine() #41082

bastionkid · 2023-04-11T17:32:55Z

Issue fixed by this PR: flutter/flutter#110138

This PR fixes an issue which is causing app crash when User tries to navigate to a new instance FlutterFragment (whose old instance is already present in the fragment backstack) in an Activity (which was restored to saved state after being killed in background due to memory pressure).

Detailed case to reproduce the crash and identify its' root cause:

Setup: Let's say we've an Activity1 which has a bottom nav bar with 3 tabs. Each of this 3 tabs are FlutterFragment i.e. Fragment1, Fragment2 & Fragment3 and all of them are using separate FlutterEngine but all of them will be cached. e.g. Multiple instances of Fragment1 will be going to use same cached FlutterEngine1.

When User opens the app, Fragment1 gets added to fragment backstack
Then User navigates to Fragment2 (gets added to backstack as well)
Then User navigates to Fragment3 (gets added to backstack as well)
Then User puts the app in background. Due to memory pressure OS/platform kills the Activity1 and all 3 FlutterFragments while the app is in background.
Then after sometime User tries to bring the app to foreground from the app stack. Since Activity1 was killed by the OS/platform the app process will try to restore the Activity1 in the same state it was before it got killed. This leads to all 3 fragments present in backstack to get instantiated and then the onAttach() gets called for all 3, but only Fragment3 gets onCreateView() lifecycle event as it was the top most visible Fragment before the FragmentManager saved the state and app went into background. All 3 FlutterFragment goes through following function calls.

FlutterFragment.onAttach() -> FlutterActivityAndFragmentDelegate.onAttach(). There is a one-to-one mapping between FlutterFragment <-> FlutterActivityAndFragmentDelegate

FlutterActivityAndFragmentDelegate.onAttach() -> FlutterEngineConnectionRegistry.attachToActivity(). There is a one-to-one mapping between FlutterEngine <-> FlutterEngineConnectionRegistry. NOTE: THIS IS VERY IMPORTANT POINT TO KEEP IN MIND TO UNDERSTAND THE ROOT CAAUSE OF THIS CRASH.

Since all the 3 FlutterFragment were just instantiated on activity restore exclusiveActivity will be null and exclusiveActivity will be assigned the host FlutterActivityAndFragmentDelegate.

6. Then FlutterFragment.onCreateView() will be called only for Fragment3 and it will be visible without an issue as its' state gets restored properly.
7. Then if User tries to navigate to Fragment2 via instantiating new instance of it (this means that now there will be two instances of Fragment2 in the backstack), then there will be crash as it'll go through following function calls.

FlutterFragment.onAttach() -> FlutterActivityAndFragmentDelegate.onAttach().

FlutterActivityAndFragmentDelegate.onAttach() -> FlutterEngineConnectionRegistry.attachToActivity().

THIS IS WHERE THE CRASH STARTS. Since this is the second instance of Fragment2 and both instances are going to use the same cached FlutterEngine and hence same FlutterEngineConnectionRegistry, this time around exclusiveActivity will be non-null as it was assigned during step 5. And since exclusiveActivity will be be non null it'll try to detach from the old ExclusiveAppComponent via calling exclusiveActivity.detachFromFlutterEngine().

FlutterActivityAndFragmentDelegate.detachFromFlutterEngine() -> FlutterFragment.detachFromFlutterEngine()

FlutterFragment.detachFromFlutterEngine() -> FlutterActivityAndFragmentDelegate.onDestroyView(). This ideally should not be called if the hosts' FlutterFragments.onCreateView() was not called in the first place. Also since the previous author has added // Redundant calls are ok. comment, I'm guessing that this is just a fallback cleanup.

THIS IS WHERE THE CRASH HAPPENS. FlutterActivityAndFragmentDelegate.onDestroyView() -> FlutterView.detachFromFlutterEngine(). Since the lifecycle of older instance of this FlutterFragment2 was capped at onAttach(), it's FlutterView property will be null and calling FlutterView.detachFromFlutterEngine() will throw NPE.

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 and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

flutter-dashboard · 2023-04-14T03:59:27Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

reidbaker · 2023-04-14T16:22:34Z

Thank you for such a well documented and researched patch!

We are normally pretty strict about not accepting pull requests without tests. That said, I am not sure what of our testing infrastructure should be used to recreate the reproduction conditions you lay out. I have an open question out in hackers-engine on discord.

If we decide it is ok to take this pr without a test we will need approval from Hixie. See https://github.com/flutter/flutter/wiki/Tree-hygiene#tests for more information on our policies.

shell/platform/android/io/flutter/embedding/android/FlutterActivityAndFragmentDelegate.java

…ivityAndFragmentDelegate.java Co-authored-by: Reid Baker <hamilton.reid.baker@gmail.com>

bastionkid · 2023-04-17T08:16:37Z

Thank you for such a well documented and researched patch!

We are normally pretty strict about not accepting pull requests without tests. That said, I am not sure what of our testing infrastructure should be used to recreate the reproduction conditions you lay out. I have an open question out in hackers-engine on discord.

If we decide it is ok to take this pr without a test we will need approval from Hixie. See https://github.com/flutter/flutter/wiki/Tree-hygiene#tests for more information on our policies.

I checked your Discord message and looks like there is one 👍 and not any replies yet. I guess I just need to wait for people to get some free time to look at the PR then right?

Break comment into 2 lines.

Hixie · 2023-04-17T17:48:17Z

shell/platform/android/io/flutter/embedding/android/FlutterActivityAndFragmentDelegate.java

-    flutterView.removeOnFirstFrameRenderedListener(flutterUiDisplayListener);
+
+    // flutterView can be null in instances where a delegate.onDestroyView is called without
+    // onCreateView being called. See https://github.com/flutter/engine/pull/41082 for more detail.


nit: in general the code should stand in isolation; there's no guarantee that the issue database will last as long as the code comments. so if there is more useful data to include it would be good to just describe it all here rather than linking to the issue.

I agree which is why there is a description before the link. That said this link has way more information than I think could (should?) fit in a comment include screenshots and statements about androids observed behavior. All in if someone wanted to understand why how onDestroyView can be called without onCreateView then the link is the best place to learn more.

In general if we think more documentation is needed, then I would recommend putting it in a README.md, or, at the extreme, in a wiki page. We shouldn't consider PR descriptions to be canonical documentation sources.

Hixie · 2023-04-17T17:48:53Z

is calling onDestroyView without calling onCreateView considered in-spec?

Hixie · 2023-04-17T19:16:45Z

(reid answered "no" on discord)

given that as far as we can tell the OS is giving us out-of-spec messages, which is not something we can really repro:
test-exempt: handles a theoretically impossible situation.

reidbaker · 2023-04-17T20:57:25Z

The next flutter contributor to approve please add auto-submit.

gmackall

LGTM. Thanks for the investigation and patch!

…achFromFlutterEngine() (flutter/engine#41082)

…125001) flutter/engine@b2d0738...20034a8 2023-04-17 godofredoc@google.com Reland "Migrate mac_host_engine to engine v2 builds." (flutter/engine#41279) 2023-04-17 41930132+hellohuanlin@users.noreply.github.com [rotation_distortion] Use "delayed swap" solution to reduce rotation distortion (flutter/engine#40730) 2023-04-17 akash.mercer@gmail.com null check added to avoid NPE while calling FlutterView.detachFromFlutterEngine() (flutter/engine#41082) 2023-04-17 5236035+fzyzcjy@users.noreply.github.com [Impeller] Make `DoMakeRasterSnapshot` output timeline event. (flutter/engine#41197) 2023-04-17 bdero@google.com [Impeller] Remove ContentContextOptions declarations from AnonymousContents (flutter/engine#41256) 2023-04-17 skia-flutter-autoroll@skia.org Roll Dart SDK from 786a70d8ef6b to a335e6724332 (1 revision) (flutter/engine#41278) 2023-04-17 skia-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from atix5Ek_OOxH-uoPA... to Cy5LG4U2InaFLkJGz... (flutter/engine#41275) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from atix5Ek_OOxH to Cy5LG4U2InaF If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC jacksongardner@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: 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

dnfield

@bastionkid - would you consider adding a test, like the ones in https://github.com/flutter/engine/blob/eadb540b8d1b3cac7458c1b7bba8761a96cdb2a5/shell/platform/android/test/io/flutter/embedding/android/FlutterActivityAndFragmentDelegateTest.java for this?

This code should be pretty easily testable if I'm reading it right.

null safety check added around FlutterView avoid crash

3c9fc2f

flutter-dashboard bot added the platform-android label Apr 11, 2023

bastionkid marked this pull request as ready for review April 12, 2023 04:44

bastionkid added 2 commits April 12, 2023 10:14

Merge branch 'main' into main

288bf54

Merge branch 'main' into main

abbe284

gmackall requested review from stuartmorgan and reidbaker April 13, 2023 18:39

Merge branch 'main' into main

f4f2f6c

flutter-dashboard bot added the needs tests label Apr 14, 2023

bastionkid changed the title ~~null check added around FlutterView to avoid NPE while calling FlutterView.detachFromFlutterEngine()~~ null check added to avoid NPE while calling FlutterView.detachFromFlutterEngine() Apr 14, 2023

Merge branch 'main' into main

9d48b29

reidbaker reviewed Apr 14, 2023

View reviewed changes

shell/platform/android/io/flutter/embedding/android/FlutterActivityAndFragmentDelegate.java Show resolved Hide resolved

bastionkid and others added 2 commits April 14, 2023 23:17

Update shell/platform/android/io/flutter/embedding/android/FlutterAct…

9908706

…ivityAndFragmentDelegate.java Co-authored-by: Reid Baker <hamilton.reid.baker@gmail.com>

Merge branch 'main' into main

fa20321

bastionkid requested a review from reidbaker April 15, 2023 04:38

reidbaker added 2 commits April 17, 2023 10:38

Update FlutterActivityAndFragmentDelegate.java

f4cb00e

Break comment into 2 lines.

Merge branch 'main' into main

a13474a

reidbaker approved these changes Apr 17, 2023

View reviewed changes

Hixie reviewed Apr 17, 2023

View reviewed changes

reidbaker requested review from gmackall and camsim99 April 17, 2023 20:57

gmackall approved these changes Apr 17, 2023

View reviewed changes

gmackall added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 17, 2023

auto-submit bot merged commit 2c6a936 into flutter:main Apr 17, 2023
34 checks passed

engine-flutter-autoroll mentioned this pull request Apr 17, 2023

Roll Flutter Engine from b2d07388ceb6 to 20034a8d62c4 (7 revisions) flutter/flutter#125001

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 17, 2023

2c6a93677 null check added to avoid NPE while calling FlutterView.det…

4376c3c

…achFromFlutterEngine() (flutter/engine#41082)

dnfield reviewed Apr 23, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

null check added to avoid NPE while calling FlutterView.detachFromFlutterEngine() #41082

null check added to avoid NPE while calling FlutterView.detachFromFlutterEngine() #41082

bastionkid commented Apr 11, 2023 •

edited

flutter-dashboard bot commented Apr 14, 2023

reidbaker commented Apr 14, 2023

bastionkid commented Apr 17, 2023

Hixie Apr 17, 2023

reidbaker Apr 18, 2023

Hixie Apr 24, 2023

Hixie commented Apr 17, 2023 •

edited

Hixie commented Apr 17, 2023

reidbaker commented Apr 17, 2023

gmackall left a comment

dnfield left a comment

null check added to avoid NPE while calling FlutterView.detachFromFlutterEngine() #41082

null check added to avoid NPE while calling FlutterView.detachFromFlutterEngine() #41082

Conversation

bastionkid commented Apr 11, 2023 • edited

Pre-launch Checklist

flutter-dashboard bot commented Apr 14, 2023

reidbaker commented Apr 14, 2023

bastionkid commented Apr 17, 2023

Hixie Apr 17, 2023

Choose a reason for hiding this comment

reidbaker Apr 18, 2023

Choose a reason for hiding this comment

Hixie Apr 24, 2023

Choose a reason for hiding this comment

Hixie commented Apr 17, 2023 • edited

Hixie commented Apr 17, 2023

reidbaker commented Apr 17, 2023

gmackall left a comment

Choose a reason for hiding this comment

dnfield left a comment

Choose a reason for hiding this comment

bastionkid commented Apr 11, 2023 •

edited

Hixie commented Apr 17, 2023 •

edited