Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Null java peer callback #11358

Merged
merged 1 commit into from
Apr 10, 2018
Merged

Null java peer callback #11358

merged 1 commit into from
Apr 10, 2018

Conversation

tobrun
Copy link
Member

@tobrun tobrun commented Mar 1, 2018

This PR adds null checking to the java peer before calling into a NativeMapView callback.
With the Mapbox#snapshot api I was able to reproduce:

JNI DETECTED ERROR IN APPLICATION: can't call void com.mapbox.mapboxsdk.maps.NativeMapView.onSnapshotReady(android.graphics.Bitmap) on null object

When requesting a snapshot as part of Activity#onPause:

  @Override
  protected void onPause() {
    super.onPause();
    mapboxMap.snapshot(new MapboxMap.SnapshotReadyCallback() {
      @Override
      public void onSnapshotReady(Bitmap snapshot) {
        Timber.e("snapshot ready");
      }
    });
    mapView.onPause();
  }

This could potentially explain crashes as #11268.

@tobrun tobrun added the Android Mapbox Maps SDK for Android label Mar 1, 2018
@tobrun tobrun added this to the android-v6.0.0 milestone Mar 1, 2018
@tobrun tobrun self-assigned this Mar 1, 2018
@tobrun tobrun requested a review from ivovandongen March 1, 2018 20:03
@tobrun tobrun changed the title Null java peer snapshot callback Null java peer callback Mar 2, 2018
@@ -104,7 +104,9 @@ void NativeMapView::notifyMapChange(mbgl::MapChange change) {

android::UniqueEnv _env = android::AttachEnv();
static auto onMapChanged = javaClass.GetMethod<void (int)>(*_env, "onMapChanged");
javaPeer->Call(*_env, onMapChanged, (int) change);
if (javaPeer != NULL) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you make this just if (javaPeer) or if (javaPeer != nulllptr) if you want to make it more explicit

@@ -408,7 +410,9 @@ void NativeMapView::scheduleSnapshot(jni::JNIEnv&) {

// invoke Mapview#OnSnapshotReady
static auto onSnapshotReady = javaClass.GetMethod<void (jni::Object<Bitmap>)>(*_env, "onSnapshotReady");
javaPeer->Call(*_env, onSnapshotReady, bitmap);
if (javaPeer != NULL) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

@ivovandongen
Copy link
Contributor

@tobrun I'm wondering why the callback is actually fired when the javaPeer is gone. If the java peer is gone, that would be because onDestroy is called on the NativeMapView right? Which would mean that the renderer is also already reset and no callbacks should follow.

Do you have a way to reproduce this with any reliability?

@tobrun
Copy link
Member Author

tobrun commented Mar 5, 2018

@ivovandongen

Do you have a way to reproduce this with any reliability?

Yes, with the code shown in OP I was able to reproduce this on demand.
I will add a test to catch any regressions around this.

@tobrun
Copy link
Member Author

tobrun commented Mar 6, 2018

When writing a test for this issue, I'm occasionally running into the exception mentioned by the user:

Abort message: 'java_vm_ext.cc:534] JNI DETECTED ERROR IN APPLICATION: use of deleted global reference 0x2'

This is a more logical exception as the one I tried addressing in OP. Been reading up on this issue though don't have an immediate fix for it.

@michaelfomin
Copy link

Michael from Wahoo Fitness here, this issue is causing a lot of crashes for us. Just wondering if this fix was scheduled soon?

@tobrun tobrun force-pushed the tvn-null-java-peer-snapshot branch from 1454c26 to 0b5c123 Compare April 5, 2018 19:26
@tobrun
Copy link
Member Author

tobrun commented Apr 5, 2018

@ivovandongen I revisit this PR with a different approach, it introduces a paused state to the map renderer and it resolves the crashes (including #11358 (comment)).

@michaelfomin getting this fixed is a priority, the fix will be backported to a 5.5.x release.

@tobrun tobrun force-pushed the tvn-null-java-peer-snapshot branch from 0b5c123 to f555cde Compare April 5, 2018 19:31
Copy link
Contributor

@ivovandongen ivovandongen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since onPause and onResume are called on the UI thread and render on the GL thread, I think we should use std::atomic<bool> here for the paused flag.

@tobrun tobrun force-pushed the tvn-null-java-peer-snapshot branch from f555cde to 0a83f4e Compare April 9, 2018 19:05
@tobrun
Copy link
Member Author

tobrun commented Apr 9, 2018

@ivovandongen thank you for flagging, fixed up the code to std::atomic<bool>

@tobrun tobrun merged commit c2a78d0 into release-boba Apr 10, 2018
@tobrun tobrun deleted the tvn-null-java-peer-snapshot branch April 10, 2018 09:10
tobrun added a commit that referenced this pull request Apr 10, 2018
tobrun added a commit that referenced this pull request Apr 10, 2018
This was referenced Apr 10, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Android Mapbox Maps SDK for Android
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants