expo: NPE Crash in GeofencingTaskConsumer.didExecuteJob()

🐛 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!