182-disposal-null-ref-crash (#259)

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` ```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]>
maplibre · Jul 4, 2023 · 492af3d · 492af3d
1 parent 5d59879
commit 492af3d
Show file tree

Hide file tree

Showing 7 changed files with 41 additions and 379 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,6 +1,8 @@
 ## upcoming version
 
 ### Breaking Change:
+* `useDelayedDisposal` was removed since its now fixed in https://github.com/maplibre/flutter-maplibre-gl/pull/259
+* `useHybridCompositionOverride` was removed since it was added in the following fix: https://github.com/maplibre/flutter-maplibre-gl/pull/203 and we reverted the fix and used another approach to fix the actual issue.
 * The default for `myLocationRenderMode` was changed from `COMPASS` to `NORMAL` in https://github.com/maplibre/flutter-maplibre-gl/pull/244, since the previous default value of `COMPASS` implicitly enables displaying the location on iOS, which could crash apps that didn't want to display the device location. If you want to continue to use `MyLocationRenderMode.COMPASS`, please explicitly specify it in the constructor like this:
 ```dart
 MaplibreMap(

diff --git a/README.md b/README.md
@@ -149,12 +149,7 @@ buildTypes {
 }
 ```
 
-## Flutter 3.x.x issues
-Since Flutter 3.x.x was introduced, it exposed some race conditions resulting in occasional crashes upon map disposal. The parameter `useDelayedDisposal` was introduced as a workaround for this issue until Flutter and/or Maplibre fix this issue properly. Use with caution.
-
-
-
-### iOS app crashes on startup
+### iOS app crashes when using location based features
 
 Please include the `NSLocationWhenInUseUsageDescription` as described [here](#location-features)
 

diff --git a/android/src/main/java/com/mapbox/mapboxgl/MapboxMapController.java b/android/src/main/java/com/mapbox/mapboxgl/MapboxMapController.java
@@ -19,7 +19,10 @@
 import android.util.Log;
 import android.view.Gravity;
 import android.view.MotionEvent;
+import android.view.TextureView;
 import android.view.View;
+import android.widget.FrameLayout;
+
 import androidx.annotation.NonNull;
 import androidx.lifecycle.DefaultLifecycleObserver;
 import androidx.lifecycle.Lifecycle;
@@ -118,6 +121,11 @@ final class MapboxMapController
   private final float density;
   private final Context context;
   private final String styleStringInitial;
+  /**
+   * This container is returned as the final platform view instead of returning `mapView`.
+   * See {@link MapboxMapController#destroyMapViewIfNecessary()} for details.
+   */
+  private FrameLayout mapViewContainer;
   private MapView mapView;
   private MapboxMap mapboxMap;
   private boolean trackCameraPosition = false;
@@ -181,6 +189,7 @@ public void onStyleLoaded(@NonNull Style style) {
     this.context = context;
     this.dragEnabled = dragEnabled;
     this.styleStringInitial = styleStringInitial;
+    this.mapViewContainer = new FrameLayout(context);
     this.mapView = new MapView(context, options);
     this.interactiveFeatureLayerIds = new HashSet<>();
     this.addedFeaturesByLayer = new HashMap<String, FeatureCollection>();
@@ -190,13 +199,14 @@ public void onStyleLoaded(@NonNull Style style) {
       this.androidGesturesManager = new AndroidGesturesManager(this.mapView.getContext(), false);
     }
 
+    mapViewContainer.addView(mapView);
     methodChannel = new MethodChannel(messenger, "plugins.flutter.io/mapbox_maps_" + id);
     methodChannel.setMethodCallHandler(this);
   }
 
   @Override
   public View getView() {
-    return mapView;
+    return mapViewContainer;
   }
 
   void init() {
@@ -1544,17 +1554,33 @@ public void onCancel() {
     }
   }
 
+  /**
+   * Destroy the MapView and cleans up listeners.
+   * It's very important to call mapViewContainer.removeView(mapView) to make sure
+   * that {@link TextureView#onDetachedFromWindowInternal()} is called which releases the
+   * underlying surface.
+   * This is required due to an FlutterEngine change that was introduce when updating from
+   * Flutter 2.10.5 to Flutter 3.10.0.
+   * This FlutterEngine change is not calling `removeView` on a PlatformView which causes the issue.
+   * <p>
+   * For more information check out:
+   * <a href="https://github.com/flutter/flutter/issues/107297">Flutter issue</a>
+   * <a href="https://github.com/flutter/engine/commit/8dc7cd1b1a33b5da561ac859cdcc49705ad1e598">Flutter Engine commit that introduced the issue</a>
+   * <a href="https://github.com/maplibre/flutter-maplibre-gl/issues/182">The reported issue in the MapLibre repo</a>
+   */
   private void destroyMapViewIfNecessary() {
     if (mapView == null) {
       return;
     }
+    mapViewContainer.removeView(mapView);
+    mapView.onStop();
+    mapView.onDestroy();
 
     if (locationComponent != null) {
       locationComponent.setLocationComponentEnabled(false);
     }
     stopListeningForLocationUpdates();
 
-    mapView.onDestroy();
     mapView = null;
   }
 

diff --git a/lib/src/mapbox_map.dart b/lib/src/mapbox_map.dart
@@ -53,8 +53,6 @@ class MaplibreMap extends StatefulWidget {
       AnnotationType.line,
       AnnotationType.circle,
     ],
-    this.useDelayedDisposal,
-    this.useHybridCompositionOverride,
   })  : assert(
           myLocationRenderMode != MyLocationRenderMode.NORMAL
               ? myLocationEnabled
@@ -221,12 +219,6 @@ class MaplibreMap extends StatefulWidget {
   /// * All fade/transition animations have completed
   final OnMapIdleCallback? onMapIdle;
 
-  /// Use delayed disposal of Android View Controller to avoid flutter 3.x.x crashes
-  final bool? useDelayedDisposal;
-
-  /// Override hybrid mode per map instance
-  final bool? useHybridCompositionOverride;
-
   /// Set `MapboxMap.useHybridComposition` to `false` in order use Virtual-Display
   /// (better for Android 9 and below but may result in errors on Android 12)
   /// or leave it `true` (default) to use Hybrid composition (Slower on Android 9 and below).
@@ -258,8 +250,6 @@ class _MaplibreMapState extends State<MaplibreMap> {
       'options': _MapboxMapOptions.fromWidget(widget).toMap(),
       //'onAttributionClickOverride': widget.onAttributionClick != null,
       'dragEnabled': widget.dragEnabled,
-      'useDelayedDisposal': widget.useDelayedDisposal,
-      'useHybridCompositionOverride': widget.useHybridCompositionOverride,
     };
     return _mapboxGlPlatform.buildView(
         creationParams, onPlatformViewCreated, widget.gestureRecognizers);

diff --git a/maplibre_gl_platform_interface/lib/maplibre_gl_platform_interface.dart b/maplibre_gl_platform_interface/lib/maplibre_gl_platform_interface.dart
@@ -8,7 +8,6 @@ import 'package:flutter/gestures.dart';
 import 'package:flutter/material.dart';
 import 'package:flutter/rendering.dart';
 import 'package:flutter/services.dart';
-part 'src/view_wrappers.dart';
 part 'src/annotation.dart';
 part 'src/callbacks.dart';
 part 'src/camera.dart';

diff --git a/maplibre_gl_platform_interface/lib/src/method_channel_mapbox_gl.dart b/maplibre_gl_platform_interface/lib/src/method_channel_mapbox_gl.dart
@@ -138,12 +138,7 @@ class MethodChannelMaplibreGl extends MapLibreGlPlatform {
       OnPlatformViewCreatedCallback onPlatformViewCreated,
       Set<Factory<OneSequenceGestureRecognizer>>? gestureRecognizers) {
     if (defaultTargetPlatform == TargetPlatform.android) {
-      final useDelayedDisposalParam =
-          (creationParams['useDelayedDisposal'] ?? false) as bool;
-      final useHybridCompositionParam =
-          (creationParams['useHybridCompositionOverride'] ??
-              useHybridComposition) as bool;
-      if (useHybridCompositionParam) {
+      if (useHybridComposition) {
         return PlatformViewLink(
           viewType: 'plugins.flutter.io/mapbox_gl',
           surfaceFactory: (
@@ -158,26 +153,15 @@ class MethodChannelMaplibreGl extends MapLibreGlPlatform {
             );
           },
           onCreatePlatformView: (PlatformViewCreationParams params) {
-            late AndroidViewController controller;
-            if (useDelayedDisposalParam) {
-              controller = WrappedPlatformViewsService.initAndroidView(
-                id: params.id,
-                viewType: 'plugins.flutter.io/mapbox_gl',
-                layoutDirection: TextDirection.ltr,
-                creationParams: creationParams,
-                creationParamsCodec: const StandardMessageCodec(),
-                onFocus: () => params.onFocusChanged(true),
-              );
-            } else {
-              controller = PlatformViewsService.initAndroidView(
-                id: params.id,
-                viewType: 'plugins.flutter.io/mapbox_gl',
-                layoutDirection: TextDirection.ltr,
-                creationParams: creationParams,
-                creationParamsCodec: const StandardMessageCodec(),
-                onFocus: () => params.onFocusChanged(true),
-              );
-            }
+            final controller = PlatformViewsService.initAndroidView(
+              id: params.id,
+              viewType: 'plugins.flutter.io/mapbox_gl',
+              layoutDirection: TextDirection.ltr,
+              creationParams: creationParams,
+              creationParamsCodec: const StandardMessageCodec(),
+              onFocus: () => params.onFocusChanged(true),
+            );
+
             controller.addOnPlatformViewCreatedListener(
               params.onPlatformViewCreated,
             );
@@ -190,15 +174,6 @@ class MethodChannelMaplibreGl extends MapLibreGlPlatform {
           },
         );
       } else {
-        if (useDelayedDisposalParam) {
-          return AndroidViewWithWrappedController(
-            viewType: 'plugins.flutter.io/mapbox_gl',
-            onPlatformViewCreated: onPlatformViewCreated,
-            gestureRecognizers: gestureRecognizers,
-            creationParams: creationParams,
-            creationParamsCodec: const StandardMessageCodec(),
-          );
-        }
         return AndroidView(
           viewType: 'plugins.flutter.io/mapbox_gl',
           onPlatformViewCreated: onPlatformViewCreated,