Closed Bug 747388 Opened 9 years ago Closed 8 years ago

.part files from failed downloads are not removed from sdcard/download

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 16
Tracking Status
firefox14 --- verified
firefox15 --- verified
firefox16 --- verified
blocking-fennec1.0 --- +

People

(Reporter: xti, Assigned: bnicholson)

References

Details

Attachments

(1 file, 2 obsolete files)

Firefox 14.0a1 (2012-04-20)
Device: Samsung Galaxy S (Captivate)
OS: Android 2.2

Steps to reproduce:
1. Open Fennec
2. Go to nightly.mozilla.org and download the Android version
3. While downloading is in progress, disable WiFi
4. Go to Settings/Download and delete the file
5. Go to sdcard/download and check if there is any .part file

Expected result:
Everything regarding the downloaded file was removed.

Actual result:
The .part file still exists.
This has nothing to do with *.part files. The Download manager currently does not [1] delete. This sounds like a future request; so I am marking as tracking.

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/aboutDownloads.js#214
tracking-fennec: --- → ?
Summary: .part files from failed downloads are not removed from sdcard/download → Downloads are not deleted when removed from Download Manager list
Blocks: 695178
(In reply to Aaron Train [:aaronmt] from comment #1)
> This has nothing to do with *.part files. The Download manager currently
> does not [1] delete. This sounds like a future request; so I am marking as
> tracking.
> 
> [1]
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/
> aboutDownloads.js#214

Actually, complete downloads are deleted both from Download Manager and SD Card. As I said in comment #0, those .part files are not removed from SD Card, even if the the file is deleted from Download Manager.
we should at least remove the .part files.
blocking-fennec1.0: --- → ?
If we need this before we land pause/resume/cancel controls, let's look at just manually canceling the download if it fails. That should get the nsIDownload (or nsIDownloadManager) to remove the .part files.
Brian - Can you try the experiment in comment 5? Let's see if a manual "cancel" will cleanup the .part files.
Assignee: nobody → bnicholson
blocking-fennec1.0: ? → +
(In reply to Cristian Nicolae (:xti) from comment #0)
> 4. Go to Settings/Download and delete the file

Do you mean you're in Fennec, you click the menu button > Downloads, you long tap on the file, and select "Delete"? On my phone, the download doesn't appear in the download manager when I turn off wifi. In other words, there's no dl-failed event, so we can't call cancel as suggested in comment 5.
(In reply to Brian Nicholson (:bnicholson) from comment #7)
> (In reply to Cristian Nicolae (:xti) from comment #0)
> > 4. Go to Settings/Download and delete the file
> 
> Do you mean you're in Fennec, you click the menu button > Downloads, you
> long tap on the file, and select "Delete"? On my phone, the download doesn't
> appear in the download manager when I turn off wifi. In other words, there's
> no dl-failed event, so we can't call cancel as suggested in comment 5.

Right, that's what I meant. When I long tap on the existing file in DM, there are 2 options: Open and Delete. On what phone/OS are you testing this issue?

I am still able to reproduce it on the latest Nightly and Aurora builds.

--
Firefox 14.0a2, Firefox 15.0a1 (2012-05-01)
Device: Samsung Captivate
OS: Android 2.2
This may no longer be an issue after bug 747354 lands.
Depends on: 747354
(In reply to Brian Nicholson (:bnicholson) from comment #9)
> This may no longer be an issue after bug 747354 lands.

This issue is still reproducible on the latest Nightly build, even if bug 747354 was fixed. The only difference now is that step 4 is not needed at all to reproduce this bug.

--
Firefox 15.0a1 (2012-05-08)
Device: Samsung Captivate
OS: Android 2.2
Summary: Downloads are not deleted when removed from Download Manager list → .part files from failed downloads are not removed from sdcard/download
I think the real behavior we want here is what desktop does. That is, if the connection is lost during the download, we don't fail; instead, we can wait until a connection is re-established and resume.
Brian - Any update on this bug? I thought you found out some weird behavior for network disconnects. Maybe we need to look at how to cleanup the .part files as a separate operation.
(In reply to Mark Finkle (:mfinkle) from comment #12)
> Brian - Any update on this bug? I thought you found out some weird behavior
> for network disconnects. Maybe we need to look at how to cleanup the .part
> files as a separate operation.

The behavior that I'm seeing now is reproducible on my Samsung Galaxy S and Samsung Galaxy Note. When I switch the phone to airplane mode during a download, about 50% of the time, the download fails immediately, giving this error: "<part file> could not be saved, because the source file could not be read." When this happens, the .part file stays in the Downloads directory even though the download has failed and the 0-byte placeholder file has been removed.

The other 50% of the time, the download simply switches to idle and resumes when the connection is restored, as expected. It looks like Cristian is also using a Galaxy S, so I wonder if this is a Samsung-only bug. I cannot reproduce this on my Droid RAZR.

It looks like most of this logic is defined in http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp. I finally got NSPR logging to work, so I'll see if I can figure out why the .part file isn't getting deleted when the download fails (and hopefully also figure out why it's immediately failing to begin with).
Attached patch Remove .part file on failure (obsolete) — Splinter Review
Here's a simple band-aid patch that removes .part files on download failure.

The real problem here is that we're failing to begin with - I think we want to switch the download to an idle state, then resume automatically when connectivity is restored.
When downloads are manually cancelled, the .part files gets deleted in nsDownloadManager.cpp (http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/nsDownloadManager.cpp#1505). As explained by the comments, nsExternalAppHandler::Cancel() can be called for pause and cancel events. This means the patch in comment 14 is incorrect since it deletes the .part file whenever the user clicks pause.

When the user removes the file in the download manager, we remove it here: http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/aboutDownloads.js#409. We can't remove the temp file the same way, though, since the download doesn't expose its temp file. nsDownloadManager::Cancel() is the only exposed method that deals with the temp file, but we can't call it after we've failed because the download isn't active: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/nsDownloadManager.cpp#1494.

As for what's causing the download to fail on the Galaxy phones, it looks like it's getting an ETIMEDOUT error after calling read() here: http://mxr.mozilla.org/mozilla-central/source/nsprpub/pr/src/pthreads/ptio.c#1287. This isn't happening on my other phones.
Here's a patch that removes the .part file when removing the download in the download manager. This patch exposes a new attribute in the nsIDownload IDL, tempFile. This patch also makes it so that mTempFile is no longer set to null in nsDownloadManager::Finalize(); I don't know the consequences of this, but it's necessary if we want to access the temp file after the download has finished.

If we want to go this route, I'll file another bug to fix the immediate failing described in the previous comments.
Attachment #627329 - Attachment is obsolete: true
Attachment #629526 - Flags: feedback?(mark.finkle)
Comment on attachment 629526 [details] [diff] [review]
Remove temp files when removing download

The thing I don't like about this is it puts the responsibility on the front-end to to cleanup the temp file.

I kinda like the previous patch a little better. It means we would never be able to resume from a .part file, but that is better than leaving .part file turds on the device.
Attachment #629526 - Flags: feedback?(mark.finkle) → feedback-
Comment on attachment 627329 [details] [diff] [review]
Remove .part file on failure

This is the patch we want to take for Fx14. Let's file a new bug for making this better.
Attachment #627329 - Attachment is obsolete: false
Attachment #627329 - Flags: review+
How about this instead? It removes the .part file on dl-failed, and it does it inside of the nsDownloadManager. I didn't do this before because I think are cases where we can still resume from failed downloads, but it doesn't sound like that's a high priority.

With this patch, pausing/resuming should still work. attachment 627329 [details] [diff] [review] would also break resuming for all devices that *do* work correctly when a connection is lost; this patch shouldn't have that problem.
Attachment #630343 - Flags: review?(mark.finkle)
Comment on attachment 630343 [details] [diff] [review]
Remove .part on failure (alt patch)

Nice. This is a better compromise.
Attachment #630343 - Flags: review?(mark.finkle) → review+
Attachment #627329 - Attachment is obsolete: true
Attachment #629526 - Attachment is obsolete: true
Comment on attachment 630343 [details] [diff] [review]
Remove .part on failure (alt patch)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: some devices will have leftover .part files after failed downloads
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): low risk, android-only
String or UUID changes made by this patch: none
Attachment #630343 - Flags: approval-mozilla-beta?
Attachment #630343 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/f5a441d6929f
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 16
Attachment #630343 - Flags: approval-mozilla-beta?
Attachment #630343 - Flags: approval-mozilla-beta+
Attachment #630343 - Flags: approval-mozilla-aurora?
Attachment #630343 - Flags: approval-mozilla-aurora+
(In reply to Brian Nicholson (:bnicholson) from comment #16)
> If we want to go this route, I'll file another bug to fix the immediate
> failing described in the previous comments.

Filed bug 762597.
.part files are no longer left in the download directory after removing failed downloads.

Verified fixed on:
Aurora 15.0a2 2012-07-08/Nightly 16.0a1 2012-07-08/ Firefox Mobile 14.0b11
Motorola Droid Pro/Samsung Captivate
Android 2.3.4/ Android 2.2
tracking-fennec: ? → ---
See Also: → 1069684
You need to log in before you can comment on or make changes to this bug.