Handle downloads started in private browsing mode

RESOLVED FIXED in mozilla24

Status

()

Toolkit
Downloads API
RESOLVED FIXED
5 years ago
a year ago

People

(Reporter: Paolo, Assigned: raymondlee)

Tracking

(Blocks: 2 bugs)

Trunk
mozilla24
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [tor])

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

5 years ago
In the jsdownloads API, downloads started in private browsing mode should set
the correct properties on the source network channel, and should be added to
the appropriate global download list.

The aOptions argument of createDownload would have the "private" property
that determines whether the newly created download is private.
(Assignee)

Updated

5 years ago
Assignee: nobody → raymond
(Assignee)

Comment 1

5 years ago
Created attachment 745033 [details] [diff] [review]
Patch for /toolkit v1
Attachment #745033 - Flags: review?(paolo.mozmail)
(Assignee)

Comment 2

5 years ago
Created attachment 745035 [details] [diff] [review]
Patch for /browser v1
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
(Reporter)

Comment 3

5 years ago
Comment on attachment 745033 [details] [diff] [review]
Patch for /toolkit v1

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

This also looks good, the main thing to do now is differentiating private and public network requests calling nsIPrivateBrowsingChannel::setPrivate with either true or false as the argument (only if the channel is an instance of nsIPrivateBrowsingChannel).

We should also add a test that looks like this:
 - Register a test-specific HTTP handler on our HTTP server,
   that verifies that no cookie is sent in the request, and
   also sets a cookie in the response
 - Execute one public download to the handler's URL
 - Then, execute one private download to the same URL
 - Clean up by clearing all private and public cookies

::: toolkit/components/jsdownloads/src/DownloadCore.jsm
@@ +420,5 @@
>     */
>    uri: null,
> +
> +  /**
> +   * The boolean indicates that the download is from a private window or not.

"Indicates whether the download originated from a private window.  This determines the context of the network request that is made to retrieve the resource."

::: toolkit/components/jsdownloads/src/Downloads.jsm
@@ +56,5 @@
>     *        {
>     *          source: {
>     *            uri: The nsIURI for the download source.
> +   *            isPrivate: The boolean indicates whether this download is
> +   *                       initialized from a private window or not.

"isPrivate: Indicates whether the download originated from a private window."

@@ +78,5 @@
>        let download = new Download();
>  
>        download.source = new DownloadSource();
>        download.source.uri = aProperties.source.uri;
> +      download.source.isPrivate = (aProperties.source.isPrivate == true);

No need to coerce this to a boolean, for now. We may add input validation in the future.

@@ +158,5 @@
> +   * Retrieves the DownloadList object for downloads that were started from
> +   * a private browsing window.
> +   *
> +   * Calling this function may cause the download list to be reloaded from the
> +   * previous session, if it wasn't loaded already.

These two lines don't apply to the private list.

::: toolkit/components/jsdownloads/test/unit/test_Downloads.js
@@ +28,5 @@
>  
>  /**
> +* Tests createDownload for private download.
> + */
> +add_task(function test_createDownloadForPrivateDownload()

test_createDownload_private (clearly separates the name of the tested function)

@@ +31,5 @@
> + */
> +add_task(function test_createDownloadForPrivateDownload()
> +{
> +  let download = yield Downloads.createDownload({
> +    source: { uri: NetUtil.newURI("about:blank"), isPrivate: true },

nit: indenting like this seems clearer:

    source: { uri: NetUtil.newURI("about:blank"),
              isPrivate: true },

@@ +41,5 @@
> +
> +/**
> + * Tests createDownload for normal (public) download.
> + */
> +add_task(function test_createDownloadForPublicDownload()

test_createDownload_public

@@ +112,5 @@
> + * Tests that the getPublicDownloadList and getPrivateDownloadList function
> + * and returns the different list.  More detailed tests are implemented
> + * separately for the DownloadList module.
> + */
> +add_task(function test_getPublicAndPrivateDownloadList()

test_public_and_private_lists_differ
Attachment #745033 - Flags: review?(paolo.mozmail)
(Reporter)

Comment 4

5 years ago
Ideally, we should also add a test for the case that the channel from which we
are downloading does not implement nsIPrivateBrowsingChannel.

Josh, do you know if there any URIs that result in channels that actually don't
implement nsIPrivateBrowsingChannel? The interface seems to be implemented by
nsBaseChannel, hence the question.
Flags: needinfo?(josh)

Comment 5

5 years ago
I'm not aware of any URIs that result in non-nsIPrivateBrowsingChannel channels.
Flags: needinfo?(josh)
(Reporter)

Comment 6

5 years ago
(In reply to Josh Matthews [:jdm] from comment #5)
> I'm not aware of any URIs that result in non-nsIPrivateBrowsingChannel
> channels.

Thanks! So, Raymond, I don't think we need to test that case, though I think it
is still a good measure to do an instanceof check for the interface.
(Assignee)

Comment 7

5 years ago
Created attachment 747416 [details] [diff] [review]
v2 WIP

Paolo: I have an issue with getting the HTTP handler to work properly and hope that you can give me some advances.

I have added a test called test_download_public_and_private in test_DownloadCore.js. The test would triggers few downloads, however, the handler is only called once which doesn't match the number of downloads.  Do you have any ideas what might cause the issue?
Attachment #745033 - Attachment is obsolete: true
Flags: needinfo?(paolo.mozmail)
(Assignee)

Updated

5 years ago
Blocks: 836483
(Reporter)

Comment 8

5 years ago
(In reply to Raymond Lee [:raymondlee] from comment #7)
> I have added a test called test_download_public_and_private in
> test_DownloadCore.js. The test would triggers few downloads, however, the
> handler is only called once which doesn't match the number of downloads.  Do
> you have any ideas what might cause the issue?

Is it possible that registering a handler for "/empty.txt" conflicts with the
existing file name? You may want to use a different file name, maybe the same
name as the test function. By the way, you also need to unregister the handler
after the last download finished.

do_test_pending() and do_test_finished() should also be unnecessary, as you're
waiting on each download to be finished before the task terminates (I guess the
missing yield for the third download is just a typo).
Flags: needinfo?(paolo.mozmail)
(Assignee)

Comment 9

5 years ago
Created attachment 747911 [details] [diff] [review]
v2 wip2

(In reply to Paolo Amadini [:paolo] from comment #8)
> (In reply to Raymond Lee [:raymondlee] from comment #7)
> > I have added a test called test_download_public_and_private in
> > test_DownloadCore.js. The test would triggers few downloads, however, the
> > handler is only called once which doesn't match the number of downloads.  Do
> > you have any ideas what might cause the issue?
> 
> Is it possible that registering a handler for "/empty.txt" conflicts with the
> existing file name? You may want to use a different file name, maybe the same
> name as the test function. By the way, you also need to unregister the
> handler
> after the last download finished.
> 
> do_test_pending() and do_test_finished() should also be unnecessary, as
> you're
> waiting on each download to be finished before the task terminates (I guess
> the
> missing yield for the third download is just a typo).

Fixed the above.

I've spent some more further time to further investigate the issue.  The handle gets called the right number of times.  However, for the second download (testCount == 1), the cookie doesn't exist at all. I wonder that do we need to add any code in DCS_execute() for cookie to work when the download carries out?
Attachment #747416 - Attachment is obsolete: true
(Reporter)

Comment 10

5 years ago
(In reply to Raymond Lee [:raymondlee] from comment #9)
> I've spent some more further time to further investigate the issue.  The
> handle gets called the right number of times.  However, for the second
> download (testCount == 1), the cookie doesn't exist at all. I wonder that do
> we need to add any code in DCS_execute() for cookie to work when the
> download carries out?

I've noticed that some xpcshell tests in "netwerk/cookie/test/unit" call:

  Services.prefs.setIntPref("network.cookie.cookieBehavior", 0);

It appears that this allows the cookie to be set. I don't know all the theory
behind this, but it seems enough for what we need to do, if we take care to
reset the preference when the test is finished using do_register_cleanup.
The cookieBehavior pref controls whether we allow third-party cookies by default or not.
(Assignee)

Comment 12

5 years ago
Created attachment 749128 [details] [diff] [review]
v2

Thanks! The cookie preference works!
Attachment #747911 - Attachment is obsolete: true
Attachment #749128 - Flags: review?(paolo.mozmail)
(Assignee)

Updated

5 years ago
Attachment #745035 - Attachment is obsolete: true
(Reporter)

Comment 13

5 years ago
Comment on attachment 749128 [details] [diff] [review]
v2

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

This looks really good! I listed a few changes below that should simplify the test.

Then, the simplified test should be replicated in test_DownloadLegacy.js, with only the third download changed to a call to promiseStartLegacyDownload. This function could be changed to accept an aIsPrivate parameter, or aOutPersist could be changed to be a general purpose aOptions object, as you prefer:

promiseStartLegacyDownload(aSourceURI, aIsPrivate, aOutPersist)
promiseStartLegacyDownload(aSourceURI, aOptions)

::: toolkit/components/jsdownloads/src/DownloadCore.jsm
@@ +606,3 @@
>        let channel = NetUtil.newChannel(download.source.uri);
> +      if (channel instanceof Ci.nsIPrivateBrowsingChannel) {
> +        channel.setPrivate(download.source.isPrivate ? true : false);

channel.setPrivate(download.source.isPrivate);

::: toolkit/components/jsdownloads/src/Downloads.jsm
@@ +55,5 @@
>     *        Provides the initial properties for the newly created download.
>     *        {
>     *          source: {
>     *            uri: The nsIURI for the download source.
> +   *            isPrivate: Indicates whether the download originated from a 

nit: whitespace at end of line

::: toolkit/components/jsdownloads/test/unit/test_DownloadCore.js
@@ +687,5 @@
> +  // Apply pref to allow all cookies.
> +  Services.prefs.setIntPref("network.cookie.cookieBehavior", 0);
> +
> +  do_register_cleanup(function() {
> +    Services.prefs.setIntPref("network.cookie.cookieBehavior", cookieBehavior);

You can just call Services.prefs.clearUserPref here, without storing the previous value.

Also, the do_register_cleanup function is executed at the end of the entire test file. To make sure that all test cases in the same file are run in the same environment, it's useful to define a local cleanup function, and call it at the and of the test case, in addition to calling do_register_cleanup on it at the beginning of the test case.

@@ +695,5 @@
> +  let source_uri = NetUtil.newURI(HTTP_BASE + source_path);
> +  let testCount = 0;
> +
> +  gHttpServer.registerPathHandler(source_path, function (aRequest, aResponse) {
> +    aResponse.setStatusLine(aRequest.httpVersion, 200, "OK");

I think setStatusLine may be unnecessary?

@@ +715,5 @@
> +      testCount++;
> +    } else if (testCount == 3)  {
> +      // Clear cookie for public download.
> +      //do_check_true(aRequest.hasHeader("Cookie"));
> +      aResponse.setHeader("Set-Cookie", "");

I don't think this actually removes the cookie. I just found an easier way:

Services.cookies.removeAll();

I didn't test this yet, you might want to verify that this works. If so, please add this call to the cleanup function.

@@ +716,5 @@
> +    } else if (testCount == 3)  {
> +      // Clear cookie for public download.
> +      //do_check_true(aRequest.hasHeader("Cookie"));
> +      aResponse.setHeader("Set-Cookie", "");
> +      gHttpServer.registerPathHandler(source_path, null);

I'd like to see this deregistration moved to the cleanup function as well. This will make the fourth download unnecessary.

@@ +725,5 @@
> +    source: { uri: source_uri },
> +    target: { file: getTempFile(TEST_TARGET_FILE_NAME) },
> +    saver: { type: "copy" },
> +  });
> +  yield download.start();

For the first two downloads, you may just do:

let targetFile = getTempFile(TEST_TARGET_FILE_NAME);
yield Downloads.simpleDownload(source_uri, targetFile);
yield Downloads.simpleDownload(source_uri, targetFile);

(We'll revise the simpleDownload API for private downloads in another bug.)
Attachment #749128 - Flags: review?(paolo.mozmail)
(Assignee)

Comment 14

5 years ago
Created attachment 752613 [details] [diff] [review]
v3

Updated the patch based on previous comment.
Attachment #749128 - Attachment is obsolete: true
Attachment #752613 - Flags: review?(paolo.mozmail)
(Reporter)

Comment 15

5 years ago
Comment on attachment 752613 [details] [diff] [review]
v3

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

This is another leap forward in the new API - thanks for helping out!

::: toolkit/components/jsdownloads/test/unit/test_DownloadCore.js
@@ +697,5 @@
> +  }
> +
> +  do_register_cleanup(function() {
> +    cleanup();
> +  });

nit: do_register_cleanup(cleanup);

::: toolkit/components/jsdownloads/test/unit/test_DownloadLegacy.js
@@ +227,5 @@
>  add_task(function test_cancel_midway()
>  {
>    let deferResponse = deferNextResponse();
>    let outPersist = {};
> +  let download = yield promiseStartLegacyDownload(TEST_INTERRUPTIBLE_URI, null,

nit: false instead of null

@@ +321,5 @@
> +  }
> +
> +  do_register_cleanup(function() {
> +    cleanup();
> +  });

nit: do_register_cleanup(cleanup);
Attachment #752613 - Flags: review?(paolo.mozmail) → review+
(Assignee)

Comment 16

5 years ago
Created attachment 752694 [details] [diff] [review]
v4

Pushed to try and waiting for results
https://tbpl.mozilla.org/?tree=Try&rev=0198879b452c
Attachment #752613 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/e968ebc8bbb1
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e968ebc8bbb1
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24

Updated

4 years ago
Whiteboard: [tor]
Blocks: 1260929
You need to log in before you can comment on or make changes to this bug.