Mark channels used for download as throttleable

RESOLVED FIXED in Firefox 55

Status

()

Toolkit
Downloads API
P1
normal
RESOLVED FIXED
9 months ago
8 months ago

People

(Reporter: mayhemer, Assigned: mayhemer)

Tracking

(Blocks: 1 bug)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

9 months ago
(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).
(Assignee)

Updated

8 months ago
Priority: -- → P1
(Assignee)

Updated

8 months ago
Assignee: nobody → honzab.moz
Status: NEW → ASSIGNED
(Assignee)

Comment 1

8 months ago
Created attachment 8856230 [details] [diff] [review]
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
Attachment #8856230 - Flags: review?(paolo.mozmail)
Attachment #8856230 - Flags: review?(hurley)
(Assignee)

Comment 2

8 months ago
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
Attachment #8856230 - Attachment is obsolete: true
Attachment #8856230 - Flags: review?(paolo.mozmail)
Attachment #8856230 - Flags: review?(hurley)
Attachment #8856237 - Flags: review?(paolo.mozmail)
Attachment #8856237 - Flags: review?(hurley)
(Assignee)

Comment 3

8 months ago
(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 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.
Attachment #8856237 - Flags: review?(hurley) → review+

Comment 5

8 months ago
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;".
Attachment #8856237 - Flags: review?(paolo.mozmail) → review-
(Assignee)

Comment 6

8 months ago
(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

8 months ago
(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.
(Assignee)

Comment 8

8 months ago
(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?
Flags: needinfo?(paolo.mozmail)
(Assignee)

Comment 9

8 months ago
Created attachment 8857528 [details] [diff] [review]
v2

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c3d6fc9eb8f6467e8c6af499f1895d4a606b92bc
Attachment #8856237 - Attachment is obsolete: true
Attachment #8857528 - Flags: review?(paolo.mozmail)
(Assignee)

Comment 10

8 months ago
Comment on attachment 8857528 [details] [diff] [review]
v2

better:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f448ab3b6386ea53e62eed3bac9f001860b0c39c
Comment hidden (mozreview-request)

Comment 12

8 months ago
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.
Flags: needinfo?(paolo.mozmail)
Attachment #8857528 - Flags: review?(paolo.mozmail) → review+

Comment 13

8 months ago
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

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

Comment 15

8 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=493be83de2a25f84108f38082d019e7c44598d58
(Assignee)

Comment 16

8 months ago
(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

8 months ago
mozreview-review
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.
Attachment #8858608 - Flags: review?(mak77) → review+

Comment 18

8 months ago
mozreview-review
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

8 months ago
(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.
Flags: needinfo?(honzab.moz)

Comment 20

8 months ago
Pushed by paolo.mozmail@amadzone.org:
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

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e566c50eeb1b
https://hg.mozilla.org/mozilla-central/rev/ffe6a7d2c74c
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Assignee)

Comment 22

8 months ago
(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.
Flags: needinfo?(honzab.moz)
(Assignee)

Updated

8 months ago
Blocks: 1360603
Depends on: 1360793
Depends on: 1360865
You need to log in before you can comment on or make changes to this bug.