Merge pull request #105 from firebase/fix_npe

Fix NPE when jobFinished() was called before onStartJob() returns.

Author: unEgor

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

@@ -194,8 +194,8 @@ public Builder setReplaceCurrent(boolean mReplaceCurrent) {
     }
 
     /**
-     * @return true if the tag, the service, and the trigger of provided {@link JobInvocation} have
-     * the same values.
+     * @return true if the tag and the service of provided {@link JobInvocation} have the same
+     * values.
      */
     @Override
     public boolean equals(Object o) {
@@ -209,15 +209,13 @@ public boolean equals(Object o) {
         JobInvocation jobInvocation = (JobInvocation) o;
 
         return mTag.equals(jobInvocation.mTag)
-                && mService.equals(jobInvocation.mService)
-                && mTrigger.equals(jobInvocation.mTrigger);
+                && mService.equals(jobInvocation.mService);
     }
 
     @Override
     public int hashCode() {
         int result = mTag.hashCode();
         result = 31 * result + mService.hashCode();
-        result = 31 * result + mTrigger.hashCode();
         return result;
     }
 }

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

@@ -23,6 +23,7 @@
 import android.os.IBinder;
 import android.os.Message;
 import android.support.annotation.IntDef;
+import android.support.annotation.MainThread;
 import android.support.annotation.NonNull;
 import android.support.annotation.Nullable;
 import android.support.annotation.VisibleForTesting;
@@ -84,10 +85,16 @@ public abstract class JobService extends Service {
 
     /**
      * The entry point to your Job. Implementations should offload work to another thread of
-     * execution as soon as possible.
+     * execution as soon as possible because this runs on the main thread. If work was offloaded,
+     * call {@link JobService#jobFinished(JobParameters, boolean)} to notify the scheduling service
+     * that the work is completed.
      *
-     * @return whether there is more work remaining.
+     * In order to reschedule use {@link JobService#jobFinished(JobParameters, boolean)}.
+     *
+     * @return {@code true} if there is more work remaining in the worker thread, {@code false} if
+     * the job was completed.
      */
+    @MainThread
     public abstract boolean onStartJob(JobParameters job);
 
     /**
@@ -99,8 +106,10 @@ public abstract class JobService extends Service {
      * @see com.firebase.jobdispatcher.JobInvocation.Builder#setRetryStrategy(RetryStrategy)
      * @see RetryStrategy
      */
+    @MainThread
     public abstract boolean onStopJob(JobParameters job);
 
+    @MainThread
     void start(JobParameters job, Message msg) {
         synchronized (runningJobs) {
             if (runningJobs.containsKey(job.getTag())) {
@@ -112,11 +121,15 @@ void start(JobParameters job, Message msg) {
 
             boolean moreWork = onStartJob(job);
             if (!moreWork) {
-                runningJobs.remove(job.getTag()).sendResult(RESULT_SUCCESS);
+                JobCallback callback = runningJobs.remove(job.getTag());
+                if (callback != null) {
+                    callback.sendResult(RESULT_SUCCESS);
+                }
             }
         }
     }
 
+    @MainThread
     void stop(JobInvocation job) {
         synchronized (runningJobs) {
             JobCallback jobCallback = runningJobs.remove(job.getTag());
@@ -133,7 +146,8 @@ void stop(JobInvocation job) {
     }
 
     /**
-     * Called to indicate that execution of the provided {@code job} has completed.
+     * Callback to inform the scheduling driver that you've finished executing. Can be called from
+     * any thread. When the system receives this message, it will release the wakelock being held.
      *
      * @param job
      * @param needsReschedule whether the job should be rescheduled

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

@@ -72,4 +72,12 @@ public void contract_hashCode_equals() {
         assertNotEquals(jobInvocation, jobInvocationNew);
         assertNotEquals(jobInvocation.hashCode(), jobInvocationNew.hashCode());
     }
+
+    @Test
+    public void contract_hashCode_equals_triggerShouldBeIgnored() {
+        JobInvocation jobInvocation = builder.build();
+        JobInvocation periodic = builder.setTrigger(Trigger.executionWindow(0, 1)).build();
+        assertEquals(jobInvocation, periodic);
+        assertEquals(jobInvocation.hashCode(), periodic.hashCode());
+    }
 }

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

@@ -238,6 +238,71 @@ public void stop_withCallback_done() {
         assertEquals(message.arg1, JobService.RESULT_FAIL_RETRY);
     }
 
+    @Test
+    public void onStartJob_jobFinishedReschedule() {
+        // Verify that a retry request from within onStartJob will cause the retry result to be sent
+        // to the bouncer service's handler, regardless of what value is ultimately returned from
+        // onStartJob.
+        JobService reschedulingService = new JobService() {
+            @Override
+            public boolean onStartJob(JobParameters job) {
+                // Reschedules job.
+                jobFinished(job, true /* retry this job */);
+                return false;
+            }
+
+            @Override
+            public boolean onStopJob(JobParameters job) {
+                return false;
+            }
+        };
+
+        Job jobSpec = TestUtil.getBuilderWithNoopValidator()
+                .setTag("tag")
+                .setService(reschedulingService.getClass())
+                .setTrigger(Trigger.NOW)
+                .build();
+        Handler mock = mock(Handler.class);
+        Message message = new Message();
+        message.setTarget(mock);
+        reschedulingService.start(jobSpec, message);
+
+        verify(mock).sendMessage(message);
+        assertEquals(message.arg1, JobService.RESULT_FAIL_RETRY);
+    }
+
+    @Test
+    public void onStartJob_jobFinishedNotReschedule() {
+        // Verify that a termination request from within onStartJob will cause the result to be sent
+        // to the bouncer service's handler, regardless of what value is ultimately returned from
+        // onStartJob.
+        JobService reschedulingService = new JobService() {
+            @Override
+            public boolean onStartJob(JobParameters job) {
+                jobFinished(job, false /* don't retry this job */);
+                return false;
+            }
+
+            @Override
+            public boolean onStopJob(JobParameters job) {
+                return false;
+            }
+        };
+
+        Job jobSpec = TestUtil.getBuilderWithNoopValidator()
+                .setTag("tag")
+                .setService(reschedulingService.getClass())
+                .setTrigger(Trigger.NOW)
+                .build();
+        Handler mock = mock(Handler.class);
+        Message message = new Message();
+        message.setTarget(mock);
+        reschedulingService.start(jobSpec, message);
+
+        verify(mock).sendMessage(message);
+        assertEquals(message.arg1, JobService.RESULT_SUCCESS);
+    }
+
     public static class ExampleJobService extends JobService {
         @Override
         public boolean onStartJob(JobParameters job) {