Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2759193003:
Downloads: Properly prune processed actions (Closed)
|
Created:
3 years, 9 months ago by David Trainor- moved to gerrit Modified:
3 years, 8 months ago Reviewers:
qinmin CC:
chromium-reviews, asanka, agrieve+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionDownloads: Properly prune processed actions
If the DownloadNotificationService is torn down during the processing of
events, make sure we properly clean up the events we did properly send
to the service. Right now we don't modify the list, which means when
the service is restarted we'll process the original few items again.
BUG=703212
Patch Set 1 #
Total comments: 4
Depends on Patchset: Messages
Total messages: 13 (6 generated)
The CQ bit was checked by dtrainor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
dtrainor@chromium.org changed reviewers: + qinmin@chromium.org
ptal thanks!
https://codereview.chromium.org/2759193003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/download/SystemDownloadNotifier.java (right): https://codereview.chromium.org/2759193003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/download/SystemDownloadNotifier.java:114: // If we lose the service mid-loop retrigger the service load and quit. 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?
https://codereview.chromium.org/2759193003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/download/SystemDownloadNotifier.java (right): https://codereview.chromium.org/2759193003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/download/SystemDownloadNotifier.java:114: // If we lose the service mid-loop retrigger the service load and quit. On 2017/03/23 23:21:00, qinmin wrote: > 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? 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?).
https://codereview.chromium.org/2759193003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/download/SystemDownloadNotifier.java (right): https://codereview.chromium.org/2759193003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/download/SystemDownloadNotifier.java:114: // If we lose the service mid-loop retrigger the service load and quit. On 2017/03/24 00:32:35, David Trainor-ping if over 24h wrote: > On 2017/03/23 23:21:00, qinmin wrote: > > 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? > > 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?). 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.
On 2017/03/24 05:05:09, qinmin wrote: > https://codereview.chromium.org/2759193003/diff/1/chrome/android/java/src/org... > File > chrome/android/java/src/org/chromium/chrome/browser/download/SystemDownloadNotifier.java > (right): > > https://codereview.chromium.org/2759193003/diff/1/chrome/android/java/src/org... > chrome/android/java/src/org/chromium/chrome/browser/download/SystemDownloadNotifier.java:114: > // If we lose the service mid-loop retrigger the service load and quit. > On 2017/03/24 00:32:35, David Trainor-ping if over 24h wrote: > > On 2017/03/23 23:21:00, qinmin wrote: > > > 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? > > > > 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?). > > 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. Had to add code to tell the SystemDownloadNotifier to detach on stopSelf(). Can't finish while bound IIUC.
On 2017/03/24 17:34:31, David Trainor-ping if over 24h wrote: > On 2017/03/24 05:05:09, qinmin wrote: > > > https://codereview.chromium.org/2759193003/diff/1/chrome/android/java/src/org... > > File > > > chrome/android/java/src/org/chromium/chrome/browser/download/SystemDownloadNotifier.java > > (right): > > > > > https://codereview.chromium.org/2759193003/diff/1/chrome/android/java/src/org... > > > chrome/android/java/src/org/chromium/chrome/browser/download/SystemDownloadNotifier.java:114: > > // If we lose the service mid-loop retrigger the service load and quit. > > On 2017/03/24 00:32:35, David Trainor-ping if over 24h wrote: > > > On 2017/03/23 23:21:00, qinmin wrote: > > > > 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? > > > > > > 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?). > > > > 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. > > Had to add code to tell the SystemDownloadNotifier to detach on stopSelf(). > Can't finish while bound IIUC. Maybe that piece of code can prune. Again, it is wierd to interrupt a for loop in the middle.
https://codereview.chromium.org/2759193003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/download/SystemDownloadNotifier.java (right): https://codereview.chromium.org/2759193003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/download/SystemDownloadNotifier.java:114: // If we lose the service mid-loop retrigger the service load and quit. On 2017/03/24 05:05:09, qinmin wrote: > On 2017/03/24 00:32:35, David Trainor-ping if over 24h wrote: > > On 2017/03/23 23:21:00, qinmin wrote: > > > 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? > > > > 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?). > > 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. 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!
Description was changed from ========== Downloads: Properly prune processed actions If the DownloadNotificationService is torn down during the processing of events, make sure we properly clean up the events we did properly send to the service. Right now we don't modify the list, which means when the service is restarted we'll process the original few items again. BUG=703212 ========== to ========== Downloads: Properly prune processed actions If the DownloadNotificationService is torn down during the processing of events, make sure we properly clean up the events we did properly send to the service. Right now we don't modify the list, which means when the service is restarted we'll process the original few items again. BUG=703212 ========== |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
