[Downloads] Download notification living beyond the lifetime of downloads |
||||||||||||
Issue descriptionThe download summary notification sometimes lives beyond the lifetime of existing downloads. It looks like a race condition with the Android framework's notification API, so it's not 100% reproducible. Seen to occur with something like: 1. Start a download 2. Start a page download 3. Wait for the page download to finish 4. Stop the download and dismiss it 5. Try to dismiss the page download complete notification What happens: The download summary notification shows. What should happen: We should not see the download summary notification. Only happens on Android OS >= N.
,
Mar 25 2017
Issue 700456 has been merged into this issue.
,
Mar 25 2017
This should be fixed on the newer pushes of M58. Closing this out. Let me know if you still see this. Thanks!
,
Mar 25 2017
[Auto-generated comment by a script] We noticed that this issue is targeted for M-58; it appears the fix may have landed after branch point, meaning a merge might be required. Please confirm if a merge is required here - if so add Merge-Request-58 label, otherwise remove Merge-TBD label. Thanks.
,
Mar 28 2017
I'm running m58 and am still seeing this rogue notification.
,
Mar 28 2017
Could you let me know what version of M58 you're using and what text you're seeing exactly? Thanks!
,
Mar 29 2017
,
Mar 29 2017
I'm on 58.0.3029.33. I have a "Chrome downloads" notification that won't go away. It won't swipe away. I'm running an O build of Android.
,
Mar 29 2017
What was the fix CL, dtrainor@?
,
Mar 29 2017
Thanks Brian! Ok reopening the bug. Sorry, more questions to help me track this down :). - Do you happen to remember the repro steps? - What device are you using? - Is the notification dismissable? - There was another bug where we had the notification living beyond the expected scope that was explicitly due to using the open menu from the snackbar (706094). Do those repro steps look similar to how you got the notification to show?
,
Mar 29 2017
I can't remember *exactly* what I did, but I went to aa.com and tried to download a boarding pass. I think I used the "down arrow" in the menu. Just tried to do that now, and got the same notification, but tapping it actually takes me to the offline version of the page, and then the notification goes away. I'm using a Pixel XL Notification was not dismissable. Tapping on it didn't open anything or dismiss the notification. Swiping didn't work. As for 706094, I don't think I tapped on a snackbar, unless it was an inadvertent tap. That's possible, because I was walking through the airport juggling bags and a phone at the time.
,
Mar 29 2017
kravula@, can you check this on the M58? Thanks.
,
Mar 29 2017
Tested this scenario on Nexus6P/N2G47M/58.0.3029.42 Scenario 1(Offline page) 1. launch the app 2. Open new tab and load any url aa.com 3. Tap on Download icon from Menu -> Page downloaded and notification displayed 4. Go to the notification tray and tap on notification Observed behavior -> page opened as offline and dismissed the notification Scenario 2(download file) 1. Launch the app 2. Open newtab and load any download page "tinyurl.com/owpgojg" 3. Tap on any download file -> Notification displayed 4. Wait until complete the download 5. Tap on notification -> navigate to download home page and notification is not dismissed
,
Mar 29 2017
@13 yep I see it thanks! I can dismiss this one, so @11 might be another entry point where we're getting into an odd scenario with Android and leaving the foreground notification up.
,
Mar 30 2017
Brian, do you still have the boarding pass that you downloaded on your device? If so do you know what type of download it is? Just curious if it was an offline page or a PDF/some other file type when you encountered the bug. The code paths are a bit different so I'm trying to nail down the exact one. You mentioned you think you used the down arrow, which would create an offline version of the page, but the file type will make it clear. Thanks!
,
Mar 30 2017
actually, it never downloaded properly. I don't believe it was a PDF, just a normal web page. I don't have it anymore, but if you happen to take an American flight, just use the mobile website to get your boarding pass. Unfortunately, I don't have access to it anymore and don't have a flight coming up.
,
Mar 30 2017
+dimich@ for fyi
,
Apr 2 2017
,
Apr 3 2017
,
Apr 4 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/803fdf80f842803c7c91dca2a87419afe1b3d60c commit 803fdf80f842803c7c91dca2a87419afe1b3d60c Author: dtrainor <dtrainor@chromium.org> Date: Tue Apr 04 16:32:20 2017 Hide the download summary on file open Notifications can be dismissed with a DownloadManager.ACTION_NOTIFICATION_CLICKED intent, which bypasses the DownloadNotificationService so we don't get a chance to hide the summary. This adds a check to the DownloadBroadcastReceiver#openDownload() path that allows us to hide any dangling summary notifications that shouldn't be visible. This also lets us catch any clicks on the summary which gives us a chance to close them out as well. BUG= 699323 Review-Url: https://codereview.chromium.org/2796573002 Cr-Commit-Position: refs/heads/master@{#461744} [modify] https://crrev.com/803fdf80f842803c7c91dca2a87419afe1b3d60c/chrome/android/java/src/org/chromium/chrome/browser/download/DownloadBroadcastReceiver.java [modify] https://crrev.com/803fdf80f842803c7c91dca2a87419afe1b3d60c/chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java [modify] https://crrev.com/803fdf80f842803c7c91dca2a87419afe1b3d60c/tools/metrics/histograms/histograms.xml
,
Apr 4 2017
#20 fixes the repro steps in #13 and adds a failsafe to try to close the notification on tap. I think we should merge this and I will continue to try to repro other cases over the next few days.
,
Apr 4 2017
Your change meets the bar and is auto-approved for M58. Please go ahead and merge the CL to branch 3029 manually. Please contact milestone owner if you have questions. Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Apr 4 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f1549937850de9b1959a59b6f0e84a33c5319faf commit f1549937850de9b1959a59b6f0e84a33c5319faf Author: David Trainor <dtrainor@chromium.org> Date: Tue Apr 04 20:16:43 2017 Hide the download summary on file open Notifications can be dismissed with a DownloadManager.ACTION_NOTIFICATION_CLICKED intent, which bypasses the DownloadNotificationService so we don't get a chance to hide the summary. This adds a check to the DownloadBroadcastReceiver#openDownload() path that allows us to hide any dangling summary notifications that shouldn't be visible. This also lets us catch any clicks on the summary which gives us a chance to close them out as well. BUG= 699323 Review-Url: https://codereview.chromium.org/2796573002 Cr-Commit-Position: refs/heads/master@{#461744} (cherry picked from commit 803fdf80f842803c7c91dca2a87419afe1b3d60c) Review-Url: https://codereview.chromium.org/2794353003 . Cr-Commit-Position: refs/branch-heads/3029@{#578} Cr-Branched-From: 939b32ee5ba05c396eef3fd992822fcca9a2e262-refs/heads/master@{#454471} [modify] https://crrev.com/f1549937850de9b1959a59b6f0e84a33c5319faf/chrome/android/java/src/org/chromium/chrome/browser/download/DownloadBroadcastReceiver.java [modify] https://crrev.com/f1549937850de9b1959a59b6f0e84a33c5319faf/chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java [modify] https://crrev.com/f1549937850de9b1959a59b6f0e84a33c5319faf/tools/metrics/histograms/histograms.xml
,
Apr 5 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/60240a376d22f0951ee9dd063c4c297183a21122 commit 60240a376d22f0951ee9dd063c4c297183a21122 Author: dtrainor <dtrainor@chromium.org> Date: Wed Apr 05 17:56:08 2017 Require native before logging a histogram The download recovery code added a call to RecordHistogram.recordBooleanHistogram to help track when we hit the recovery path, but this can happen when the native library isn't loaded. Since we just want a rough idea of if this is happening, guard it by requiring native to be loaded before logging the result. BUG= 699323 Review-Url: https://codereview.chromium.org/2804743002 Cr-Commit-Position: refs/heads/master@{#462140} [modify] https://crrev.com/60240a376d22f0951ee9dd063c4c297183a21122/chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java
,
Apr 7 2017
,
Apr 10 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/58f9ba38304ec8c47f2025d14bc6f5f6d5e4a507 commit 58f9ba38304ec8c47f2025d14bc6f5f6d5e4a507 Author: David Trainor <dtrainor@chromium.org> Date: Mon Apr 10 15:43:42 2017 Require native before logging a histogram The download recovery code added a call to RecordHistogram.recordBooleanHistogram to help track when we hit the recovery path, but this can happen when the native library isn't loaded. Since we just want a rough idea of if this is happening, guard it by requiring native to be loaded before logging the result. BUG= 699323 Review-Url: https://codereview.chromium.org/2804743002 Cr-Commit-Position: refs/heads/master@{#462140} (cherry picked from commit 60240a376d22f0951ee9dd063c4c297183a21122) Review-Url: https://codereview.chromium.org/2814493002 . Cr-Commit-Position: refs/branch-heads/3029@{#642} Cr-Branched-From: 939b32ee5ba05c396eef3fd992822fcca9a2e262-refs/heads/master@{#454471} [modify] https://crrev.com/58f9ba38304ec8c47f2025d14bc6f5f6d5e4a507/chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java
,
Apr 12 2017
Marking this as fixed. Fixed the associated blocker and added a fail safe when downloading an item/offline page. kravula@ can you validate in case there are other scenarios that I missed? It has to do with a race condition in the Android notification APIs so it's hard to be 100% sure it's fixed.
,
Apr 12 2017
Verified on Chrome:58.0.3029.70 Device:Pixel/N2G47Q |
||||||||||||
►
Sign in to add a comment |
||||||||||||
Comment 1 by dtrainor@chromium.org
, Mar 15 2017