Closed
Bug 747388
Opened 12 years ago
Closed 12 years ago
.part files from failed downloads are not removed from sdcard/download
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox14 verified, firefox15 verified, firefox16 verified, blocking-fennec1.0 +)
VERIFIED
FIXED
Firefox 16
People
(Reporter: xti, Assigned: bnicholson)
References
Details
Attachments
(1 file, 2 obsolete files)
1.47 KB,
patch
|
mfinkle
:
review+
mfinkle
:
approval-mozilla-aurora+
mfinkle
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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.
Comment 1•12 years ago
|
||
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
Reporter | ||
Comment 2•12 years ago
|
||
(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.
Comment 4•12 years ago
|
||
Ignore comment #1
Comment 5•12 years ago
|
||
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.
Comment 6•12 years ago
|
||
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: ? → +
Assignee | ||
Comment 7•12 years ago
|
||
(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.
Reporter | ||
Comment 8•12 years ago
|
||
(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
status-firefox14:
--- → affected
status-firefox15:
--- → affected
Assignee | ||
Comment 9•12 years ago
|
||
This may no longer be an issue after bug 747354 lands.
Depends on: 747354
Reporter | ||
Comment 10•12 years ago
|
||
(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
Assignee | ||
Comment 11•12 years ago
|
||
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.
Comment 12•12 years ago
|
||
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.
Assignee | ||
Comment 13•12 years ago
|
||
(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).
Assignee | ||
Comment 14•12 years ago
|
||
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.
Assignee | ||
Comment 15•12 years ago
|
||
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.
Assignee | ||
Comment 16•12 years ago
|
||
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 17•12 years ago
|
||
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 18•12 years ago
|
||
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+
Assignee | ||
Comment 19•12 years ago
|
||
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 20•12 years ago
|
||
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+
Updated•12 years ago
|
Attachment #627329 -
Attachment is obsolete: true
Updated•12 years ago
|
Attachment #629526 -
Attachment is obsolete: true
Assignee | ||
Comment 21•12 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/f5a441d6929f
Assignee | ||
Comment 22•12 years ago
|
||
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?
Comment 23•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f5a441d6929f
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 16
Updated•12 years ago
|
Attachment #630343 -
Flags: approval-mozilla-beta?
Attachment #630343 -
Flags: approval-mozilla-beta+
Attachment #630343 -
Flags: approval-mozilla-aurora?
Attachment #630343 -
Flags: approval-mozilla-aurora+
Assignee | ||
Comment 24•12 years ago
|
||
http://hg.mozilla.org/releases/mozilla-aurora/rev/0ec4ebcc5514 http://hg.mozilla.org/releases/mozilla-beta/rev/921ea0a97cf1
Assignee | ||
Comment 25•12 years ago
|
||
(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.
Updated•12 years ago
|
Comment 26•12 years ago
|
||
.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
Status: RESOLVED → VERIFIED
Updated•10 years ago
|
tracking-fennec: ? → ---
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•