quitting private browsing mode does not delete partially downloaded files

RESOLVED FIXED in Firefox 22

Status

()

Toolkit
Downloads API
--
major
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Christian Ascheberg, Assigned: Christian Ascheberg)

Tracking

({privacy})

Trunk
mozilla24
x86_64
Windows 7
privacy
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox21 wontfix, firefox22+ verified, firefox23+ verified, firefox24+ fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
Created attachment 738735 [details] [diff] [review]
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.
Attachment #738735 - Flags: review?(mak77)
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?
Flags: needinfo?(paolo.mozmail)

Comment 2

5 years ago
(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.
Flags: needinfo?(paolo.mozmail)
(Assignee)

Comment 3

5 years ago
(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.
(Assignee)

Comment 4

5 years ago
Created attachment 745558 [details] [diff] [review]
patch v2

also fix problem mentioned in comment 3
Assignee: nobody → c.ascheberg
Attachment #738735 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #738735 - Flags: review?(mak77)
Attachment #745558 - Flags: review?(mak77)
Keywords: privacy
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.
Attachment #745558 - Flags: review?(mak77) → feedback+
(Assignee)

Comment 6

5 years ago
Created attachment 749577 [details] [diff] [review]
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.
Attachment #745558 - Attachment is obsolete: true
Attachment #749577 - Flags: review?(mak77)
(Assignee)

Updated

5 years ago
status-firefox21: --- → affected
status-firefox22: --- → affected
status-firefox23: --- → affected
status-firefox24: --- → affected
(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.
Flags: needinfo?(paolo.mozmail)
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.
Attachment #749577 - Flags: review?(mak77) → review+
(Assignee)

Comment 9

5 years ago
(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.
I will push to Try for you.
Flags: needinfo?(mak77)

Comment 11

5 years ago
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.
Flags: needinfo?(paolo.mozmail)
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
Flags: needinfo?(mak77)
(Assignee)

Comment 13

5 years ago
(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?
(Assignee)

Updated

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

Updated

5 years ago
status-firefox24: affected → fixed
(Assignee)

Comment 16

5 years ago
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
Attachment #749577 - Flags: approval-mozilla-beta?
Attachment #749577 - Flags: approval-mozilla-aurora?

Updated

5 years ago
status-firefox21: affected → wontfix
tracking-firefox22: --- → +
tracking-firefox23: --- → +
tracking-firefox24: --- → +

Updated

5 years ago
Attachment #749577 - Flags: approval-mozilla-beta?
Attachment #749577 - Flags: approval-mozilla-beta+
Attachment #749577 - Flags: approval-mozilla-aurora?
Attachment #749577 - Flags: approval-mozilla-aurora+

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/releases/mozilla-aurora/rev/6e9ae3ba6200
https://hg.mozilla.org/releases/mozilla-beta/rev/09b627ea659b
status-firefox22: affected → fixed
status-firefox23: affected → fixed
Keywords: checkin-needed
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.
status-firefox22: fixed → verified
QA Contact: manuela.muntean
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)
status-firefox23: fixed → verified
You need to log in before you can comment on or make changes to this bug.