Misc Android Crash Fixes #963
Conversation
ClayC-WeatherBug
commented
Jul 23, 2020
ClayC-WeatherBug
commented
- Add null checks when queueing features and making region payload to avoid crashes.
- Modified disposeNativeMapView threading to fix a crash.
| Log.e(getClass().getSimpleName() , " disposeNativeMapView() exception destroying map view", ex); | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
This is missing a closing } for the if statement.
There was a problem hiding this comment.
Whoops - good catch and thanks. I'll update the PR shortly.
There was a problem hiding this comment.
Sorry, I think, now there is a missing ) on line 334 :(
There was a problem hiding this comment.
It's causing a build error in our CI tools
There was a problem hiding this comment.
Ah shoot - sorry that was sloppy on my part. I just pushed an update. Thanks for pointing that out!
| if (mapView != null) | ||
| { | ||
| runOnUiThread(new Runnable() | ||
| { | ||
| @Override | ||
| public void run() | ||
| { | ||
| try | ||
| { | ||
| mapView.dispose(); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| Log.e(getClass().getSimpleName() , " disposeNativeMapView() exception destroying map view", ex); | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
Also, I'm not that well versed in Java, can you explain to me the benefit of your solution versus the one already implemented?
Is it the fact, that Runnable is being created inline and thus only if mapView != null versus the old implementation that is creating it nevertheless?
Thanks
There was a problem hiding this comment.
The future task which we were using earlier will not be executed immediately. It will be executed after some amount of time, but until then it will block the main thread. That causes ANR issues, and we found this while changing the orientation in Android tablets. It causes the app to freeze.
There was a problem hiding this comment.
The wait for dispose was introduced in:
nitaliano/react-native-mapbox-gl#1075
I'm a bit confused, as i don't get all the details behind 1075 either.
So you're saying that on android waiting for mapView.dispose() caused App Not Responding. But how is it related to changing orientation on Android tablet?!
There was a problem hiding this comment.
On Orientation Change our Android Activity will be destroyed and recreated. In this case, we are seeing a black screen and the app freezes forever. After doing some analysis, we found that Future Task is blocking UI/Main thread.
android/rctmgl/src/main/java/com/mapbox/rctmgl/components/mapview/RCTMGLMapViewManager.java
Show resolved
Hide resolved
| @@ -1171,27 +1171,35 @@ public void onHostDestroy() { | |||
|
|
|||
| private WritableMap makeRegionPayload(Boolean isAnimated) { | |||
| CameraPosition position = mMap.getCameraPosition(); | |||
| if(position == null || position.target == null){ | |||
There was a problem hiding this comment.
Just curious, what causes position or position.target to be null?!
There was a problem hiding this comment.
We are not sure, but we were occasionally seeing a crash at this line and this seemed to fix it.
| properties.putArray("visibleBounds", GeoJSONUtils.fromLatLngBounds(visibleRegion.latLngBounds)); | ||
| } catch(Exception ex) | ||
| { | ||
| ex.printStackTrace(); |
There was a problem hiding this comment.
In what cases would it fail?! And what is the exception?!
We should consider using com.mapbox.mapboxsdk.log.Logger as that can be processed on the JS side, see Logger.
There was a problem hiding this comment.
Good call on using Logger. I'll send an update to the PR shortly. Same comment here as above - we were occasionally seeing crashes, and the stack traces pointed to this part of the code. We added the null check and try/catch, and it seems to have fixed the crashes.
|
I apologize for the delay in responding to questions on this PR. I was out all last week and didn't get a chance to respond to any of this. These changes came from some changes my team made internally to fix crashes. I've reached out to the developers and either I or one of them will respond with more details to the questions asap. |
… avoid crashes. - Modified disposeNativeMapView threading to fix a crash.
ClayC-WeatherBug
force-pushed
the
fix-android-misc-crashes
branch
from
9ed2fc1
to
8165c45
Compare
|
Sorry for the delay @ClayC-WeatherBug, I've checked again, there is a missing
I think we're good to go |
|
@mfazekas , can we merge this, or is there anything else you wanted to address? |
|
@ferdicus it's good to go |
