1348062 - Mark channels used for download as throttleable

Status: RESOLVED FIXED

(Toolkit :: Downloads API, enhancement, P1)

Description

[Honza Bambas (:mayhemer)]

(not sure of the component)

This might "just work" for most direct download links when we fix bug 1348044.  But we also must take care for channels the are created for resumed downloads (session/manual resume).

Comment 1

[Honza Bambas (:mayhemer)]

Attachment: v1

- nsHttpChannel::SetChannelIsForDownload sets the class and adds the channel to the throttler service
- resumed downloads are correctly marked as 'for download'
- I changed the MOZ_ASSERT(false) to just return NS_OK when we are trying to add the same channel twice ; reason is that regardless if the channel is SetChannelIsForDownload(true) before or after AsyncOpen, both SetChannelIsForDownload and BeginConnect adds (and have to) the channel to the throttler service.  SetChannelIsForDownload can be called even after OnStartRequest.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ecc17b0da36c73c4cc243cd36fbb0d9df0f6382a

Comment 2

[Honza Bambas (:mayhemer)]

Attachment: v1.1

Fixed JS exception, otherwise identical to the patch from comment 1

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb6a4e01428533ec2749406eb4608f5ba61d915b

Comment 3

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #2)
> Created attachment 8856237 [details] [diff] [review]
> v1.1
> 
> Fixed JS exception, otherwise identical to the patch from comment 1
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=bb6a4e01428533ec2749406eb4608f5ba61d915b

An interesting perma failure:
https://treeherder.mozilla.org/logviewer.html#?job_id=89918125&repo=try

Comment 4

[u408661]

Comment on attachment 8856237 [details] [diff] [review]
v1.1

Review of attachment 8856237 [details] [diff] [review]:
-----------------------------------------------------------------

One comment below, once that's addressed, I'm good with this.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +6312,5 @@
> +  nsIThrottlingService *throttler = gHttpHandler->GetThrottlingService();
> +  if (throttler) {
> +    if (aChannelIsForDownload) {
> +      AddClassFlags(nsIClassOfService::Throttleable);
> +      throttler->AddChannel(this);

I mentioned this in the bug where you explicitly stop reading from the network for h1 connections, but I'll mention it again here - it seems like we should automagically call the throttler methods in OnClassFlagsChanged. That way we can avoid potential future situations where we call {Add,Remove}ClassFlags without updating the throttler's view of the world.

Comment 5

[:Paolo Amadini]

Comment on attachment 8856237 [details] [diff] [review]
v1.1

Review of attachment 8856237 [details] [diff] [review]:
-----------------------------------------------------------------

This needs a test in common_test_Download.js, for both states of gUseLegacySaver. The DownloadCopySaver is in fact only used for downloads that are resumed and for some downloads initiated from the browser interface, but the most common case is for downloads to start from the external helper application service.

The test should at least ensure that we set the correct properties on the channel. I don't know the throttling code, so I don't know if we can test if the data is actually throttled, but I expect that testing the channel properties should be enough.

::: toolkit/components/jsdownloads/src/DownloadCore.jsm
@@ +1975,5 @@
>                download.source.referrer) {
>              channel.referrer = NetUtil.newURI(download.source.referrer);
>            }
>  
> +          // This makes the channel be corretly throttled during page loads.

There is a comment in nsExternalHelperAppService that says that channelIsForDownload affects caching:

https://dxr.mozilla.org/mozilla-central/rev/b1364675bdf5dffe63fd60373034293de0b513d5/uriloader/exthandler/nsExternalHelperAppService.cpp#1674-1679

I don't know that code, but we should make sure that this has no adverse effect here, considering that this code path is used for resuming downloads. Normally we keep data in ".part" files, but when this is not the case we shouldn't regress data usage over the network. I think you know caching behavior better than me, so I'll leave this to you ;-)

The two comments here and in nsExternalHelperAppService should also be updated so that they are consistent with each other.

@@ +1976,5 @@
>              channel.referrer = NetUtil.newURI(download.source.referrer);
>            }
>  
> +          // This makes the channel be corretly throttled during page loads.
> +          if (channel instanceof Ci.nsIHttpChannelInternal) {

The instanceof operator makes the interface available on the operand, so you can just call "channel.channelIsForDownload = true;".

Comment 6

[Honza Bambas (:mayhemer)]

(In reply to :Paolo Amadini from comment #5)
> Comment on attachment 8856237 [details] [diff] [review]
> v1.1
> 
> Review of attachment 8856237 [details] [diff] [review]:

Thanks.

> -----------------------------------------------------------------
> 
> This needs a test in common_test_Download.js, for both states of
> gUseLegacySaver. The DownloadCopySaver is in fact only used for downloads
> that are resumed and for some downloads initiated from the browser
> interface,

Which is exactly what I want.

> but the most common case is for downloads to start from the
> external helper application service.

I know.

> 
> The test should at least ensure that we set the correct properties on the
> channel. 
> I don't know the throttling code, so I don't know if we can test if
> the data is actually throttled, but I expect that testing the channel
> properties should be enough.

We can check it's set isForDownload, that should be enough.  Can I just modify existing tests?

The throttling code should be tested elsewhere (frankly, we don't plane any).

> 
> ::: toolkit/components/jsdownloads/src/DownloadCore.jsm
> @@ +1975,5 @@
> >                download.source.referrer) {
> >              channel.referrer = NetUtil.newURI(download.source.referrer);
> >            }
> >  
> > +          // This makes the channel be corretly throttled during page loads.
> 
> There is a comment in nsExternalHelperAppService that says that
> channelIsForDownload affects caching:
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> b1364675bdf5dffe63fd60373034293de0b513d5/uriloader/exthandler/
> nsExternalHelperAppService.cpp#1674-1679
> 
> I don't know that code, but we should make sure that this has no adverse
> effect here, considering that this code path is used for resuming downloads.
> Normally we keep data in ".part" files, but when this is not the case we
> shouldn't regress data usage over the network. I think you know caching
> behavior better than me, so I'll leave this to you ;-)

Actually, it was a bug we *didn't* set isForDownload on channels that the saver spawned.  There is no implication to set isForDownload for resumed downloads.  But there may be when not ;)

> 
> The two comments here and in nsExternalHelperAppService should also be
> updated so that they are consistent with each other.
> 
> @@ +1976,5 @@
> >              channel.referrer = NetUtil.newURI(download.source.referrer);
> >            }
> >  
> > +          // This makes the channel be corretly throttled during page loads.
> > +          if (channel instanceof Ci.nsIHttpChannelInternal) {
> 
> The instanceof operator makes the interface available on the operand, so you
> can just call "channel.channelIsForDownload = true;".

Cool!  Didn't know, thanks.

Comment 7

[:Paolo Amadini]

(In reply to Honza Bambas (:mayhemer) from comment #6)
> We can check it's set isForDownload, that should be enough.  Can I just
> modify existing tests?

Yep, modifying an existing general test case works for me just like adding a new test case.

Comment 8

[Honza Bambas (:mayhemer)]

(In reply to :Paolo Amadini from comment #7)
> (In reply to Honza Bambas (:mayhemer) from comment #6)
> > We can check it's set isForDownload, that should be enough.  Can I just
> > modify existing tests?
> 
> Yep, modifying an existing general test case works for me just like adding a
> new test case.

Looking into common_test_Download.js, I am thinking of modifying test_basic.  But I have no idea how to reach the channel from a Download object.  Other option is to modify test_adjustChannel where the callback that exposes the download's channel is defined.  The second option seems to me simpler to do.

Then, do we need some modifications to the legacy code as well?

Comment 9

[Honza Bambas (:mayhemer)]

Attachment: v2

one small necko change moved to the base patch (bug 1348061).

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c3d6fc9eb8f6467e8c6af499f1895d4a606b92bc

Comment 10

[Honza Bambas (:mayhemer)]

Comment on attachment 8857528 [details] [diff] [review]
v2

better:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f448ab3b6386ea53e62eed3bac9f001860b0c39c

Comment 11

[:Paolo Amadini]

Attachment: Bug 1348062 - Test that channels are properly marked for downloads.

Downloads handled by nsIExternalHelperAppService pass a null request to the final onStateChange notification, thus we need to hold a reference to the request earlier. This also allows unit tests to access the request while the download is running.

Review commit: https://reviewboard.mozilla.org/r/130608/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/130608/

Comment 12

[:Paolo Amadini]

Comment on attachment 8857528 [details] [diff] [review]
v2

r+ but revert the change to common_test_Download.js, as I filed a separate patch.

Implementing a test for both DowloadCopySaver and DownloadLegacySaver was indeed tricky, and the change ended up being complex enough that I'd need review.

It seems to me that downloads started via nsIWebBrowserPersist are still not throttled. These may include downloads started by "Save Page As" or "Save Link As". If this is an issue, it should likely be handled in a separate bug.

Comment 13

[:Paolo Amadini]

Also, I just borrowed the test line from you patch, but it doesn't test anything about the "AddClassFlags(nsIClassOfService::Throttleable);" part, so the test would succeed even if that part didn't work as expected.

If there is a way to test these class flags, just tell me and I'll add it to my patch.

Comment 14

[:Paolo Amadini]

And I guess that FTP downloads are out of scope for this throttling code?

Comment 15

[:Paolo Amadini]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=493be83de2a25f84108f38082d019e7c44598d58

Comment 16

[Honza Bambas (:mayhemer)]

(In reply to :Paolo Amadini from comment #13)
> Also, I just borrowed the test line from you patch, but it doesn't test
> anything about the "AddClassFlags(nsIClassOfService::Throttleable);" part,
> so the test would succeed even if that part didn't work as expected.
> 
> If there is a way to test these class flags, just tell me and I'll add it to
> my patch.

Just QI the channel (request) to nsIClassOfService and read the |classFlags| attribute or in C++ go with GetClassFlags(uint32_t ** result).  All the flags are defined on that interface too.

Comment 17

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8858608 [details]
Bug 1348062 - Test that channels are properly marked for downloads.

https://reviewboard.mozilla.org/r/130608/#review134916

::: toolkit/components/jsdownloads/src/DownloadCore.jsm:2416
(Diff revision 1)
>     *        are never added to history even if this parameter is false.
>     */
>    onTransferStarted(aRequest, aAlreadyAddedToHistory) {
> +    // Store a reference to the request, used in some cases when handling
> +    // completion, and also checked during the download by unit tests.
> +    this.request = aRequest;

just to be sure, iirc on redirect the request changes, but I assume this happens after any eventual redirects already happened. I'm a bit rusty on this code, please drop if it's like that.

Comment 18

[:Paolo Amadini]

Comment on attachment 8858608 [details]
Bug 1348062 - Test that channels are properly marked for downloads.

https://reviewboard.mozilla.org/r/130608/#review135662

::: toolkit/components/jsdownloads/src/DownloadCore.jsm:2416
(Diff revision 1)
>     *        are never added to history even if this parameter is false.
>     */
>    onTransferStarted(aRequest, aAlreadyAddedToHistory) {
> +    // Store a reference to the request, used in some cases when handling
> +    // completion, and also checked during the download by unit tests.
> +    this.request = aRequest;

I verified that we have already processed all the redirects when we reach this point anyways.

Comment 19

[:Paolo Amadini]

(In reply to :Paolo Amadini from comment #12)
> It seems to me that downloads started via nsIWebBrowserPersist are still not
> throttled. These may include downloads started by "Save Page As" or "Save
> Link As". If this is an issue, it should likely be handled in a separate bug.

Honza, feel free to file a bug or not, based on whether this is relevant for throttling.

I'll land your patch and the test soon, including the verification for nsIClassOfService.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e566c50eeb1b
Mark channels used for downloads as throttable, r=nick+paolo
https://hg.mozilla.org/integration/mozilla-inbound/rev/ffe6a7d2c74c
Test that channels are properly marked for downloads. r=mak

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e566c50eeb1b
https://hg.mozilla.org/mozilla-central/rev/ffe6a7d2c74c

Comment 22

[Honza Bambas (:mayhemer)]

(In reply to :Paolo Amadini from comment #19)
> (In reply to :Paolo Amadini from comment #12)
> > It seems to me that downloads started via nsIWebBrowserPersist are still not
> > throttled. These may include downloads started by "Save Page As" or "Save
> > Link As". If this is an issue, it should likely be handled in a separate bug.
> 
> Honza, feel free to file a bug or not, based on whether this is relevant for
> throttling.
> 
> I'll land your patch and the test soon, including the verification for
> nsIClassOfService.

Oh, thanks for catching this!  Yes, I'll file a separate bug for it.