Residual file left after cancelling download

RESOLVED FIXED in Firefox 26

Status

Core Graveyard
File Handling
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: Bader Zaidan, Assigned: Paolo)

Tracking

Trunk
mozilla27

Firefox Tracking Flags

(firefox24 unaffected, firefox25 unaffected, firefox26 fixed, firefox27 verified)

Details

(Whiteboard: [bugday-20130918])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

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

Updated

5 years ago
Whiteboard: [bugday-20130918]
(Reporter)

Updated

5 years ago
Version: 27 Branch → Trunk
OS: Linux → All
Hardware: x86_64 → All
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 2

5 years ago
Tracking this bug until we confirm whether this is a Downloads API regression.
Blocks: 907082
(Assignee)

Comment 3

5 years ago
Created attachment 809843 [details] [diff] [review]
The patch
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Attachment #809843 - Flags: review?(enndeakin)

Comment 4

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

Comment 5

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

Updated

5 years ago
Attachment #810605 - Flags: review?(enndeakin) → review+

Updated

5 years ago
Duplicate of this bug: 921904
(Assignee)

Comment 7

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

https://hg.mozilla.org/integration/fx-team/rev/4888e018458d
https://hg.mozilla.org/mozilla-central/rev/4888e018458d
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
I suggest landing this fix on the same milestone as bug 847863 since it is a nontrivial bug introduced by that.
(Assignee)

Comment 10

5 years ago
Created attachment 816214 [details] [diff] [review]
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
Attachment #810605 - Attachment is obsolete: true
Attachment #816214 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

5 years ago
status-firefox24: --- → unaffected
status-firefox25: --- → unaffected
status-firefox26: --- → affected
status-firefox27: --- → fixed
Attachment #816214 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
I confirm the fix is verified using Latest Aurora 27 on Windows 7 x64, Mac OS 10.8 and Ubuntu 13.10:
BuildID: 20131031004003
status-firefox27: fixed → verified
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.