New issue
Advanced search Search tips

Issue 593020 link

Starred by 3 users

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug

Blocking:
issue 590079



Sign in to add a comment

Eliminate download IDs

Project Member Reported by asanka@chromium.org, Mar 8 2016

Issue description

Android (and possibly other things like download extensions) have been using download IDs on the assumption that a download ID will refer to exactly one download, even across sessions. This is only a safe assumption to make within a single session.

In order to safely persist and use IDs outside of core downloads, one would need to observe and react to every download creation and removal happening within a profile and its associated OTR profiles. Otherwise, there could be changes to the list of downloads that could cause the ID to be assigned to a different download.

Fixing this is necessary for automatic resumption of downloads on Android where a background process that has no insight into the users profile needs to produce resumption signals for a download. I.e. the background process needs a reference to a download that survives across sessions. The download itself doesn't need to survive, but the reference should never resolve to a different download. This is currently not possible via download IDs.

One proposal: GUIDs! Let's give each download a GUID.

Pros: Fulfills all requirements. Also, we don't need to wait for the history DB to create the first ID (i.e. DownloadItem::Start() doesn't need an async hop via the delegate to determine its ID).

Cons: Downloads extensions API lists the ID as a 'long'. The documentation in downloads.idl says:

    // An identifier that is persistent across browser sessions.
    long id;

N.b. "persistent across browser sessions" is misleading since unbeknownst to the extension, Chrome could reuse the ID to refer to some other download.

After discussing with benjhayden@ and asargent@ among others, we've come up with the following plan:

1. Introduce GUIDs for downloads, which uniquely identify the download during
   its lifetime. Each new download gets a new GUID, hence a lookup based on an
   old GUID won't accidentally refer to a different download. Regular old
   download IDs will still be available for the moment.

2. Android services that need to refer to downloads can shift to using GUIDs at
   this point.

3. I'll start moving consumers away from download IDs.

3.1. Extensions will expose the GUID, but will also maintain its own mapping
   based on hashing the GUID. In the absence of unlikely conflicts with other
   known downloads, this would mean that download IDs will be stable across
   sessions. I'll also update the documentation so that it's clear what the
   guarantees of IDs are and recommend switching over to GUIDs if cross session
   stability is desired.

3.2. Native consumers who are interested in the lifetime of downloads will
   switch to observing them rather than looking them up at arbitrary times using IDs.

3.3. Native consumers who want to invoke a method on DownloadItem at some
   arbitrary time will instead be switched over to using callback based APIs
   (assuming they can't be switched over to being DownloadItem::Observers.

4. Once all interested parties switch away from download IDs, we'll remove IDs
   entirely.

Steps 1 and 4 involve download history schema changes. Extensions won't observe any changes until step 3.1 is complete and no changes from then onwards.

Note that at step 3.1, the mapping between downloads and existing IDs will break since the new ID assigned to a download will likely be different from the one assigned to it using the current scheme. However, from that point on, the IDs should be stable again.

 
Cc: qin...@chromium.org
Project Member

Comment 2 by bugdroid1@chromium.org, Mar 14 2016

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

commit eef62b0282ec19ebc040785d0b7ac36de398cbc1
Author: asanka <asanka@chromium.org>
Date: Mon Mar 14 21:23:11 2016

[Downloads] Introduce GUIDs for downloads.

Chrome on Android and extensions attempt to keep references to downloads
across browser sessions using download IDs. This isn't safe since a
subsequent browser session could reuse a download ID to refer to a
different download.

In order to provide a weak cross-session reference, this CL introduces
GUIDs. At worst, attempting to lookup a download via a GUID will fail
rather than end up referring to a different download. This assumes that
there will not be any conflicts among the GUIDs, which is apparently a
safe assumption to make.

This CL includes the following changes:

* Introduce a GetGuid() method to DownloadItem and its subclasses.

* Allow DownloadItemImpl to create a new GUID for itself for new
  downloads using base::GenerateGuid().

* Store and retrieve GUIDs from downloads history for persisted
  downloads.

* Generate GUIDs for downloads in the history DB which are being
  migrated due to a browser upgrade.

* Since we are revving the history DB schema, also adds the fields for
  HTTP method and hash fields.

  Note that in order to keep the size of the CL sane, code for
  incorporating HTTP method and hash into the downloads system is not
  included in this CL. The latter is
  https://codereview.chromium.org/1751603002 .

BUG=593020

Review URL: https://codereview.chromium.org/1781983002

Cr-Commit-Position: refs/heads/master@{#381071}

[modify] https://crrev.com/eef62b0282ec19ebc040785d0b7ac36de398cbc1/chrome/browser/android/download/download_manager_service_unittest.cc
[modify] https://crrev.com/eef62b0282ec19ebc040785d0b7ac36de398cbc1/chrome/browser/download/download_history.cc
[modify] https://crrev.com/eef62b0282ec19ebc040785d0b7ac36de398cbc1/chrome/browser/download/download_history_unittest.cc
[modify] https://crrev.com/eef62b0282ec19ebc040785d0b7ac36de398cbc1/chrome/browser/download/download_ui_controller_unittest.cc
[modify] https://crrev.com/eef62b0282ec19ebc040785d0b7ac36de398cbc1/chrome/browser/extensions/api/downloads/downloads_api_browsertest.cc
[modify] https://crrev.com/eef62b0282ec19ebc040785d0b7ac36de398cbc1/chrome/browser/safe_browsing/incident_reporting/last_download_finder_unittest.cc
[modify] https://crrev.com/eef62b0282ec19ebc040785d0b7ac36de398cbc1/components/history/core/browser/download_database.cc
[modify] https://crrev.com/eef62b0282ec19ebc040785d0b7ac36de398cbc1/components/history/core/browser/download_database.h
[modify] https://crrev.com/eef62b0282ec19ebc040785d0b7ac36de398cbc1/components/history/core/browser/download_row.cc
[modify] https://crrev.com/eef62b0282ec19ebc040785d0b7ac36de398cbc1/components/history/core/browser/download_row.h
[modify] https://crrev.com/eef62b0282ec19ebc040785d0b7ac36de398cbc1/components/history/core/browser/history_backend_db_unittest.cc
[modify] https://crrev.com/eef62b0282ec19ebc040785d0b7ac36de398cbc1/components/history/core/browser/history_database.cc
[modify] https://crrev.com/eef62b0282ec19ebc040785d0b7ac36de398cbc1/components/history/core/test/history_backend_db_base_test.cc
[modify] https://crrev.com/eef62b0282ec19ebc040785d0b7ac36de398cbc1/components/history/core/test/history_backend_db_base_test.h
[add] https://crrev.com/eef62b0282ec19ebc040785d0b7ac36de398cbc1/components/test/data/history/history.29.sql
[modify] https://crrev.com/eef62b0282ec19ebc040785d0b7ac36de398cbc1/content/browser/download/download_item_factory.h
[modify] https://crrev.com/eef62b0282ec19ebc040785d0b7ac36de398cbc1/content/browser/download/download_item_impl.cc
[modify] https://crrev.com/eef62b0282ec19ebc040785d0b7ac36de398cbc1/content/browser/download/download_item_impl.h
[modify] https://crrev.com/eef62b0282ec19ebc040785d0b7ac36de398cbc1/content/browser/download/download_manager_impl.cc
[modify] https://crrev.com/eef62b0282ec19ebc040785d0b7ac36de398cbc1/content/browser/download/download_manager_impl.h
[modify] https://crrev.com/eef62b0282ec19ebc040785d0b7ac36de398cbc1/content/browser/download/download_manager_impl_unittest.cc
[modify] https://crrev.com/eef62b0282ec19ebc040785d0b7ac36de398cbc1/content/public/browser/download_item.h
[modify] https://crrev.com/eef62b0282ec19ebc040785d0b7ac36de398cbc1/content/public/browser/download_manager.h
[modify] https://crrev.com/eef62b0282ec19ebc040785d0b7ac36de398cbc1/content/public/test/mock_download_item.h
[modify] https://crrev.com/eef62b0282ec19ebc040785d0b7ac36de398cbc1/content/public/test/mock_download_manager.cc
[modify] https://crrev.com/eef62b0282ec19ebc040785d0b7ac36de398cbc1/content/public/test/mock_download_manager.h
[modify] https://crrev.com/eef62b0282ec19ebc040785d0b7ac36de398cbc1/tools/metrics/histograms/histograms.xml

Project Member

Comment 3 by bugdroid1@chromium.org, Mar 26 2016

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

commit 30ac0f57a0fcae1a13e6374c05927a04caf526e5
Author: qinmin <qinmin@chromium.org>
Date: Sat Mar 26 04:13:38 2016

Switch to use download GUID to indentify download items

Download GUID is introduced by
http://crrev.com/eef62b0282ec19ebc040785d0b7ac36de398cbc1
Unlike download Id, the GUID can live across browser sessions and will
not be reused.
This change switches the java class to also use the
GUID.
Chrome will temporarily use the old download Id as the notification Id.
This is because the android notification requires an int to identify it,
rather than a string.
In the future, we will generate the notification Id from java side.

The download Ids generated by the Android DownloadManager are not
affected by this CL.
However, they are only used in OMA downloads and when we call
addCompletedDownload().

BUG=593020

Review URL: https://codereview.chromium.org/1809203006

Cr-Commit-Position: refs/heads/master@{#383443}

[modify] https://crrev.com/30ac0f57a0fcae1a13e6374c05927a04caf526e5/chrome/android/java/src/org/chromium/chrome/browser/download/ChromeDownloadDelegate.java
[add] https://crrev.com/30ac0f57a0fcae1a13e6374c05927a04caf526e5/chrome/android/java/src/org/chromium/chrome/browser/download/DownloadItem.java
[modify] https://crrev.com/30ac0f57a0fcae1a13e6374c05927a04caf526e5/chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java
[modify] https://crrev.com/30ac0f57a0fcae1a13e6374c05927a04caf526e5/chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java
[modify] https://crrev.com/30ac0f57a0fcae1a13e6374c05927a04caf526e5/chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotifier.java
[add] https://crrev.com/30ac0f57a0fcae1a13e6374c05927a04caf526e5/chrome/android/java/src/org/chromium/chrome/browser/download/DownloadSharedPreferenceEntry.java
[modify] https://crrev.com/30ac0f57a0fcae1a13e6374c05927a04caf526e5/chrome/android/java/src/org/chromium/chrome/browser/download/DownloadSnackbarController.java
[modify] https://crrev.com/30ac0f57a0fcae1a13e6374c05927a04caf526e5/chrome/android/java/src/org/chromium/chrome/browser/download/OMADownloadHandler.java
[modify] https://crrev.com/30ac0f57a0fcae1a13e6374c05927a04caf526e5/chrome/android/java/src/org/chromium/chrome/browser/download/SystemDownloadNotifier.java
[modify] https://crrev.com/30ac0f57a0fcae1a13e6374c05927a04caf526e5/chrome/android/java_sources.gni
[modify] https://crrev.com/30ac0f57a0fcae1a13e6374c05927a04caf526e5/chrome/android/javatests/src/org/chromium/chrome/browser/download/DownloadManagerServiceTest.java
[modify] https://crrev.com/30ac0f57a0fcae1a13e6374c05927a04caf526e5/chrome/android/javatests/src/org/chromium/chrome/browser/download/DownloadNotificationServiceTest.java
[modify] https://crrev.com/30ac0f57a0fcae1a13e6374c05927a04caf526e5/chrome/android/javatests/src/org/chromium/chrome/browser/download/DownloadTestBase.java
[modify] https://crrev.com/30ac0f57a0fcae1a13e6374c05927a04caf526e5/chrome/android/javatests/src/org/chromium/chrome/browser/download/MockDownloadNotificationService.java
[modify] https://crrev.com/30ac0f57a0fcae1a13e6374c05927a04caf526e5/chrome/android/javatests/src/org/chromium/chrome/browser/download/SystemDownloadNotifierTest.java
[modify] https://crrev.com/30ac0f57a0fcae1a13e6374c05927a04caf526e5/chrome/browser/android/download/chrome_download_delegate.cc
[modify] https://crrev.com/30ac0f57a0fcae1a13e6374c05927a04caf526e5/chrome/browser/android/download/download_manager_service.cc
[modify] https://crrev.com/30ac0f57a0fcae1a13e6374c05927a04caf526e5/chrome/browser/android/download/download_manager_service.h
[modify] https://crrev.com/30ac0f57a0fcae1a13e6374c05927a04caf526e5/chrome/browser/android/download/download_manager_service_unittest.cc
[modify] https://crrev.com/30ac0f57a0fcae1a13e6374c05927a04caf526e5/chrome/browser/android/download/mock_download_controller_android.cc
[modify] https://crrev.com/30ac0f57a0fcae1a13e6374c05927a04caf526e5/chrome/browser/android/download/mock_download_controller_android.h
[modify] https://crrev.com/30ac0f57a0fcae1a13e6374c05927a04caf526e5/content/browser/android/download_controller_android_impl.cc
[modify] https://crrev.com/30ac0f57a0fcae1a13e6374c05927a04caf526e5/content/browser/android/download_controller_android_impl.h
[modify] https://crrev.com/30ac0f57a0fcae1a13e6374c05927a04caf526e5/content/public/android/java/src/org/chromium/content/browser/ContentViewDownloadDelegate.java
[modify] https://crrev.com/30ac0f57a0fcae1a13e6374c05927a04caf526e5/content/public/android/java/src/org/chromium/content/browser/DownloadController.java
[modify] https://crrev.com/30ac0f57a0fcae1a13e6374c05927a04caf526e5/content/public/android/java/src/org/chromium/content/browser/DownloadInfo.java
[modify] https://crrev.com/30ac0f57a0fcae1a13e6374c05927a04caf526e5/content/public/browser/android/download_controller_android.h

Cc: asanka@chromium.org
Owner: ----
Status: Untriaged (was: Assigned)

Comment 5 by qin...@chromium.org, Jun 21 2018

Owner: qin...@chromium.org
Status: Assigned (was: Untriaged)

Sign in to add a comment