Bug 1025937 - Silently drop null intents in background IntentServices. r=mcomella, a=sledru
authorRichard Newman <rnewman@mozilla.com>
Mon, 16 Jun 2014 16:18:59 -0700
changeset 199525 ae79355021e505aac2a18bdeb0bffe1b5f3fb640
parent 199524 e64edb137dca260e46b08633b56fda64863f807f
child 199526 a076e4f9d95d0611737fd21d4e672038c4b45c1e
push id3654
push userryanvm@gmail.com
push dateWed, 18 Jun 2014 19:27:56 +0000
treeherdermozilla-beta@a076e4f9d95d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmcomella, sledru
bugs1025937
milestone31.0
Bug 1025937 - Silently drop null intents in background IntentServices. r=mcomella, a=sledru
mobile/android/base/background/announcements/AnnouncementsBroadcastService.java
mobile/android/base/background/announcements/AnnouncementsService.java
mobile/android/base/background/healthreport/HealthReportBroadcastService.java
mobile/android/base/background/healthreport/prune/HealthReportPruneService.java
mobile/android/base/background/healthreport/upload/HealthReportUploadService.java
mobile/android/base/fxa/receivers/FxAccountDeletedService.java
mobile/android/base/sync/receivers/SyncAccountDeletedService.java
mobile/android/tests/background/junit3/src/healthreport/prune/TestHealthReportPruneService.java
--- a/mobile/android/base/background/announcements/AnnouncementsBroadcastService.java
+++ b/mobile/android/base/background/announcements/AnnouncementsBroadcastService.java
@@ -114,16 +114,22 @@ public class AnnouncementsBroadcastServi
   public static void setPollInterval(final Context context, long interval) {
     final SharedPreferences preferences = getSharedPreferences(context);
     preferences.edit().putLong(AnnouncementsConstants.PREF_ANNOUNCE_FETCH_INTERVAL_MSEC, interval).commit();
   }
 
   @Override
   protected void onHandleIntent(Intent intent) {
     Logger.setThreadLogTag(AnnouncementsConstants.GLOBAL_LOG_TAG);
+
+    // Intent can be null. Bug 1025937.
+    if (intent == null) {
+        Logger.debug(LOG_TAG, "Short-circuiting on null intent.");
+    }
+
     final String action = intent.getAction();
     Logger.debug(LOG_TAG, "Broadcast onReceive. Intent is " + action);
 
     if (AnnouncementsConstants.ACTION_ANNOUNCEMENTS_PREF.equals(action)) {
       handlePrefIntent(intent);
       return;
     }
 
@@ -138,18 +144,20 @@ public class AnnouncementsBroadcastServi
     // Failure case.
     Logger.warn(LOG_TAG, "Unknown intent " + action);
   }
 
   /**
    * Handle the intent sent by the browser when it wishes to notify us
    * of the value of the user preference. Look at the value and toggle the
    * alarm service accordingly.
+   *
+   * @param intent must be non-null.
    */
-  protected void handlePrefIntent(Intent intent) {
+  private void handlePrefIntent(Intent intent) {
     if (!intent.hasExtra("enabled")) {
       Logger.warn(LOG_TAG, "Got ANNOUNCEMENTS_PREF intent without enabled. Ignoring.");
       return;
     }
 
     final boolean enabled = intent.getBooleanExtra("enabled", true);
     Logger.debug(LOG_TAG, intent.getStringExtra("branch") + "/" +
                           intent.getStringExtra("pref")   + " = " +
--- a/mobile/android/base/background/announcements/AnnouncementsService.java
+++ b/mobile/android/base/background/announcements/AnnouncementsService.java
@@ -112,16 +112,22 @@ public class AnnouncementsService extend
    * If it's time to do a fetch -- we've waited long enough,
    * we're allowed to use background data, etc. -- then issue
    * a fetch. The subsequent background check is handled implicitly
    * by the AlarmManager.
    */
   @Override
   public void onHandleIntent(Intent intent) {
     Logger.setThreadLogTag(AnnouncementsConstants.GLOBAL_LOG_TAG);
+
+    // Intent can be null. Bug 1025937.
+    if (intent == null) {
+        Logger.debug(LOG_TAG, "Short-circuiting on null intent.");
+    }
+
     Logger.debug(LOG_TAG, "Running AnnouncementsService.");
 
     if (AnnouncementsConstants.DISABLED) {
       Logger.debug(LOG_TAG, "Announcements disabled. Returning from AnnouncementsService.");
       return;
     }
 
     if (!shouldFetchAnnouncements()) {
--- a/mobile/android/base/background/healthreport/HealthReportBroadcastService.java
+++ b/mobile/android/base/background/healthreport/HealthReportBroadcastService.java
@@ -95,29 +95,36 @@ public class HealthReportBroadcastServic
     final long pollInterval = getSubmissionPollInterval();
     scheduleAlarm(pollInterval, pending);
   }
 
   @Override
   protected void onHandleIntent(Intent intent) {
     Logger.setThreadLogTag(HealthReportConstants.GLOBAL_LOG_TAG);
 
+    // Intent can be null. Bug 1025937.
+    if (intent == null) {
+        Logger.debug(LOG_TAG, "Short-circuiting on null intent.");
+    }
+
     // The same intent can be handled by multiple methods so do not short-circuit evaluate.
     boolean handled = attemptHandleIntentForUpload(intent);
     handled = attemptHandleIntentForPrune(intent) ? true : handled;
 
     if (!handled) {
       Logger.warn(LOG_TAG, "Unhandled intent with action " + intent.getAction() + ".");
     }
   }
 
   /**
    * Attempts to handle the given intent for FHR document upload. If it cannot, false is returned.
+   *
+   * @param intent must be non-null.
    */
-  protected boolean attemptHandleIntentForUpload(final Intent intent) {
+  private boolean attemptHandleIntentForUpload(final Intent intent) {
     if (HealthReportConstants.UPLOAD_FEATURE_DISABLED) {
       Logger.debug(LOG_TAG, "Health report upload feature is compile-time disabled; not handling intent.");
       return false;
     }
 
     final String action = intent.getAction();
     Logger.debug(LOG_TAG, "Health report upload feature is compile-time enabled; attempting to " +
         "handle intent with action " + action + ".");
@@ -137,18 +144,20 @@ public class HealthReportBroadcastServic
 
     return false;
   }
 
   /**
    * Handle the intent sent by the browser when it wishes to notify us
    * of the value of the user preference. Look at the value and toggle the
    * alarm service accordingly.
+   *
+   * @param intent must be non-null.
    */
-  protected void handleUploadPrefIntent(Intent intent) {
+  private void handleUploadPrefIntent(Intent intent) {
     if (!intent.hasExtra("enabled")) {
       Logger.warn(LOG_TAG, "Got " + HealthReportConstants.ACTION_HEALTHREPORT_UPLOAD_PREF + " intent without enabled. Ignoring.");
       return;
     }
 
     final boolean enabled = intent.getBooleanExtra("enabled", true);
     Logger.debug(LOG_TAG, intent.getStringExtra("branch") + "/" +
                           intent.getStringExtra("pref")   + " = " +
@@ -189,18 +198,20 @@ public class HealthReportBroadcastServic
     // The user can toggle us off or on, or we can have obsolete documents to
     // remove.
     final boolean serviceEnabled = hasObsoleteIds || enabled;
     toggleSubmissionAlarm(this, profileName, profilePath, enabled, serviceEnabled);
   }
 
   /**
    * Attempts to handle the given intent for FHR data pruning. If it cannot, false is returned.
+   *
+   * @param intent must be non-null.
    */
-  protected boolean attemptHandleIntentForPrune(final Intent intent) {
+  private boolean attemptHandleIntentForPrune(final Intent intent) {
     final String action = intent.getAction();
     Logger.debug(LOG_TAG, "Prune: Attempting to handle intent with action, " + action + ".");
 
     if (HealthReportConstants.ACTION_HEALTHREPORT_PRUNE.equals(action)) {
       handlePruneIntent(intent);
       return true;
     }
 
@@ -210,17 +221,20 @@ public class HealthReportBroadcastServic
           GlobalConstants.GECKO_PREFERENCES_CLASS,
           GlobalConstants.GECKO_BROADCAST_HEALTHREPORT_PRUNE_METHOD);
       return true;
     }
 
     return false;
   }
 
-  protected void handlePruneIntent(final Intent intent) {
+  /**
+   * @param intent must be non-null.
+   */
+  private void handlePruneIntent(final Intent intent) {
     final String profileName = intent.getStringExtra("profileName");
     final String profilePath = intent.getStringExtra("profilePath");
 
     if (profileName == null || profilePath == null) {
       Logger.warn(LOG_TAG, "Got " + HealthReportConstants.ACTION_HEALTHREPORT_PRUNE + " intent " +
           "without profilePath or profileName. Ignoring.");
       return;
     }
--- a/mobile/android/base/background/healthreport/prune/HealthReportPruneService.java
+++ b/mobile/android/base/background/healthreport/prune/HealthReportPruneService.java
@@ -34,16 +34,22 @@ public class HealthReportPruneService ex
 
   protected SharedPreferences getSharedPreferences() {
     return this.getSharedPreferences(HealthReportConstants.PREFS_BRANCH, GlobalConstants.SHARED_PREFERENCES_MODE);
   }
 
   @Override
   public void onHandleIntent(Intent intent) {
     Logger.setThreadLogTag(HealthReportConstants.GLOBAL_LOG_TAG);
+
+    // Intent can be null. Bug 1025937.
+    if (intent == null) {
+        Logger.debug(LOG_TAG, "Short-circuiting on null intent.");
+    }
+
     Logger.debug(LOG_TAG, "Handling prune intent.");
 
     if (!isIntentValid(intent)) {
       Logger.warn(LOG_TAG, "Intent not valid - returning.");
       return;
     }
 
     final String profileName = intent.getStringExtra("profileName");
@@ -54,17 +60,21 @@ public class HealthReportPruneService ex
   }
 
   // Generator function wraps constructor for testing purposes.
   protected PrunePolicy getPrunePolicy(final String profilePath) {
     final PrunePolicyStorage storage = new PrunePolicyDatabaseStorage(this, profilePath);
     return new PrunePolicy(storage, getSharedPreferences());
   }
 
-  protected boolean isIntentValid(final Intent intent) {
+  /**
+   * @param intent must be non-null.
+   * @return true if the supplied intent contains both profileName and profilePath.
+   */
+  private static boolean isIntentValid(final Intent intent) {
     boolean isValid = true;
 
     final String profileName = intent.getStringExtra("profileName");
     if (profileName == null) {
       Logger.warn(LOG_TAG, "Got intent without profileName.");
       isValid = false;
     }
 
--- a/mobile/android/base/background/healthreport/upload/HealthReportUploadService.java
+++ b/mobile/android/base/background/healthreport/upload/HealthReportUploadService.java
@@ -38,16 +38,21 @@ public class HealthReportUploadService e
   protected SharedPreferences getSharedPreferences() {
     return this.getSharedPreferences(HealthReportConstants.PREFS_BRANCH, GlobalConstants.SHARED_PREFERENCES_MODE);
   }
 
   @Override
   public void onHandleIntent(Intent intent) {
     Logger.setThreadLogTag(HealthReportConstants.GLOBAL_LOG_TAG);
 
+    // Intent can be null. Bug 1025937.
+    if (intent == null) {
+        Logger.debug(LOG_TAG, "Short-circuiting on null intent.");
+    }
+
     if (HealthReportConstants.UPLOAD_FEATURE_DISABLED) {
       Logger.debug(LOG_TAG, "Health report upload feature is compile-time disabled; not handling upload intent.");
       return;
     }
 
     Logger.debug(LOG_TAG, "Health report upload feature is compile-time enabled; handling upload intent.");
 
     String profileName = intent.getStringExtra("profileName");
--- a/mobile/android/base/fxa/receivers/FxAccountDeletedService.java
+++ b/mobile/android/base/fxa/receivers/FxAccountDeletedService.java
@@ -24,16 +24,21 @@ public class FxAccountDeletedService ext
   public static final String LOG_TAG = FxAccountDeletedService.class.getSimpleName();
 
   public FxAccountDeletedService() {
     super(LOG_TAG);
   }
 
   @Override
   protected void onHandleIntent(final Intent intent) {
+    // Intent can, in theory, be null. Bug 1025937.
+    if (intent == null) {
+        Logger.debug(LOG_TAG, "Short-circuiting on null intent.");
+    }
+
     final Context context = this;
 
     long intentVersion = intent.getLongExtra(
         FxAccountConstants.ACCOUNT_DELETED_INTENT_VERSION_KEY, 0);
     long expectedVersion = FxAccountConstants.ACCOUNT_DELETED_INTENT_VERSION;
     if (intentVersion != expectedVersion) {
       Logger.warn(LOG_TAG, "Intent malformed: version " + intentVersion + " given but " +
           "version " + expectedVersion + "expected. Not cleaning up after deleted Account.");
--- a/mobile/android/base/sync/receivers/SyncAccountDeletedService.java
+++ b/mobile/android/base/sync/receivers/SyncAccountDeletedService.java
@@ -27,16 +27,21 @@ public class SyncAccountDeletedService e
   public static final String LOG_TAG = "SyncAccountDeletedService";
 
   public SyncAccountDeletedService() {
     super(LOG_TAG);
   }
 
   @Override
   protected void onHandleIntent(Intent intent) {
+    // Intent can, in theory, be null. Bug 1025937.
+    if (intent == null) {
+      Logger.debug(LOG_TAG, "Short-circuiting on null intent.");
+    }
+
     final Context context = this;
 
     long intentVersion = intent.getLongExtra(Constants.JSON_KEY_VERSION, 0);
     long expectedVersion = SyncConstants.SYNC_ACCOUNT_DELETED_INTENT_VERSION;
     if (intentVersion != expectedVersion) {
       Logger.warn(LOG_TAG, "Intent malformed: version " + intentVersion + " given but version " + expectedVersion + "expected. " +
           "Not cleaning up after deleted Account.");
       return;
--- a/mobile/android/tests/background/junit3/src/healthreport/prune/TestHealthReportPruneService.java
+++ b/mobile/android/tests/background/junit3/src/healthreport/prune/TestHealthReportPruneService.java
@@ -31,21 +31,16 @@ public class TestHealthReportPruneServic
       } catch (InterruptedException e) {
         fail("Awaiting thread should not be interrupted.");
       } catch (BrokenBarrierException e) {
         // This will happen on timeout - do nothing.
       }
     }
 
     @Override
-    public boolean isIntentValid(final Intent intent) {
-      return super.isIntentValid(intent);
-    }
-
-    @Override
     public PrunePolicy getPrunePolicy(final String profilePath) {
       final PrunePolicyStorage storage = new PrunePolicyDatabaseStorage(new MockContext(), profilePath);
       prunePolicy = new MockPrunePolicy(storage, getSharedPreferences());
       return prunePolicy;
     }
 
     public boolean wasTickCalled() {
       if (prunePolicy == null) {