Skip to content

Commit

Permalink
182-disposal-null-ref-crash (maplibre#259)
Browse files Browse the repository at this point in the history
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 <julian.bissekkou@tapped.dev>
Co-authored-by: Stefan Schaller <stefan.schaller@tapped.dev>
  • Loading branch information
3 people authored and JanikoNaber committed Aug 23, 2023
1 parent 9cfa0af commit b1b3935
Show file tree
Hide file tree
Showing 7 changed files with 41 additions and 379 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -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(
Expand Down
7 changes: 1 addition & 6 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
30 changes: 28 additions & 2 deletions android/src/main/java/com/mapbox/mapboxgl/MapboxMapController.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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>();
Expand All @@ -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() {
Expand Down Expand Up @@ -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;
}

Expand Down
10 changes: 0 additions & 10 deletions lib/src/mapbox_map.dart
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,6 @@ class MaplibreMap extends StatefulWidget {
AnnotationType.line,
AnnotationType.circle,
],
this.useDelayedDisposal,
this.useHybridCompositionOverride,
}) : assert(
myLocationRenderMode != MyLocationRenderMode.NORMAL
? myLocationEnabled
Expand Down Expand Up @@ -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).
Expand Down Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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: (
Expand All @@ -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,
);
Expand All @@ -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,
Expand Down
Loading

0 comments on commit b1b3935

Please sign in to comment.