868795 - Firefox fails to delete cancelled downloads .part file because it keeps on writing data

Status: VERIFIED FIXED

(Toolkit :: Downloads API, defect)

Description

[Christian Ascheberg]

Attachment: minimized Process Monitor logfile of patched Firefox

I am using the current nightly build on Win7 and I think there is a problem where Firefox sometimes keeps on writing data to the .part file of a download although the download should be cancelled and deleted. As a result, the .part file cannot be deleted.

In those cases, DeleteFileW in xpcom/io/nsLocalFileWin.cpp returns a SHARING VIOLATION when it is trying to delete the downloads temp file. The download manager does not check the return value, so there is no error handling.
I tried setting the FILE_SHARE_DELETE flag in CreateFileW and that solves the problem. Though, as a side effect, anyone can delete the .part file during the download.

I checked the output of Microsofts Process Monitor and it shows that Firefox is writing data to the .part file after trying to delete the file.
I attached a minimized Process Monitor logfile of a patched Firefox version that has the FILE_SHARE_DELETE flag set.

Description of the attachment:

Sequence 18: CreateFile, ShareMode: Read, Write, Delete
 -> The download starts, sharemode is only "Read, Write" originally.

Sequence 115: CreateFile, Desired Access: Read Attributes, Delete
 -> The download was cancelled, the .part file is going to be removed. This sometimes fails with a SHARING VIOLATION if FILE_SHARE_DELETE is not set.

Sequence 117: SetDispositionInformationFile, Delete: True
 -> This delete action would not be performed if sequence 115 failed.

Sequence 120: Write
 -> I think this makes sequence 115 fail. Writing to the file should not happen anymore at this point because the download manager cancelled the downloads transfer prior to deleting the .part file.

I do not know why the write operation in sequence 120 is happening in some cases.

Result:
- The download is cancelled, but the associated .part file still exists.

Comment 1

[:Paolo Amadini]

Single-click downloads 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 may look into removing the file after the notification is sent, however
we should verify that this interacts correctly with nsIWebBrowserPersist.

Comment 2

[Marco Bonardo [:mak]]

Do you want to mentor this bug paolo?
I think Christian may be interested in evaluating it (but please correct me if I'm wrong).

Comment 3

[:Paolo Amadini]

The simplest solution is probably to set a flag on the nsDownload object in
nsDownload::Cancel(), then delete the files when nsDownload::OnStateChange is
called if this flag, STATE_STOP, and STATE_IS_NETWORK are all true.

I can definitely help with reviews here.

Comment 4

[Christian Ascheberg]

Did bug 899102 fully fix this bug? I was at least unable to reproduce it in a quick check.

Comment 5

[:Paolo Amadini]

(In reply to Christian Ascheberg from comment #4)
> Did bug 899102 fully fix this bug? I was at least unable to reproduce it in
> a quick check.

This means that the case is now handled correctly by the new Downloads API, thanks for verifying this!

I don't know about the status in nsDownloadManager.cpp, but this is not important because the module is now deprecated.