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.

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

182-disposal-null-ref-crash #259

Merged
merged 6 commits into from
Jul 4, 2023
Merged

182-disposal-null-ref-crash #259

merged 6 commits into from
Jul 4, 2023

Conversation

JulianBissekkou
Copy link
Collaborator

@JulianBissekkou JulianBissekkou commented Jun 23, 2023

This PR should fix the crash as described in #182 and #257
The status is still in draft because I would like to get feedback from android devs of maplibre-gl-native.

Problem

The crash started to occure with flutter 3.x and after some investigations we were able to detect the exact change that broke it. See flutter/flutter#107297 or flutter/flutter#107297 (comment) for details.

The engine change in flutter/engine@8dc7cd1 is not calling removeView in all cases which might be the reason why this issue is happening.
I digged into the framework and saw that removeView will trigger onDetachedFromWindow in the subviews. This is important since once of the subviews is TextureView which destorys the surface texture:

Here is TextureView.onDeatachedFromWindowInternal

    @Override
    @UnsupportedAppUsage(maxTargetSdk = Build.VERSION_CODES.R, trackingBug = 170729553)
    protected void onDetachedFromWindowInternal() {
        destroyHardwareLayer();
        releaseSurfaceTexture();
        super.onDetachedFromWindowInternal();
    }

My guess: If this is not called we might still draw onto a surface which triggers the crash.

The fix

If my assumptions are correct then this should be fixed in the engine and not in the libraries that need a platform view. You can find the workaround in this PR. We simply create a view container that calls removeView to trigger the onDetachedFromWindow.
We tested this in an example app with success and also in some of our internal projects.

Please give some feedback if my assumptions are correct. Thanks!

@m0nac0
Copy link
Collaborator

m0nac0 commented Jun 30, 2023

Wow, that's some great debuggingtective work, thank you! I think you (and @GaelleJoubert) know a lot more about this issue than I do. So if this fixes the issue in your tests and doesn't cause any new issues (I couldn't think of any, judging from the code) this is ready to merge from my side.

We can still roll this back if it gets fixed in Flutter.

@stefanschaller stefanschaller changed the title Draft: 182-disposal-null-ref-crash 182-disposal-null-ref-crash Jul 4, 2023
@stefanschaller stefanschaller changed the title 182-disposal-null-ref-crash Draft: 182-disposal-null-ref-crash Jul 4, 2023
@JulianBissekkou
Copy link
Collaborator Author

@m0nac0
Please take a look again. I removed the useDelayedDisposal flag and upated the docs accordingly :D

@stefanschaller
Copy link
Collaborator

@m0nac0 @JulianBissekkou Could you please check out the last two commits from my side?

  • Removed useHybridCompositionOverride
  • Added information to the Changelog

@JulianBissekkou JulianBissekkou changed the title Draft: 182-disposal-null-ref-crash 182-disposal-null-ref-crash Jul 4, 2023
@JulianBissekkou JulianBissekkou merged commit 492af3d into maplibre:main Jul 4, 2023
6 checks passed
@JulianBissekkou JulianBissekkou deleted the 182-disposal-null-ref-crash branch July 4, 2023 11:16
@felix-ht
Copy link
Contributor

felix-ht commented Jul 6, 2023

note that we had a very similar fix in the "upstream" flutter-mapbox-gl/maps#1293

Adding the onStop before the onDestory is all that was needed to fix the issue. As a matter of fact this crash already was happening on older versions of the flutter, howbeit very rarely.

@m0nac0
Copy link
Collaborator

m0nac0 commented Jul 6, 2023

@stefanschaller @JulianBissekkou Thank you for the great work!

JanikoNaber pushed a commit to JanikoNaber/flutter-maplibre-gl that referenced this pull request Aug 23, 2023
This PR should fix the crash as described in maplibre#182  and maplibre#257 
The status is still in draft because I would like to get feedback from
android devs of maplibre-gl-native.

### Problem
The crash started to occure with flutter 3.x and after some
investigations we were able to detect the exact change that broke it.
See flutter/flutter#107297 or
flutter/flutter#107297 (comment)
for details.

The engine change in
flutter/engine@8dc7cd1
is not calling `removeView` in all cases which might be the reason why
this issue is happening.
I digged into the framework and saw that `removeView` will trigger
`onDetachedFromWindow` in the subviews. This is important since once of
the subviews is `TextureView` which destorys the surface texture:


Here is `TextureView.onDeatachedFromWindowInternal`
```java
    @OverRide
    @UnsupportedAppUsage(maxTargetSdk = Build.VERSION_CODES.R, trackingBug = 170729553)
    protected void onDetachedFromWindowInternal() {
        destroyHardwareLayer();
        releaseSurfaceTexture();
        super.onDetachedFromWindowInternal();
    }
```

My guess: If this is not called we might still draw onto a surface which
triggers the crash.

### The fix
If my assumptions are correct then this should be fixed in the engine
and not in the libraries that need a platform view. You can find the
workaround in this PR. We simply create a view container that calls
`removeView` to trigger the `onDetachedFromWindow`.
We tested this in an example app with success and also in some of our
internal projects.

Please give some feedback if my assumptions are correct. Thanks!

---------

Co-authored-by: Julian Bissekkou <[email protected]>
Co-authored-by: Stefan Schaller <[email protected]>
@cfspeier
Copy link

Hey guys thank you so much. I cannot reproduce the crash from issue #182 (which I think can get closed now)
Thank you for this outstanding work. 🎈 👍 Really helps our project!

Thank you @JulianBissekkou . Thank you @stefanschaller . Thank you @felix-ht . Thank you @m0nac0.

Where is the "buy me a coffee" button? ☕ 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants