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

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

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

Comment 1

[Kartikaya Gupta (email:[e-mail])]

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?

Comment 2

[:Paolo Amadini]

(In reply to Kartikaya Gupta (email:[e-mail]) 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.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

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

Comment 4

[:Paolo Amadini]

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.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

(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

[:Paolo Amadini]

Comment on attachment 8574398 [details] [diff] [review]
Patch

(In reply to Kartikaya Gupta (email:[e-mail]) 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:[e-mail]) 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.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8574398 [details] [diff] [review]
Patch

Might as well, mostly as an FYI that this change is happening.

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8574398 [details] [diff] [review]
Patch

Looks safe enough, especially if it helps reduce left-over temp files. Thanks!

Comment 9

[Kartikaya Gupta (email:[e-mail])]

Added a comment about cleaning up temp files and landed:

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

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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