maplibre / flutter-maplibre-gl

Customizable, performant and vendor-free vector and raster maps, flutter wrapper for maplibre-native and maplibre-gl-js (fork of flutter-mapbox-gl/maps)
https://pub.dev/packages/maplibre_gl
Other
226 stars 125 forks source link

182-disposal-null-ref-crash #259

Closed JulianBissekkou closed 1 year ago

JulianBissekkou commented 1 year ago

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 https://github.com/flutter/flutter/issues/107297 or https://github.com/flutter/flutter/issues/107297#issuecomment-1602222575 for details.

The engine change in https://github.com/flutter/engine/commit/8dc7cd1b1a33b5da561ac859cdcc49705ad1e598 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 commented 1 year ago

Wow, that's some great de~bugging~tective 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.

JulianBissekkou commented 1 year ago

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

stefanschaller commented 1 year ago

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

felix-ht commented 1 year ago

note that we had a very similar fix in the "upstream" https://github.com/flutter-mapbox-gl/maps/pull/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 commented 1 year ago

@stefanschaller @JulianBissekkou Thank you for the great work!

cfspeier commented 1 year ago

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. :balloon: :+1: 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? :coffee: :smiley: