Fix DownloadNotificationService foreground state

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

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 package org.chromium.chrome.browser.download;
 
 import android.annotation.TargetApi;
 import android.app.DownloadManager;
 import android.app.Notification;
 import android.app.NotificationManager;
@@ skipping 196 matching lines @@
      */
     public class LocalBinder extends Binder {
         DownloadNotificationService getService() {
             return DownloadNotificationService.this;
         }
     }
 
     @Override
     public void onTaskRemoved(Intent rootIntent) {
         super.onTaskRemoved(rootIntent);
-        // If we've lost all Activities, cancel the off the record downloads.
+        // If we've lost all Activities, cancel the off the record downloads and validate that we
+        // should still be showing any download notifications at all.
         if (ApplicationStatus.isEveryActivityDestroyed()) {
             cancelOffTheRecordDownloads();
-            hideSummaryNotificationIfNecessary();
+            hideSummaryNotificationIfNecessary(null);
         }
     }
 
     @Override
     public void onCreate() {
         mContext = ContextUtils.getApplicationContext();
         mNotificationManager = (NotificationManager) mContext.getSystemService(
                 Context.NOTIFICATION_SERVICE);
         mSharedPrefs = ContextUtils.getAppSharedPreferences();
         mNumAutoResumptionAttemptLeft = mSharedPrefs.getInt(KEY_AUTO_RESUMPTION_ATTEMPT_LEFT,
                 MAX_RESUMPTION_ATTEMPT_LEFT);
         mDownloadSharedPreferenceHelper = DownloadSharedPreferenceHelper.getInstance();
         mNextNotificationId = mSharedPrefs.getInt(
                 KEY_NEXT_DOWNLOAD_NOTIFICATION_ID, STARTING_NOTIFICATION_ID);
     }
 
     @Override
     public void onDestroy() {
         updateNotificationsForShutdown();
         rescheduleDownloads();
         super.onDestroy();
     }
 
     @Override
     public int onStartCommand(final Intent intent, int flags, int startId) {
         if (intent == null) {
+            // Intent is only null during a process restart because of returning START_STICKY.  In
+            // this case cancel the off the record notifications and put the normal notifications
+            // into a pending state, then try to restart.  Finally validate that we are actually
+            // showing something.
             updateNotificationsForShutdown();
             handleDownloadOperation(
                     new Intent(DownloadNotificationService.ACTION_DOWNLOAD_RESUME_ALL));
-            hideSummaryNotificationIfNecessary();
+            hideSummaryNotificationIfNecessary(null);
         } else if (isDownloadOperationIntent(intent)) {
             handleDownloadOperation(intent);
             DownloadResumptionScheduler.getDownloadResumptionScheduler(mContext).cancelTask();
             // Limit the number of auto resumption attempts in case Chrome falls into a vicious
             // cycle.
             if (ACTION_DOWNLOAD_RESUME_ALL.equals(intent.getAction())) {
                 if (mNumAutoResumptionAttemptLeft > 0) {
                     mNumAutoResumptionAttemptLeft--;
                     updateResumptionAttemptLeft();
                 }
@@ skipping 18 matching lines @@
 
     /**
      * Removes {@code observer}, which will no longer be notified when this class decides to start
      * stopping itself.
      */
     public void removeObserver(Observer observer) {
         mObservers.removeObserver(observer);
     }
 
     /**
-     * Called when browser is killed. Schedule a resumption task and pause all the download
-     * notifications.
+     * On >= O Android releases, puts this service into a background state.
+     * @param killNotification Whether or not this call should kill the summary notification or not.
+     *                         Not killing it puts the service into the background, but leaves the
+     *                         download notifications visible.
      */
     @VisibleForTesting
-    void shutdownService() {
-        stopForeground();
-        for (Observer observer : mObservers) observer.onServiceShutdownRequested();
-        stopSelf();
+    @TargetApi(Build.VERSION_CODES.N)
+    void stopForegroundInteral(boolean killNotification) {

[qinmin, 2017/03/05 06:09:29]

nit:s/Interal/Internal/

[David Trainor- moved to gerrit, 2017/03/06 18:56:08]

Done.

+        if (!BuildInfo.isAtLeastO()) return;
+        stopForeground(killNotification ? STOP_FOREGROUND_REMOVE : STOP_FOREGROUND_DETACH);
     }
 
-    private void stopForegroundIfNecessary() {
-        if (mDownloadsInProgress.size() == 0) stopForeground();
-    }
-
+    /**
+     * On >= O Android releases, puts this service into a foreground state, binding it to the
+     * {@link Notification} generated by {@link #getSummaryNotification(Context)}.
+     */
     @VisibleForTesting
-    @TargetApi(Build.VERSION_CODES.N)
-    void stopForeground() {
-        if (!BuildInfo.isAtLeastO()) return;
-        stopForeground(STOP_FOREGROUND_DETACH);
-    }
-
-    @VisibleForTesting
-    void startForeground() {
+    void startForegroundInternal() {
         if (!BuildInfo.isAtLeastO()) return;
         Notification notification = getSummaryNotification(getApplicationContext());
         startForeground(NotificationConstants.NOTIFICATION_ID_DOWNLOAD_SUMMARY, notification);
     }
 
     private void rescheduleDownloads() {
         List<DownloadSharedPreferenceEntry> entries = mDownloadSharedPreferenceHelper.getEntries();
         if (entries.isEmpty()) return;
 
         boolean scheduleAutoResumption = false;
@@ skipping 40 matching lines @@
             if (cancelActualDownload) {
                 DownloadServiceDelegate delegate = getServiceDelegate(entry.itemType);
                 delegate.cancelDownload(entry.downloadGuid, entry.isOffTheRecord, false);
                 delegate.destroyServiceDelegate();
             }
             for (Observer observer : mObservers) observer.onDownloadCanceled(entry.downloadGuid);
         }
     }
 
     /**
-     * Track in-progress downloads here and, if on an Android version greater or equal to O, make
+     * Track in-progress downloads here and, if on an Android version >= O, make
      * this a foreground service.
      * @param downloadGuid The guid of the download that has been started and should be tracked.
      */
     private void startTrackingInProgressDownload(String downloadGuid) {
-        if (mDownloadsInProgress.size() == 0) startForeground();
+        if (mDownloadsInProgress.size() == 0) startForegroundInternal();
         if (!mDownloadsInProgress.contains(downloadGuid)) mDownloadsInProgress.add(downloadGuid);
     }
 
     /**
-     * Stop tracking the download represented by {@code downloadGuid}.  If on an Android version
-     * greater or equal to O, stop making this a foreground service.
+     * Stop tracking the download represented by {@code downloadGuid}.  If on an Android version >=
+     * O, stop making this a foreground service.
      * @param downloadGuid The guid of the download that has been paused or canceled and shouldn't
      *                     be tracked.
+     * @param allowStopForeground Whether or not this should check internal state and stop the
+     *                            foreground notification from showing.  This could be false if we
+     *                            plan on removing the notification in the near future.  We don't
+     *                            want to just detach here, because that will put us in a
+     *                            potentially bad state where we cannot dismiss the notification.
      */
-    private void stopTrackingInProgressDownload(String downloadGuid) {
+    private void stopTrackingInProgressDownload(String downloadGuid, boolean allowStopForeground) {
         mDownloadsInProgress.remove(downloadGuid);
-        stopForegroundIfNecessary();
+        if (allowStopForeground && mDownloadsInProgress.size() == 0) stopForegroundInteral(false);
     }
 
     /**
+     * Returns whether or not there are any download notifications showing that aren't the summary
+     * notification.
+     * @param notificationIdToIgnore If not {@code null}, the id of a notification to ignore and
+     *                               assume is closing or about to be closed.
+     * @return Whether or not there are valid download notifications currently visible.
+     */
+    @VisibleForTesting
+    @TargetApi(Build.VERSION_CODES.M)
+    boolean hasDownloadNotifications(Integer notificationIdToIgnore) {
+        if (!BuildInfo.isAtLeastO()) return false;
+
+        StatusBarNotification[] notifications = mNotificationManager.getActiveNotifications();
+        for (StatusBarNotification notification : notifications) {
+            boolean isDownloadsGroup = TextUtils.equals(notification.getNotification().getGroup(),
+                    NotificationConstants.GROUP_DOWNLOADS);
+            boolean isSummaryNotification =
+                    notification.getId() == NotificationConstants.NOTIFICATION_ID_DOWNLOAD_SUMMARY;
+            boolean isIgnoredNotification = notificationIdToIgnore != null
+                    && notificationIdToIgnore == notification.getId();
+            if (isDownloadsGroup && !isSummaryNotification && !isIgnoredNotification) return true;
+        }
+
+        return false;
+    }
+
+    /**
+     * @return The summary {@link StatusBarNotification} if one is showing.
+     */
+    @VisibleForTesting
+    @TargetApi(Build.VERSION_CODES.M)
+    StatusBarNotification getSummaryNotification() {
+        if (!BuildInfo.isAtLeastO()) return null;
+
+        StatusBarNotification[] notifications = mNotificationManager.getActiveNotifications();
+        for (StatusBarNotification notification : notifications) {
+            boolean isDownloadsGroup = TextUtils.equals(notification.getNotification().getGroup(),
+                    NotificationConstants.GROUP_DOWNLOADS);
+            boolean isSummaryNotification =
+                    notification.getId() == NotificationConstants.NOTIFICATION_ID_DOWNLOAD_SUMMARY;
+            if (isDownloadsGroup && isSummaryNotification) return notification;
+        }
+
+        return null;
+    }
+
+    /**
+     * Cancels the existing summary notification.  Moved to a helper method for test mocking.
+     */
+    @VisibleForTesting
+    void cancelSummaryNotification() {
+        mNotificationManager.cancel(NotificationConstants.NOTIFICATION_ID_DOWNLOAD_SUMMARY);
+    }
+
+    /**
      * Check all current notifications and hide the summary notification if we have no downloads
      * notifications left.  On Android if the user swipes away the last download notification the
      * summary will be dismissed.  But if the last downloads notification is dismissed via
      * {@link NotificationManager#cancel(int)}, the summary will remain, so we need to check and
      * manually remove it ourselves.
+     * @param notificationIdToIgnore Canceling a notification and querying for the current list of
+     *                               active notifications isn't synchronous.  Pass a notification id
+     *                               here if there is a notification that should be assumed gone.
+     *                               Or pass {@code null} if no notification fits that criteria.
      */
     @TargetApi(Build.VERSION_CODES.M)
-    void hideSummaryNotificationIfNecessary() {
+    void hideSummaryNotificationIfNecessary(Integer notificationIdToIgnore) {
         if (!BuildInfo.isAtLeastO()) return;
+        if (mDownloadsInProgress.size() > 0) return;
 
-        stopForegroundIfNecessary();
+        if (hasDownloadNotifications(notificationIdToIgnore)) return;
 
-        StatusBarNotification[] notifications = mNotificationManager.getActiveNotifications();
-        for (StatusBarNotification notification : notifications) {
-            if (TextUtils.equals(notification.getNotification().getGroup(),
-                        NotificationConstants.GROUP_DOWNLOADS)
-                    && notification.getId()
-                            != NotificationConstants.NOTIFICATION_ID_DOWNLOAD_SUMMARY) {
-                return;
+        StatusBarNotification notification = getSummaryNotification();
+        if (notification != null) {
+            // We have a valid summary notification, but how we dismiss it depends on whether or not
+            // it is currently bound to this service via startForeground(...).
+            if ((notification.getNotification().flags & Notification.FLAG_FOREGROUND_SERVICE)
+                    != 0) {
+                // If we are a foreground service and we are hiding the notification, we have no
+                // other downloads notifications showing, so we need to remove the notification and
+                // unregister it from this service at the same time.
+                stopForegroundInteral(true);
+            } else {
+                // If we are not a foreground service, remove the notification via the
+                // NotificationManager.  The notification is not bound to this service, so any call
+                // to stopForeground() won't affect the notification.
+                cancelSummaryNotification();
             }
+        } else {
+            // If we don't have a valid summary, just guarantee that we aren't in the foreground for
+            // safety.
+            stopForegroundInteral(false);
         }
-        mNotificationManager.cancel(NotificationConstants.NOTIFICATION_ID_DOWNLOAD_SUMMARY);
 
-        // Added to shut down the service when everything is gone.
-        // TODO(dtrainor): Make sure this makes sense.
-        shutdownService();
+        // Notify all observers that we are requesting a chance to shut down.  This will let any
+        // observers unbind from us if necessary.
+        for (Observer observer : mObservers) observer.onServiceShutdownRequested();
+
+        // Stop the service which should start the destruction process.  At this point we should be
+        // (1) a background service and (2) unbound from any clients.
+        stopSelf();
     }
 
     @Override
     public IBinder onBind(Intent intent) {
         return mBinder;
     }
 
     /**
      * Helper method to update the remaining number of background resumption attempts left.
      */
@@ skipping 134 matching lines @@
 
     /**
      * Cancel a download notification.
      * @param notificationId Notification ID of the download
      * @param downloadGuid GUID of the download.
      */
     @VisibleForTesting
     void cancelNotification(int notificationId, String downloadGuid) {
         mNotificationManager.cancel(NOTIFICATION_NAMESPACE, notificationId);
         mDownloadSharedPreferenceHelper.removeSharedPreferenceEntry(downloadGuid);
-        stopTrackingInProgressDownload(downloadGuid);
-        hideSummaryNotificationIfNecessary();
+
+        // Since we are about to go through the process of validating whether or not we can shut
+        // down, don't stop foreground if we have no download notifications left to show.  Hiding
+        // the summary will take care of that for us.
+        stopTrackingInProgressDownload(downloadGuid, hasDownloadNotifications(notificationId));
+        hideSummaryNotificationIfNecessary(notificationId);
     }
 
     /**
      * Called when a download is canceled.
      * @param downloadGuid GUID of the download.
      */
     @VisibleForTesting
     public void notifyDownloadCanceled(String downloadGuid) {
         DownloadSharedPreferenceEntry entry =
                 mDownloadSharedPreferenceHelper.getDownloadSharedPreferenceEntry(downloadGuid);
@@ skipping 15 matching lines @@
         if (!isResumable) {
             notifyDownloadFailed(entry.isOfflinePage(), downloadGuid, entry.fileName);
             return;
         }
         // If download is already paused, do nothing.
         if (!entry.isAutoResumable) return;
         // If download is interrupted due to network disconnection, show download pending state.
         if (isAutoResumable) {
             notifyDownloadPending(entry.downloadGuid, entry.fileName, entry.isOffTheRecord,
                     entry.canDownloadWhileMetered, entry.isOfflinePage());
-            stopTrackingInProgressDownload(entry.downloadGuid);
+            stopTrackingInProgressDownload(entry.downloadGuid, true);
             return;
         }
 
         String contentText = mContext.getResources().getString(
                 R.string.download_notification_paused);
         ChromeNotificationBuilder builder =
                 buildNotification(R.drawable.ic_download_pause, entry.fileName, contentText);
 
         // Clicking on an in-progress download sends the user to see all their downloads.
         Intent downloadHomeIntent = buildActionIntent(
@@ skipping 16 matching lines @@
 
         Intent dismissIntent = new Intent(cancelIntent);
         dismissIntent.putExtra(EXTRA_NOTIFICATION_DISMISSED, true);
         builder.setDeleteIntent(buildPendingIntent(dismissIntent, entry.notificationId));
 
         updateNotification(entry.notificationId, builder.build(), downloadGuid,
                 entry.isOfflinePage(),
                 new DownloadSharedPreferenceEntry(entry.notificationId, entry.isOffTheRecord,
                         entry.canDownloadWhileMetered, entry.downloadGuid, entry.fileName,
                         entry.itemType, isAutoResumable));
-        stopTrackingInProgressDownload(downloadGuid);
+        stopTrackingInProgressDownload(downloadGuid, true);
     }
 
     /**
      * Add a download successful notification.
      * @param downloadGuid GUID of the download.
      * @param filePath Full path to the download.
      * @param fileName Filename of the download.
      * @param systemDownloadId Download ID assigned by system DownloadManager.
      * @param isOfflinePage Whether the download is for offline page.
      * @param isSupportedMimeType Whether the MIME type can be viewed inside browser.
@@ skipping 22 matching lines @@
         intent.setComponent(component);
         builder.setContentIntent(PendingIntent.getBroadcast(
                 mContext, notificationId, intent, PendingIntent.FLAG_UPDATE_CURRENT));
         if (mDownloadSuccessLargeIcon == null) {
             Bitmap bitmap = BitmapFactory.decodeResource(
                     mContext.getResources(), R.drawable.offline_pin);
             mDownloadSuccessLargeIcon = getLargeNotificationIcon(bitmap);
         }
         builder.setLargeIcon(mDownloadSuccessLargeIcon);
         updateNotification(notificationId, builder.build(), downloadGuid, isOfflinePage, null);
-        stopTrackingInProgressDownload(downloadGuid);
+        stopTrackingInProgressDownload(downloadGuid, true);
         return notificationId;
     }
 
     /**
      * Add a download failed notification.
      * @param isOfflinePage Whether or not the download was for an offline page.
      * @param downloadGuid GUID of the download.
      * @param fileName GUID of the download.
      */
     @VisibleForTesting
     public void notifyDownloadFailed(boolean isOfflinePage, String downloadGuid, String fileName) {
         // If the download is not in history db, fileName could be empty. Get it from
         // SharedPreferences.
         if (TextUtils.isEmpty(fileName)) {
             DownloadSharedPreferenceEntry entry =
                     mDownloadSharedPreferenceHelper.getDownloadSharedPreferenceEntry(downloadGuid);
             if (entry == null) return;
             fileName = entry.fileName;
         }
 
         int notificationId = getNotificationId(downloadGuid);
         ChromeNotificationBuilder builder =
                 buildNotification(android.R.drawable.stat_sys_download_done, fileName,
                         mContext.getResources().getString(R.string.download_notification_failed));
         updateNotification(notificationId, builder.build(), downloadGuid, isOfflinePage, null);
-        stopTrackingInProgressDownload(downloadGuid);
+        stopTrackingInProgressDownload(downloadGuid, true);
     }
 
     /**
      * Helper method to build a PendingIntent from the provided intent.
      * @param intent Intent to broadcast.
      * @param notificationId ID of the notification.
      */
     private PendingIntent buildPendingIntent(Intent intent, int notificationId) {
         return PendingIntent.getBroadcast(
                 mContext, notificationId, intent, PendingIntent.FLAG_UPDATE_CURRENT);
@@ skipping 87 matching lines @@
      * @param intent Intent with the download operation.
      */
     private void handleDownloadOperation(final Intent intent) {
         // TODO(qinmin): Figure out how to properly handle this case.
         boolean isOfflinePage =
                 IntentUtils.safeGetBooleanExtra(intent, EXTRA_IS_OFFLINE_PAGE, false);
         final DownloadSharedPreferenceEntry entry = getDownloadEntryFromIntent(intent);
         if (entry == null
                 && !(isOfflinePage && TextUtils.equals(intent.getAction(), ACTION_DOWNLOAD_OPEN))) {
             handleDownloadOperationForMissingNotification(intent);
-            hideSummaryNotificationIfNecessary();
+            hideSummaryNotificationIfNecessary(null);
             return;
         }
 
         if (ACTION_DOWNLOAD_PAUSE.equals(intent.getAction())) {
             // If browser process already goes away, the download should have already paused. Do
             // nothing in that case.
             if (!DownloadManagerService.hasDownloadManagerService()) {
                 notifyDownloadPaused(entry.downloadGuid, !entry.isOffTheRecord, false);
-                hideSummaryNotificationIfNecessary();
+                hideSummaryNotificationIfNecessary(null);
                 return;
             }
         } else if (ACTION_DOWNLOAD_RESUME.equals(intent.getAction())) {
             // If user manually resumes a download, update the network type if it
             // is not metered previously.
             boolean canDownloadWhileMetered = entry.canDownloadWhileMetered
                     || DownloadManagerService.isActiveNetworkMetered(mContext);
             // Update the SharedPreference entry.
             mDownloadSharedPreferenceHelper.addOrReplaceSharedPreferenceEntry(
                     new DownloadSharedPreferenceEntry(entry.notificationId, entry.isOffTheRecord,
                             canDownloadWhileMetered, entry.downloadGuid, entry.fileName,
                             entry.itemType, true));
         } else if (ACTION_DOWNLOAD_RESUME_ALL.equals(intent.getAction())
                 && (mDownloadSharedPreferenceHelper.getEntries().isEmpty()
                         || DownloadManagerService.hasDownloadManagerService())) {
-            hideSummaryNotificationIfNecessary();
+            hideSummaryNotificationIfNecessary(null);
             return;
         } else if (ACTION_DOWNLOAD_OPEN.equals(intent.getAction())) {
             // TODO(fgorski): Do we even need to do anything special here, before we launch Chrome?
         }
 
         BrowserParts parts = new EmptyBrowserParts() {
             @Override
             public boolean shouldStartGpuProcess() {
                 return false;
             }
@@ skipping 31 matching lines @@
                         resumeAllPendingDownloads();
                 } else if (ACTION_DOWNLOAD_OPEN.equals(intent.getAction())) {
                         OfflinePageDownloadBridge.openDownloadedPage(
                                 IntentUtils.safeGetStringExtra(intent, EXTRA_DOWNLOAD_GUID));
                 } else {
                         Log.e(TAG, "Unrecognized intent action.", intent);
                 }
                 if (!ACTION_DOWNLOAD_OPEN.equals(intent.getAction())) {
                     downloadServiceDelegate.destroyServiceDelegate();
                 }
-                hideSummaryNotificationIfNecessary();
+
+                hideSummaryNotificationIfNecessary(ACTION_DOWNLOAD_CANCEL.equals(intent.getAction())
+                                ? entry.notificationId
+                                : null);
             }
         };
         try {
             ChromeBrowserInitializer.getInstance(mContext).handlePreNativeStartup(parts);
             ChromeBrowserInitializer.getInstance(mContext).handlePostNativeStartup(true, parts);
         } catch (ProcessInitException e) {
             Log.e(TAG, "Unable to load native library.", e);
             ChromeApplication.reportStartupErrorAndExit(e);
         }
     }
@@ skipping 185 matching lines @@
             return context.getString(R.string.remaining_duration_minutes, minutes);
         } else if (minutes > 0) {
             return context.getString(R.string.remaining_duration_one_minute);
         } else if (seconds == 1) {
             return context.getString(R.string.remaining_duration_one_second);
         } else {
             return context.getString(R.string.remaining_duration_seconds, seconds);
         }
     }
 }