Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Unmount everything and dispose States when host activity dies #19358

Open
xster opened this issue Jul 13, 2018 · 9 comments
Open

Unmount everything and dispose States when host activity dies #19358

xster opened this issue Jul 13, 2018 · 9 comments
Labels
c: new feature Nothing broken; request for a new capability engine flutter/engine repository. See also e: labels. framework flutter/packages/flutter repository. See also f: labels. P2 Important issues not at the top of the work list team-engine Owned by Engine team triaged-engine Triaged by Engine team

Comments

@xster
Copy link
Member

xster commented Jul 13, 2018

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:

  1. Have FlutterView also handle onDestroy() (and equivalent on iOS)
  2. Have a new AppLifecycleState for finishing
  3. 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
@xster xster added c: new feature Nothing broken; request for a new capability framework flutter/packages/flutter repository. See also f: labels. engine flutter/engine repository. See also e: labels. labels Jul 13, 2018
@matthew-carroll
Copy link
Contributor

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
Copy link
Contributor

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 xster mentioned this issue Jul 13, 2018
Closed
@xster
Copy link
Member Author

xster commented Jul 13, 2018 •
edited
Loading

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
Copy link
Contributor

Hixie commented 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
Copy link
Member Author

xster commented 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
Copy link
Contributor

@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
Copy link
Contributor

Hixie commented 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 nichtverstehen mentioned this issue Aug 24, 2018
Closed
@nichtverstehen
Copy link

nichtverstehen commented Aug 24, 2018 •
edited
Loading

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, VideoPlayer plugin on Android allocates a platform MediaPlayer object for each VideoPlayer plugin in the widget tree. MediaPlayer objects are destroyed when corresponding Flutter widgets go away. But if a whole FlutterView is destroyed, dispose methods don't run, so MediaPlayer instances 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.

@nichtverstehen nichtverstehen mentioned this issue Aug 24, 2018
Open
nichtverstehen added a commit to nichtverstehen/plugins that referenced this issue Aug 24, 2018
@nichtverstehen
Release all resources allocated for VideoPlayers when the owning Flut…
c908433 
…terView is being destroyed.

Otherwise the exoplayer MediaPlayer objects continue generating frames after FlutterNativeView is destroyed and onFrameAvailableListener for the video target SurfaceTexture tries to access a missing engine instance.

This is a workaround for widgets not receiving dispose signals when FlutterView is destroyed (flutter/flutter#19358).
flutter/flutter#20989 tracks ensuring that resources are released.
@nichtverstehen nichtverstehen mentioned this issue Aug 24, 2018
Merged
nichtverstehen added a commit to nichtverstehen/plugins that referenced this issue Aug 24, 2018
@nichtverstehen
Release all resources allocated for VideoPlayers when the owning Flut…
54bf55e 
…terView is being destroyed.

Otherwise the exoplayer MediaPlayer objects continue generating frames after FlutterNativeView is destroyed and onFrameAvailableListener for the video target SurfaceTexture tries to access a missing engine instance.

This is a workaround for widgets not receiving dispose signals when FlutterView is destroyed (flutter/flutter#19358).
flutter/flutter#20989 tracks ensuring that resources are released.
nichtverstehen added a commit to nichtverstehen/plugins that referenced this issue Aug 24, 2018
@nichtverstehen
Release all resources allocated for VideoPlayers when the owning Flut…
93d3f27 
…terView is being destroyed.

Otherwise the exoplayer MediaPlayer objects continue generating frames after FlutterNativeView is destroyed and onFrameAvailableListener for the video target SurfaceTexture tries to access a missing engine instance.

This is a workaround for widgets not receiving dispose signals when FlutterView is destroyed (flutter/flutter#19358).
flutter/flutter#20989 tracks ensuring that resources are released.
nichtverstehen added a commit to nichtverstehen/plugins that referenced this issue Aug 24, 2018
@nichtverstehen
Release all resources allocated for VideoPlayers when the owning Flut…
7a8affb 
…terView is being destroyed.

Otherwise the exoplayer MediaPlayer objects continue generating frames after FlutterNativeView is destroyed and onFrameAvailableListener for the video target SurfaceTexture tries to access a missing engine instance.

This is a workaround for widgets not receiving dispose signals when FlutterView is destroyed (flutter/flutter#19358).
flutter/flutter#20989 tracks ensuring that resources are released.
nichtverstehen added a commit to nichtverstehen/plugins that referenced this issue Aug 27, 2018
@nichtverstehen
Release all resources allocated for VideoPlayers when the owning Flut…
de62a61 
…terView is being destroyed.

Otherwise the exoplayer MediaPlayer objects continue generating frames after FlutterNativeView is destroyed and onFrameAvailableListener for the video target SurfaceTexture tries to access a missing engine instance.

This is a workaround for widgets not receiving dispose signals when FlutterView is destroyed (flutter/flutter#19358).
flutter/flutter#20989 tracks ensuring that resources are released.
nichtverstehen added a commit to nichtverstehen/plugins that referenced this issue Aug 27, 2018
@nichtverstehen
Release all resources allocated for VideoPlayers when the owning Flut…
f531a08 
…terView is being destroyed.

Otherwise the exoplayer MediaPlayer objects continue generating frames after FlutterNativeView is destroyed and onFrameAvailableListener for the video target SurfaceTexture tries to access a missing engine instance.

This is a workaround for widgets not receiving dispose signals when FlutterView is destroyed (flutter/flutter#19358).
flutter/flutter#20989 tracks ensuring that resources are released.
mehmetf pushed a commit to flutter/plugins that referenced this issue Aug 27, 2018
@nichtverstehen @mehmetf
Release all resources allocated for VideoPlayers when the owning Flut…
9f80468 
…terView is being destroyed. (#739)

Otherwise the exoplayer MediaPlayer objects continue generating frames after FlutterNativeView is destroyed and onFrameAvailableListener for the video target SurfaceTexture tries to access a missing engine instance.

This is a workaround for widgets not receiving dispose signals when FlutterView is destroyed (flutter/flutter#19358).
flutter/flutter#20989 tracks ensuring that resources are released.
@ened
Copy link
Contributor

ened commented Sep 6, 2018

Hi Flutter Team, did you find time to work on this issue, yet? I have been running into similar issues rgd. the App State while Androids' debug setting "Don't keep activities" is turned on.

nichtverstehen added a commit to nichtverstehen/plugins that referenced this issue Sep 6, 2018
@nichtverstehen
Release all resources allocated for VideoPlayers when the owning Flut…
dea041f 
…terView is being destroyed.

Otherwise the exoplayer MediaPlayer objects continue generating frames after FlutterNativeView is destroyed and onFrameAvailableListener for the video target SurfaceTexture tries to access a missing engine instance.

This is a workaround for widgets not receiving dispose signals when FlutterView is destroyed (flutter/flutter#19358).
flutter/flutter#20989 tracks ensuring that resources are released.
@zoechi zoechi added this to the Goals milestone Feb 14, 2019
andreidiaconu pushed a commit to andreidiaconu/plugins that referenced this issue Feb 17, 2019
@nichtverstehen @andreidiaconu
Release all resources allocated for VideoPlayers when the owning Flut…
e235935 
…terView is being destroyed. (flutter#739)

Otherwise the exoplayer MediaPlayer objects continue generating frames after FlutterNativeView is destroyed and onFrameAvailableListener for the video target SurfaceTexture tries to access a missing engine instance.

This is a workaround for widgets not receiving dispose signals when FlutterView is destroyed (flutter/flutter#19358).
flutter/flutter#20989 tracks ensuring that resources are released.
@cbracken cbracken mentioned this issue May 6, 2019
Closed
12 tasks
ryanheise pushed a commit to ryanheise/background_video_player that referenced this issue Sep 29, 2019
@nichtverstehen @mehmetf
Release all resources allocated for VideoPlayers when the owning Flut…
85e4402 
…terView is being destroyed. (#739)

Otherwise the exoplayer MediaPlayer objects continue generating frames after FlutterNativeView is destroyed and onFrameAvailableListener for the video target SurfaceTexture tries to access a missing engine instance.

This is a workaround for widgets not receiving dispose signals when FlutterView is destroyed (flutter/flutter#19358).
flutter/flutter#20989 tracks ensuring that resources are released.
@xster xster mentioned this issue Mar 26, 2020
Closed
@kf6gpe kf6gpe added the P2 Important issues not at the top of the work list label May 29, 2020
@Hixie Hixie removed this from the None. milestone Aug 17, 2020
@flutter-triage-bot flutter-triage-bot bot added multiteam-retriage-candidate team-engine Owned by Engine team triaged-engine Triaged by Engine team labels Jul 7, 2023
auto-submit bot pushed a commit to flutter/packages that referenced this issue Apr 16, 2024
@0xZOne
[video_player] Calls onDestroy instead of initialize in onDetache…
b2bd8e5 
…dFromEngine (#6501)

In the `onDetachedFromEngine` function, the `onDestroy` method should be called instead of the `initialize` method, even though the implementation of the two functions is no different.

```java
  private void onDestroy() {
    // The whole FlutterView is being destroyed. Here we release resources acquired for all
    // instances
    // of VideoPlayer. Once flutter/flutter#19358 is resolved this may
    // be replaced with just asserting that videoPlayers.isEmpty().
    // flutter/flutter#20989 tracks this.
    disposeAllPlayers();
  }

  public void initialize() {
    disposeAllPlayers();
  }
```
TecHaxter pushed a commit to TecHaxter/flutter_packages that referenced this issue May 22, 2024
@0xZOne @TecHaxter
[video_player] Calls onDestroy instead of initialize in onDetache…
059747f 
…dFromEngine (flutter#6501)

In the `onDetachedFromEngine` function, the `onDestroy` method should be called instead of the `initialize` method, even though the implementation of the two functions is no different.

```java
  private void onDestroy() {
    // The whole FlutterView is being destroyed. Here we release resources acquired for all
    // instances
    // of VideoPlayer. Once flutter/flutter#19358 is resolved this may
    // be replaced with just asserting that videoPlayers.isEmpty().
    // flutter/flutter#20989 tracks this.
    disposeAllPlayers();
  }

  public void initialize() {
    disposeAllPlayers();
  }
```
zhouyuanbo pushed a commit to zhouyuanbo/video_player_android_2.4.14 that referenced this issue May 24, 2024
@0xZOne
[video_player] Calls onDestroy instead of initialize in onDetache…
003274c 
…dFromEngine (#6501)

In the `onDetachedFromEngine` function, the `onDestroy` method should be called instead of the `initialize` method, even though the implementation of the two functions is no different.

```java
  private void onDestroy() {
    // The whole FlutterView is being destroyed. Here we release resources acquired for all
    // instances
    // of VideoPlayer. Once flutter/flutter#19358 is resolved this may
    // be replaced with just asserting that videoPlayers.isEmpty().
    // flutter/flutter#20989 tracks this.
    disposeAllPlayers();
  }

  public void initialize() {
    disposeAllPlayers();
  }
```
arc-yong pushed a commit to Arctuition/packages-arc that referenced this issue Jun 14, 2024
@0xZOne @arc-yong
[video_player] Calls onDestroy instead of initialize in onDetache…
3c6d28a 
…dFromEngine (flutter#6501)

In the `onDetachedFromEngine` function, the `onDestroy` method should be called instead of the `initialize` method, even though the implementation of the two functions is no different.

```java
  private void onDestroy() {
    // The whole FlutterView is being destroyed. Here we release resources acquired for all
    // instances
    // of VideoPlayer. Once flutter/flutter#19358 is resolved this may
    // be replaced with just asserting that videoPlayers.isEmpty().
    // flutter/flutter#20989 tracks this.
    disposeAllPlayers();
  }

  public void initialize() {
    disposeAllPlayers();
  }
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
c: new feature Nothing broken; request for a new capability engine flutter/engine repository. See also e: labels. framework flutter/packages/flutter repository. See also f: labels. P2 Important issues not at the top of the work list team-engine Owned by Engine team triaged-engine Triaged by Engine team
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
8 participants
@xster @ened @zoechi @Hixie @nichtverstehen @kf6gpe @matthew-carroll @markusaksli-nc