Merge branch 'master' into dconeybe/BloomFilterUnicodeTestSimplification

Author: dconeybe

firebase-crashlytics-ndk/gradle.properties

@@ -1,2 +1,2 @@
-version=18.3.8
+version=18.4.0
 latestReleasedVersion=18.3.7

firebase-crashlytics/gradle.properties

@@ -1,2 +1,2 @@
-version=18.3.8
+version=18.4.0
 latestReleasedVersion=18.3.7

firebase-messaging-directboot/gradle.properties

@@ -1,3 +1,3 @@
-version=23.1.3
+version=23.2.0
 latestReleasedVersion=23.1.2
 android.enableUnitTestBinaryResources=true

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/gradle.properties

@@ -1,3 +1,3 @@
-version=23.1.3
+version=23.2.0
 latestReleasedVersion=23.1.2
 android.enableUnitTestBinaryResources=true

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/FirebaseMessaging.java

@@ -65,17 +65,6 @@
  * <p>In order to receive Firebase messages, declare an implementation of <br>
  * {@link FirebaseMessagingService} in the app manifest. To process messages, override base class
  * methods to handle any events required by the application.
- *
- * <p>Client apps can send <a
- * href="https://firebase.google.com/docs/cloud-messaging/android/upstream">upstream messages</a>
- * back to the app server using the XMPP-based Cloud Connection Server. For example:
- *
- * <pre>
- * FirebaseMessaging.getInstance().send(
- *     new RemoteMessage.Builder(SENDER_ID + "&#64;fcm.googleapis.com")
- *     .setMessageId(id)
- *     .addData("key", "value")
- *     .build());</pre>
  */
 public class FirebaseMessaging {
 
@@ -447,7 +436,7 @@ public Task<Void> deleteToken() {
    * <p>The subscribe operation is persisted and will be retried until successful.
    *
    * <p>This uses an FCM registration token to identify the app instance, generating one if it does
-   * not exist (see {@link #getToken()}), which perodically sends data to the Firebase backend when
+   * not exist (see {@link #getToken()}), which periodically sends data to the Firebase backend when
    * auto-init is enabled. To delete the data, delete the token ({@link #deleteToken}) and the
    * Firebase Installations ID ({@code FirebaseInstallations.delete()}). To stop the periodic
    * sending of data, disable auto-init ({@link #setAutoInitEnabled}).
@@ -487,9 +476,9 @@ public Task<Void> unsubscribeFromTopic(@NonNull String topic) {
    * <p>When there is an active connection the message will be sent immediately, otherwise the
    * message will be queued up to the time to live (TTL) set in the message.
    *
-   * @deprecated FCM upstream is deprecated and will be decommissioned in June 2024. Learn more in
-   *     the <a href="https://firebase.google.com/support/faq#fcm-23-deprecation">FAQ about FCM
-   *     features deprecated in June 2023</a>.
+   * @deprecated FCM upstream messaging is deprecated and will be decommissioned in June 2024. Learn
+   *     more in the <a href="https://firebase.google.com/support/faq#fcm-23-deprecation">FAQ about
+   *     FCM features deprecated in June 2023</a>.
    */
   @Deprecated
   public void send(@NonNull RemoteMessage message) {

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

@@ -85,6 +85,9 @@ public class FirebaseMessagingService extends EnhancedIntentService {
   /**
    * Called when a message is received.
    *
+   * <p>This should complete within 20 seconds. Taking longer may interfere with your ability to
+   * complete your work and may affect pending messages.
+   *
    * <p>This is also called when a notification message is received while the app is in the
    * foreground. The notification parameters can be retrieved with {@link
    * RemoteMessage#getNotification()}.
@@ -143,7 +146,6 @@ public void onSendError(@NonNull String msgId, @NonNull Exception exception) {}
    */
   @WorkerThread
   public void onNewToken(@NonNull String token) {}
-  ;
 
   /** @hide */
   @Override

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);
   }
 }