918466 - Residual file left after cancelling download

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect)

Description

[Bader Zaidan]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:27.0) Gecko/20100101 Firefox/27.0 (Beta/Release)
Build ID: 20130919030202

Steps to reproduce:

1. Click on a link to download
2. Download started, and both original and .part file created
3. Cancel said download


Actual results:

The .part file was removed, but the original blank file remains in the download directory.


Expected results:

No temporary file should remain if a download is cancelled.

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

This bug seems to be related to bug 847863. Disabling prefs "browser.download.useJSTransfer" which is enabled by the 8th patch of that bug could solve this problem.

Comment 2

[:Paolo Amadini]

Tracking this bug until we confirm whether this is a Downloads API regression.

Comment 3

[:Paolo Amadini]

Attachment: The patch

Comment 4

[Neil Deakin]

Comment on attachment 809843 [details] [diff] [review]
The patch

>+add_task(function test_cancel_midway_tryToKeepPartialData()
>+{
>+  let download = yield promiseStartDownload_tryToKeepPartialData();
>+
>+  do_check_true(yield OS.File.exists(download.target.path));
>+  do_check_true(yield OS.File.exists(download.target.partFilePath));
>+
>+  yield download.cancel();
>+  yield download.removePartialData();
>+

What happens if you don't call removePartialData? Should the empty file still be around?

Should we also have a test that handles the download failing?

Comment 5

[:Paolo Amadini]

Attachment: Updated patch

(In reply to Neil Deakin from comment #4)
> What happens if you don't call removePartialData? Should the empty file
> still be around?

Good question. We didn't consistently handle placeholders previously, and
currently we delete the placeholder while the download is stopped.

However, we might also decide to keep it, to reserve the file name. The
downside is that having a zero-sized file with the final extension kept in
place for a long time might be confusing.

I think we should investigate this further, so I filed bug 921052 and didn't
add a test preferring one case over the other. This case mostly concerns what
we show as "paused" downloads, that are a minority, so I don't think we need
to rush.

> Should we also have a test that handles the download failing?

I added a check to the existing source failure test, but I also realized that
we don't have failure tests for tryToKeepPartialData. I've filed the follow-up
bug 921054.

Comment 7

[:Paolo Amadini]

Landed with a change to error reporting because of failing tests:

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

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4888e018458d

Comment 9

[Xidorn Quan [:xidorn] UTC+11]

I suggest landing this fix on the same milestone as bug 847863 since it is a nontrivial bug introduced by that.

Comment 10

[:Paolo Amadini]

Attachment: Landed patch

(In reply to Xidorn Quan from comment #9)
> I suggest landing this fix on the same milestone as bug 847863 since it is a
> nontrivial bug introduced by that.

Agreed.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 847863
User impact if declined: Regression described in comment 0
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): Low, has tests
String or IDL/UUID changes made by this patch: None

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a73d0546a814

Comment 12

[Mihai Morar, (:MihaiMorar)]

I confirm the fix is verified using Latest Aurora 27 on Windows 7 x64, Mac OS 10.8 and Ubuntu 13.10:
BuildID: 20131031004003