908246 - Private downloads should be finalized when the last private browsing window is closed

Status: VERIFIED FIXED

(Toolkit :: Downloads API, defect)

Description

[:Paolo Amadini]

The list of private downloads should be cleared on the "last-pb-context-exited"
notification. This includes in-progress downloads, since we already prompt
before allowing private browsing mode to finish.

Comment 1

[Raymond Lee [:raymondlee]]

Attachment: v1

Comment 2

[:Paolo Amadini]

Comment on attachment 800690 [details] [diff] [review]
v1

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

::: toolkit/components/jsdownloads/src/DownloadIntegration.jsm
@@ +770,5 @@
>        case "last-pb-context-exiting":
>          downloadsCount = this._privateInProgressDownloads.size;
>          this._confirmCancelDownloads(aSubject, downloadsCount, p,
>                                       p.ON_LEAVE_PRIVATE_BROWSING);
> +        if (aSubject.data) {

You should just listen for "last-pb-context-exited", because other "last-pb-context-exiting" observers may cancel the operation and in this case we shouldn't cancel the downloads.

@@ +773,5 @@
>                                       p.ON_LEAVE_PRIVATE_BROWSING);
> +        if (aSubject.data) {
> +          Task.spawn(function() {
> +            let list = yield Downloads.getPrivateDownloadList();
> +            list.removeAll();

I'd just inline this call for now, until we need a removeAll functionality elsewhere.

Note that you should call "finalize" on the downloads in addition to removing them from the list.

Comment 3

[Raymond Lee [:raymondlee]]

Attachment: v2

Comment 4

[Raymond Lee [:raymondlee]]

The patch requires a method in the patch of bug 908240

Comment 5

[:Paolo Amadini]

Comment on attachment 801398 [details] [diff] [review]
v2

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

::: toolkit/components/jsdownloads/src/DownloadIntegration.jsm
@@ +776,5 @@
> +          let list = yield Downloads.getPrivateDownloadList();
> +          let downloads = yield list.getAll();
> +
> +          while (downloads.length > 0) {
> +            let download = downloads.splice(0, 1)[0];

You may use "for (let download of downloads)", the array is already a static snapshot.

@@ +786,5 @@
> +
> +        // Handle test mode
> +        if (DownloadIntegration.testMode) {
> +          DownloadIntegration.testPromptDownloads =
> +            this._privateInProgressDownloads.size;

I don't get why we are creating a Set here, leftover from a previous implementation?

::: toolkit/components/jsdownloads/test/unit/test_DownloadIntegration.js
@@ +302,5 @@
>    privateList.remove(download2);
>  });
> +
> +/**
> + * Tests both the downloads list and the in-progress downloads are clear 

nit: whitespace at end of line

@@ +311,5 @@
> +  enableObserversTestMode();
> +
> +  let privateList = yield promiseNewPrivateDownloadList();
> +  let download1 = yield promiseNewDownload(httpUrl("interruptible.txt"));
> +  let download2 = yield promiseNewDownload(httpUrl("interruptible.txt"));

You should use "interruptible.txt" together with mustInterruptResponses() only for the download that shouldn't complete, the other should be read from "source.txt".

@@ +328,5 @@
> +  // Simulate exiting the private browsing but not canceling the download.
> +  DownloadIntegration.testPromptDownloads = -1;
> +  let promoseObserved = promiseTopicObserved("last-pb-context-exited");
> +  Services.obs.notifyObservers(null, "last-pb-context-exited", null);
> +  yield promoseObserved;

Waiting for the notification won't guarantee that the removal task is completed. We should use our own test mode promise, resolved when the task completes, like we do in the "launch" test cases.

Comment 6

[Raymond Lee [:raymondlee]]

Attachment: v3

(In reply to :Paolo Amadini from comment #5)
> Comment on attachment 801398 [details] [diff] [review]
> v2
> 
> Review of attachment 801398 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/jsdownloads/src/DownloadIntegration.jsm
> @@ +776,5 @@
> > +          let list = yield Downloads.getPrivateDownloadList();
> > +          let downloads = yield list.getAll();
> > +
> > +          while (downloads.length > 0) {
> > +            let download = downloads.splice(0, 1)[0];
> 
> You may use "for (let download of downloads)", the array is already a static
> snapshot.

Updated.

> 
> @@ +786,5 @@
> > +
> > +        // Handle test mode
> > +        if (DownloadIntegration.testMode) {
> > +          DownloadIntegration.testPromptDownloads =
> > +            this._privateInProgressDownloads.size;
> 
> I don't get why we are creating a Set here, leftover from a previous
> implementation?
> 

I have removed that.  Since we use DownloadList.remove() to remove downloads from the list, this._privateInProgressDownloads would be updated by view.onDownloadRemoved() automatically.


> ::: toolkit/components/jsdownloads/test/unit/test_DownloadIntegration.js
> @@ +302,5 @@
> >    privateList.remove(download2);
> >  });
> > +
> > +/**
> > + * Tests both the downloads list and the in-progress downloads are clear 
> 
> nit: whitespace at end of line
> 

Updated.

> @@ +311,5 @@
> > +  enableObserversTestMode();
> > +
> > +  let privateList = yield promiseNewPrivateDownloadList();
> > +  let download1 = yield promiseNewDownload(httpUrl("interruptible.txt"));
> > +  let download2 = yield promiseNewDownload(httpUrl("interruptible.txt"));
> 
> You should use "interruptible.txt" together with mustInterruptResponses()
> only for the download that shouldn't complete, the other should be read from
> "source.txt".
> 

Updated.

> @@ +328,5 @@
> > +  // Simulate exiting the private browsing but not canceling the download.
> > +  DownloadIntegration.testPromptDownloads = -1;
> > +  let promoseObserved = promiseTopicObserved("last-pb-context-exited");
> > +  Services.obs.notifyObservers(null, "last-pb-context-exited", null);
> > +  yield promoseObserved;
> 
> Waiting for the notification won't guarantee that the removal task is
> completed. We should use our own test mode promise, resolved when the task
> completes, like we do in the "launch" test cases.

Updated.

Comment 7

[:Paolo Amadini]

Comment on attachment 802129 [details] [diff] [review]
v3

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

Looks good! Thank you for helping with fixing this bug. r+ with the following change:

::: toolkit/components/jsdownloads/src/DownloadIntegration.jsm
@@ +824,5 @@
> +            download.finalize(true).then(null, Cu.reportError);
> +          }
> +        });
> +        deferred.then((value) => { DownloadIntegration._deferTestClearPrivateList.resolve("success"); },
> +                      (error) => { DownloadIntegration._deferTestClearPrivateList.reject(error); });

This should be done in test mode only, otherwise _deferTestClearPrivateList will be null.

Comment 8

[Raymond Lee [:raymondlee]]

Attachment: bug-908246-v4.patch

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

Comment 9

[Raymond Lee [:raymondlee]]

(In reply to Raymond Lee [:raymondlee] from comment #8)
> Created attachment 802753 [details] [diff] [review]
> bug-908246-v4.patch
> 
> Pushed to try and waiting for results
> https://tbpl.mozilla.org/?tree=Try&rev=d45acd4d3858

Passed Try!

Comment 10

[:Paolo Amadini]

https://hg.mozilla.org/integration/fx-team/rev/ed9f0499230d

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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

Comment 12

[Mihai Morar, (:MihaiMorar)]

I confirm the fix is verified on Latest Nightly 27 using Windows 7 x64 and Mac OS 10.8