Closed Bug 460609 Opened 12 years ago Closed 12 years ago

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

Categories

(Core Graveyard :: File Handling, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED
mozilla1.9.1b2

People

(Reporter: whimboo, Assigned: ehsan)

References

Details

(Keywords: privacy, user-doc-needed, verified1.9.1)

Attachments

(1 file, 2 obsolete files)

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.
No longer blocks: PrivateBrowsing
Summary: Temporary files for helper applications are not deleted when leaving Private Browing mode → Temporary files for helper applications are not deleted when leaving Private Browsing mode
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!
Depends on: 460689
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.
No longer depends on: 460689
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?
Assignee: nobody → ehsan.akhgari
Status: NEW → ASSIGNED
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.
Component: General → File Handling
Product: Firefox → Core
QA Contact: general → file-handling
Attached patch Patch (v1) (obsolete) — Splinter Review
Make the external app launcher service private browsing aware.
Attachment #347134 - Flags: superreview?(bzbarsky)
Attachment #347134 - Flags: review?(cbiesinger)
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.
Attached patch Patch (v1.1) (obsolete) — Splinter Review
Addressed Boris' comments in comment 6.
Attachment #347134 - Attachment is obsolete: true
Attachment #347169 - Flags: superreview?(bzbarsky)
Attachment #347169 - Flags: review?(cbiesinger)
Attachment #347134 - Flags: superreview?(bzbarsky)
Attachment #347134 - Flags: review?(cbiesinger)
Is there a reason all those expunge methods aren't just |void| return instead of nsresult?
They're effectively void; they just return a "fake" value which is ignored by the callers!  Do you want me to make them void?
Attached patch Patch (v1.2)Splinter Review
OK, I changed the "expunge" functions to make them void, instead of returning the nsresult.
Attachment #347169 - Attachment is obsolete: true
Attachment #347273 - Flags: superreview?(bzbarsky)
Attachment #347273 - Flags: review?(cbiesinger)
Attachment #347169 - Flags: superreview?(bzbarsky)
Attachment #347169 - Flags: review?(cbiesinger)
Comment on attachment 347273 [details] [diff] [review]
Patch (v1.2)

r+sr=bzbarsky
Attachment #347273 - Flags: superreview?(bzbarsky)
Attachment #347273 - Flags: superreview+
Attachment #347273 - Flags: review?(cbiesinger)
Attachment #347273 - Flags: review+
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.
Attachment #347273 - Flags: approval1.9.1b2?
Flags: in-litmus?
Flags: blocking1.9.1+
Comment on attachment 347273 [details] [diff] [review]
Patch (v1.2)

a=beltzner, though I just made the bug a blocker as well
Attachment #347273 - Flags: approval1.9.1b2? → approval1.9.1b2+
http://hg.mozilla.org/mozilla-central/rev/5c289d7b5f17
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b2
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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?
I think privacy trumps consistency at this point.  The about:privatebrowsing page says downloads get removed, right?
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...
(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.
Target Milestone: mozilla1.9.1b2 → mozilla1.9.1
(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.
(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.
Depends on: 465756
(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.
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → VERIFIED
Target Milestone: mozilla1.9.1 → mozilla1.9.1b2
Added as Litmus test:
https://litmus.mozilla.org/show_test.cgi?id=7440
Flags: in-litmus? → in-litmus+
See bug 496123 comment 10 about exactly what needs to be documented here.
Keywords: user-doc-needed
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.