Closed Bug 836439 Opened 11 years ago Closed 11 years ago

Handle downloads started in private browsing mode

Categories

(Toolkit :: Downloads API, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: Paolo, Assigned: raymondlee)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [tor])

Attachments

(1 file, 6 obsolete files)

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: nobody → raymond
Attached patch Patch for /toolkit v1 (obsolete) — Splinter Review
Attachment #745033 - Flags: review?(paolo.mozmail)
Attached patch Patch for /browser v1 (obsolete) — Splinter Review
Status: NEW → ASSIGNED
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)
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)
I'm not aware of any URIs that result in non-nsIPrivateBrowsingChannel channels.
Flags: needinfo?(josh)
(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.
Attached patch v2 WIP (obsolete) — Splinter Review
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)
Blocks: 836483
(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)
Attached patch v2 wip2 (obsolete) — Splinter Review
(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
(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.
Attached patch v2 (obsolete) — Splinter Review
Thanks! The cookie preference works!
Attachment #747911 - Attachment is obsolete: true
Attachment #749128 - Flags: review?(paolo.mozmail)
Attachment #745035 - Attachment is obsolete: true
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)
Attached patch v3 (obsolete) — Splinter Review
Updated the patch based on previous comment.
Attachment #749128 - Attachment is obsolete: true
Attachment #752613 - Flags: review?(paolo.mozmail)
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+
Attached patch v4Splinter Review
Pushed to try and waiting for results
https://tbpl.mozilla.org/?tree=Try&rev=0198879b452c
Attachment #752613 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e968ebc8bbb1
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Whiteboard: [tor]
Blocks: meta_tor
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: