836439 - Handle downloads started in private browsing mode

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[:Paolo Amadini]

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.

Comment 1

[Raymond Lee [:raymondlee]]

Attachment: Patch for /toolkit v1

Comment 2

[Raymond Lee [:raymondlee]]

Attachment: Patch for /browser v1

Comment 3

[:Paolo Amadini]

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

Comment 4

[:Paolo Amadini]

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.

Comment 5

[Josh Matthews [:jdm]]

I'm not aware of any URIs that result in non-nsIPrivateBrowsingChannel channels.

Comment 6

[:Paolo Amadini]

(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.

Comment 7

[Raymond Lee [:raymondlee]]

Attachment: 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?

Comment 8

[:Paolo Amadini]

(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).

Comment 9

[Raymond Lee [:raymondlee]]

Attachment: 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?

Comment 10

[:Paolo Amadini]

(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.

Comment 11

[Josh Matthews [:jdm]]

The cookieBehavior pref controls whether we allow third-party cookies by default or not.

Comment 12

[Raymond Lee [:raymondlee]]

Attachment: v2

Thanks! The cookie preference works!

Comment 13

[:Paolo Amadini]

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.)

Comment 14

[Raymond Lee [:raymondlee]]

Attachment: v3

Updated the patch based on previous comment.

Comment 15

[:Paolo Amadini]

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);

Comment 16

[Raymond Lee [:raymondlee]]

Attachment: v4

Pushed to try and waiting for results
https://tbpl.mozilla.org/?tree=Try&rev=0198879b452c

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e968ebc8bbb1

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e968ebc8bbb1