Closed
Bug 460609
Opened 15 years ago
Closed 14 years ago
Temporary files for helper applications are not deleted when leaving Private Browsing mode
Categories
(Core Graveyard :: File Handling, defect)
Core Graveyard
File Handling
Tracking
(Not tracked)
VERIFIED
FIXED
mozilla1.9.1b2
People
(Reporter: whimboo, Assigned: ehsan.akhgari)
References
Details
(Keywords: privacy, user-doc-needed, verified1.9.1)
Attachments
(1 file, 2 obsolete files)
8.19 KB,
patch
|
bzbarsky
:
review+
bzbarsky
:
superreview+
beltzner
:
approval1.9.1b2+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•15 years ago
|
No longer blocks: PrivateBrowsing
Reporter | ||
Updated•15 years ago
|
Blocks: PrivateBrowsing
Assignee | ||
Updated•15 years ago
|
No longer blocks: PrivateBrowsing
Assignee | ||
Updated•15 years ago
|
Blocks: PrivateBrowsing
Updated•15 years ago
|
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
Assignee | ||
Comment 1•15 years ago
|
||
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
Reporter | ||
Comment 2•15 years ago
|
||
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.
Assignee | ||
Comment 3•15 years ago
|
||
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
Assignee | ||
Comment 4•14 years ago
|
||
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
Assignee | ||
Comment 5•14 years ago
|
||
Make the external app launcher service private browsing aware.
Attachment #347134 -
Flags: superreview?(bzbarsky)
Attachment #347134 -
Flags: review?(cbiesinger)
![]() |
||
Comment 6•14 years ago
|
||
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.
Assignee | ||
Comment 7•14 years ago
|
||
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)
![]() |
||
Comment 8•14 years ago
|
||
Is there a reason all those expunge methods aren't just |void| return instead of nsresult?
Assignee | ||
Comment 9•14 years ago
|
||
They're effectively void; they just return a "fake" value which is ignored by the callers! Do you want me to make them void?
Assignee | ||
Comment 10•14 years ago
|
||
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 11•14 years ago
|
||
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+
Assignee | ||
Comment 12•14 years ago
|
||
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?
Assignee | ||
Updated•14 years ago
|
Flags: in-litmus?
Updated•14 years ago
|
Flags: blocking1.9.1+
Comment 13•14 years ago
|
||
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+
Assignee | ||
Comment 14•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/5c289d7b5f17
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b2
Reporter | ||
Comment 15•14 years ago
|
||
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 → ---
Assignee | ||
Comment 16•14 years ago
|
||
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•14 years ago
|
||
I think privacy trumps consistency at this point. The about:privatebrowsing page says downloads get removed, right?
Assignee | ||
Comment 18•14 years ago
|
||
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•14 years ago
|
||
(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
Assignee | ||
Comment 20•14 years ago
|
||
(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•14 years ago
|
||
(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.
Assignee | ||
Comment 22•14 years ago
|
||
(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: 14 years ago → 14 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 23•14 years ago
|
||
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
Updated•14 years ago
|
Keywords: fixed1.9.1
Reporter | ||
Comment 24•14 years ago
|
||
Added as Litmus test: https://litmus.mozilla.org/show_test.cgi?id=7440
Flags: in-litmus? → in-litmus+
Keywords: fixed1.9.1 → verified1.9.1
Assignee | ||
Comment 25•14 years ago
|
||
See bug 496123 comment 10 about exactly what needs to be documented here.
Keywords: user-doc-needed
Updated•7 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•