Download notification: Fix a crash on O+.

When startForeground called with a cancelled notification update,
startForeground() will be bypassed in DownloadForegroundServiceManager.
startOrUpdateForegroundService(). This leads to an Android crash.

Instead we need to create an dummy notification and still call
startForeground() with it to avoid the crash. This dummy notification
will be dropped immediately in DownloadNotificationService.

Bug: 1121096
Change-Id: I0f68bff47403b7478f504af6ee03f154c727c82f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2436511
Commit-Queue: Xing Liu <xingliu@chromium.org>
Reviewed-by: David Trainor <dtrainor@chromium.org>
Cr-Commit-Position: refs/heads/master@{#811871}

chrome/android/java/src/org/chromium/chrome/browser/download/DownloadForegroundServiceManager.java

@@ -11,6 +11,8 @@ import android.content.ComponentName;
 import android.content.Context;
 import android.content.Intent;
 import android.content.ServiceConnection;
+import android.os.Build.VERSION;
+import android.os.Build.VERSION_CODES;
 import android.os.Handler;
 import android.os.IBinder;
 
@@ -20,6 +22,11 @@ import androidx.annotation.VisibleForTesting;
 import org.chromium.base.ContextUtils;
 import org.chromium.base.Log;
 import org.chromium.chrome.browser.download.DownloadNotificationService.DownloadStatus;
+import org.chromium.chrome.browser.notifications.NotificationUmaTracker;
+import org.chromium.chrome.browser.notifications.NotificationWrapperBuilderFactory;
+import org.chromium.chrome.browser.notifications.channels.ChromeChannelDefinitions;
+import org.chromium.components.browser_ui.notifications.NotificationMetadata;
+import org.chromium.components.browser_ui.notifications.NotificationWrapperBuilder;
 
 import java.util.HashMap;
 import java.util.Iterator;
@@ -29,14 +36,14 @@ import java.util.Map;
  * Manager to stop and start the foreground service associated with downloads.
  */
 public class DownloadForegroundServiceManager {
-    private static class DownloadUpdate {
+    protected static class DownloadUpdate {
         int mNotificationId;
         Notification mNotification;
         @DownloadNotificationService.DownloadStatus
         int mDownloadStatus;
         Context mContext;
 
-        DownloadUpdate(int notificationId, Notification notification,
+        DownloadUpdate(int notificationId, @Nullable Notification notification,
                 @DownloadNotificationService.DownloadStatus int downloadStatus, Context context) {
             mNotificationId = notificationId;
             mNotification = notification;
@@ -67,6 +74,11 @@ public class DownloadForegroundServiceManager {
 
     // This is true when context.bindService has been called and before context.unbindService.
     private boolean mIsServiceBound;
+
+    // Whether startForeground() is called. startForeground() must be called in 5 seconds after the
+    // service is started.
+    private boolean mStartForegroundCalled;
+
     // This is non-null when onServiceConnected has been called (aka service is active).
     private DownloadForegroundService mBoundService;
 
@@ -81,9 +93,19 @@ public class DownloadForegroundServiceManager {
 
     public DownloadForegroundServiceManager() {}
 
+    /**
+     * Updates download notification status. In progress notification will have a foreground service
+     * associated. If all notifications are not in progress, foreground service will stop.
+     * @param context Android {@link Context}.
+     * @param downloadStatus Download status. In progress notification will have a foreground
+     *         service.
+     * @param notificationId The notification id.
+     * @param notification The notification associated with the id. Can be null if {@link
+     *         DownloadNotificationService} tries to cancel a notification.
+     */
     public void updateDownloadStatus(Context context,
             @DownloadNotificationService.DownloadStatus int downloadStatus, int notificationId,
-            Notification notification) {
+            @Nullable Notification notification) {
         if (downloadStatus != DownloadNotificationService.DownloadStatus.IN_PROGRESS) {
             Log.w(TAG,
                     "updateDownloadStatus status: " + downloadStatus + ", id: " + notificationId);
@@ -123,8 +145,7 @@ public class DownloadForegroundServiceManager {
         // In the pending case, start foreground with specific notificationId and notification.
         if (isProcessingPending) {
             Log.w(TAG, "Starting service with type " + downloadUpdate.mDownloadStatus);
-            startOrUpdateForegroundService(
-                    downloadUpdate.mNotificationId, downloadUpdate.mNotification);
+            startOrUpdateForegroundService(downloadUpdate);
 
             // Post a delayed task to eventually check to see if service needs to be stopped.
             postMaybeStopServiceRunnable();
@@ -147,8 +168,7 @@ public class DownloadForegroundServiceManager {
         // Make sure the pinned notification is still active, if not, update.
         if (mDownloadUpdateQueue.get(mPinnedNotificationId) == null
                 || !isActive(mDownloadUpdateQueue.get(mPinnedNotificationId).mDownloadStatus)) {
-            startOrUpdateForegroundService(
-                    downloadUpdate.mNotificationId, downloadUpdate.mNotification);
+            startOrUpdateForegroundService(downloadUpdate);
         }
 
         // Clear out inactive download updates in queue if there is at least one active download.
@@ -195,6 +215,7 @@ public class DownloadForegroundServiceManager {
     void startAndBindService(Context context) {
         Log.w(TAG, "startAndBindService");
         mIsServiceBound = true;
+        mStartForegroundCalled = false;
         startAndBindServiceInternal(context);
     }
 
@@ -231,8 +252,21 @@ public class DownloadForegroundServiceManager {
     /** Helper code to start or update foreground service. */
 
     @VisibleForTesting
-    void startOrUpdateForegroundService(int notificationId, Notification notification) {
-        Log.w(TAG, "startOrUpdateForegroundService id: " + notificationId);
+    void startOrUpdateForegroundService(DownloadUpdate update) {
+        Log.w(TAG, "startOrUpdateForegroundService id: " + update.mNotificationId);
+
+        int notificationId = update.mNotificationId;
+        Notification notification = update.mNotification;
+
+        // On O+, we must call startForeground or Android will crash. If the last update
+        // is DownloadStatus.CANCELLED, then create an empty notification. See crbug.com/1121096.
+        // Notices the empty notification will be cancelled immediately in
+        // DownloadNotificationService afterward.
+        if (VERSION.SDK_INT >= VERSION_CODES.O && notification == null && !mStartForegroundCalled) {
+            assert update.mDownloadStatus == DownloadStatus.CANCELLED;
+            notification = createEmptyNotification(notificationId, update.mContext);
+        }
+
         if (mBoundService != null && notificationId != INVALID_NOTIFICATION_ID
                 && notification != null) {
             // If there was an originally pinned notification, get its id and notification.
@@ -246,12 +280,25 @@ public class DownloadForegroundServiceManager {
             // Start service and handle notifications.
             mBoundService.startOrUpdateForegroundService(notificationId, notification,
                     mPinnedNotificationId, oldNotification, killOldNotification);
+            mStartForegroundCalled = true;
 
             // After the service has been started and the notification handled, change stored id.
             mPinnedNotificationId = notificationId;
         }
     }
 
+    // Creates an empty notification to feed to startForeground().
+    private Notification createEmptyNotification(int notificationId, Context context) {
+        NotificationWrapperBuilder builder =
+                NotificationWrapperBuilderFactory.createNotificationWrapperBuilder(
+                        true /* preferCompat */, ChromeChannelDefinitions.ChannelId.DOWNLOADS,
+                        null /* remoteAppPackageName */,
+                        new NotificationMetadata(
+                                NotificationUmaTracker.SystemNotificationType.DOWNLOAD_FILES, null,
+                                notificationId));
+        return builder.build();
+    }
+
     /** Helper code to stop and unbind service. */
 
     @VisibleForTesting

chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java

@@ -286,9 +286,9 @@ public class DownloadNotificationService {
                 mDownloadSharedPreferenceHelper.getDownloadSharedPreferenceEntry(id);
         if (entry == null) return;
 
-        cancelNotification(entry.notificationId, id);
         mDownloadForegroundServiceManager.updateDownloadStatus(ContextUtils.getApplicationContext(),
                 DownloadStatus.CANCELLED, entry.notificationId, null);
+        cancelNotification(entry.notificationId, id);
     }
 
     /**

chrome/android/javatests/src/org/chromium/chrome/browser/download/DownloadForegroundServiceManagerTest.java

@@ -79,9 +79,9 @@ public final class DownloadForegroundServiceManagerTest {
         }
 
         @Override
-        void startOrUpdateForegroundService(int notificationId, Notification notification) {
-            mUpdatedNotificationId = notificationId;
-            super.startOrUpdateForegroundService(notificationId, notification);
+        void startOrUpdateForegroundService(DownloadUpdate downloadUpdate) {
+            mUpdatedNotificationId = downloadUpdate.mNotificationId;
+            super.startOrUpdateForegroundService(downloadUpdate);
         }
 
         // Skip waiting for delayed runnable in tests.