New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 699323 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Apr 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug

Blocked on:
issue 709560



Sign in to add a comment

[Downloads] Download notification living beyond the lifetime of downloads

Project Member Reported by dtrainor@chromium.org, Mar 8 2017

Issue description

The 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.
 
Issue 701892 has been merged into this issue.
Issue 700456 has been merged into this issue.
Cc: krav...@chromium.org
Status: Fixed (was: Started)
This should be fixed on the newer pushes of M58.  Closing this out.  Let me know if you still see this.  Thanks!
Labels: Merge-TBD
[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.
I'm running m58 and am still seeing this rogue notification. 
Could you let me know what version of M58 you're using and what text you're seeing exactly?  Thanks!
Cc: brakowski@google.com
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.
What was the fix CL, dtrainor@?
Status: Started (was: Fixed)
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?

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.
kravula@, can you check this on the M58?  Thanks.

Comment 13 by kravula@google.com, 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




@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.
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!
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.
Cc: dim...@chromium.org
+dimich@ for fyi
Cc: dtrainor@chromium.org
 Issue 699325  has been merged into this issue.
Labels: -Merge-TBD
Project Member

Comment 20 by bugdroid1@chromium.org, 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

Labels: Merge-Request-58
#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.
Project Member

Comment 22 by sheriffbot@chromium.org, Apr 4 2017

Labels: -Merge-Request-58 Hotlist-Merge-Approved Merge-Approved-58
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
Project Member

Comment 23 by bugdroid1@chromium.org, Apr 4 2017

Labels: -merge-approved-58 merge-merged-3029
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

Project Member

Comment 24 by bugdroid1@chromium.org, 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

Blockedon: 709560
Project Member

Comment 26 by bugdroid1@chromium.org, 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

Status: Fixed (was: Started)
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.
Verified on Chrome:58.0.3029.70 Device:Pixel/N2G47Q

Sign in to add a comment