New issue
Advanced search Search tips

Issue 769426 link

Starred by 1 user

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug


Participants' hotlists:
Download-Service-Stability
PF-DL-bugs


Sign in to add a comment

Investigate high rate of DOWNLOAD_ERROR

Project Member Reported by dewittj@chromium.org, Sep 27 2017

Issue description

We expect that DOWNLOAD_ERROR should be limited to networking problems because the URL is a known-good Google server URL.  However, it is currently at 30% of all pages that reach the download phase.

This could be a side-effect of Issue 769421 and should probably be revisited once that is resolved.
 
One possible cause is the file is already on disk, and DownloadTargetDeterminter cancel the download in certain cases.

Sometimes I can get a android toast with text IDS_DOWNLOAD_FAILED_REASON_FILE_ALREADY_EXISTS in android_chrome_strings.xml, download service needs to handle android code path correctly in certain cases.
More datapoints:
https://uma.googleplex.com/variations?sid=19f077ead9bcd7c7310e73304bfaf454

The metric OfflinePages.Prefetching.DownloadFinishedUpdate shows us what we do when a download finishes. There are 4 buckets:

OK  --> Download successful, matched something in our DB (48%)
OK  --> Download failed, matched something in our DB (24%)
OMG --> Download successful, unknown GUID (15%)
OMG --> Download failed, unknown GUID(13%)

We are seeing >25% of downloads not matching anything in our database.  This is possible if we retry a download because it is not represented in the list when we get the OnServiceInitialized call at downloads startup.

This indicates that for 15% of downloads, the bytes and file are thrown away! and contribute to the BACKOFF problem we are seeing.

My best guess is some race condition in the DL initialization logic that causes entries to not be included in the DownloadMetaData list in some cases, when in fact they are in the DB and still valid downloads.
Thanks for sharing this, will keep looking.

In prefetch, I saw we mark a db record in zombie state and then some other task will remove the record from db, is it possible that DS db didn't remove these where prefetch has removed these, this may cause unknown guid issue.
Project Member

Comment 5 by bugdroid1@chromium.org, Oct 5 2017

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

commit 222f6c8a6c9501abab1353501d662a5c403d06e6
Author: David Trainor <dtrainor@chromium.org>
Date: Thu Oct 05 16:45:12 2017

Fix download resumption to handle cancel case

Reworks the DownloadService's code to interpret DownloadItem states
better.  We have some unexpected states/transitions from DownloadItem to
DriverEntry that are causing our resumption/fail logic to get confused.
This brings over the concept if "done" and "can_resume" into the state
rectification logic.

TBR=sky@chromium.org

Bug: 769426
Change-Id: Iad7bc612a68421471ff4204e6a94d52eaea1b825
Reviewed-on: https://chromium-review.googlesource.com/699562
Commit-Queue: David Trainor <dtrainor@chromium.org>
Reviewed-by: Shakti Sahu <shaktisahu@chromium.org>
Reviewed-by: Xing Liu <xingliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#506764}
[modify] https://crrev.com/222f6c8a6c9501abab1353501d662a5c403d06e6/components/download/content/internal/download_driver_impl.cc
[modify] https://crrev.com/222f6c8a6c9501abab1353501d662a5c403d06e6/components/download/content/internal/download_driver_impl_unittest.cc
[modify] https://crrev.com/222f6c8a6c9501abab1353501d662a5c403d06e6/components/download/internal/config.cc
[modify] https://crrev.com/222f6c8a6c9501abab1353501d662a5c403d06e6/components/download/internal/config.h
[modify] https://crrev.com/222f6c8a6c9501abab1353501d662a5c403d06e6/components/download/internal/controller.h
[modify] https://crrev.com/222f6c8a6c9501abab1353501d662a5c403d06e6/components/download/internal/controller_impl.cc
[modify] https://crrev.com/222f6c8a6c9501abab1353501d662a5c403d06e6/components/download/internal/controller_impl_unittest.cc
[modify] https://crrev.com/222f6c8a6c9501abab1353501d662a5c403d06e6/components/download/internal/driver_entry.cc
[modify] https://crrev.com/222f6c8a6c9501abab1353501d662a5c403d06e6/components/download/internal/driver_entry.h
[modify] https://crrev.com/222f6c8a6c9501abab1353501d662a5c403d06e6/components/download/internal/entry.cc
[modify] https://crrev.com/222f6c8a6c9501abab1353501d662a5c403d06e6/components/download/internal/entry.h
[modify] https://crrev.com/222f6c8a6c9501abab1353501d662a5c403d06e6/components/download/internal/proto/entry.proto
[modify] https://crrev.com/222f6c8a6c9501abab1353501d662a5c403d06e6/components/download/internal/proto_conversions.cc
[modify] https://crrev.com/222f6c8a6c9501abab1353501d662a5c403d06e6/components/download/internal/proto_conversions_unittest.cc
[modify] https://crrev.com/222f6c8a6c9501abab1353501d662a5c403d06e6/components/download/internal/stats.cc
[modify] https://crrev.com/222f6c8a6c9501abab1353501d662a5c403d06e6/components/download/internal/test/entry_utils.cc
[modify] https://crrev.com/222f6c8a6c9501abab1353501d662a5c403d06e6/components/download/internal/test/entry_utils.h
[modify] https://crrev.com/222f6c8a6c9501abab1353501d662a5c403d06e6/content/public/test/fake_download_item.cc
[modify] https://crrev.com/222f6c8a6c9501abab1353501d662a5c403d06e6/content/public/test/fake_download_item.h
[modify] https://crrev.com/222f6c8a6c9501abab1353501d662a5c403d06e6/tools/metrics/histograms/enums.xml

Project Member

Comment 6 by bugdroid1@chromium.org, Oct 5 2017

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

commit 204048a0acfd5567583bffa606fcc9b3f044d4f6
Author: David Trainor <dtrainor@chromium.org>
Date: Thu Oct 05 20:48:00 2017

Add statistics tracking for service events

Add the following logs to help us have an idea of what the service
behavior is:
- Download.Service.Entry.Event - Logs event actions taken on an Entry
like start, resume, retry, and suspend.
- Download.Service.Entry.RetryCount - At the time of a retry, logs which
retry attempt this is.
- Download.Service.Entry.ResumptionCount - At the time of resumption,
logs which resumption attempt this is.

Bug: 769426
Change-Id: I11e5974521c6ba610d9d58940b4dae61ae8c1fbb
Reviewed-on: https://chromium-review.googlesource.com/701835
Commit-Queue: David Trainor <dtrainor@chromium.org>
Reviewed-by: Shakti Sahu <shaktisahu@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Reviewed-by: Xing Liu <xingliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#506846}
[modify] https://crrev.com/204048a0acfd5567583bffa606fcc9b3f044d4f6/components/download/internal/controller_impl.cc
[modify] https://crrev.com/204048a0acfd5567583bffa606fcc9b3f044d4f6/components/download/internal/stats.cc
[modify] https://crrev.com/204048a0acfd5567583bffa606fcc9b3f044d4f6/components/download/internal/stats.h
[modify] https://crrev.com/204048a0acfd5567583bffa606fcc9b3f044d4f6/tools/metrics/histograms/enums.xml
[modify] https://crrev.com/204048a0acfd5567583bffa606fcc9b3f044d4f6/tools/metrics/histograms/histograms.xml

Project Member

Comment 7 by bugdroid1@chromium.org, Oct 5 2017

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

commit b8b90143fe7723118041beecd79d813923c509e7
Author: Xing Liu <xingliu@chromium.org>
Date: Thu Oct 05 23:26:36 2017

Download Service: Handle transient download in target determiner.

This CL fixed an issue that transient download may trigger android UI
and cancel the download in download resumption.


How it happens: In resumption, we feed current path as initial path to
target determiner instead of forced path. Target determiner may change
the download path from DS internal storage to external storage, this
creates the following issues:
1. External storage needs permission.
2. Conflict files are not rewritten, but needs user confirmation with
a snackbar, because we don't have webcontents, target determiner will
delete itself and fail the download with reason USER_CANCEL.

All the Android leak will call into
DownloadManagerService::OnDownloadCanceled, where it shows two kinds of
error messages in toast.

This issue may be also related to issue 762173.

Bug:  769805 , 769426
Change-Id: I1ac6d2446fcc05c00517a65b465d7bf1ea2ccfeb
Reviewed-on: https://chromium-review.googlesource.com/693400
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Reviewed-by: Min Qin <qinmin@chromium.org>
Reviewed-by: David Trainor <dtrainor@chromium.org>
Commit-Queue: Xing Liu <xingliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#506908}
[modify] https://crrev.com/b8b90143fe7723118041beecd79d813923c509e7/chrome/browser/android/download/download_manager_service.cc
[modify] https://crrev.com/b8b90143fe7723118041beecd79d813923c509e7/chrome/browser/download/chrome_download_manager_delegate.cc
[modify] https://crrev.com/b8b90143fe7723118041beecd79d813923c509e7/chrome/browser/download/download_path_reservation_tracker.h
[modify] https://crrev.com/b8b90143fe7723118041beecd79d813923c509e7/chrome/browser/download/download_stats.cc
[modify] https://crrev.com/b8b90143fe7723118041beecd79d813923c509e7/chrome/browser/download/download_stats.h
[modify] https://crrev.com/b8b90143fe7723118041beecd79d813923c509e7/chrome/browser/download/download_target_determiner.cc
[modify] https://crrev.com/b8b90143fe7723118041beecd79d813923c509e7/chrome/browser/download/download_target_determiner_unittest.cc
[modify] https://crrev.com/b8b90143fe7723118041beecd79d813923c509e7/content/public/browser/download_item.h
[modify] https://crrev.com/b8b90143fe7723118041beecd79d813923c509e7/content/public/browser/download_url_parameters.h
[modify] https://crrev.com/b8b90143fe7723118041beecd79d813923c509e7/tools/metrics/histograms/enums.xml
[modify] https://crrev.com/b8b90143fe7723118041beecd79d813923c509e7/tools/metrics/histograms/histograms.xml

Project Member

Comment 8 by bugdroid1@chromium.org, Oct 11 2017

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

commit 9b002038b6aea4e711dceeb9475ec76013704783
Author: Xing Liu <xingliu@chromium.org>
Date: Wed Oct 11 23:51:04 2017

Download service: Fix an issue that downloads started too early.

This CL adds a delay to wait for network stack to be ready.

After download service database loaded, the service and its client can
issue download request. However, net::HostResolverImpl needs to wait
for a kernel call to get the ip address and it will shake off all the
requests after getting the new ip address.

This can happen after download requests are sent, and
net::ERROR_NETWORK_CHANGED is reported to download core.


Bug: 771252, 769426
Change-Id: I4b50d5cbe905d3ea71d5cf8200270b5582f790ce
Reviewed-on: https://chromium-review.googlesource.com/701502
Commit-Queue: Xing Liu <xingliu@chromium.org>
Reviewed-by: David Trainor <dtrainor@chromium.org>
Cr-Commit-Position: refs/heads/master@{#508179}
[modify] https://crrev.com/9b002038b6aea4e711dceeb9475ec76013704783/components/download/content/factory/download_service_factory.cc
[modify] https://crrev.com/9b002038b6aea4e711dceeb9475ec76013704783/components/download/internal/config.cc
[modify] https://crrev.com/9b002038b6aea4e711dceeb9475ec76013704783/components/download/internal/config.h
[modify] https://crrev.com/9b002038b6aea4e711dceeb9475ec76013704783/components/download/internal/controller_impl_unittest.cc
[modify] https://crrev.com/9b002038b6aea4e711dceeb9475ec76013704783/components/download/internal/scheduler/device_status.cc
[modify] https://crrev.com/9b002038b6aea4e711dceeb9475ec76013704783/components/download/internal/scheduler/device_status.h
[modify] https://crrev.com/9b002038b6aea4e711dceeb9475ec76013704783/components/download/internal/scheduler/device_status_listener.cc
[modify] https://crrev.com/9b002038b6aea4e711dceeb9475ec76013704783/components/download/internal/scheduler/device_status_listener.h
[modify] https://crrev.com/9b002038b6aea4e711dceeb9475ec76013704783/components/download/internal/scheduler/device_status_listener_unittest.cc
[modify] https://crrev.com/9b002038b6aea4e711dceeb9475ec76013704783/components/download/internal/test/test_device_status_listener.cc
[modify] https://crrev.com/9b002038b6aea4e711dceeb9475ec76013704783/components/download/internal/test/test_device_status_listener.h

Sign in to add a comment