Remove callback onUnbind, fix NPE in sendResult

There are 4 places where the callbacks in JobService#runningJobs are
accessed:

 * start, where we initially stash the JobCallback

 * jobFinished, where we remove the JobCallback and use it to send the
   result

 * stop, where we remove the JobCallback and use it to send the result

 * onUnbind, where we weren't removing the JobCallback but were using it
   to send the result

The problem is that JobCallback uses a provided Message to transmit results,
which is immediately recycled by the system after being sent. Users were
experiencing NPEs where onUnbind ran first, using the Message but leaving it in
the runningJobs collection. Future calls to jobFinished would try and send the
same Message after it had already been used.

The fix was to make sure that onUnbind removes the JobCallback from the
collection. This should solve GitHub issues #109 and #139.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=167070874

jobdispatcher/src/main/java/com/firebase/jobdispatcher/JobService.java

@@ -186,14 +186,10 @@ public final IBinder onBind(Intent intent) {
   public final boolean onUnbind(Intent intent) {
     synchronized (runningJobs) {
       for (int i = runningJobs.size() - 1; i >= 0; i--) {
-        JobCallback callback = runningJobs.get(runningJobs.keyAt(i));
+        JobCallback callback = runningJobs.remove(runningJobs.keyAt(i));
         if (callback != null) {
-          callback.sendResult(
-              onStopJob((JobParameters) callback.message.obj)
-                  // returned true, would like to be rescheduled
-                  ? RESULT_FAIL_RETRY
-                  // returned false, but was interrupted so consider it a fail
-                  : RESULT_FAIL_NORETRY);
+          boolean shouldRetry = onStopJob((JobParameters) callback.message.obj);
+          callback.sendResult(shouldRetry ? RESULT_FAIL_RETRY : RESULT_FAIL_NORETRY);
         }
       }
     }

jobdispatcher/src/test/java/com/firebase/jobdispatcher/JobServiceTest.java

@@ -19,8 +19,10 @@
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
+import static org.mockito.Mockito.any;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.reset;
 import static org.mockito.Mockito.spy;
 import static org.mockito.Mockito.verify;
 
@@ -39,6 +41,7 @@
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
+import org.mockito.ArgumentCaptor;
 import org.robolectric.RobolectricTestRunner;
 import org.robolectric.annotation.Config;
 
@@ -313,6 +316,70 @@ public boolean onStopJob(JobParameters job) {
     assertEquals(JobService.RESULT_SUCCESS, message.arg1);
   }
 
+  @Test
+  public void onUnbind_removesUsedCallbacks_withBackgroundWork() throws Exception {
+    verifyOnUnbindCausesResult(
+        new JobService() {
+          @Override
+          public boolean onStartJob(JobParameters job) {
+            return true; // More work to do in background
+          }
+
+          @Override
+          public boolean onStopJob(JobParameters job) {
+            return true; // Still doing background work
+          }
+        },
+        JobService.RESULT_FAIL_RETRY);
+  }
+
+  @Test
+  public void onUnbind_removesUsedCallbacks_noBackgroundWork() throws Exception {
+    verifyOnUnbindCausesResult(
+        new JobService() {
+          @Override
+          public boolean onStartJob(JobParameters job) {
+            return true; // more work to do in background
+          }
+
+          @Override
+          public boolean onStopJob(JobParameters job) {
+            return false; // Done with background work
+          }
+        },
+        JobService.RESULT_FAIL_NORETRY);
+  }
+
+  private static void verifyOnUnbindCausesResult(JobService service, int expectedResult)
+      throws Exception {
+    Handler mock = mock(Handler.class);
+    Message msg = new Message();
+    msg.setTarget(mock);
+
+    Job jobSpec =
+        TestUtil.getBuilderWithNoopValidator()
+            .setTag("tag")
+            .setService(service.getClass())
+            .setTrigger(Trigger.NOW)
+            .build();
+
+    // start the service
+    service.start(jobSpec, msg);
+    // shouldn't have sent a result message yet (still doing background work)
+    verify(mock, never()).sendMessage(msg);
+    // manually trigger the onUnbind hook
+    service.onUnbind(new Intent());
+
+    ArgumentCaptor<Message> msgCaptor = ArgumentCaptor.forClass(Message.class);
+    verify(mock).sendMessage(msgCaptor.capture());
+    assertEquals(expectedResult, msgCaptor.getValue().arg1);
+
+    // Calling jobFinished should not attempt to send a second message
+    reset(mock);
+    service.jobFinished(jobSpec, false);
+    verify(mock, never()).sendMessage(any(Message.class));
+  }
+
   /** A simple JobService that just counts down the {@link #countDownLatch}. */
   public static class ExampleJobService extends JobService {
     @Override