Downloads: Properly prune processed actions

chrome/android/java/src/org/chromium/chrome/browser/download/SystemDownloadNotifier.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.content.ComponentName;
 import android.content.Context;
 import android.content.Intent;
 import android.content.ServiceConnection;
@@ skipping 93 matching lines @@
         mBoundService = service;
     }
 
     /**
      * Handles all the pending notifications that hasn't been processed.
      */
     @VisibleForTesting
     void handlePendingNotifications() {
         if (mPendingNotifications.isEmpty()) return;
         for (int i = 0; i < mPendingNotifications.size(); i++) {
             // If we lose the service mid-loop retrigger the service load and quit.

[qinmin, 2017/03/23 23:21:00]

can this actually happen in the middle of a running function? 
autoRelease only happen for the last notification in the list, and all other
method that tries to call unbindService happens on the same UI thread, right?

[David Trainor- moved to gerrit, 2017/03/24 00:32:35]

I think the service can try to stop itself if it has no more notifications to
show, which could in theory make it attempt to shutdown (e.g. the cancel case?).

[qinmin, 2017/03/24 05:05:09]

you mean service.stopself()? I don't think that matters. Stopself() won't be
executed immediately, it still put a task on a message queue to wait the current
code to finish.

[David Trainor- moved to gerrit, 2017/03/25 03:56:24]

Gah lost a big reply by closing the tab :(.  So tl;dr there is an issue but it
might not end up being a problem.  The issue is Service.stopSelf() doesn't stop
the service if there are clients bound.  But once the client unbinds the service
will be destroyed.  To make this deterministic and make sense, I just have the
service tell this notifier to unbind itself now so we don't have this weird 'in
between' state.

If we don't manually tell the service to unbind, we'll only unbind when we have
no running downloads.  So the scenario is:

Download ID | State    | Batch
1             PROGRESS   1
1             CANCEL     2   // Service calls stopSelf.  Destroy gets queued.
2             PROGRESS   2   // Gets processed because we're still bound.
                             // Now we are a foreground service that will die?
2             PAUSED     3   // No longer foreground.  Unbind.  Does the service
die?

If the service dies here, we should be good as long as we've detached the
notification.  I need to test what happens.  I can recreate it this weekend or
on Monday.

If this isn't a problem I need to remove the callback method.  I'll write up a
patch that does that and send it to you while I investigate.  Thanks!

             if (mBoundService == null) {
                 startAndBindToServiceIfNeeded();
+                // Prune the list by pulling out the items we've already processed.
+                mPendingNotifications = new ArrayList<PendingNotificationInfo>(
+                        mPendingNotifications.subList(i, mPendingNotifications.size()));
                 return;
             }
             updateDownloadNotification(
                     mPendingNotifications.get(i), i == mPendingNotifications.size() - 1);
         }
         mPendingNotifications.clear();
     }
 
     /**
      * Starts and binds to the download notification service if needed.
@@ skipping 171 matching lines @@
             default:
                 assert false;
         }
 
         // Don't need to expose the notification id to ignore.  Cancel will automatically call this
         // method as well and pass it in.
         if (mBoundService != null) mBoundService.hideSummaryNotificationIfNecessary(-1);
         if (autoRelease) unbindServiceIfNeeded();
     }
 }