Closed Bug 863063 Opened 7 years ago Closed 7 years ago

quitting private browsing mode does not delete partially downloaded files

Categories

(Toolkit :: Downloads API, defect, major)

x86_64
Windows 7
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla24
Tracking Status
firefox21 --- wontfix
firefox22 + verified
firefox23 + verified
firefox24 + fixed

People

(Reporter: c.ascheberg, Assigned: c.ascheberg)

References

Details

(Keywords: privacy)

Attachments

(1 file, 2 obsolete files)

Attached patch patch v1 (obsolete) — Splinter Review
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)
(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)
(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.
Attached patch patch v2 (obsolete) — Splinter Review
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+
Attached patch patch + testSplinter Review
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)
(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+
(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)
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)
(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?
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f53bf9672936
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
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?
Attachment #749577 - Flags: approval-mozilla-beta?
Attachment #749577 - Flags: approval-mozilla-beta+
Attachment #749577 - Flags: approval-mozilla-aurora?
Attachment #749577 - Flags: approval-mozilla-aurora+
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.
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)
You need to log in before you can comment on or make changes to this bug.