Closed
Bug 420355
Opened 16 years ago
Closed 11 years ago
residual .part file when I cancel a download from Save As dialog
Categories
(Core Graveyard :: File Handling, defect)
Core Graveyard
File Handling
Tracking
(firefox24 wontfix, firefox25 verified, firefox26 verified)
VERIFIED
FIXED
mozilla26
People
(Reporter: afrappe, Assigned: xidorn, NeedInfo)
References
Details
(Whiteboard: [engineering-friend], [bugday-20130918])
Attachments
(1 file, 7 obsolete files)
1.11 KB,
patch
|
Biesinger
:
review+
bajaj
:
approval-mozilla-aurora+
bajaj
:
approval-mozilla-beta-
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b4pre) Gecko/2008022804 Minefield/3.0b4pre Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b4pre) Gecko/2008022804 Minefield/3.0b4pre After click a link which points to a file a residual file .part is created on last download directory, but If I cancel this download the .part file stills. Residual filenames, like y7vmU1P0.dmg.part or so Reproducible: Always Steps to Reproduce: 1. Click on a link to download 2. Residual .part file is created on last download directory 3. Save as dialog appears, then I choose to cancel this download Actual Results: 4. Residual .part stills Expected Results: No residual file on directory When Cancel a download delte .part file
Reporter | ||
Comment 1•16 years ago
|
||
Comment 2•16 years ago
|
||
could you generate that diff with -u8p please for more context?
Reporter | ||
Comment 3•16 years ago
|
||
From file /mozilla/uriloader/exthandler/nsExternalHelperAppService.cpp
Attachment #319011 -
Attachment is obsolete: true
Updated•16 years ago
|
Assignee: nobody → afrappe
Status: UNCONFIRMED → NEW
Ever confirmed: true
Updated•16 years ago
|
Attachment #319021 -
Flags: superreview?(cbiesinger)
Attachment #319021 -
Flags: review?(cbiesinger)
Comment 4•16 years ago
|
||
isn't mReceivedDispositionInfo supposed to be false in this case?
Reporter | ||
Comment 5•16 years ago
|
||
I agree, it seems that mReceivedDispositionInfo must be false, or the condition must ask for NS_BINDING_ABORTED too
Reporter | ||
Comment 6•16 years ago
|
||
A more elegant way to solve this bug.
Updated•16 years ago
|
Summary: residual .part file when If I cancel a download from Save As dialog → residual .part file when I cancel a download from Save As dialog
Reporter | ||
Comment 7•16 years ago
|
||
(In reply to comment #4) > isn't mReceivedDispositionInfo supposed to be false in this case? > I'll set to true before calling Cancel, sure it works too, but if I do that, how to be sure that this change won't impact any other piece of code?
Reporter | ||
Comment 8•16 years ago
|
||
From Christian Biesinger's suggestion set mReceivedDispositionInfo = PR_FALSE before call Cancel. It seems that this solution is more elegant, and don't impact on other pieces of code.
Comment 9•16 years ago
|
||
That is not what I meant... I meant isn't mReceived... only set to true after you click OK on the save dialog?
Component: Download Manager → File Handling
Product: Firefox → Core
QA Contact: download.manager → file-handling
Comment 10•16 years ago
|
||
oh I see... we set that before this dialog... hrm. I think a better fix is to only set mReceived... to true after we've shown this dialog.
Reporter | ||
Comment 11•16 years ago
|
||
(In reply to comment #10) > oh I see... we set that before this dialog... hrm. I think a better fix is to > only set mReceived... to true after we've shown this dialog. > Let's do it! I'm going to compile it. Thanks for your insight
Reporter | ||
Comment 12•16 years ago
|
||
As Christian proposed, mReceivedDispositionInfo = PR_FALSE before call Cancel
Comment 13•16 years ago
|
||
Arturo, any further progress on this bug from your side? What's the current status of this bug? If I follow the steps from comment 0 I get following exception: Error: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIHelperAppLauncher.saveToDisk]" nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: file:///Applications/Shiretoko.app/Contents/MacOS/components/nsHelperAppDlg.js :: anonymous :: line 465" data: no] Source File: file:///Applications/Shiretoko.app/Contents/MacOS/components/nsHelperAppDlg.js Line: 465 Would be nice to have this fixed for Firefox3.1.
Status: NEW → ASSIGNED
Flags: wanted1.9.1?
Hardware: PowerPC → All
Reporter | ||
Comment 14•15 years ago
|
||
It was solved from comment #12, and Firefox 3.0, I'll try the same fix with 3.1 Keep in touch. (In reply to comment #13) > Arturo, any further progress on this bug from your side? What's the current > status of this bug? > > If I follow the steps from comment 0 I get following exception: > Error: [Exception... "Component returned failure code: 0x80004005 > (NS_ERROR_FAILURE) [nsIHelperAppLauncher.saveToDisk]" nsresult: "0x80004005 > (NS_ERROR_FAILURE)" location: "JS frame :: > file:///Applications/Shiretoko.app/Contents/MacOS/components/nsHelperAppDlg.js > :: anonymous :: line 465" data: no] > Source File: > file:///Applications/Shiretoko.app/Contents/MacOS/components/nsHelperAppDlg.js > Line: 465 > > Would be nice to have this fixed for Firefox3.1.
Reporter | ||
Updated•15 years ago
|
Attachment #319328 -
Flags: review?(afrappe)
Updated•15 years ago
|
Attachment #319021 -
Attachment is obsolete: true
Attachment #319021 -
Flags: superreview?(cbiesinger)
Attachment #319021 -
Flags: review?(cbiesinger)
Updated•15 years ago
|
Attachment #319328 -
Flags: superreview?(cbiesinger)
Attachment #319328 -
Flags: review?(cbiesinger)
Attachment #319328 -
Flags: review?(afrappe)
Updated•15 years ago
|
Attachment #319186 -
Attachment is obsolete: true
Updated•15 years ago
|
Attachment #319239 -
Attachment is obsolete: true
Comment 15•15 years ago
|
||
Arturo, as what I can see only the last patch seems to be valid for now. Lets obsolete the others. You should also ask for r/sr from Christian and not from you. I've updated the flags.
Comment 16•15 years ago
|
||
Comment on attachment 319328 [details] [diff] [review] mReceivedDispositionInfo = PR_FALSE I don't think you should set mReceivedDispositionInfo to false here. Instead, you should make sure that setting to true only happens later. That means: - Moving this line in OnStartRequest to the end of its block: mReceivedDispositionInfo = PR_TRUE; // no need to wait for a response from the user - and moving the line in SaveToDisk like you did in this patch (note that your indentation is incorrect here: + // The helper app dialog has told us what to do. + mReceivedDispositionInfo = PR_TRUE; )
Attachment #319328 -
Flags: superreview?(cbiesinger)
Attachment #319328 -
Flags: review?(cbiesinger)
Attachment #319328 -
Flags: review-
Reporter | ||
Comment 17•15 years ago
|
||
bug lost I'm writting this using Mozilla Firefox 1.5.0.4. The reported bug in this thread is gone, I can't reproduce it. I tried with Firefox 2.0.0.18, 2.0.0.20, 3.0.6, and 1.5.0.4. I'm using MacOS 10.4.11 It seems like a genius fix this bug, it's scary.
Comment 18•15 years ago
|
||
The .part file doesn't land at the latest download location anymore but on the given download folder from within the preferences. This was fixed by bug 476174 or one of its dependencies. But it shouldn't have any effect to older builds like 1.5.0.x, 2.0.0.x or 3.0.x. This bug is still persistent. The .part file is not deleted when canceling the OS save as dialog.
Comment 21•11 years ago
|
||
Duplicate: 475008
Reporter | ||
Comment 22•11 years ago
|
||
(In reply to Arturo Frappé from comment #12) The attachment in #12 solves the bug, but I don't how to submit it to be considered. Maybe I'm too lazy to be more social and ask to be considered. > Created attachment 319328 [details] [diff] [review] > mReceivedDispositionInfo = PR_FALSE > > As Christian proposed, mReceivedDispositionInfo = PR_FALSE before call Cancel
Comment 23•11 years ago
|
||
(In reply to Arturo Frappé from comment #22) Reattach the update with Christian's edits, add him for review and a flag will need to be added for "approval-mozilla-aurora" and "approval-mozilla-beta". Once sign-off is received, then I believe we can proceed further.
Comment 24•11 years ago
|
||
I'm seeing this now with Firefox 20.0 on MacOS X 10.6.8 and 10.7.3. I hadn't seen this behavior in previous versions, but now it's 100% reproducible by canceling any "save as" dialog and watching how the downloads folder gets littered with ".part" files.
Comment 25•11 years ago
|
||
I can reproduce as well but on OS X 10.8.3 in Firefox 20
Comment 27•11 years ago
|
||
This is still an issue in Firefox 21.
Comment 28•11 years ago
|
||
This continues to be an issue in Firefox 22.
Comment 29•11 years ago
|
||
Also in Firefox 23.
Comment 30•11 years ago
|
||
Arturo, do you still want to continue to work on this bug? As given in comment 23 you will have to update the patch from comment 12, and request review from Christian. If you don't have the time please unassign from this bug so that others have the chance to pick it up. Thanks.
Flags: wanted1.9.1? → needinfo?(afrappe)
Assignee | ||
Updated•11 years ago
|
OS: Mac OS X → All
Assignee | ||
Updated•11 years ago
|
Version: unspecified → Trunk
Assignee | ||
Comment 31•11 years ago
|
||
This bug occurs on both Mac and Windows, though the .part file is stored in temp dir on Windows which makes it noteless. The condition of it seems to be that, the file has been totally downloaded in background without explicitly saving or opening. If Arturo doesn't want to continue working on this bug, I'd like to take over it.
Assignee | ||
Comment 32•11 years ago
|
||
Assignee: afrappe → quanxunzhen
Attachment #787310 -
Flags: review?(cbiesinger)
Comment 33•11 years ago
|
||
Comment on attachment 787310 [details] [diff] [review] patch + // When data transfer has been complete, mSaver is dereferenced with temp + // file left which should be removed by mSaver if aReason is failed. I'm not sure I understand this comment. I thought whenever we have a temp file, we have mSaver. Is that not correct? Where do we set mSaver to null?
Assignee | ||
Comment 34•11 years ago
|
||
(In reply to Christian :Biesinger (don't email me, ping me on IRC) from comment #33) > Comment on attachment 787310 [details] [diff] [review] > patch > > + // When data transfer has been complete, mSaver is dereferenced with > temp > + // file left which should be removed by mSaver if aReason is failed. > > I'm not sure I understand this comment. I thought whenever we have a temp > file, we have mSaver. Is that not correct? Where do we set mSaver to null? When the OnSaveComplete callback is called, mSaver is dereferenced. Code here: http://dxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#l1814 Since BackgroundFileSaver does not care about whether the file it downloads is a temp file or not, it always calls the callback when the data has been totally received. If the download has not been complete, file should be deleted here if cancelled: http://dxr.mozilla.org/mozilla-central/source/netwerk/base/src/BackgroundFileSaver.cpp#l629
Comment 35•11 years ago
|
||
BackgroundFileSaver is instructed by nsExternalHelperAppService.cpp not to delete the ".part" file, because Cancel is the same command used when a download is paused. Changing this would prevent partial data from being kept on pausing. I suspect that what's happening here is the same as the race condition described in bug 868795. The new JavaScript API for downloads is also subject to this issue, filed as bug 899102.
Assignee | ||
Comment 36•11 years ago
|
||
(In reply to Paolo Amadini [:paolo] from comment #35) > BackgroundFileSaver is instructed by nsExternalHelperAppService.cpp not to > delete > the ".part" file, because Cancel is the same command used when a download is > paused. Changing this would prevent partial data from being kept on pausing. No, it won't. When pausing, mSaver has not been dereferenced when calling ::Cancel, so it won't go into the else if branch I added in the patch. > I suspect that what's happening here is the same as the race condition > described > in bug 868795. The new JavaScript API for downloads is also subject to this > issue, filed as bug 899102. No it isn't. Let me describe. In a normal flow, when you cancel a download, nsHelperAppDlg calls nsExternalAppHandler::Cancel with a failure reason (NS_BINDING_ABORTED) [1]. Then, ::Cancel calls mSaver->Finish with the same reason [2]. mStatus of mSaver is set to the reason in Finish method [3], and after that, through a complex path, finally BackgroundFileSaver::CheckCompletion is called, in which the temp file will be removed [4]. The problem is that, when the transfer has been finished, BackgroundFileSaver called nsExternalAppHandler::OnSaveComplete [5], which dereferenced mSaver [6]. When you cancel the download, the normal flow I mentioned above won't work properly since mSaver has been nullptr, and mSaver->Finish will not been invoked. As a result, the part file which should be removed at [4] is left untouched. [1] http://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/nsHelperAppDlg.js#l165 [2] http://dxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#l2213 [3] http://dxr.mozilla.org/mozilla-central/source/netwerk/base/src/BackgroundFileSaver.cpp#l211 [4] http://dxr.mozilla.org/mozilla-central/source/netwerk/base/src/BackgroundFileSaver.cpp#l629 [5] http://dxr.mozilla.org/mozilla-central/source/netwerk/base/src/BackgroundFileSaver.cpp#l681 [6] http://dxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#l1825
Comment 37•11 years ago
|
||
(In reply to Xidorn Quan from comment #36) > The problem is that, when the transfer has been finished, > BackgroundFileSaver called nsExternalAppHandler::OnSaveComplete [5], which > dereferenced mSaver [6]. Thanks for the detailed explanation! The fact is that OnSaveComplete will never be called unless mSaver->Finish has been called explicitly before it (either with a success or failure code), even if we transferred all the data from the source. Can you explain which code path calls Finish in your example above?
Assignee | ||
Comment 38•11 years ago
|
||
(In reply to Paolo Amadini [:paolo] from comment #37) > (In reply to Xidorn Quan from comment #36) > > The problem is that, when the transfer has been finished, > > BackgroundFileSaver called nsExternalAppHandler::OnSaveComplete [5], which > > dereferenced mSaver [6]. > > Thanks for the detailed explanation! The fact is that OnSaveComplete will > never > be called unless mSaver->Finish has been called explicitly before it (either > with a success or failure code), even if we transferred all the data from the > source. Can you explain which code path calls Finish in your example above? mSaver->Finish is called explicitly by nsExternalAppHandler::OnStopRequest, but, well, I do not find out the path calls OnStopRequest.
Comment 39•11 years ago
|
||
Comment on attachment 787310 [details] [diff] [review] patch Review of attachment 787310 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for looking into this and highlighting the code path that is causing the issue. ::: uriloader/exthandler/nsExternalHelperAppService.cpp @@ +2211,5 @@ > mCanceled = true; > if (mSaver) { > mSaver->Finish(aReason); > mSaver = nullptr; > + } else if (NS_FAILED(aReason)) { The NS_ENSURE_ARG(NS_FAILED(aReason)) at the beginning of the function already ensures that this condition is true. I think you should check mStopRequestIssued here. @@ +2220,5 @@ > + NS_ENSURE_SUCCESS(rv, rv); > + if (tempFileExists) { > + mTempFile->SetPermissions(0600); > + mTempFile->Remove(false); > + } I don't think we create read-only ".part" files anymore, no need to set the permissions. Also, we can avoid the additional check for existence, just make it explicit that we ignore the return value using "(void)mTempFile->Remove(false);".
Attachment #787310 -
Flags: feedback+
Comment 40•11 years ago
|
||
Also, we should null-check mTempFile to be sure that we succeeded in setting it.
Updated•11 years ago
|
Whiteboard: [engineering-friend]
Assignee | ||
Comment 41•11 years ago
|
||
Attachment #787310 -
Attachment is obsolete: true
Attachment #787310 -
Flags: review?(cbiesinger)
Attachment #788518 -
Flags: review?(cbiesinger)
Comment 42•11 years ago
|
||
Comment on attachment 788518 [details] [diff] [review] patch + // If request is stopped before cancellation, the temp file has to be + // removed here, because mSaver, which ought to remove the file, has + // been dereferenced. please call this "released", dereferenced sounds like http://en.wikipedia.org/wiki/Dereference_operator Also, I think the comment would read easier with "If request is stopped" -> "If the request has completed". I would also add a comment like "This can only happen when the user cancels the helper app dialog or file picker, because afterwards it is impossible to cancel when we are finished downloading". Finally, the mTempFile check could just be part of the other if: } else if (mStopRequestIssued && mTempFile) {
Attachment #788518 -
Flags: review?(cbiesinger) → review-
Assignee | ||
Comment 43•11 years ago
|
||
Attachment #788518 -
Attachment is obsolete: true
Attachment #791592 -
Flags: review?(cbiesinger)
Comment 44•11 years ago
|
||
Comment on attachment 791592 [details] [diff] [review] patch Thanks for bearing with me!
Attachment #791592 -
Flags: review?(cbiesinger) → review+
Assignee | ||
Comment 45•11 years ago
|
||
(In reply to Christian :Biesinger (don't email me, ping me on IRC) from comment #44) > Comment on attachment 791592 [details] [diff] [review] > patch > > Thanks for bearing with me! Not at all. I shall thank you for your patiently reviewing.
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Updated•11 years ago
|
Attachment #319328 -
Attachment is obsolete: true
Comment 46•11 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/de84eec0f87a
Keywords: checkin-needed
Whiteboard: [engineering-friend] → [engineering-friend][fixed-in-fx-team]
Comment 47•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/de84eec0f87a
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [engineering-friend][fixed-in-fx-team] → [engineering-friend]
Target Milestone: --- → mozilla26
Assignee | ||
Comment 48•11 years ago
|
||
Comment on attachment 791592 [details] [diff] [review] patch [Approval Request Comment] Bug caused by (feature/regressing bug #): background download when showing helper dialog User impact if declined: may left a .tmp file if user cancel the download, especially when file is small Testing completed (on m-c, etc.): none Risk to taking this patch (and alternatives if risky): none String or IDL/UUID changes made by this patch: none
Attachment #791592 -
Flags: approval-mozilla-beta?
Assignee | ||
Comment 49•11 years ago
|
||
Comment on attachment 791592 [details] [diff] [review] patch [Approval Request Comment] See comment 48
Attachment #791592 -
Flags: approval-mozilla-aurora?
Comment 50•11 years ago
|
||
This doesn't seem like a good candidate for beta uplifting, given that we're releasing in two weeks and this has been a known problem for years.
Comment 51•11 years ago
|
||
Although a nice to have, we have crossed the deadline for Fx24 to take any longstanding regression's at this point :( Approving on aurora though so it can get fixed in Fx25.
Updated•11 years ago
|
Attachment #791592 -
Flags: approval-mozilla-beta?
Attachment #791592 -
Flags: approval-mozilla-beta-
Attachment #791592 -
Flags: approval-mozilla-aurora?
Attachment #791592 -
Flags: approval-mozilla-aurora+
Comment 52•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/923719705762
status-firefox25:
--- → fixed
status-firefox26:
--- → fixed
Updated•11 years ago
|
status-firefox24:
--- → wontfix
Comment 53•11 years ago
|
||
Under GNU/Linux, the .part file is removed as soon as the download is cancelled, but another the file (under the original name, without .part) remains there. For example: Dowloading this file: http://mirror.9fags.net/9front-2865.eb4ba8ae2123.iso.bz2 results in these files in the download directory: Size Date File 0 Sep 18 9front-2865.eb4ba8ae2123.iso.bz2 545472 Sep 18 9front-2865.eb4ba8ae2123.iso.bz2.part After the download is cancelled, the .part is deleted, but the first file still exists: Size Date File 0 Sep 18 9front-2865.eb4ba8ae2123.iso.bz2 Firefox Nightly 27, under Trisquel GNU/Linux 6.0, 64 bit.
Comment 54•11 years ago
|
||
Thanks Bader. I think we should possibly re-open this bug since the expected result is not achieved, ie. no files should remain after canceling a download. Xidorn, can you address that in this bug or should a new follow-up bug be filed?
Flags: needinfo?(quanxunzhen)
Assignee | ||
Comment 56•11 years ago
|
||
(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #54) > Thanks Bader. > > I think we should possibly re-open this bug since the expected result is not > achieved, ie. no files should remain after canceling a download. Xidorn, can > you address that in this bug or should a new follow-up bug be filed? According to my test, the bug reported by Bader is neither the bug described here nor a bug introduced by my patch. So IMO, a new bug should be filed for it.
Flags: needinfo?(quanxunzhen)
Updated•11 years ago
|
Whiteboard: [engineering-friend] → [engineering-friend], [bugday-20130918]
Comment 57•11 years ago
|
||
Thank you Xidorn, I'm going to call this verified fixed based on previous testing. Bader, please file a new bug for your issue.
Status: RESOLVED → VERIFIED
Keywords: verifyme
Comment 58•11 years ago
|
||
A bug has been filed, as requested: https://bugzilla.mozilla.org/show_bug.cgi?id=918466
Updated•8 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•