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

Status: VERIFIED FIXED

(Core Graveyard :: File Handling, defect)

Description

[Arturo Frappé]

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

Comment 1

[Arturo Frappé]

Attachment: It solves the bug, looking if .part filesize equals zero

Comment 2

[Shawn Wilsher :sdwilsh]

could you generate that diff with -u8p please for more context?

Comment 3

[Arturo Frappé]

Attachment: patch created with -pU 8

From file /mozilla/uriloader/exthandler/nsExternalHelperAppService.cpp

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

isn't mReceivedDispositionInfo supposed to be false in this case?

Comment 5

[Arturo Frappé]

I agree, it seems that mReceivedDispositionInfo must be false, or the condition must ask for NS_BINDING_ABORTED too

Comment 6

[Arturo Frappé]

Attachment: asking for NS_BINDING_ABORTED

A more elegant way to solve this bug.

Comment 7

[Arturo Frappé]

(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?

Comment 8

[Arturo Frappé]

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

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

That is not what I meant... I meant isn't mReceived... only set to true after you click OK on the save dialog?

Comment 10

[Christian :Biesinger (don't email me, ping me on IRC)]

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.

Comment 11

[Arturo Frappé]

(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

Comment 12

[Arturo Frappé]

Attachment: mReceivedDispositionInfo = PR_FALSE

As Christian proposed, mReceivedDispositionInfo = PR_FALSE before call Cancel

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 14

[Arturo Frappé]

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.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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

[Christian :Biesinger (don't email me, ping me on IRC)]

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;
)

Comment 17

[Arturo Frappé]

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

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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

[David H]

Duplicate: 475008

Comment 22

[Arturo Frappé]

(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

[David H]

(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

[Carlos Rodrigues]

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

[David H]

I can reproduce as well but on OS X 10.8.3 in Firefox 20

Comment 27

[Carlos Rodrigues]

This is still an issue in Firefox 21.

Comment 28

[Carlos Rodrigues]

This continues to be an issue in Firefox 22.

Comment 29

[Jethro Hemmann]

Also in Firefox 23.

Comment 30

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 31

[Xidorn Quan [:xidorn] UTC+11]

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.

Comment 32

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch

Comment 33

[Christian :Biesinger (don't email me, ping me on IRC)]

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?

Comment 34

[Xidorn Quan [:xidorn] UTC+11]

(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

[:Paolo Amadini]

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.

Comment 36

[Xidorn Quan [:xidorn] UTC+11]

(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

[:Paolo Amadini]

(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?

Comment 38

[Xidorn Quan [:xidorn] UTC+11]

(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

[:Paolo Amadini]

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);".

Comment 40

[:Paolo Amadini]

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

Comment 41

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch

Comment 42

[Christian :Biesinger (don't email me, ping me on IRC)]

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) {

Comment 43

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch

Comment 44

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 791592 [details] [diff] [review]
patch

Thanks for bearing with me!

Comment 45

[Xidorn Quan [:xidorn] UTC+11]

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

Comment 46

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/de84eec0f87a

Comment 47

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/de84eec0f87a

Comment 48

[Xidorn Quan [:xidorn] UTC+11]

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

Comment 49

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 791592 [details] [diff] [review]
patch

[Approval Request Comment]
See comment 48

Comment 50

[Josh Matthews [:jdm]]

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

[bhavana bajaj [:bajaj]]

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.

Comment 52

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/923719705762

Comment 53

[Bader Zaidan]

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

[u279076]

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?

Comment 56

[Xidorn Quan [:xidorn] UTC+11]

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

Comment 57

[u279076]

Thank you Xidorn, I'm going to call this verified fixed based on previous testing.

Bader, please file a new bug for your issue.

Comment 58

[Bader Zaidan]

A bug has been filed, as requested: 
https://bugzilla.mozilla.org/show_bug.cgi?id=918466