residual .part file when I cancel a download from Save As dialog

VERIFIED FIXED in Firefox 25

Status

Core Graveyard
File Handling
VERIFIED FIXED
10 years ago
2 years ago

People

(Reporter: Arturo Frappé, Assigned: xidorn, NeedInfo)

Tracking

Trunk
mozilla26

Firefox Tracking Flags

(firefox24 wontfix, firefox25 verified, firefox26 verified)

Details

(Whiteboard: [engineering-friend], [bugday-20130918])

Attachments

(1 attachment, 7 obsolete attachments)

(Reporter)

Description

10 years ago
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

10 years ago
Created attachment 319011 [details] [diff] [review]
It solves the bug, looking if .part filesize equals zero
could you generate that diff with -u8p please for more context?
(Reporter)

Comment 3

10 years ago
Created attachment 319021 [details] [diff] [review]
patch created with -pU 8

From file /mozilla/uriloader/exthandler/nsExternalHelperAppService.cpp
Attachment #319011 - Attachment is obsolete: true

Updated

10 years ago
Assignee: nobody → afrappe
Status: UNCONFIRMED → NEW
Ever confirmed: true

Updated

10 years ago
Attachment #319021 - Flags: superreview?(cbiesinger)
Attachment #319021 - Flags: review?(cbiesinger)
isn't mReceivedDispositionInfo supposed to be false in this case?
(Reporter)

Comment 5

10 years ago
I agree, it seems that mReceivedDispositionInfo must be false, or the condition must ask for NS_BINDING_ABORTED too
(Reporter)

Comment 6

10 years ago
Created attachment 319186 [details] [diff] [review]
asking for NS_BINDING_ABORTED

A more elegant way to solve this bug.
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

10 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

10 years ago
Created attachment 319239 [details] [diff] [review]
From Christian Biesinger's suggestion

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.
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
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

10 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

10 years ago
Created attachment 319328 [details] [diff] [review]
mReceivedDispositionInfo = PR_FALSE

As Christian proposed, mReceivedDispositionInfo = PR_FALSE before call Cancel
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

10 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

10 years ago
Attachment #319328 - Flags: review?(afrappe)
Attachment #319021 - Attachment is obsolete: true
Attachment #319021 - Flags: superreview?(cbiesinger)
Attachment #319021 - Flags: review?(cbiesinger)
Attachment #319328 - Flags: superreview?(cbiesinger)
Attachment #319328 - Flags: review?(cbiesinger)
Attachment #319328 - Flags: review?(afrappe)
Attachment #319186 - Attachment is obsolete: true
Attachment #319239 - Attachment is obsolete: true
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 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

9 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.
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.

Updated

7 years ago
Duplicate of this bug: 686923

Updated

6 years ago
Duplicate of this bug: 548534

Comment 21

5 years ago
Duplicate: 475008
(Reporter)

Comment 22

5 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

5 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

5 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

5 years ago
I can reproduce as well but on OS X 10.8.3 in Firefox 20
Duplicate of this bug: 866519

Comment 27

5 years ago
This is still an issue in Firefox 21.

Comment 28

5 years ago
This continues to be an issue in Firefox 22.

Comment 29

5 years ago
Also in Firefox 23.
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

5 years ago
OS: Mac OS X → All
(Assignee)

Updated

5 years ago
Version: unspecified → Trunk
(Assignee)

Comment 31

5 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

5 years ago
Created attachment 787310 [details] [diff] [review]
patch
Assignee: afrappe → quanxunzhen
Attachment #787310 - Flags: review?(cbiesinger)
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

5 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

5 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

5 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

5 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

5 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

5 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

5 years ago
Also, we should null-check mTempFile to be sure that we succeeded in setting it.

Updated

5 years ago
Whiteboard: [engineering-friend]
(Assignee)

Comment 41

5 years ago
Created attachment 788518 [details] [diff] [review]
patch
Attachment #787310 - Attachment is obsolete: true
Attachment #787310 - Flags: review?(cbiesinger)
Attachment #788518 - Flags: review?(cbiesinger)
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

5 years ago
Created attachment 791592 [details] [diff] [review]
patch
Attachment #788518 - Attachment is obsolete: true
Attachment #791592 - Flags: review?(cbiesinger)
Comment on attachment 791592 [details] [diff] [review]
patch

Thanks for bearing with me!
Attachment #791592 - Flags: review?(cbiesinger) → review+
(Assignee)

Comment 45

5 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

5 years ago
Keywords: checkin-needed
Attachment #319328 - Attachment is obsolete: true
https://hg.mozilla.org/integration/fx-team/rev/de84eec0f87a
Keywords: checkin-needed
Whiteboard: [engineering-friend] → [engineering-friend][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/de84eec0f87a
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [engineering-friend][fixed-in-fx-team] → [engineering-friend]
Target Milestone: --- → mozilla26
(Assignee)

Comment 48

5 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

5 years ago
Comment on attachment 791592 [details] [diff] [review]
patch

[Approval Request Comment]
See comment 48
Attachment #791592 - Flags: approval-mozilla-aurora?
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.
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

5 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+
https://hg.mozilla.org/releases/mozilla-aurora/rev/923719705762
status-firefox25: --- → fixed
status-firefox26: --- → fixed
status-firefox24: --- → wontfix
Keywords: verifyme

Comment 53

5 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.
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)

Updated

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

Comment 56

5 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

5 years ago
Whiteboard: [engineering-friend] → [engineering-friend], [bugday-20130918]
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
status-firefox25: fixed → verified
status-firefox26: fixed → verified
Keywords: verifyme

Comment 58

5 years ago
A bug has been filed, as requested: 
https://bugzilla.mozilla.org/show_bug.cgi?id=918466
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.