[Downloads] Use the initiating StoragePartition for resumption.

[Downloads] Use the initiating StoragePartition for resumption.

The URLRequest created for downloads resumption should be created using
the same StoragePartition as the one used to initiate the download.
Downloads used to use the request context returned via
ResourceContext::GetURLRequestContext(), which can yield the wrong
URLRequestContext (and the wrong StoragePartition) when multiple
StoragePartitions are in use.

Currently non-default StoragePartitions are used for implementing
WebView tag in platform apps. Hence a download that's initiated in a
WebView, if resumed, could result in a network request being issued
that's backed by the wrong StoragePartition without this fix.

This change:

  * Adds the site instance URL to the metadata that's persisted for each
    download.

  * Updates history database schema to accommodate site URL.

  * Updates DownloadURLParameters to take a URLRequestContextGetter
    instead of a ResourceContext. The latter can be uniquely determined
    by the DownloadManager.

  * Updates DownloadItemImpl to use the site instance URL to determine
    the correct StoragePartition to use during resumption.

The rest is assorted tests and plumbing.

R=creis@chromium.org,sky@chromium.org,asargent@chromium.org,jam@chromium.org,svaldez@chromium.org
BUG=606627
Committed: https://crrev.com/3b4be1260ebe931143425302a867b7e7407d002b
Cr-Commit-Position: refs/heads/master@{#392849}

[asanka, 2016-05-04 19:30:45]

Description was changed from

==========
[Downloads] Use the initiating StoragePartition for resumption.

The URLRequest created for downloads resumption should be created using
the same StoragePartition as the one used to initiate the download.
Downloads used to use the request context returned via
ResourceContext::GetURLRequestContext(), which can be incorrect when
used with site isolation.

R=creis@chromium.org,sky@chromium.org
BUG=606627
==========

to

==========
[Downloads] Use the initiating StoragePartition for resumption.

The URLRequest created for downloads resumption should be created using
the same StoragePartition as the one used to initiate the download.
Downloads used to use the request context returned via
ResourceContext::GetURLRequestContext(), which can be incorrect when
used with site isolation.

This change:

  * Adds the site instance URL to the metadata that's persisted for each
    download.

  * Updates history database schema to accommodate site URL.

  * Updates DownloadURLParameters to take a URLRequestContextGetter
    instead of a ResourceContext. The latter can be uniquely determined
    by the DownloadManager.

  * Updates DownloadItemImpl to use the site instance URL to determine
    the correct StoragePartition to use during resumption.

The rest is assorted tests and plumbing.

R=creis@chromium.org,sky@chromium.org,asargent@chromium.org,jam@chromium.org
BUG=606627
==========

[asanka, 2016-05-04 19:43:41]

asanka@chromium.org changed reviewers:
+ asargent@chromium.org, jam@chromium.org

[asanka, 2016-05-04 19:45:34]

Description was changed from

==========
[Downloads] Use the initiating StoragePartition for resumption.

The URLRequest created for downloads resumption should be created using
the same StoragePartition as the one used to initiate the download.
Downloads used to use the request context returned via
ResourceContext::GetURLRequestContext(), which can be incorrect when
used with site isolation.

This change:

  * Adds the site instance URL to the metadata that's persisted for each
    download.

  * Updates history database schema to accommodate site URL.

  * Updates DownloadURLParameters to take a URLRequestContextGetter
    instead of a ResourceContext. The latter can be uniquely determined
    by the DownloadManager.

  * Updates DownloadItemImpl to use the site instance URL to determine
    the correct StoragePartition to use during resumption.

The rest is assorted tests and plumbing.

R=creis@chromium.org,sky@chromium.org,asargent@chromium.org,jam@chromium.org
BUG=606627
==========

to

==========
[Downloads] Use the initiating StoragePartition for resumption.

The URLRequest created for downloads resumption should be created using
the same StoragePartition as the one used to initiate the download.
Downloads used to use the request context returned via
ResourceContext::GetURLRequestContext(), which can be incorrect when
used with site isolation.

This change:

  * Adds the site instance URL to the metadata that's persisted for each
    download.

  * Updates history database schema to accommodate site URL.

  * Updates DownloadURLParameters to take a URLRequestContextGetter
    instead of a ResourceContext. The latter can be uniquely determined
    by the DownloadManager.

  * Updates DownloadItemImpl to use the site instance URL to determine
    the correct StoragePartition to use during resumption.

The rest is assorted tests and plumbing.

R=creis@chromium.org,sky@chromium.org,asargent@chromium.org,jam@chromium.org,...
BUG=606627
==========

[asanka, 2016-05-04 19:45:34]

asanka@chromium.org changed reviewers:
+ svaldez@chromium.org

[asanka, 2016-05-04 19:55:22]

svaldez:
  chrome/browser/download/*
  content/browser/download/*

sky:
  components/history/*
  components/test/data/history/history.31.sql
 
chrome/browser/safe_browsing/incident_reporting/last_download_finder_unittest.cc

asargent:
  chrome/browser/apps/guest_view/web_view_browsertest.cc
  chrome/browser/extensions/api/downloads/*
  chrome/browser/extensions/webstore_installer.cc
  chrome/test/data/extensions/platform_apps/web_view/download_cookie_isolation/*

jam:
  content/public/browser/*
  content/public/test/*
  content/browser/loader/*
  content/browser/renderer_host/*

creis:  Could you look at the use of StoragePartition in the following files?
  chrome/browser/extensions/api/downloads/downloads_api.cc
  chrome/browser/extensions/webstore_installer.cc
  content/browser/download/download_item_impl.cc
  content/public/browser/download_url_parameters.cc

[sky, 2016-05-04 20:49:27]

Said files LGTM

[asargent_no_longer_on_chrome, 2016-05-04 22:30:47]

extensions/apps stuff lgtm

[svaldez, 2016-05-05 14:23:27]

lgtm

https://codereview.chromium.org/1924473003/diff/60001/chrome/browser/download...
File chrome/browser/download/download_ui_controller_unittest.cc (right):

https://codereview.chromium.org/1924473003/diff/60001/chrome/browser/download...
chrome/browser/download/download_ui_controller_unittest.cc:228:
EXPECT_CALL(*item, GetSiteInstanceUrl())
nit: Same order as elsewhere (Referrer->SiteInstance->Tab->TabReferrer).

https://codereview.chromium.org/1924473003/diff/60001/content/browser/downloa...
File content/browser/download/download_create_info.h (right):

https://codereview.chromium.org/1924473003/diff/60001/content/browser/downloa...
content/browser/download/download_create_info.h:55: // Site URL for the site
instance that initiated the download.
nit: Same order as elsewhere (referrer->site_instance->tab->tab_referrer).

[jam, 2016-05-05 15:49:41]

The cl description says "site isolation", although the difference for storage
partitions is not site isolation related. Different SPs are for things like
webview tag in chrome apps.

Can you fix up the cl desc? Also the bug has basically no information. It should
have a repro at least.

On 2016/05/04 19:55:22, asanka wrote:
> svaldez:
>   chrome/browser/download/*
>   content/browser/download/*
> 
> sky:
>   components/history/*
>   components/test/data/history/history.31.sql
>  
>
chrome/browser/safe_browsing/incident_reporting/last_download_finder_unittest.cc
> 
> asargent:
>   chrome/browser/apps/guest_view/web_view_browsertest.cc
>   chrome/browser/extensions/api/downloads/*
>   chrome/browser/extensions/webstore_installer.cc
>  
chrome/test/data/extensions/platform_apps/web_view/download_cookie_isolation/*
> 
> jam:
>   content/public/browser/*
>   content/public/test/*
>   content/browser/loader/*
>   content/browser/renderer_host/*

lgtm

> 
> creis:  Could you look at the use of StoragePartition in the following files?
>   chrome/browser/extensions/api/downloads/downloads_api.cc
>   chrome/browser/extensions/webstore_installer.cc
>   content/browser/download/download_item_impl.cc
>   content/public/browser/download_url_parameters.cc

[asanka, 2016-05-05 16:56:28]

Description was changed from

==========
[Downloads] Use the initiating StoragePartition for resumption.

The URLRequest created for downloads resumption should be created using
the same StoragePartition as the one used to initiate the download.
Downloads used to use the request context returned via
ResourceContext::GetURLRequestContext(), which can be incorrect when
used with site isolation.

This change:

  * Adds the site instance URL to the metadata that's persisted for each
    download.

  * Updates history database schema to accommodate site URL.

  * Updates DownloadURLParameters to take a URLRequestContextGetter
    instead of a ResourceContext. The latter can be uniquely determined
    by the DownloadManager.

  * Updates DownloadItemImpl to use the site instance URL to determine
    the correct StoragePartition to use during resumption.

The rest is assorted tests and plumbing.

R=creis@chromium.org,sky@chromium.org,asargent@chromium.org,jam@chromium.org,...
BUG=606627
==========

to

==========
[Downloads] Use the initiating StoragePartition for resumption.

The URLRequest created for downloads resumption should be created using
the same StoragePartition as the one used to initiate the download.
Downloads used to use the request context returned via
ResourceContext::GetURLRequestContext(), which can be incorrect.

This change:

  * Adds the site instance URL to the metadata that's persisted for each
    download.

  * Updates history database schema to accommodate site URL.

  * Updates DownloadURLParameters to take a URLRequestContextGetter
    instead of a ResourceContext. The latter can be uniquely determined
    by the DownloadManager.

  * Updates DownloadItemImpl to use the site instance URL to determine
    the correct StoragePartition to use during resumption.

The rest is assorted tests and plumbing.

R=creis@chromium.org,sky@chromium.org,asargent@chromium.org,jam@chromium.org,...
BUG=606627
==========

[asanka, 2016-05-05 17:33:00]

Description was changed from

==========
[Downloads] Use the initiating StoragePartition for resumption.

The URLRequest created for downloads resumption should be created using
the same StoragePartition as the one used to initiate the download.
Downloads used to use the request context returned via
ResourceContext::GetURLRequestContext(), which can be incorrect.

This change:

  * Adds the site instance URL to the metadata that's persisted for each
    download.

  * Updates history database schema to accommodate site URL.

  * Updates DownloadURLParameters to take a URLRequestContextGetter
    instead of a ResourceContext. The latter can be uniquely determined
    by the DownloadManager.

  * Updates DownloadItemImpl to use the site instance URL to determine
    the correct StoragePartition to use during resumption.

The rest is assorted tests and plumbing.

R=creis@chromium.org,sky@chromium.org,asargent@chromium.org,jam@chromium.org,...
BUG=606627
==========

to

==========
[Downloads] Use the initiating StoragePartition for resumption.

The URLRequest created for downloads resumption should be created using
the same StoragePartition as the one used to initiate the download.
Downloads used to use the request context returned via
ResourceContext::GetURLRequestContext(), which can yield the wrong
URLRequestContext (and the wrong StoragePartition) when multiple
StoragePartitions are in use.

Currently non-default StoragePartitions are used for implementing
WebView tag in platform apps. Hence a download that's initiated in a
WebView, if resumed, could result in a network request being issued
that's backed by the wrong StoragePartition without this fix.

This change:

  * Adds the site instance URL to the metadata that's persisted for each
    download.

  * Updates history database schema to accommodate site URL.

  * Updates DownloadURLParameters to take a URLRequestContextGetter
    instead of a ResourceContext. The latter can be uniquely determined
    by the DownloadManager.

  * Updates DownloadItemImpl to use the site instance URL to determine
    the correct StoragePartition to use during resumption.

The rest is assorted tests and plumbing.

R=creis@chromium.org,sky@chromium.org,asargent@chromium.org,jam@chromium.org,...
BUG=606627
==========

[asanka, 2016-05-05 17:38:05]

jam: I updated the CL description and added a short blurb to the bug to explain
the scope of this bug. You're correct that this only concerns situations where
there's a download initiated from a non-default StoragePartition. That can
happen currently if a WebView initiates a download.

Resuming such downloads would use the wrong StoragePartition without this patch.

https://codereview.chromium.org/1924473003/diff/60001/chrome/browser/download...
File chrome/browser/download/download_ui_controller_unittest.cc (right):

https://codereview.chromium.org/1924473003/diff/60001/chrome/browser/download...
chrome/browser/download/download_ui_controller_unittest.cc:228:
EXPECT_CALL(*item, GetSiteInstanceUrl())
On 2016/05/05 at 14:23:27, svaldez wrote:
> nit: Same order as elsewhere (Referrer->SiteInstance->Tab->TabReferrer).

Done.

https://codereview.chromium.org/1924473003/diff/60001/content/browser/downloa...
File content/browser/download/download_create_info.h (right):

https://codereview.chromium.org/1924473003/diff/60001/content/browser/downloa...
content/browser/download/download_create_info.h:55: // Site URL for the site
instance that initiated the download.
On 2016/05/05 at 14:23:27, svaldez wrote:
> nit: Same order as elsewhere (referrer->site_instance->tab->tab_referrer).

Done.

[asanka, 2016-05-05 17:40:49]

asargent: I added a cross-session test for WebViewTest. This is to test that we
correctly associate webviews and their respective downloads with the correct
StoragePartition across browser sessions.

[Charlie Reis, 2016-05-05 21:43:23]

[CC nick@ for use of GetSiteURL]

Thanks for fixing this, and sorry for the delay!  (My review queue has been
abnormally high this week.)

LGTM with nits, if you think the DownloadUrlParameters::FromWebContents case
isn't a problem.

Also, does this mean that we won't attempt to resume downloads started from a
<webview> with a non-persistent StoragePartition, if that StoragePartition has
gone away (e.g., in a new session)?  Hopefully so.  (StoragePartitions of
<webview>s are non-persistent by default.)

https://codereview.chromium.org/1924473003/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/webstore_installer.cc (right):

https://codereview.chromium.org/1924473003/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/webstore_installer.cc:676:
contents->GetMainFrame()->GetRoutingID(),
nit: Can use |render_frame_host| here.

https://codereview.chromium.org/1924473003/diff/80001/components/history/core...
File components/history/core/browser/download_database.cc (right):

https://codereview.chromium.org/1924473003/diff/80001/components/history/core...
components/history/core/browser/download_database.cc:280: "site_instance_url
VARCHAR NOT NULL,"  // Site URL for initiating site
site_url (see comment in download_item.h)

https://codereview.chromium.org/1924473003/diff/80001/content/browser/downloa...
File content/browser/download/download_create_info.h (right):

https://codereview.chromium.org/1924473003/diff/80001/content/browser/downloa...
content/browser/download/download_create_info.h:50: GURL site_instance_url;
site_url

(See explanation in download_item.h.)

https://codereview.chromium.org/1924473003/diff/80001/content/public/browser/...
File content/public/browser/download_item.h (right):

https://codereview.chromium.org/1924473003/diff/80001/content/public/browser/...
content/public/browser/download_item.h:195: virtual const GURL&
GetSiteInstanceUrl() const = 0;
This should really be GetSiteURL.  SiteInstance is one instance of a
Site, but the Site URL is the same for all instances of it.  (Applies throughout
the CL.)

I see that there aren't any comments in this section, but it seems like we might
want to add a comment on this saying it's used to determine StoragePartition. 
(There are many uses of SiteInstance::GetSiteURL that are currently incorrect
that nick@ is cleaning up, but this one should be safe.)

https://codereview.chromium.org/1924473003/diff/80001/content/public/browser/...
File content/public/browser/download_manager.h (right):

https://codereview.chromium.org/1924473003/diff/80001/content/public/browser/...
content/public/browser/download_manager.h:149: const GURL& site_instance_url,
site_url

https://codereview.chromium.org/1924473003/diff/80001/content/public/browser/...
File content/public/browser/download_url_parameters.cc (right):

https://codereview.chromium.org/1924473003/diff/80001/content/public/browser/...
content/public/browser/download_url_parameters.cc:40:
std::unique_ptr<DownloadUrlParameters> DownloadUrlParameters::FromWebContents(
I'm a bit concerned about this method, which assumes the download is in the main
frame.  It's called from places like WebContentsImpl::SaveFrameWithHeaders and
RenderViewContextMenu::ExecuteCommand-- are we sure those aren't called when you
interact with a <webview> on a page like chrome://signin?

Maybe this is ok for now, with a bug/TODO to move it to be frame specific if
necessary?  (I'm not sure how it's used, so maybe it's safe.)

https://codereview.chromium.org/1924473003/diff/80001/content/public/test/moc...
File content/public/test/mock_download_manager.h (right):

https://codereview.chromium.org/1924473003/diff/80001/content/public/test/moc...
content/public/test/mock_download_manager.h:122: const GURL& site_instance_ur,
Typo (missing "l").  Also, should be site_url like the others.

[ncarter (slow), 2016-05-05 23:29:06]

nick@chromium.org changed reviewers:
+ nick@chromium.org

[ncarter (slow), 2016-05-05 23:29:07]

https://codereview.chromium.org/1924473003/diff/80001/content/public/browser/...
File content/public/browser/download_item.h (right):

https://codereview.chromium.org/1924473003/diff/80001/content/public/browser/...
content/public/browser/download_item.h:195: virtual const GURL&
GetSiteInstanceUrl() const = 0;
On 2016/05/05 21:43:22, Charlie Reis wrote:
> This should really be GetSiteURL.  SiteInstance is one instance of a
> Site, but the Site URL is the same for all instances of it.  (Applies
throughout
> the CL.)
> 
> I see that there aren't any comments in this section, but it seems like we
might
> want to add a comment on this saying it's used to determine StoragePartition. 
> (There are many uses of SiteInstance::GetSiteURL that are currently incorrect
> that nick@ is cleaning up, but this one should be safe.)

Agree that this is safe.

[ncarter (slow), 2016-05-05 23:29:18]

Description was changed from

==========
[Downloads] Use the initiating StoragePartition for resumption.

The URLRequest created for downloads resumption should be created using
the same StoragePartition as the one used to initiate the download.
Downloads used to use the request context returned via
ResourceContext::GetURLRequestContext(), which can yield the wrong
URLRequestContext (and the wrong StoragePartition) when multiple
StoragePartitions are in use.

Currently non-default StoragePartitions are used for implementing
WebView tag in platform apps. Hence a download that's initiated in a
WebView, if resumed, could result in a network request being issued
that's backed by the wrong StoragePartition without this fix.

This change:

  * Adds the site instance URL to the metadata that's persisted for each
    download.

  * Updates history database schema to accommodate site URL.

  * Updates DownloadURLParameters to take a URLRequestContextGetter
    instead of a ResourceContext. The latter can be uniquely determined
    by the DownloadManager.

  * Updates DownloadItemImpl to use the site instance URL to determine
    the correct StoragePartition to use during resumption.

The rest is assorted tests and plumbing.

R=creis@chromium.org,sky@chromium.org,asargent@chromium.org,jam@chromium.org,...
BUG=606627
==========

to

==========
[Downloads] Use the initiating StoragePartition for resumption.

The URLRequest created for downloads resumption should be created using
the same StoragePartition as the one used to initiate the download.
Downloads used to use the request context returned via
ResourceContext::GetURLRequestContext(), which can yield the wrong
URLRequestContext (and the wrong StoragePartition) when multiple
StoragePartitions are in use.

Currently non-default StoragePartitions are used for implementing
WebView tag in platform apps. Hence a download that's initiated in a
WebView, if resumed, could result in a network request being issued
that's backed by the wrong StoragePartition without this fix.

This change:

  * Adds the site instance URL to the metadata that's persisted for each
    download.

  * Updates history database schema to accommodate site URL.

  * Updates DownloadURLParameters to take a URLRequestContextGetter
    instead of a ResourceContext. The latter can be uniquely determined
    by the DownloadManager.

  * Updates DownloadItemImpl to use the site instance URL to determine
    the correct StoragePartition to use during resumption.

The rest is assorted tests and plumbing.

R=creis@chromium.org,sky@chromium.org,asargent@chromium.org,jam@chromium.org,...
BUG=606627
==========

[ncarter (slow), 2016-05-05 23:29:18]

nick@chromium.org changed reviewers:
- nick@chromium.org

[asargent_no_longer_on_chrome, 2016-05-06 00:02:15]

On 2016/05/05 17:40:49, asanka wrote:
> asargent: I added a cross-session test for WebViewTest. This is to test that
we
> correctly associate webviews and their respective downloads with the correct
> StoragePartition across browser sessions.

new test changes lgtm

[asanka, 2016-05-07 00:24:45]

Thanks everyone!

> Also, does this mean that we won't attempt to resume downloads started from a
<webview>
> with a non-persistent StoragePartition, if that StoragePartition has gone away
> (e.g., in a new session)?  Hopefully so.
>  (StoragePartitions of <webview>s are non-persistent by default.)

This is a bit of a problem and I'll wait for your input before landing this.

Downloads can't tell (without very fragile logic) whether a storage partition
has technically been discarded. Currently storage partitions are always
instantiated if necessary when a requester shows up with a site URL. So in the
case you mention where an in-memory storage partition was used to instantiate a
download and we are trying to resume said download in a subsequent browser
session, the request will be created and it will proceed with the "same" storage
partition. Except it won't have any cookies in it.

I've added a test for this to the web_view_browsertest.

https://codereview.chromium.org/1924473003/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/webstore_installer.cc (right):

https://codereview.chromium.org/1924473003/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/webstore_installer.cc:676:
contents->GetMainFrame()->GetRoutingID(),
On 2016/05/05 at 21:43:22, Charlie Reis wrote:
> nit: Can use |render_frame_host| here.

Done.

https://codereview.chromium.org/1924473003/diff/80001/components/history/core...
File components/history/core/browser/download_database.cc (right):

https://codereview.chromium.org/1924473003/diff/80001/components/history/core...
components/history/core/browser/download_database.cc:280: "site_instance_url
VARCHAR NOT NULL,"  // Site URL for initiating site
On 2016/05/05 at 21:43:22, Charlie Reis wrote:
> site_url (see comment in download_item.h)

Done. Here and elsewhere.

https://codereview.chromium.org/1924473003/diff/80001/content/public/browser/...
File content/public/browser/download_item.h (right):

https://codereview.chromium.org/1924473003/diff/80001/content/public/browser/...
content/public/browser/download_item.h:195: virtual const GURL&
GetSiteInstanceUrl() const = 0;
On 2016/05/05 at 23:29:07, ncarter wrote:
> On 2016/05/05 21:43:22, Charlie Reis wrote:
> > This should really be GetSiteURL.  SiteInstance is one instance of a
> > Site, but the Site URL is the same for all instances of it.  (Applies
throughout
> > the CL.)
> > 
> > I see that there aren't any comments in this section, but it seems like we
might
> > want to add a comment on this saying it's used to determine
StoragePartition. 
> > (There are many uses of SiteInstance::GetSiteURL that are currently
incorrect
> > that nick@ is cleaning up, but this one should be safe.)
> 
> Agree that this is safe.

Cool. Thanks. I added a comment here and changed the names to
site_url/GetSiteUrl().

https://codereview.chromium.org/1924473003/diff/80001/content/public/browser/...
File content/public/browser/download_url_parameters.cc (right):

https://codereview.chromium.org/1924473003/diff/80001/content/public/browser/...
content/public/browser/download_url_parameters.cc:40:
std::unique_ptr<DownloadUrlParameters> DownloadUrlParameters::FromWebContents(
On 2016/05/05 at 21:43:23, Charlie Reis wrote:
> I'm a bit concerned about this method, which assumes the download is in the
main frame.  It's called from places like WebContentsImpl::SaveFrameWithHeaders
and RenderViewContextMenu::ExecuteCommand-- are we sure those aren't called when
you interact with a <webview> on a page like chrome://signin?
> 
> Maybe this is ok for now, with a bug/TODO to move it to be frame specific if
necessary?  (I'm not sure how it's used, so maybe it's safe.)

I think this should be fixed,  but I think that should be a separate change.

For the moment, <webview>s are guest WebContents, correct? Hence talking to the
WebContents should be fine. There are problems with how we deal with context
menus when OOPIF are involved, but that's something that'll need to be dealt
with separately for this change.

https://codereview.chromium.org/1924473003/diff/80001/content/public/test/moc...
File content/public/test/mock_download_manager.h (right):

https://codereview.chromium.org/1924473003/diff/80001/content/public/test/moc...
content/public/test/mock_download_manager.h:122: const GURL& site_instance_ur,
On 2016/05/05 at 21:43:23, Charlie Reis wrote:
> Typo (missing "l").  Also, should be site_url like the others.

Done :)

[asanka, 2016-05-10 20:31:35]

On 2016/05/07 at 00:24:45, asanka wrote:
> Thanks everyone!
> 
> > Also, does this mean that we won't attempt to resume downloads started from
a <webview>
> > with a non-persistent StoragePartition, if that StoragePartition has gone
away
> > (e.g., in a new session)?  Hopefully so.
> >  (StoragePartitions of <webview>s are non-persistent by default.)
> 
> This is a bit of a problem and I'll wait for your input before landing this.
> 
> Downloads can't tell (without very fragile logic) whether a storage partition
has technically been discarded. Currently storage partitions are always
instantiated if necessary when a requester shows up with a site URL. So in the
case you mention where an in-memory storage partition was used to instantiate a
download and we are trying to resume said download in a subsequent browser
session, the request will be created and it will proceed with the "same" storage
partition. Except it won't have any cookies in it.
> 
> I've added a test for this to the web_view_browsertest.

creis: ping.

>
https://codereview.chromium.org/1924473003/diff/80001/chrome/browser/extensio...
> File chrome/browser/extensions/webstore_installer.cc (right):
> 
>
https://codereview.chromium.org/1924473003/diff/80001/chrome/browser/extensio...
> chrome/browser/extensions/webstore_installer.cc:676:
contents->GetMainFrame()->GetRoutingID(),
> On 2016/05/05 at 21:43:22, Charlie Reis wrote:
> > nit: Can use |render_frame_host| here.
> 
> Done.
> 
>
https://codereview.chromium.org/1924473003/diff/80001/components/history/core...
> File components/history/core/browser/download_database.cc (right):
> 
>
https://codereview.chromium.org/1924473003/diff/80001/components/history/core...
> components/history/core/browser/download_database.cc:280: "site_instance_url
VARCHAR NOT NULL,"  // Site URL for initiating site
> On 2016/05/05 at 21:43:22, Charlie Reis wrote:
> > site_url (see comment in download_item.h)
> 
> Done. Here and elsewhere.
> 
>
https://codereview.chromium.org/1924473003/diff/80001/content/public/browser/...
> File content/public/browser/download_item.h (right):
> 
>
https://codereview.chromium.org/1924473003/diff/80001/content/public/browser/...
> content/public/browser/download_item.h:195: virtual const GURL&
GetSiteInstanceUrl() const = 0;
> On 2016/05/05 at 23:29:07, ncarter wrote:
> > On 2016/05/05 21:43:22, Charlie Reis wrote:
> > > This should really be GetSiteURL.  SiteInstance is one instance of a
> > > Site, but the Site URL is the same for all instances of it.  (Applies
throughout
> > > the CL.)
> > > 
> > > I see that there aren't any comments in this section, but it seems like we
might
> > > want to add a comment on this saying it's used to determine
StoragePartition. 
> > > (There are many uses of SiteInstance::GetSiteURL that are currently
incorrect
> > > that nick@ is cleaning up, but this one should be safe.)
> > 
> > Agree that this is safe.
> 
> Cool. Thanks. I added a comment here and changed the names to
site_url/GetSiteUrl().
> 
>
https://codereview.chromium.org/1924473003/diff/80001/content/public/browser/...
> File content/public/browser/download_url_parameters.cc (right):
> 
>
https://codereview.chromium.org/1924473003/diff/80001/content/public/browser/...
> content/public/browser/download_url_parameters.cc:40:
std::unique_ptr<DownloadUrlParameters> DownloadUrlParameters::FromWebContents(
> On 2016/05/05 at 21:43:23, Charlie Reis wrote:
> > I'm a bit concerned about this method, which assumes the download is in the
main frame.  It's called from places like WebContentsImpl::SaveFrameWithHeaders
and RenderViewContextMenu::ExecuteCommand-- are we sure those aren't called when
you interact with a <webview> on a page like chrome://signin?
> > 
> > Maybe this is ok for now, with a bug/TODO to move it to be frame specific if
necessary?  (I'm not sure how it's used, so maybe it's safe.)
> 
> I think this should be fixed,  but I think that should be a separate change.
> 
> For the moment, <webview>s are guest WebContents, correct? Hence talking to
the WebContents should be fine. There are problems with how we deal with context
menus when OOPIF are involved, but that's something that'll need to be dealt
with separately for this change.
> 
>
https://codereview.chromium.org/1924473003/diff/80001/content/public/test/moc...
> File content/public/test/mock_download_manager.h (right):
> 
>
https://codereview.chromium.org/1924473003/diff/80001/content/public/test/moc...
> content/public/test/mock_download_manager.h:122: const GURL& site_instance_ur,
> On 2016/05/05 at 21:43:23, Charlie Reis wrote:
> > Typo (missing "l").  Also, should be site_url like the others.
> 
> Done :)

[Charlie Reis, 2016-05-10 22:50:03]

On 2016/05/07 00:24:45, asanka wrote:
> Thanks everyone!
> 
> > Also, does this mean that we won't attempt to resume downloads started from
a
> <webview>
> > with a non-persistent StoragePartition, if that StoragePartition has gone
away
> > (e.g., in a new session)?  Hopefully so.
> >  (StoragePartitions of <webview>s are non-persistent by default.)
> 
> This is a bit of a problem and I'll wait for your input before landing this.
> 
> Downloads can't tell (without very fragile logic) whether a storage partition
> has technically been discarded. Currently storage partitions are always
> instantiated if necessary when a requester shows up with a site URL. So in the
> case you mention where an in-memory storage partition was used to instantiate
a
> download and we are trying to resume said download in a subsequent browser
> session, the request will be created and it will proceed with the "same"
storage
> partition. Except it won't have any cookies in it.
> 
> I've added a test for this to the web_view_browsertest.
> 

Interesting.  I could see an argument for not resuming the download if you come
back and the StoragePartition isn't persistent, but it's probably ok to just try
to resume anyway (and possibly fail due to the lack of cookies).  I'm ok with
that.

Anyway, very sorry for the delay, and LGTM.

https://codereview.chromium.org/1924473003/diff/80001/content/public/browser/...
File content/public/browser/download_url_parameters.cc (right):

https://codereview.chromium.org/1924473003/diff/80001/content/public/browser/...
content/public/browser/download_url_parameters.cc:40:
std::unique_ptr<DownloadUrlParameters> DownloadUrlParameters::FromWebContents(
On 2016/05/07 00:24:45, asanka wrote:
> On 2016/05/05 at 21:43:23, Charlie Reis wrote:
> > I'm a bit concerned about this method, which assumes the download is in the
> main frame.  It's called from places like
WebContentsImpl::SaveFrameWithHeaders
> and RenderViewContextMenu::ExecuteCommand-- are we sure those aren't called
when
> you interact with a <webview> on a page like chrome://signin?
> > 
> > Maybe this is ok for now, with a bug/TODO to move it to be frame specific if
> necessary?  (I'm not sure how it's used, so maybe it's safe.)
> 
> I think this should be fixed,  but I think that should be a separate change.
> 
> For the moment, <webview>s are guest WebContents, correct? Hence talking to
the
> WebContents should be fine. There are problems with how we deal with context
> menus when OOPIF are involved, but that's something that'll need to be dealt
> with separately for this change.

Ok.  I wasn't sure how context menus worked for <webview>s, so hopefully it's
not an issue.

(Tangent: I'd be curious to know what issues you're referrng to with context
menus and OOPIFs.  Maybe you can file a bug and CC me if there's a known issue?)

https://codereview.chromium.org/1924473003/diff/100001/chrome/browser/apps/gu...
File chrome/browser/apps/guest_view/web_view_browsertest.cc (right):

https://codereview.chromium.org/1924473003/diff/100001/chrome/browser/apps/gu...
chrome/browser/apps/guest_view/web_view_browsertest.cc:2578: // Of the two
downloads, ?cookie=first will succeed and ?cookie=second with
nit: s/with/will/

(Thanks for testing this case!)

[asanka, 2016-05-11 03:14:53]

https://codereview.chromium.org/1924473003/diff/80001/content/public/browser/...
File content/public/browser/download_url_parameters.cc (right):

https://codereview.chromium.org/1924473003/diff/80001/content/public/browser/...
content/public/browser/download_url_parameters.cc:40:
std::unique_ptr<DownloadUrlParameters> DownloadUrlParameters::FromWebContents(
On 2016/05/10 at 22:50:02, Charlie Reis wrote:
> On 2016/05/07 00:24:45, asanka wrote:
> > On 2016/05/05 at 21:43:23, Charlie Reis wrote:
> > > I'm a bit concerned about this method, which assumes the download is in
the
> > main frame.  It's called from places like
WebContentsImpl::SaveFrameWithHeaders
> > and RenderViewContextMenu::ExecuteCommand-- are we sure those aren't called
when
> > you interact with a <webview> on a page like chrome://signin?
> > > 
> > > Maybe this is ok for now, with a bug/TODO to move it to be frame specific
if
> > necessary?  (I'm not sure how it's used, so maybe it's safe.)
> > 
> > I think this should be fixed,  but I think that should be a separate change.
> > 
> > For the moment, <webview>s are guest WebContents, correct? Hence talking to
the
> > WebContents should be fine. There are problems with how we deal with context
> > menus when OOPIF are involved, but that's something that'll need to be dealt
> > with separately for this change.
> 
> Ok.  I wasn't sure how context menus worked for <webview>s, so hopefully it's
not an issue.
> 
> (Tangent: I'd be curious to know what issues you're referrng to with context
menus and OOPIFs.  Maybe you can file a bug and CC me if there's a known issue?)

I was referring partly to https://codereview.chromium.org/1836973003/#msg11

https://codereview.chromium.org/1924473003/diff/100001/chrome/browser/apps/gu...
File chrome/browser/apps/guest_view/web_view_browsertest.cc (right):

https://codereview.chromium.org/1924473003/diff/100001/chrome/browser/apps/gu...
chrome/browser/apps/guest_view/web_view_browsertest.cc:2578: // Of the two
downloads, ?cookie=first will succeed and ?cookie=second with
On 2016/05/10 at 22:50:02, Charlie Reis wrote:
> nit: s/with/will/
> 
> (Thanks for testing this case!)

Done. THanks!

[asanka, 2016-05-11 03:16:36]

The CQ bit was checked by asanka@chromium.org

[asanka, 2016-05-11 03:16:36]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
svaldez@chromium.org, jam@chromium.org, asargent@chromium.org,
creis@chromium.org
Link to the patchset: https://codereview.chromium.org/1924473003/#ps120001
(title: "Fix typo in comment")

[commit-bot: I haz the power, 2016-05-11 03:16:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1924473003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1924473003/120001

[commit-bot: I haz the power, 2016-05-11 04:28:07]

Description was changed from

==========
[Downloads] Use the initiating StoragePartition for resumption.

The URLRequest created for downloads resumption should be created using
the same StoragePartition as the one used to initiate the download.
Downloads used to use the request context returned via
ResourceContext::GetURLRequestContext(), which can yield the wrong
URLRequestContext (and the wrong StoragePartition) when multiple
StoragePartitions are in use.

Currently non-default StoragePartitions are used for implementing
WebView tag in platform apps. Hence a download that's initiated in a
WebView, if resumed, could result in a network request being issued
that's backed by the wrong StoragePartition without this fix.

This change:

  * Adds the site instance URL to the metadata that's persisted for each
    download.

  * Updates history database schema to accommodate site URL.

  * Updates DownloadURLParameters to take a URLRequestContextGetter
    instead of a ResourceContext. The latter can be uniquely determined
    by the DownloadManager.

  * Updates DownloadItemImpl to use the site instance URL to determine
    the correct StoragePartition to use during resumption.

The rest is assorted tests and plumbing.

R=creis@chromium.org,sky@chromium.org,asargent@chromium.org,jam@chromium.org,...
BUG=606627
==========

to

==========
[Downloads] Use the initiating StoragePartition for resumption.

The URLRequest created for downloads resumption should be created using
the same StoragePartition as the one used to initiate the download.
Downloads used to use the request context returned via
ResourceContext::GetURLRequestContext(), which can yield the wrong
URLRequestContext (and the wrong StoragePartition) when multiple
StoragePartitions are in use.

Currently non-default StoragePartitions are used for implementing
WebView tag in platform apps. Hence a download that's initiated in a
WebView, if resumed, could result in a network request being issued
that's backed by the wrong StoragePartition without this fix.

This change:

  * Adds the site instance URL to the metadata that's persisted for each
    download.

  * Updates history database schema to accommodate site URL.

  * Updates DownloadURLParameters to take a URLRequestContextGetter
    instead of a ResourceContext. The latter can be uniquely determined
    by the DownloadManager.

  * Updates DownloadItemImpl to use the site instance URL to determine
    the correct StoragePartition to use during resumption.

The rest is assorted tests and plumbing.

R=creis@chromium.org,sky@chromium.org,asargent@chromium.org,jam@chromium.org,...
BUG=606627
==========

[commit-bot: I haz the power, 2016-05-11 04:28:09]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-05-11 04:29:30]

Description was changed from

==========
[Downloads] Use the initiating StoragePartition for resumption.

The URLRequest created for downloads resumption should be created using
the same StoragePartition as the one used to initiate the download.
Downloads used to use the request context returned via
ResourceContext::GetURLRequestContext(), which can yield the wrong
URLRequestContext (and the wrong StoragePartition) when multiple
StoragePartitions are in use.

Currently non-default StoragePartitions are used for implementing
WebView tag in platform apps. Hence a download that's initiated in a
WebView, if resumed, could result in a network request being issued
that's backed by the wrong StoragePartition without this fix.

This change:

  * Adds the site instance URL to the metadata that's persisted for each
    download.

  * Updates history database schema to accommodate site URL.

  * Updates DownloadURLParameters to take a URLRequestContextGetter
    instead of a ResourceContext. The latter can be uniquely determined
    by the DownloadManager.

  * Updates DownloadItemImpl to use the site instance URL to determine
    the correct StoragePartition to use during resumption.

The rest is assorted tests and plumbing.

R=creis@chromium.org,sky@chromium.org,asargent@chromium.org,jam@chromium.org,...
BUG=606627
==========

to

==========
[Downloads] Use the initiating StoragePartition for resumption.

The URLRequest created for downloads resumption should be created using
the same StoragePartition as the one used to initiate the download.
Downloads used to use the request context returned via
ResourceContext::GetURLRequestContext(), which can yield the wrong
URLRequestContext (and the wrong StoragePartition) when multiple
StoragePartitions are in use.

Currently non-default StoragePartitions are used for implementing
WebView tag in platform apps. Hence a download that's initiated in a
WebView, if resumed, could result in a network request being issued
that's backed by the wrong StoragePartition without this fix.

This change:

  * Adds the site instance URL to the metadata that's persisted for each
    download.

  * Updates history database schema to accommodate site URL.

  * Updates DownloadURLParameters to take a URLRequestContextGetter
    instead of a ResourceContext. The latter can be uniquely determined
    by the DownloadManager.

  * Updates DownloadItemImpl to use the site instance URL to determine
    the correct StoragePartition to use during resumption.

The rest is assorted tests and plumbing.

R=creis@chromium.org,sky@chromium.org,asargent@chromium.org,jam@chromium.org,...
BUG=606627

Committed: https://crrev.com/3b4be1260ebe931143425302a867b7e7407d002b
Cr-Commit-Position: refs/heads/master@{#392849}
==========

[commit-bot: I haz the power, 2016-05-11 04:29:31]

Patchset 7 (id:??) landed as
https://crrev.com/3b4be1260ebe931143425302a867b7e7407d002b
Cr-Commit-Position: refs/heads/master@{#392849}