Finish background FCM broadcasts after handling the message.

firebase-messaging/CHANGELOG.md

@@ -3,6 +3,11 @@
   [FAQ](https://firebase.google.com/support/faq#fcm-23-deprecation) for more
   details.
 
+* [changed] Changed to finish a background broadcast after the message has been
+  handled, subject to a timeout. This keeps the `FirebaseMessagingService`'s
+  process in an active state while it is handling an FCM message, up to the
+  20 seconds allowed.
+
 # 23.1.2
 * [fixed] Fixed a breakage related to Jetpack core library related to an
   [upstream update](https://android-review.googlesource.com/c/platform/frameworks/support/+/2399893).

firebase-messaging/src/main/java/com/google/firebase/messaging/EnhancedIntentService.java

@@ -36,6 +36,10 @@
  */
 @SuppressLint("UnwrappedWakefulBroadcastReceiver") // Not used within GmsCore
 public abstract class EnhancedIntentService extends Service {
+
+  // Allow apps 20 seconds to handle a message.
+  static final long MESSAGE_TIMEOUT_S = 20;
+
   private static final String TAG = "EnhancedIntentService";
 
   // Use a different thread per service instance, so it can be reclaimed once the service is done.

firebase-messaging/src/main/java/com/google/firebase/messaging/FcmBroadcastProcessor.java

@@ -87,7 +87,7 @@ public Task<Integer> startMessagingService(Context context, Intent intent) {
     boolean isHighPriority = (intent.getFlags() & Intent.FLAG_RECEIVER_FOREGROUND) != 0;
 
     if (subjectToBackgroundCheck && !isHighPriority) {
-      return bindToMessagingService(context, intent);
+      return bindToMessagingService(context, intent, isHighPriority);
     }
 
     // If app isn't subject to background check or if message is high priority, use startService
@@ -106,28 +106,40 @@ public Task<Integer> startMessagingService(Context context, Intent intent) {
           // On O, if we're not able to start the service fall back to binding. This could happen if
           // the app isn't targeting O, but the user manually applied restrictions, or if the temp
           // whitelist has already expired.
-          return bindToMessagingService(context, intent)
+          return bindToMessagingService(context, intent, isHighPriority)
               .continueWith(
                   // ok to use direct executor because we're just immediately returning an int
                   Runnable::run,
                   t -> ServiceStarter.ERROR_ILLEGAL_STATE_EXCEPTION_FALLBACK_TO_BIND);
         });
   }
 
-  private static Task<Integer> bindToMessagingService(Context context, Intent intent) {
+  private static Task<Integer> bindToMessagingService(
+      Context context, Intent intent, boolean isForegroundBroadcast) {
     if (Log.isLoggable(TAG, Log.DEBUG)) {
       Log.d(TAG, "Binding to service");
     }
-    if (ServiceStarter.getInstance().hasWakeLockPermission(context)) {
-      WakeLockHolder.sendWakefulServiceIntent(
-          context, getServiceConnection(context, ServiceStarter.ACTION_MESSAGING_EVENT), intent);
+
+    WithinAppServiceConnection connection =
+        getServiceConnection(context, ServiceStarter.ACTION_MESSAGING_EVENT);
+
+    if (isForegroundBroadcast) {
+      // Foreground broadcast queue, finish the broadcast immediately
+      // (by returning a completed Task) to avoid ANRs.
+      if (ServiceStarter.getInstance().hasWakeLockPermission(context)) {
+        WakeLockHolder.sendWakefulServiceIntent(context, connection, intent);
+      } else {
+        connection.sendIntent(intent);
+      }
+      return Tasks.forResult(ServiceStarter.SUCCESS);
     } else {
-      // Ignore result since we're no longer blocking on the service handling the intent.
-      Task<Void> unused =
-          getServiceConnection(context, ServiceStarter.ACTION_MESSAGING_EVENT).sendIntent(intent);
+      // Background broadcast queue, finish the broadcast after the message has been handled
+      // (which times out after 20 seconds to avoid ANRs and to limit how long the app is active).
+      return connection
+          .sendIntent(intent)
+          // ok to use direct executor because we're just immediately returning an int
+          .continueWith(Runnable::run, t -> ServiceStarter.SUCCESS);
     }
-
-    return Tasks.forResult(ServiceStarter.SUCCESS);
   }
 
   /** Connect to a service via bind. This is used to process intents in Android O+ */
@@ -151,4 +163,16 @@ public static void reset() {
       fcmServiceConn = null;
     }
   }
+
+  /**
+   * Sets WithinAppServiceConnection for testing.
+   *
+   * @hide
+   */
+  @VisibleForTesting
+  public static void setServiceConnection(WithinAppServiceConnection connection) {
+    synchronized (lock) {
+      fcmServiceConn = connection;
+    }
+  }
 }

firebase-messaging/src/main/java/com/google/firebase/messaging/WithinAppServiceConnection.java

@@ -14,7 +14,6 @@
 package com.google.firebase.messaging;
 
 import static com.google.firebase.messaging.FirebaseMessaging.TAG;
-import static com.google.firebase.messaging.WakeLockHolder.WAKE_LOCK_ACQUIRE_TIMEOUT_MILLIS;
 
 import android.annotation.SuppressLint;
 import android.content.ComponentName;
@@ -30,6 +29,7 @@
 import com.google.android.gms.common.util.concurrent.NamedThreadFactory;
 import com.google.android.gms.tasks.Task;
 import com.google.android.gms.tasks.TaskCompletionSource;
+import com.google.errorprone.annotations.CanIgnoreReturnValue;
 import java.util.ArrayDeque;
 import java.util.Queue;
 import java.util.concurrent.ScheduledExecutorService;
@@ -43,10 +43,6 @@
  */
 class WithinAppServiceConnection implements ServiceConnection {
 
-  // Time out WakeLock after 9s to match previous behavior of forcing the broadcast to finish after
-  // that much time.
-  private static final int REQUEST_TIMEOUT_MS = 9000;
-
   static class BindRequest {
     final Intent intent;
     private final TaskCompletionSource<Void> taskCompletionSource = new TaskCompletionSource<>();
@@ -56,22 +52,20 @@ static class BindRequest {
     }
 
     void arrangeTimeout(ScheduledExecutorService executor) {
-      // Allow the same maximum WakeLock duration for high priority messages as when using
-      // startService(), otherwise allow up to 9 seconds to match the time that the service was
-      // allowed to run before finishing the BroadcastReceiver.
-      boolean isHighPriority = (intent.getFlags() & Intent.FLAG_RECEIVER_FOREGROUND) != 0;
+      // Timeout after 20 seconds by finishing the Task. This will finish a background broadcast,
+      // which waits for the message to be handled.
       ScheduledFuture<?> timeoutFuture =
           executor.schedule(
               () -> {
                 Log.w(
                     TAG,
                     "Service took too long to process intent: "
                         + intent.getAction()
-                        + " Releasing WakeLock.");
+                        + " finishing.");
                 finish();
               },
-              isHighPriority ? WAKE_LOCK_ACQUIRE_TIMEOUT_MILLIS : REQUEST_TIMEOUT_MS,
-              TimeUnit.MILLISECONDS);
+              EnhancedIntentService.MESSAGE_TIMEOUT_S,
+              TimeUnit.SECONDS);
 
       getTask()
           .addOnCompleteListener(
@@ -127,6 +121,7 @@ void finish() {
     this.scheduledExecutorService = scheduledExecutorService;
   }
 
+  @CanIgnoreReturnValue
   synchronized Task<Void> sendIntent(Intent intent) {
     if (Log.isLoggable(TAG, Log.DEBUG)) {
       Log.d(TAG, "new intent queued in the bind-strategy delivery");

firebase-messaging/src/test/java/com/google/firebase/messaging/FcmBroadcastProcessorRoboTest.java

@@ -18,14 +18,14 @@
 import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
-import static org.robolectric.Shadows.shadowOf;
 
 import android.app.Application;
 import android.content.Context;
 import android.content.Intent;
 import android.os.Build.VERSION_CODES;
 import androidx.test.core.app.ApplicationProvider;
 import com.google.android.gms.tasks.Task;
+import com.google.android.gms.tasks.TaskCompletionSource;
 import com.google.firebase.messaging.testing.FakeScheduledExecutorService;
 import org.junit.After;
 import org.junit.Before;
@@ -37,6 +37,7 @@
 import org.mockito.junit.MockitoRule;
 import org.robolectric.RobolectricTestRunner;
 import org.robolectric.annotation.Config;
+import org.robolectric.shadows.ShadowLooper;
 import org.robolectric.shadows.ShadowPowerManager;
 
 /** Robolectric test for FcmBroadcastProcessor. */
@@ -51,13 +52,18 @@ public class FcmBroadcastProcessorRoboTest {
   private FcmBroadcastProcessor processor;
   private FakeScheduledExecutorService fakeExecutorService;
   @Mock private ServiceStarter serviceStarter;
+  @Mock private WithinAppServiceConnection mockConnection;
+  private TaskCompletionSource<Void> sendIntentTask;
 
   @Before
   public void setUp() {
     context = ApplicationProvider.getApplicationContext();
     ServiceStarter.setForTesting(serviceStarter);
     fakeExecutorService = new FakeScheduledExecutorService();
     processor = new FcmBroadcastProcessor(context, fakeExecutorService);
+    FcmBroadcastProcessor.setServiceConnection(mockConnection);
+    sendIntentTask = new TaskCompletionSource<>();
+    when(mockConnection.sendIntent(any(Intent.class))).thenReturn(sendIntentTask.getTask());
   }
 
   @After
@@ -70,37 +76,82 @@ public void resetStaticState() {
 
   @Test
   @Config(sdk = VERSION_CODES.O)
-  public void testStartMessagingService_NormalPriorityBackgroundCheck() {
-    // Subject to background check when run on Android O and targetSdkVersion set to O.
-    context.getApplicationInfo().targetSdkVersion = VERSION_CODES.O;
-    when(serviceStarter.hasWakeLockPermission(any(Context.class))).thenReturn(true);
+  public void testStartMessagingService_Background() {
+    setSubjectToBackgroundCheck();
+    setWakeLockPermission(true);
+    Intent intent = new Intent(ACTION_FCM_MESSAGE);
 
-    Task<Integer> startServiceTask =
-        processor.startMessagingService(context, new Intent(ACTION_FCM_MESSAGE));
+    Task<Integer> startServiceTask = processor.startMessagingService(context, intent);
 
-    // Should return immediately with SUCCESS, bind to the Service, and acquire a WakeLock.
-    assertThat(startServiceTask.getResult()).isEqualTo(ServiceStarter.SUCCESS);
+    // Should send the intent through the connection and not acquire a WakeLock.
+    assertThat(startServiceTask.isComplete()).isFalse();
     verify(serviceStarter, never()).startMessagingService(any(), any());
-    assertThat(shadowOf(context).getBoundServiceConnections()).hasSize(1);
+    verify(mockConnection).sendIntent(intent);
+    assertThat(ShadowPowerManager.getLatestWakeLock()).isNull();
+
+    // After the message has been handled, the task should be completed successfully.
+    sendIntentTask.setResult(null);
+    ShadowLooper.idleMainLooper();
+
+    assertThat(startServiceTask.getResult()).isEqualTo(ServiceStarter.SUCCESS);
+  }
+
+  @Test
+  @Config(sdk = VERSION_CODES.O)
+  public void testStartMessagingService_ForegroundBindWithWakeLock() {
+    setWakeLockPermission(true);
+    setStartServiceFails();
+    Intent intent = new Intent(ACTION_FCM_MESSAGE);
+    intent.addFlags(Intent.FLAG_RECEIVER_FOREGROUND);
+
+    Task<Integer> startServiceTask = processor.startMessagingService(context, intent);
+
+    // Should return immediately with SUCCESS, bind to the Service, and acquire a WakeLock.
+    fakeExecutorService.runAll();
+    assertThat(startServiceTask.isComplete()).isTrue();
+    assertThat(startServiceTask.getResult())
+        .isEqualTo(ServiceStarter.ERROR_ILLEGAL_STATE_EXCEPTION_FALLBACK_TO_BIND);
+    verify(mockConnection).sendIntent(any(Intent.class));
     assertThat(ShadowPowerManager.getLatestWakeLock()).isNotNull();
     assertThat(ShadowPowerManager.getLatestWakeLock().isHeld()).isTrue();
+
+    // After the message has been handled, the WakeLock should be released.
+    sendIntentTask.setResult(null);
+    ShadowLooper.idleMainLooper();
+
+    assertThat(ShadowPowerManager.getLatestWakeLock().isHeld()).isFalse();
   }
 
   @Test
   @Config(sdk = VERSION_CODES.O)
-  public void testStartMessagingService_bindNoWakeLockPermission() {
+  public void testStartMessagingService_ForegroundBindNoWakeLock() {
+    setWakeLockPermission(false);
+    setStartServiceFails();
+    Intent intent = new Intent(ACTION_FCM_MESSAGE);
+    intent.addFlags(Intent.FLAG_RECEIVER_FOREGROUND);
+
+    Task<Integer> startServiceTask = processor.startMessagingService(context, intent);
+
+    // Should return immediately with SUCCESS, bind to the Service, and not acquire a WakeLock.
+    fakeExecutorService.runAll();
+    assertThat(startServiceTask.isComplete()).isTrue();
+    assertThat(startServiceTask.getResult())
+        .isEqualTo(ServiceStarter.ERROR_ILLEGAL_STATE_EXCEPTION_FALLBACK_TO_BIND);
+    verify(mockConnection).sendIntent(any(Intent.class));
+    assertThat(ShadowPowerManager.getLatestWakeLock()).isNull();
+  }
+
+  private void setSubjectToBackgroundCheck() {
     // Subject to background check when run on Android O and targetSdkVersion set to O.
     context.getApplicationInfo().targetSdkVersion = VERSION_CODES.O;
-    when(serviceStarter.hasWakeLockPermission(any(Context.class))).thenReturn(false);
+  }
 
-    Task<Integer> startServiceTask =
-        processor.startMessagingService(context, new Intent(ACTION_FCM_MESSAGE));
+  private void setWakeLockPermission(boolean permission) {
+    when(serviceStarter.hasWakeLockPermission(any(Context.class))).thenReturn(permission);
+  }
 
-    // Should return immediately with SUCCESS and bind to the Service, but not try to acquire a
-    // WakeLock since it doesn't hold the permission.
-    assertThat(startServiceTask.getResult()).isEqualTo(ServiceStarter.SUCCESS);
-    verify(serviceStarter, never()).startMessagingService(any(), any());
-    assertThat(shadowOf(context).getBoundServiceConnections()).hasSize(1);
-    assertThat(ShadowPowerManager.getLatestWakeLock()).isNull();
+  private void setStartServiceFails() {
+    when(serviceStarter.startMessagingService(any(Context.class), any(Intent.class)))
+        .thenReturn(ServiceStarter.ERROR_ILLEGAL_STATE_EXCEPTION);
   }
 }

firebase-messaging/src/test/java/com/google/firebase/messaging/WithinAppServiceConnectionRoboTest.java

@@ -44,10 +44,7 @@ public class WithinAppServiceConnectionRoboTest {
   private static final String TEST_BIND_ACTION = "testBindAction";
 
   // The amount of time that a broadcast receiver takes to time out
-  private static final long RECEIVER_TIMEOUT_S = 10;
-
-  // The amount of time to allow a foreground broadcast's service to run.
-  private static final long FOREGROUND_RECEIVER_TIMEOUT_S = 60;
+  private static final long RECEIVER_TIMEOUT_S = 20;
 
   private Application context;
   private FakeScheduledExecutorService fakeExecutor;
@@ -71,27 +68,7 @@ public void testReceiverTimesOut() {
 
     // Check the runnable doesn't run early, and that after it should have run the pending
     // result is finished.
-    fakeExecutor.simulateNormalOperationFor(RECEIVER_TIMEOUT_S - 2, TimeUnit.SECONDS);
-    assertThat(pendingResult.isComplete()).isFalse();
-    fakeExecutor.simulateNormalOperationFor(1, TimeUnit.SECONDS);
-    assertThat(pendingResult.isComplete()).isTrue();
-  }
-
-  @Test
-  public void testReceiverTimesOut_ForegroundBroadcast() {
-    WithinAppServiceConnection connection =
-        new WithinAppServiceConnection(context, TEST_BIND_ACTION, fakeExecutor);
-    setMockBinder(TEST_BIND_ACTION);
-
-    // Send a foreground broadcst intent, verify the pending result isn't finished
-    Intent foregroundBroadcastIntent = new Intent();
-    foregroundBroadcastIntent.addFlags(Intent.FLAG_RECEIVER_FOREGROUND);
-    Task<Void> pendingResult = connection.sendIntent(foregroundBroadcastIntent);
-    assertThat(pendingResult.isComplete()).isFalse();
-
-    // Check the runnable doesn't run early, and that after it should have run the pending
-    // result is finished.
-    fakeExecutor.simulateNormalOperationFor(FOREGROUND_RECEIVER_TIMEOUT_S - 1, TimeUnit.SECONDS);
+    fakeExecutor.simulateNormalOperationFor(RECEIVER_TIMEOUT_S - 1, TimeUnit.SECONDS);
     assertThat(pendingResult.isComplete()).isFalse();
     fakeExecutor.simulateNormalOperationFor(1, TimeUnit.SECONDS);
     assertThat(pendingResult.isComplete()).isTrue();