794606 - nsDownload::Resume creates a channel out of thin air

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[Josh Matthews [:jdm]]

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/nsDownloadManager.cpp#2963

This means that a previously private download can be paused and resumed, and it won't be treated as private any more (ie. disk cache, etc.) This is bad. This will be easiest to fix after bug 722859 lands, when we'll have the per-download privacy information available.

Comment 1

[(no longer active)]

Does that mean that bug 722859 needs to get fixed for 18 as well?

Comment 2

[Josh Matthews [:jdm]]

I was under the impression that was already the case, since I believe mobile is using the download manager.

Comment 3

[(no longer active)]

(In reply to comment #2)
> I was under the impression that was already the case, since I believe mobile is
> using the download manager.

Not sure what mobile has to do with this...  I'm talking about the impact of this as far as the cache is concerned.

Comment 4

[Josh Matthews [:jdm]]

Attachment: Make resuming a download correctly share the privacy status of the original download.

The fix here is really simple. The test, on the other hand, only passes if the three dependent bugs are fixed. It's delightful.

Comment 5

[Josh Matthews [:jdm]]

Also in terms of the cache, I believe we're fine. We refuse to cache any HTTP channel that is an explicit resume action (ie. has had ResumeAt called on it). Hence, I had to resort to playing with cookies to get results out of the test.

Comment 6

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

Comment on attachment 665784 [details] [diff] [review]
Make resuming a download correctly share the privacy status of the original download.

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

::: toolkit/components/downloads/test/unit/test_private_resume.js
@@ +5,5 @@
> +this.__defineGetter__("dm", function() {
> +  delete this.dm;
> +  return this.dm = Cc["@mozilla.org/download-manager;1"].
> +                   getService(Ci.nsIDownloadManager);
> +});

just use downloadUtils.downloadManager? it's defined in head_download_manager.js

@@ +33,5 @@
> +    }
> +    let full = "";
> +    let body = "ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff"; //60
> +    for (var i = 0; i < 1000; i++)
> +      full += body;

nit: please always brace loops

Comment 7

[Josh Matthews [:jdm]]

Attachment: Make resuming a download correctly share the privacy status of the original download.

Comment 8

[Josh Matthews [:jdm]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/e5269e137290

Comment 9

[Josh Matthews [:jdm]]

Backed out. Relanded in https://hg.mozilla.org/integration/mozilla-inbound/rev/0e4560cb5a35.

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0e4560cb5a35