[Downloads] Downloads should work even if history database fails |
|||||||||
Issue descriptionCurrently 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.
,
Jul 10 2017
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.
,
Jul 11 2017
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.
,
Jul 11 2017
,
Jul 21 2017
,
Jul 21 2017
+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.
,
Jul 21 2017
Is there anyway to register a cleanup task to add that item to the db at a later time?
,
Jul 24 2017
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.
,
Jul 24 2017
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.
,
Jul 24 2017
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?
,
Jul 24 2017
Sure, thanks, we will make download works without history db and log UMA for this.
,
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
,
Jul 28 2017
,
Jul 28 2017
Please add appropriate OSs.
,
Jul 28 2017
Done.
,
Jul 29 2017
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
,
Jul 30 2017
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
,
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
,
Aug 7 2017
Just as an aside, how will this affect the download service (if at all)?
,
Aug 7 2017
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.
,
Aug 7 2017
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.
,
Aug 7 2017
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.
,
Aug 10 2017
|
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by dtrainor@chromium.org
, Jun 23 2017Status: Assigned (was: Untriaged)