Repository navigation
Unmount everything and dispose States when host activity dies #19358
Description
Looking at #19159, it seems reasonable looking at the API doc to expect the State.dispose to be called when the FlutterActivity is finishing/being destroyed. Yet it never gets called and there's an inbalance between things registered and/or created during initState() and things unregistered and/or released during dispose().
Should we:
- Have FlutterView also handle onDestroy() (and equivalent on iOS)
- Have a new AppLifecycleState for finishing
- Have the widget framework unmount and dispose everything on onDestroy
or at least have the State.dispose() API doc reference AppLifecycleState if we don't want to do this automatically?
cc @Hixie @jason-simmons
also cc @matthew-carroll since this scenario will surface more frequently with add-to-app
Activity
matthew-carroll commentedon Jul 13, 2018
One thing that needs to be considered here is the possibility that Flutter is used across Activitys. I haven't looked much at that use-case, so I'm not sure how much of that scenario is about persisting the Dart process vs persisting Flutter hierarchy. But let's not immediately assume that Flutter should disappear when an Activity or FlutterView is destroyed - if that's what is being suggested, then someone needs to make that case very carefully and deeply.
Don't forget, for example, that not all Widgets are UI. Consider an AudioPlayer widget. The UI may have disappeared, but the audio should probably keep playing. Now, half of that story is on the platform side, but it doesn't necessarily mean you want to lose the Flutter state related to that Widget/Element.
A middle ground here might be an explicit signal sent to the FlutterView that you truly want to destroy the entire Flutter system. Then the developer can opt-in/opt-out.
matthew-carroll commentedon Jul 13, 2018
WRT to #19159 - if the true problem of that ticket is a failure to unregister, why would the ticket require 20 Activity restarts to develop the issue? I might expect a memory leak in that scenario, but #19159 says the issue is an ANR which is a CPU lockup, not a memory pressure issue.
I'd like to understand more clearly why this disposal problem is supposedly causing that other issue...
xster commentedon Jul 13, 2018
https://github.com/flutter/engine/pull/5640/files seems to be making the case of calling out backgroundable 'hosts' explicitly but making the Flutter side disposal an explicit signal (which may be auto-hooked) sounds great.
Hixie commentedon Jul 14, 2018
Generally speaking you can't rely on the OS doing anything graceful to your app anyway, FWIW, so depending on dispose being called is asking for trouble. Consider what happens when the user pulls the battery, or the OS kills your app for using too much memory, or whatnot.
xster commentedon Jul 14, 2018
I think with add-to-app, we should start to change how we think about platform interactions and pull parts out from the "OS doing things to your app" group into "mixed navigation" group. Related discussion (internal).
In your pulling the battery scenario, we'd be in the same situation as Flutter to Flutter navigation (no automatic calls to dispose) and native to native navigation (no automatic calls to onDestroy).
But otherwise, I think we should make mixed navigation API convenience a first class citizen too as opposed to our present alternative (which would involve an encapsulated Flutter view logic handling
dispose()in case it's navigated away from inside Flutter and also having a custom FlutterFragment which handles onDestroy and have a platform channel with a custom listener registration mechanism on the Flutter side that views need to register to to perform the same dispose() task when it's navigated away from natively).Whether they both trigger the same function (
dispose()) and whether the native navigating-away triggers it automatically is debatable like Matt is saying, but I don't think the present state is sufficient.matthew-carroll commentedon Jul 14, 2018
@xster would you mind putting together a few sequence diagrams to show what we have now vs what you're recommending? I'm having a difficult time comprehending the recommendation in paragraph form.
Hixie commentedon Jul 15, 2018
To clarify, I definitely agree that we should be calling dispose methods and so on during shutdown (otherwise I'd have suggested closing the bug!). I'm just saying that we shouldn't encourage people to rely on it.
nichtverstehen commentedon Aug 24, 2018
I'd like to reiterate that resolving this is very important for add-to-app use cases, since it would allow plugins that acquire per-widget resources to release them gracefully when widgets are disposed of.
Specifically,
VideoPlayerplugin on Android allocates a platformMediaPlayerobject for eachVideoPlayerplugin in the widget tree.MediaPlayerobjects are destroyed when corresponding Flutter widgets go away. But if a wholeFlutterViewis destroyed,disposemethods don't run, soMediaPlayerinstances are not released. This leads to leaks and also crashes (since the player continues to provide frames for non-existent surfaces; internal bug: b/112692333). See #20989.Release all resources allocated for VideoPlayers when the owning Flut…
Release all resources allocated for VideoPlayers when the owning Flut…
Release all resources allocated for VideoPlayers when the owning Flut…
Release all resources allocated for VideoPlayers when the owning Flut…
7 remaining items
Release all resources allocated for VideoPlayers when the owning Flut…
[video_player] Calls `onDestroy` instead of `initialize` in onDetache…
[video_player] Calls `onDestroy` instead of `initialize` in onDetache…
[video_player] Calls `onDestroy` instead of `initialize` in onDetache…
[video_player] Calls `onDestroy` instead of `initialize` in onDetache…
[video_player] Calls `onDestroy` instead of `initialize` in onDetache…