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

Issue 736511 link

Starred by 3 users

Issue metadata

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



Sign in to add a comment

[Downloads] Downloads should work even if history database fails

Project Member Reported by dtrainor@chromium.org, Jun 23 2017

Issue description

Currently if the history database fails to load, all download attempts get queued up asking for a downloads id and sit in this intermediate state forever.

We need to make ChromeDownloadManagerDelegate not use "next id == kInvalidId" as the "waiting to initialize" indicator.

From there, we can do one of two things:
1. We can return an error from ChromeDownloadManagerDelegate and fail the download.

or.

2. We can figure out what kind of id to use in this case so we can still download, even if nothing will get saved to the history db.
 
Cc: amineer@chromium.org
Status: Assigned (was: Untriaged)
+amineer@ for tracking this issue.  I marked it as M-61 because it's fairly late in the M-60 timeline.  Shakti and Xing can you work with Alex to figure out if at least some preliminary failure indication to the user is worth pulling into M-60?  Probably not, but in the case that it is, some form of a solution effectively becomes a P0 due to the current timeline.  Thanks!
I'll probably fail the download. If download can't be saved to history, even if the download works, next time the user can't find the record.
I assume this isn't a new issue (e.g. a problem we introduced recently) thus I don't see any reason to rush something into M60, which ships its final beta next Wednesday.  Come talk to me if you disagree or that's incorrect.
Cc: -amineer@chromium.org

Comment 5 Deleted

Cc: dtrainor@chromium.org
Cc: dah...@chromium.org
+dahlke@ for more thoughts, when the history database fails, we can make the download still works.

The user can find the file in download directory, but next time it won't be shown in the download home or chrome://downloads. Similar to download in incognito. 

Comment 8 by dah...@chromium.org, Jul 21 2017

Is there anyway to register a cleanup task to add that item to the db at a later time?
After we restart, if our DB was down, we have no metadata about new files on disk.  So we wouldn't know if a new file came from Chrome or some external source.

If we move Chrome downloads to more of a 'file' view for everything in the downloads directory, we could address this.  This gets more difficult/impossible on desktop though where you can save files anywhere so we can't just search one directory for new files.
No clear way to add the item later as far as I know. The core download code is in native and even if we can build something with android background job api, it's really error prone and really hard to fit into content logic. 

For the two options we have:

1. Download without db will cause left-over file issue, as David mentioned. And it might be hard for user to find the file on Android.

2. Fail the download, when history db dies, it's more like the whole download system should not work. On desktop, we have UI to inform the user, which is not implemented on Android. Inform the user for each individual download probably doesn't make too much sense.

Ok, thanks for investigating. Option 1 seems like the right approach. Can we log how often this happens to make sure this isn't a systematic part of the downloads experience?
Sure, thanks, we will make download works without history db and log UMA for this.
Project Member

Comment 13 by bugdroid1@chromium.org, Jul 28 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d1bc5d0b5f6336fe076466ee42644b2e8fef4bc6

commit d1bc5d0b5f6336fe076466ee42644b2e8fef4bc6
Author: Xing Liu <xingliu@chromium.org>
Date: Fri Jul 28 23:29:55 2017

Make downloads work when history database fails to initialize.

Previously all download calls will be stuck when history database fails
to initialize.

This CL assigns 1 as the first id when history database failed to load,
all downloads in this browser session will not be persisted to history
database.

Bug:  736511 
Change-Id: If4db04ca518633a7dd999f45629e60e97fc78b1a
Reviewed-on: https://chromium-review.googlesource.com/587327
Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
Reviewed-by: David Trainor <dtrainor@chromium.org>
Commit-Queue: Xing Liu <xingliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#490540}
[modify] https://crrev.com/d1bc5d0b5f6336fe076466ee42644b2e8fef4bc6/chrome/browser/download/chrome_download_manager_delegate.cc
[modify] https://crrev.com/d1bc5d0b5f6336fe076466ee42644b2e8fef4bc6/chrome/browser/download/chrome_download_manager_delegate.h
[modify] https://crrev.com/d1bc5d0b5f6336fe076466ee42644b2e8fef4bc6/chrome/browser/download/chrome_download_manager_delegate_unittest.cc
[modify] https://crrev.com/d1bc5d0b5f6336fe076466ee42644b2e8fef4bc6/chrome/browser/download/download_stats.cc
[modify] https://crrev.com/d1bc5d0b5f6336fe076466ee42644b2e8fef4bc6/chrome/browser/download/download_stats.h
[modify] https://crrev.com/d1bc5d0b5f6336fe076466ee42644b2e8fef4bc6/tools/metrics/histograms/histograms.xml

Labels: Merge-Request-61
Please add appropriate OSs.
Labels: OS-Android
Done.
Project Member

Comment 17 by sheriffbot@chromium.org, Jul 29 2017

Labels: -Merge-Request-61 Hotlist-Merge-Approved Merge-Approved-61
Your change meets the bar and is auto-approved for M61. Please go ahead and merge the CL to branch 3163 manually. Please contact milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), ketakid @(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 18 by bugdroid1@chromium.org, Jul 30 2017

Labels: -merge-approved-61 merge-merged-3163
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b4fb4e51010c7376cb117b71df4fdf89f2cae965

commit b4fb4e51010c7376cb117b71df4fdf89f2cae965
Author: Xing Liu <xingliu@chromium.org>
Date: Sun Jul 30 20:14:22 2017

Make downloads work when history database fails to initialize.

Previously all download calls will be stuck when history database fails
to initialize.

This CL assigns 1 as the first id when history database failed to load,
all downloads in this browser session will not be persisted to history
database.

TBR=xingliu@chromium.org

(cherry picked from commit d1bc5d0b5f6336fe076466ee42644b2e8fef4bc6)

Bug:  736511 
Change-Id: If4db04ca518633a7dd999f45629e60e97fc78b1a
Reviewed-on: https://chromium-review.googlesource.com/587327
Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
Reviewed-by: David Trainor <dtrainor@chromium.org>
Commit-Queue: Xing Liu <xingliu@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#490540}
Reviewed-on: https://chromium-review.googlesource.com/593048
Reviewed-by: Xing Liu <xingliu@chromium.org>
Cr-Commit-Position: refs/branch-heads/3163@{#136}
Cr-Branched-From: ff259bab28b35d242e10186cd63af7ed404fae0d-refs/heads/master@{#488528}
[modify] https://crrev.com/b4fb4e51010c7376cb117b71df4fdf89f2cae965/chrome/browser/download/chrome_download_manager_delegate.cc
[modify] https://crrev.com/b4fb4e51010c7376cb117b71df4fdf89f2cae965/chrome/browser/download/chrome_download_manager_delegate.h
[modify] https://crrev.com/b4fb4e51010c7376cb117b71df4fdf89f2cae965/chrome/browser/download/chrome_download_manager_delegate_unittest.cc
[modify] https://crrev.com/b4fb4e51010c7376cb117b71df4fdf89f2cae965/chrome/browser/download/download_stats.cc
[modify] https://crrev.com/b4fb4e51010c7376cb117b71df4fdf89f2cae965/chrome/browser/download/download_stats.h
[modify] https://crrev.com/b4fb4e51010c7376cb117b71df4fdf89f2cae965/tools/metrics/histograms/histograms.xml

Project Member

Comment 19 by bugdroid1@chromium.org, Aug 2 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ceed071596100c2410dfc2584beb4b35b0e12e3f

commit ceed071596100c2410dfc2584beb4b35b0e12e3f
Author: F . <zpeng@chromium.org>
Date: Wed Aug 02 17:08:13 2017

Revert "Make downloads work when history database fails to initialize."

This reverts commit d1bc5d0b5f6336fe076466ee42644b2e8fef4bc6.

Reason for revert: Introduces new C++ static initializers. See  crbug.com/751735 

Original change's description:
> Make downloads work when history database fails to initialize.
> 
> Previously all download calls will be stuck when history database fails
> to initialize.
> 
> This CL assigns 1 as the first id when history database failed to load,
> all downloads in this browser session will not be persisted to history
> database.
> 
> Bug:  736511 
> Change-Id: If4db04ca518633a7dd999f45629e60e97fc78b1a
> Reviewed-on: https://chromium-review.googlesource.com/587327
> Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
> Reviewed-by: David Trainor <dtrainor@chromium.org>
> Commit-Queue: Xing Liu <xingliu@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#490540}

TBR=rkaplow@chromium.org,dtrainor@chromium.org,xingliu@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  736511 ,  751735 
Change-Id: Ifa73c694e919bda82eb37ea88020cf6b0ba89751
Reviewed-on: https://chromium-review.googlesource.com/598347
Reviewed-by: F . <zpeng@chromium.org>
Commit-Queue: F . <zpeng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#491410}
[modify] https://crrev.com/ceed071596100c2410dfc2584beb4b35b0e12e3f/chrome/browser/download/chrome_download_manager_delegate.cc
[modify] https://crrev.com/ceed071596100c2410dfc2584beb4b35b0e12e3f/chrome/browser/download/chrome_download_manager_delegate.h
[modify] https://crrev.com/ceed071596100c2410dfc2584beb4b35b0e12e3f/chrome/browser/download/chrome_download_manager_delegate_unittest.cc
[modify] https://crrev.com/ceed071596100c2410dfc2584beb4b35b0e12e3f/chrome/browser/download/download_stats.cc
[modify] https://crrev.com/ceed071596100c2410dfc2584beb4b35b0e12e3f/chrome/browser/download/download_stats.h
[modify] https://crrev.com/ceed071596100c2410dfc2584beb4b35b0e12e3f/tools/metrics/histograms/histograms.xml

Just as an aside, how will this affect the download service (if at all)?
Hmm, this change is probably not good for download service, if the service want to resume a download, it will start from beginning since there is no history records. It may also rename the file.

Basically we can't have strong assumption on the history records.

Even if we can check the actual files on disk in file monitor, there is no secure way to tell those files are from download service.

Should I relanded this CL? If we decide to scrap this fix, we need to also revert it on M61 branch.
No I think the fix is fine.  We just have to figure out how we want the service to handle it (e.g. will we be ok with a renamed file?) I think.
As a follow up, service won't even be used until 62 dev/canary.  So we can land any fixes on trunk and not worry about 61 IMO.
Status: Fixed (was: Assigned)

Sign in to add a comment