460609 - Temporary files for helper applications are not deleted when leaving Private Browsing mode

Status: VERIFIED FIXED

(Core Graveyard :: File Handling, defect)

Description

[Henrik Skupin (:whimboo) [⌚️UTC+2] (away 07/24 - 08/10)]

While testing the latest try server build on bug 248970 I noticed that
temporary files for helper applications are not deleted when leaving the Private Browsing mode. They still resist on disk until the browser is closed. 

Steps:
1. Start Firefox and activate Private Browsing (save current session)
2. Search for "pdf" on Google
3. Open a pdf file with an external application
4. Close external application
5. Leave Private Browsing mode
6. Open temporary folder (%temp%)

With step 6 you can see that the downloaded pdf file is still existent in the temporary folder. On Windows it's not directly visible and they are deleted on shutdown but on OS X the files are located on the Desktop (bug 311292) and don't get deleted on shutdown.

Comment 1

[:ehsan akhgari]

Henrik,

I suspect that this is the same issue as bug 460689, and it _might_ be caused by the incorrect pref name in your profile (see bug 460689 comment 8).  I'd like to know a) if you can reproduce this bug with the correct pref name, and if yes, b) if it happens only in private mode.

Thanks!

Comment 2

[Henrik Skupin (:whimboo) [⌚️UTC+2] (away 07/24 - 08/10)]

Ehsan, this issue still persists. After leaving the Private Browsing mode all the downloaded temporary files still remain in the folder. They will be deleted on shutdown but until this step all these files are accessible by everyone.

Comment 3

[:ehsan akhgari]

OK, I investigated this issue, and the reason is that the download manager uses nsPIExternalAppLauncher.deleteTemporaryFileOnExit to delete the temporary files when the application closed (on receiving the profile-before-change notification actually <http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#1037>).

There are two solutions here:
1. Store the list of temporary files in the private mode inside the download manager history and handle their deletion there when leaving the private mode instead of handing it off to nsPIExternalAppLauncher in the first place.
2. Make the external app launcher service aware of the private mode, to support deleting the temporary files added in private mode when leaving it.

I'm not sure which solution is better though.  Shawn, what do you think?

Comment 4

[:ehsan akhgari]

I think we want the second solution here, because the handling in download manager is only done for resumed downloads, and both resumed and normal downloads will be handled correctly if this is done at external app launcher service level.

Moving to Core::File Handling.  I'll post my patch shortly.

Comment 5

[:ehsan akhgari]

Attachment: Patch (v1)

Make the external app launcher service private browsing aware.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 347134 [details] [diff] [review]
Patch (v1)

>+++ b/uriloader/exthandler/nsExternalHelperAppService.cpp
> nsresult nsExternalHelperAppService::Init()
>+    (void)pbs->GetPrivateBrowsingEnabled(&mInPrivateBrowsing);

No need for the (void) part.

nsExternalHelperAppService::ExpungeTemporaryPrivateFiles()

Instead of copying this code, how about a static helper method that takes an nsCOMArray<nsILocalFile> and does what's needed?  Then call that method from both Expunge methods.

The rest looks fine to me.

Comment 7

[:ehsan akhgari]

Attachment: Patch (v1.1)

Addressed Boris' comments in comment 6.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Is there a reason all those expunge methods aren't just |void| return instead of nsresult?

Comment 9

[:ehsan akhgari]

They're effectively void; they just return a "fake" value which is ignored by the callers!  Do you want me to make them void?

Comment 10

[:ehsan akhgari]

Attachment: Patch (v1.2)

OK, I changed the "expunge" functions to make them void, instead of returning the nsresult.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 347273 [details] [diff] [review]
Patch (v1.2)

r+sr=bzbarsky

Comment 12

[:ehsan akhgari]

Comment on attachment 347273 [details] [diff] [review]
Patch (v1.2)

This would possibly help prevent having a number of duplicate bug reports from users in Beta 2.

Comment 13

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 347273 [details] [diff] [review]
Patch (v1.2)

a=beltzner, though I just made the bug a blocker as well

Comment 14

[:ehsan akhgari]

http://hg.mozilla.org/mozilla-central/rev/5c289d7b5f17

Comment 15

[Henrik Skupin (:whimboo) [⌚️UTC+2] (away 07/24 - 08/10)]

Ok, I did a test-run on OS X and Vista. On both systems the temporary files are getting deleted when leaving the Private Browsing mode. But on OS X you have to add a hidden pref (Boolean: browser.helperApps.deleteTempFileOnExit=true) to get the expected behavior.

IMO we shouldn't obey this pref when leaving the Private Browsing mode on OS X. This is still a way to have access to temporary downloaded files on a shared computer. It's even worth that everything will be downloaded to the desktop on OS X. Means it's more than an eye-catcher.

Josh and/or Steven, do you have any objection to not respecting this pref for that particular case?

Let's reopen this bug for now.

Comment 16

[:ehsan akhgari]

The comments in the file explicitly state that Mac users do not want this behavior.  I think that, it would be justifiable to delete the temp files when leaving the private mode by default, but should we have another pref to allow Mac users to explicitly override this choice?

Comment 17

[Shawn Wilsher :sdwilsh]

I think privacy trumps consistency at this point.  The about:privatebrowsing page says downloads get removed, right?

Comment 18

[:ehsan akhgari]

Actually it says:

"However, any files you download or bookmarks you create will be preserved."

But I think we would want to revise that sentence...

Comment 19

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #16)
> The comments in the file explicitly state that Mac users do not want this

Which file? I understand that OSX users don't want helper files removed, but I don't think that applies to partial downloads as much as it does to PDFs which are downloaded and shown in Preview by default.

> Actually it says:
> 
> "However, any files you download or bookmarks you create will be preserved."
> 
> But I think we would want to revise that sentence...

Well, the files themselves are preserved, but the record of you downloading them are not. I think the issue here is that a user might not expect that if they cancel a download, or a download fails, that there will be any evidence of it on their system.

Scenario:
 - enter private browsing mode
 - start downloading secret.file
 - pause, cancel or have download fail
 - shrug, and exit private browsing mode

At this point I don't have a secret.file in my downloads folder, but on my system I do have a secret.file.part which might be unexpected. Also, should I re-enter Private Browsing Mode, it's not like that partially downloaded file will ever be helpful for me, as we've purged the download history (or do we sniff for .part files and try to resume?)

Either way, where we are is good enough for Beta 2, so retargetting at final.

Comment 20

[:ehsan akhgari]

(In reply to comment #19)
> (In reply to comment #16)
> > The comments in the file explicitly state that Mac users do not want this
> 
> Which file? I understand that OSX users don't want helper files removed, but I
> don't think that applies to partial downloads as much as it does to PDFs which
> are downloaded and shown in Preview by default.

Seems like they want to keep those files as well:

<http://mxr.mozilla.org/mozilla-central/search?string=verbal+about+temp+files>

> > Actually it says:
> > 
> > "However, any files you download or bookmarks you create will be preserved."
> > 
> > But I think we would want to revise that sentence...
> 
> Well, the files themselves are preserved, but the record of you downloading
> them are not.

No, this bug is about physically deleting the files which were used as temporary ones for helper applications.

> I think the issue here is that a user might not expect that if
> they cancel a download, or a download fails, that there will be any evidence of
> it on their system.

Exactly.

> Scenario:
>  - enter private browsing mode
>  - start downloading secret.file
>  - pause, cancel or have download fail
>  - shrug, and exit private browsing mode
> 
> At this point I don't have a secret.file in my downloads folder, but on my
> system I do have a secret.file.part which might be unexpected.

Oh, that is a new bug.  Please see comment 0.  If you think the scenario you're describing is worth handling as well, please file a new bug and assign it to me.

> Also, should I
> re-enter Private Browsing Mode, it's not like that partially downloaded file
> will ever be helpful for me, as we've purged the download history (or do we
> sniff for .part files and try to resume?)

No, at the end of private browsing mode, we cancel any active downloads, and forget all about them, as far as the download DB is concerned.  We currently do _not_ handle partially downloaded files at all, unless the API to cancel them does that for us, that is.  Shawn, can you please confirm this?

> Either way, where we are is good enough for Beta 2, so retargetting at final.

Agreed.

Comment 21

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #15)
> Let's reopen this bug for now.

Can we actually get this one marked FIXED and file a follow-up? It's just a little easier to track that way.

Comment 22

[:ehsan akhgari]

(In reply to comment #21)
> (In reply to comment #15)
> > Let's reopen this bug for now.
> 
> Can we actually get this one marked FIXED and file a follow-up? It's just a
> little easier to track that way.

Done.  Filed bug 465756 as a followup.

Comment 23

[Henrik Skupin (:whimboo) [⌚️UTC+2] (away 07/24 - 08/10)]

Thanks Ehsan! Verified fixed with:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b2pre) Gecko/20081121 Minefield/3.1b2pre ID:20081121034512

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b2pre) Gecko/20081121 Minefield/3.1b2pre and browser.helperApps.deleteTempFileOnExit set to true

Resetting TM to beta2.

Comment 24

[Henrik Skupin (:whimboo) [⌚️UTC+2] (away 07/24 - 08/10)]

Added as Litmus test:
https://litmus.mozilla.org/show_test.cgi?id=7440

Comment 25

[:ehsan akhgari]

See bug 496123 comment 10 about exactly what needs to be documented here.