NPE Crash in GeofencingTaskConsumer.didExecuteJob()

[briefjudofox]

🐛 Bug Report

Environment

  Expo CLI 3.0.6 environment info:
    System:
      OS: macOS 10.14.5
      Shell: 3.2.57 - /bin/bash
    Binaries:
      Node: 12.6.0 - ~/.nvm/versions/node/v12.6.0/bin/node
      Yarn: 1.17.3 - ~/.nvm/versions/node/v12.6.0/bin/yarn
      npm: 6.9.0 - ~/.nvm/versions/node/v12.6.0/bin/npm
    IDEs:
      Android Studio: 3.4 AI-183.6156.11.34.5692245
      Xcode: 10.2.1/10E1001 - /usr/bin/xcodebuild
    npmPackages:
      expo: ^33.0.4 => 33.0.5
      react: 16.8.3 => 16.8.3
      react-native: https://github.com/expo/react-native/archive/sdk-33.0.0.tar.gz => 0.59.8
      react-navigation: ^3.11.0 => 3.11.0
    npmGlobalPackages:
      expo-cli: 3.0.6

Seems to be a problem on Android only. I've been testing on a Pixel 2 running Android 9.

Steps to Reproduce

1. Clone the reproducable case below
2. Build and install the app on your phone as a standalone app
3. Open and close the app in quick succession to reproduce.

If you're tailing the logs you should see:

    --------- beginning of crash
2019-08-05 16:14:17.516 10944-10944/? E/AndroidRuntime: FATAL EXCEPTION: main
    Process: com.faketown.usa, PID: 10944
    java.lang.RuntimeException: java.lang.NullPointerException: Attempt to invoke interface method 'void org.unimodules.b.j.d.execute(android.os.Bundle, java.lang.Error)' on a null object reference
        at android.app.job.JobServiceEngine$JobHandler.handleMessage(JobServiceEngine.java:112)
        at android.os.Handler.dispatchMessage(Handler.java:106)
        at android.os.Looper.loop(Looper.java:193)
        at android.app.ActivityThread.main(ActivityThread.java:6718)
        at java.lang.reflect.Method.invoke(Native Method)
        at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:493)
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:858)
     Caused by: java.lang.NullPointerException: Attempt to invoke interface method 'void org.unimodules.b.j.d.execute(android.os.Bundle, java.lang.Error)' on a null object reference
        at expo.modules.location.taskConsumers.GeofencingTaskConsumer.didExecuteJob(GeofencingTaskConsumer.java:128)
        at expo.modules.taskManager.TaskService.handleJob(TaskService.java:331)
        at expo.modules.taskManager.TaskJobService.onStartJob(TaskJobService.java:13)
        at android.app.job.JobService$1.onStartJob(JobService.java:62)
        at android.app.job.JobServiceEngine$JobHandler.handleMessage(JobServiceEngine.java:108)
        at android.os.Handler.dispatchMessage(Handler.java:106) 
        at android.os.Looper.loop(Looper.java:193) 
        at android.app.ActivityThread.main(ActivityThread.java:6718) 
        at java.lang.reflect.Method.invoke(Native Method) 
        at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:493) 
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:858) 

Additional Context

In order to troubleshoot I ejected and added some additional logging and null checks to GeofencingTaskConsumer:

  @Override
  public boolean didExecuteJob(JobService jobService, JobParameters params) {
    if (mTask == null) {
      Log.i(TAG, "--------->mTask is null at the beginning of didExecuteJob().");
      return false;
    }

    List<PersistableBundle> data = getTaskManagerUtils().extractDataFromJobParams(params);

    for (PersistableBundle item : data) {
      Bundle bundle = new Bundle();
      Bundle region = new Bundle();

      region.putAll(item.getPersistableBundle("region"));
      bundle.putInt("eventType", item.getInt("eventType"));
      bundle.putBundle("region", region);
      if (mTask == null) {
        Log.e(TAG, "--------->mTask is null now, but wasn't when this method was called.");
        return false;
      }
      mTask.execute(bundle, null);
    }
    return true;
  }

I noticed that the member variable mTask can be set to null by didUnregister while didExecuteJob is still doing work so perhaps this is a thread safety issue?
There may be a better way, but adding the null check and early return in the middle of the for loop did stop the crashing for me.

Expected Behavior

The app should not crash when you restart a Geofence task by adding/removing regions, etc.

Actual Behavior

The app can crash when mTask is set to null while didExecuteJob is still doing work.

Reproducible Demo

I am unable to share our actual app, but here's a small app that I've used to reproduce this problem. Clone it, build and run it on your phone as a standalone app, and open and close it in quick succession. Note, that our actual app may run for several hours or even days without encountering this crash, but eventually it happens. Opening and closing quickly (or anything that would call startAsync on the task multiple times in a short amount of time ) seems to be the quickest way to reproduce.

Thanks!

[cruzach]

How many cycles of opening/closing does it usually take for this to crash? I just tried to repro and wasn't able to get the crash to happen

[briefjudofox]

Charlie, thanks for taking a look. It's usually pretty quick for me after opening and closing a few times. I just crashed it several times, but I did have a couple of runs where I opened and closed it for a minute without crashing. Here's a video capture of a typical scenario:
crash3.mp4.zip

Out of curiosity what phone and OS version are you running?

[cruzach]

Pixel, Android 9

[briefjudofox]

Hi Charlie. I assume you weren't able to reproduce on your end? We're still encountering this in our real app. Perhaps calling Location.startGeofencingAsync(TASK_NAME, regions) in a loop with a bunch of regions would help increase the odds of witnessing this crash? When I have some more time I'll try this out in the test app I referenced above.

Either way, can you see how the mTask can be set to null while didExecuteJob is executing?

Thanks again for checking this out.

[vascoconde]

@briefjudofox Did you ever figure out a way to get around this issue?

I just published an app running on SDK 35 and I'm getting a lot of crash reports exactly like yours.

I took the app you created to reproduce this problem, updated it to the latest Expo version and I could still replicate the crash. So the issue still seems unresolved.

[briefjudofox]

Hi @vascoconde, I ended up ejecting and updating didExecuteJob to early return if mTask is null. I'd love to un-eject and get back to a managed build. @cruzach are you able to take another look at this?

Thanks

[cruzach]

Yeah, thanks for the ping. I still haven't been able to repro a crash, but since you said sometimes it takes several hours or days to exhibit the crash, that would explain why I haven't seen it.

@briefjudofox if you would like, you could open a PR or lay out the fix you've implemented to give @tsapeta a clear idea of the issue and your proposed solution

[briefjudofox]

Sounds good @cruzach. I'll try to pull something together soon. In the mean time, @tsapeta , I think my original post here mostly lays out the problem and potential fix so if you have time to take a look and it makes sense to you maybe you're able to make that change? Please lmk if you have other ideas.

[tsapeta]

Hey @briefjudofox,
Yeah, thanks for posting an issue and giving us enough details 😉 I've assigned @Szymon20000 to take care of this as he may have more bandwidth than I to fix this before SDK36 release. We'll keep you posted here 🙂

[briefjudofox]

Thanks @tsapeta & @Szymon20000 ! I appreciate it.

[vascoconde]

Hey @tsapeta & @Szymon20000, thank you so much for taking a look at this issue.
Just to note that @briefjudofox posted some more detailed logs in the original forum thread (https://forums.expo.io/t/android-standalone-app-crashes-with-geofencingtaskconsumer/25733) that might help identify the root cause of the issue. These logs match the ones I get from my app.
I tested the fix proposed above, and it does indeed stop the app from crashing.
Let me know if you're having a hard time reproducing the bug, or if there is anything else I might be able to help with.

[DaviesHuang]

I am also seeing similar issues on Android (expo and standalone) when using Geofencing. Is there a way to fix this/get around this without ejecting at JavaScript level?

[rcabarreto]

I'm also facing similar problems. Any idea how to fix this?

Caused by: java.lang.NullPointerException:
at expo.modules.location.taskConsumers.GeofencingTaskConsumer.didExecuteJob (GeofencingTaskConsumer.java:128)
at expo.modules.taskManager.TaskService.handleJob (TaskService.java:331)
at expo.modules.taskManager.TaskJobService.onStartJob (TaskJobService.java:13)
at android.app.job.JobService$1.onStartJob (JobService.java:62)
at android.app.job.JobServiceEngine$JobHandler.handleMessage (JobServiceEngine.java:108)

[willenglishiv]

same, I'm facing similar problems, any idea of a fix?

[briefjudofox]

FYI @tsapeta @Szymon20000 @cruzach I just created a PR for this. It's the solution outlined above.