If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Fennec leaves behind .part files in the download directory when viewing some videos in a helper app

RESOLVED FIXED in Firefox 39

Status

Core Graveyard
File Handling
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: kats, Assigned: kats)

Tracking

Trunk
mozilla39
ARM
Android

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(1 attachment)

I'm finding a lot of .part files left behind in the download directory, and it appears to happen on certain videos which get opened in the Android video player. Specifically, the temp file is created at [1] and then when I step through the rest of this function I get to [2] and then the function exits. I'm not sure how the temp file is supposed to be cleaned up - is it the helper app that's supposed to be doing it?

In my case the videos are not publicly accessible but I can probably set up a test site to repro the problem if needed.

[1] http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp?rev=82b5487e4ca9#1619
[2] http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp?rev=82b5487e4ca9#1717
Paolo, you seem to have reviewed a number of patches in this area, do you know what's supposed to be happening here, or can you redirect to somebody who knows?
Flags: needinfo?(paolo.mozmail)

Comment 2

3 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #0)
> http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/
> nsExternalHelperAppService.cpp?rev=82b5487e4ca9#1717

If rv is a failure code here, I believe the comment right after that line is correct and we should call:

Cancel(NS_BINDING_ABORTED);

This should ensure the part file is deleted in case of failure. But I believe your downloads were successful?

Do you have any add-ons installed that may be overriding the registered dialog? This code should normally proceed to the "show" function in "mobile/android/components/HelperAppDialog.js". If this doesn't invoke the expected callback, we may keep the part file around forever. Background downloading may still continue while the user makes a choice, so the file will be completely downloaded in the end, but it is still a partial file from our point of view.

In fact, normally we should complete the entire download process before opening any helper application, and at that point the temporary ".part" files are registered for deletion when the browser exits.
Flags: needinfo?(paolo.mozmail)
Created attachment 8574398 [details] [diff] [review]
Patch

Thanks for the info, that helped me track down the problem. The code at [1] was not calling the aLauncher.cancel callback after it was done, like all the other code paths through that function do. This also explains why I was only seeing the .part file left behind after I picked "Use Always" on the video player as the helper app. The attached patch fixes the problem for me.

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/HelperAppDialog.js?rev=a5099b806777#165
Assignee: nobody → bugmail.mozilla
Attachment #8574398 - Flags: review?(paolo.mozmail)

Comment 4

3 years ago
Comment on attachment 8574398 [details] [diff] [review]
Patch

Review of attachment 8574398 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/components/HelperAppDialog.js
@@ +156,5 @@
> +    let callback = function(app) {
> +      aLauncher.MIMEInfo.preferredAction = Ci.nsIMIMEInfo.useHelperApp;
> +      if (!app.launch(aLauncher.source)) {
> +        aLauncher.cancel(Cr.NS_BINDING_ABORTED);
> +      }

Something is unclear to me here. If we pass the source URI to the external helper application and it uses this to fetch the file, shouldn't we just remove our ".part" file unconditionally?

Also, you should check that setting preferredAction to useHelperApp has no side effects for the case you've just changed, I'm not sure about how it works on Android.
(In reply to :Paolo Amadini from comment #4)
> Comment on attachment 8574398 [details] [diff] [review]
> Patch
> 
> Review of attachment 8574398 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/components/HelperAppDialog.js
> @@ +156,5 @@
> > +    let callback = function(app) {
> > +      aLauncher.MIMEInfo.preferredAction = Ci.nsIMIMEInfo.useHelperApp;
> > +      if (!app.launch(aLauncher.source)) {
> > +        aLauncher.cancel(Cr.NS_BINDING_ABORTED);
> > +      }
> 
> Something is unclear to me here. If we pass the source URI to the external
> helper application and it uses this to fetch the file, shouldn't we just
> remove our ".part" file unconditionally?

I think in practice that is what happens. The "app.launch" function is defined at [1] and it always returns false. The only one that doesn't is the one at [2] in which case we do want to save it to disk and keep the file around.

> Also, you should check that setting preferredAction to useHelperApp has no
> side effects for the case you've just changed, I'm not sure about how it
> works on Android.

As far as I can tell it from using Fennec it doesn't have any side effects. The action that gets returned by GetPreferredAction (in nsExternalAppHandler::OnStartRequest) is already useHelperApp so it should be a no-op. I can flag a Fennec peer for additional review if you would like that.

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/modules/HelperApps.jsm?rev=c379987d0266#35
[2] http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/HelperAppDialog.js?rev=a5099b806777#148

Comment 6

3 years ago
Comment on attachment 8574398 [details] [diff] [review]
Patch

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #5)
> I think in practice that is what happens. The "app.launch" function is
> defined at [1] and it always returns false. The only one that doesn't is the
> one at [2] in which case we do want to save it to disk and keep the file
> around.

Worth explaining this as a comment, but makes sense, thanks for the info!

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #5)
> I can flag a Fennec peer for additional review if you would like that.

I'll leave this up to you, you probably can evaluate the trade-offs better than me in this area of the code.
Attachment #8574398 - Flags: review?(paolo.mozmail) → review+
Comment on attachment 8574398 [details] [diff] [review]
Patch

Might as well, mostly as an FYI that this change is happening.
Attachment #8574398 - Flags: review?(mark.finkle)
Comment on attachment 8574398 [details] [diff] [review]
Patch

Looks safe enough, especially if it helps reduce left-over temp files. Thanks!
Attachment #8574398 - Flags: review?(mark.finkle) → review+
Added a comment about cleaning up temp files and landed:

https://hg.mozilla.org/integration/fx-team/rev/e6839726fe1d
https://hg.mozilla.org/mozilla-central/rev/e6839726fe1d
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.