863063 - quitting private browsing mode does not delete partially downloaded files

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[Christian Ascheberg]

Attachment: patch v1

STR:
1. Open Private Browsing window
2. Start Download (in PB mode)
3. Close PB window
4. Confirm download cancel dialog
5. check download folder

Result:
partially downloaded file still exists

Expected:
partially downloaded file should have been deleted

The attached patch seems to fix the problem.

Comment 1

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

So, I think the problem ends up being explained by this condition
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/nsDownloadManager.cpp#234
That means that if we pause, and then remove a download, it won't invoke Cancel(), and Cancel() is where the .part file is removed
Though, the old code was doing the same, so I'm not sure why it should differ.
Paolo, any idea?
Off-hand I don't think we should retain .part files of private downloads and I'm not sure why we would ever need to pause them here.
Do you know if we have any tests for this?

Comment 2

[:Paolo Amadini]

(In reply to Marco Bonardo [:mak] from comment #1)
> That means that if we pause, and then remove a download, it won't invoke
> Cancel(), and Cancel() is where the .part file is removed

That's correct. Christian, can you tell us what happens when a download is
paused manually before closing the last private browsing window?

> Though, the old code was doing the same, so I'm not sure why it should
> differ.

Maybe this appeared when private browsing downloads were differentiated per window?

> Do you know if we have any tests for this?

I don't think so... I noticed a few test for the Cancel case, though.

Comment 3

[Christian Ascheberg]

(In reply to Paolo Amadini [:paolo] from comment #2)
> Christian, can you tell us what happens when a download is
> paused manually before closing the last private browsing window?

In that case the partially downloaded file will not be deleted either.
My patch does not fix the bug in that special case because I did not think of it, but it needs to be fixed as well.

Comment 4

[Christian Ascheberg]

Attachment: patch v2

also fix problem mentioned in comment 3

Comment 5

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

Comment on attachment 745558 [details] [diff] [review]
patch v2

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

The patch looks good, though I'd very much appreciate a test.

You may get inspiration from toolkit/components/downloads/test/unit/test_privatebrowsing_cancel.js or toolkit/components/downloads/test/unit/test_privatebrowsing.js
OR you may even just add a check or a sub test to one of these, that checks the .part file is removed.

Comment 6

[Christian Ascheberg]

Attachment: patch + test

So the problem is that private downloads are paused instead of being cancelled when exiting PB mode. Thus the files are not deleted.
The patch fixes this by always cancelling private downloads when RemoveAllDownloads is called.
I adjusted the unit test to check for the correct behavior. I also added a test that is intended to explicitly check that manually paused private downloads are cancelled in that case.
There already is a separate test to check if files of cancelled downloads are actually removed. I found bug 868795 though.

Comment 7

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

(In reply to Christian Ascheberg from comment #6)
> I found bug 868795 though.

Does this prevent us from having the test, since it may fail intermittently?

I wonder if the async behavior may cause the network code to keep writing to the file due to pending onDataAvailable, or something like that, basically that Cancel() is not an instant action (as other network actions). Maybe due to having an async stream that just closes the pipe but may still serve what's into it?
In such a case we'd need a way to be notified when the Cancel operation effectively happened to be able to remove the file, I see async streams have an asyncWait method to be notified of stream close... but likely I'm guessing a bit too much at that point.
Paolo knows better about the interaction with the network, so setting a needinfo in case he may help figuring out the problem, or even a solution, with such knowledge.

Comment 8

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

Comment on attachment 749577 [details] [diff] [review]
patch + test

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

apart the above, the patch looks good, please have a Tryserver run to verify for that intermittent possibility and wait for Paolo's feedback.

Comment 9

[Christian Ascheberg]

(In reply to Marco Bonardo [:mak] from comment #7)
> Does this prevent us from having the test, since it may fail intermittently?

I do not think so, because the part I added only checks for the correct download managers status (i.e. CANCELED), as the other parts in that unit test do, too.

The reason I am not checking for the files to be actually removed here is:
(In reply to Christian Ascheberg from comment #6)
> There already is a separate test to check if files of cancelled downloads
> are actually removed.

Here I was referring to test_cancel_download_files_removed.js. So that is the test I would expect to already fail intermittently. It might be that the test does not check if the ".part" file of the download was removed as well. So as a result it would not fail in case of bug 868795.

Note that this bug concerns both files though, the ".part" file and the actual (empty) download file. So I think bug 868795 can be fixed independently.

(In reply to Marco Bonardo [:mak] from comment #8)
> please have a Tryserver run to verify for that intermittent possibility

I do not have Tryserver access.

Comment 10

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

I will push to Try for you.

Comment 11

[:Paolo Amadini]

Since the nsIDownload object is just a control interface over an underlying
saving component, what happens when Cancel is called depends on the component.

The tests use nsIWebBrowserPersist, that implements Cancel synchronously on the
main thread, so we're fine there.

Single-click downloads instead use nsExternalHelperAppService, that saves the
file on a background thread. The issue is that we try to remove the temporary
file in nsDownloadManager as soon as Cancel returns, and this happens before
the actual download failed notification is sent to the download object.

The medium-term solution is to move towards the JavaScript API for downloads,
that is designed to handle all these asynchronous cases correctly. Short-term,
we should look into removing the file after the notification is sent, however
we should verify that this interacts correctly with nsIWebBrowserPersist.

I've also posted this information in bug 868795.

Comment 12

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

thanks, we should thus manage that separate issue in bug 868795, while this bug can proceed.

Here is the try push, will take some hours:
https://tbpl.mozilla.org/?tree=Try&rev=ede6b2276a3f

Comment 13

[Christian Ascheberg]

(In reply to Marco Bonardo [:mak] from comment #12)
> thanks, we should thus manage that separate issue in bug 868795, while this
> bug can proceed.
> 
> Here is the try push, will take some hours:
> https://tbpl.mozilla.org/?tree=Try&rev=ede6b2276a3f

Based on feedback and try results, I assume the patch is ready to be checked-in?

Comment 14

[Phil Ringnalda (:philor)]

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

Comment 15

[Phil Ringnalda (:philor)]

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

Comment 16

[Christian Ascheberg]

Comment on attachment 749577 [details] [diff] [review]
patch + test

[Approval Request Comment]
Bug caused by (feature/regressing bug #): per-window private browsing / bug 722859
User impact if declined: files of unfinished private downloads are never removed when quitting private browsing mode
Testing completed (on m-c, etc.): on m-c for one week, in testsuite
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6e9ae3ba6200
https://hg.mozilla.org/releases/mozilla-beta/rev/09b627ea659b

Comment 18

[Manuela Muntean [Away]]

Verified fixed with Firefox 22 beta 5 (build ID: 20130612084701) on a Windows 7 64bit machine. Partially downloaded files can't be found anymore in the downloads folder.

Comment 19

[Cornel Ionce [:noni] [Hubs QA]]

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20100101 Firefox/23.0
Verified as fixed on Firefox 23 Beta 5 (build ID: 20130711122148)