63105 - Temporary files used by helper apps are not removed.

Status: VERIFIED FIXED

(Core Graveyard :: File Handling, defect, P2)

Description

[K Chayka]

Win32 build 2000121520 (Win98SE), RealPlayer 8 plug-in

1. Go to the above url
2. Click on any of the RP links
3. The Downloading dialog opens. RealPlayer displays as the app in the Using box
4. Select the "Open Using" radio button
5. A file is created in c:\windows\temp containing the url of the sound file
6. RP starts up. The temp file name displays in its location box
7. The temp file is not removed after closing RP

Since I use RP for MP3 files as well, this also happens when I pick the MPEG
files on this site, except that the entire MP3 file is downloaded to temp before
RP starts. The expected results are that any file created in c:\windows\temp is
removed when another sound file is selected, or when the app is closed.

c:\windows\temp is getting very cluttered.

Comment 1

[shrirang khanzode]

well, I see no different behaviour in 4.7 as well. So ,what to do...

Comment 2

[Ian Rowan]

Win98, build 2000121804: Confirmed and marked NEW. The most reasonable time to
delete such temporary files, however, would probably be when Mozilla is exited?

Comment 3

[Todd Pringle]

av, is this something that we need to fix, or Real needs to fix?  Or do we know?

Comment 4

[av (gone)]

Does it also happen on NT4.0? I doubt this is us. Further investigation needed.

Comment 5

[Peter Lubczynski]

CC:ing [e-mail].

If Real creates the temp files it should remove them, otherwise, we should.

Comment 6

[karnaze (gone)]

Moving to m1.0 and reassigning to peterl.

Comment 7

[Peter Lubczynski]

Does this still happen? adding qawanted

Nick from Real: can you provide some input?

Comment 8

[K Chayka]

As of build 2001031604, RP downloads the files to the c:\windows\temp directory
and leaves them there if the mime types have not been manually added to helper apps.

I added the mime types as per the instructions on the RP website, but now RP
doesn't work at all. I'm having other PC/app problems at the moment, so the RP
bustage could be caused by something else altogether.

I can't confirm what RP does with mime types added until I get my general PC
problems resolved.

Comment 9

[robbe]

K Chayka: Are you sure that RP acts as a plugin, not as an "ordinary" helper
application? Because your step-by-step sure looks the same for helpers. RP shows
up in "about:plugins"?

Comment 10

[Peter Lubczynski]

Would you fix for Acrobat work for this too?

Comment 11

[av (gone)]

Probably not. That fix is for PostUrl from file. Looks like this is not the case 
here.

Comment 12

[Peter Lubczynski]

You are right, this is not plugins. I think this is something with the way 
the URILoader handles helper apps. How did 4.x handle this?

Over to mscott who may know more about this.

Comment 13

[Boris Zbarsky [:bzbarsky]]

*** Bug 76092 has been marked as a duplicate of this bug. ***

Comment 14

[Boris Zbarsky [:bzbarsky]]

This is a problem on all operating systems.

Comment 15

[Aleksey Nogin]

Copying from bug 76092:

A potential solution for Unix helper apps would be spawning sh -c "helper_app
tmpfile; rm -f tmpfile". This would also solve bug #56662 and probably bug
#58667 and also allow people to use shell syntax in the helper app
specification. If I remember correctly, this is exactly what 4.x did

Comment 16

[Peter Lubczynski]

Doug/Arun: is this bug impacted after our metting about https and plugins?

Comment 17

[Phil Peterson]

*** Bug 86242 has been marked as a duplicate of this bug. ***

Comment 18

[Phil Peterson]

> How did 4.x handle this?

The 4.x WinFE (not sure about the MacFE or XFE) kept a list of temporary files
in the Registry which were deleted when the app exited or started up (in case of
crash). There didn't seem to be a way to know when we were done with the temp file.

Nominating for mozilla0.9.2 since these temp files seem to just leak forever,
filling up the temp directory :-(

Comment 19

[Doug Turner (:dougt)]

Peter: this bug does not impacted https and plugins.  https cached files are 
maintained seperately.

Comment 20

[Scott MacGregor]

Attachment: the fix

Comment 21

[Scott MacGregor]

I just attached the fix. The external helper app service now supports the
ability to delete temporary files on exit. There's a public API which you can
use to add a nsIFile you want deleted on exit. We end up creating a pref branch
called browser.temporaryFiles.temp-0, temp-1, etc. The value of each pref entry
is a nsILocalFile. When we receive a shut down notification, we enumerate over
this pref branch and delete each temp file.

One problems with this patch: 
nsIPrefBranch::DeleteBranch isn't working currently. It doesn't really delete
the branch! I have to follow up with the pref module owners to fix this. I also
ran into another pref bug with nsIPrefBranch::GetComplexValue which I hacked
around but that needs fixed too. 

Adding keyword mumbo jumbo.

Comment 22

[Scott MacGregor]

I made a one line change to the above patch to work around the DeleteBranch bug.
I now explicitly save out the prefs after deleting the branch by calling:

prefService->SavePrefFile(nsnull)

I have to do this because I'm deleting the branch inside a shut down observer
notification we are getting from XPCOM. There seems to be a bug in how prefs
shuts down such that they save the prefs file for the last time BEFORE all
observers are notified that we are going to shutdown. As a result any changes a
shutdown observer makes to pref values has no effect. 

Comment 23

[Scott MacGregor]

For those that care, the extra one line comes right after the call to DeleteBranch:
  // now delete the branch completely since we've tried to delete all the
relevant files...
  prefBranch->DeleteBranch(nsnull);

  // the prefs service may have already saved prefs on exit. so we need to
manually resave the prefs.
  prefService->SavePrefFile(nsnull);
  return NS_OK;

Comment 24

[(not reading, please use [e-mail] instead)]

r=sspitzer

for the benefit of the super reviewer, here are some addtional questions I had 
and the answers mscott gave me:

Q: my only worry is are prefs still ok during shutdown?
A: they are ok. we haven't actually shut down yet this is the notifiation to 
observers telling folks we are going to be shutting down

Q: what does the pref service do on that notification?  what happens if prefs 
gets notified first, before you?
A: the pref service does nothing, it doesn't even pay attention to the shut 
down observer notification

Comment 25

[Alec Flett]

sorry for the delay, I was trying to track you down on AIM/etc
Anyway, my big question about all of this is why you are going through the pref
service to store these? i.e. if you're not going to persist the values across
sessions, why use prefs at all? why not just store the nsIFile pointers in an
nsISupportsArray which is stored in memory and destroyed on exit? 

Comment 26

[Scott MacGregor]

because I wanted a persistent means to manage this (like 4.x). If you crash
during a session, we'll never remember to clean up these temp files the next
time you run.  By storing the files in a pref, the next time you cleanly exit,
we'll clean everything up.

Comment 27

[Aleksey Nogin]

But what if you exit Mozilla while the helper app is still running? I believe
that the best thing to do (at least of Unix) is what 4.x did - to run the helper
app using 
sh "<helper app cmdline>; rm -f <tmpfile>"
See also my comment from 2001-06-17 02:30

Comment 28

[Scott MacGregor]

Attachment: new fix to store temp files in nsISupportsArray

Comment 29

[Scott MacGregor]

My argument about storing the temporary file list in prefs for persistence
reasons is faulty. unless I explicitly saved the prefs every time I added a temp
file to the list, there's not guaruntee my pref changes would be there after a
crash. 

I simpliefied the code and the concerns of modifying prefs during the shutdown
process by just storing the temp files in an nsISupportsArray. This simplifies
the code and the risk. 

Comment 30

[Scott MacGregor]

after Seth's code review, he noticed that I was calling
tempFile->Delete(PR_TRUE) when I should be passing PR_FALSE (for non recursive
delete). So changed the delete call to pass in false. 

Comment 31

[David :Bienvenu]

sr=bienvenu, with the change that Seth suggested.

Comment 32

[Scott MacGregor]

r=sspitzer from an aim message I got from him. 

Comment 33

[Scott MacGregor]

this has been checked into the trunk. 

Comment 34

[sairuh (rarely reading bugmail)]

sorry for the delay. this is fixed on winNT [2001.11.27.13-comm] and linux
[rh7.2, 2001.11.28.12-comm]. but i still run into this problem on mac --at least
on OS X. filed bug 112526 for that, and verifying this one.

Comment 35

[sairuh (rarely reading bugmail)]

ooh, forgot to mention the verification tests i used:

linux: 
* tested with a .ram and .rm file [added RealPlayer to handle those types].
* RealPlayer successfully launched.
* temp files were saved to /tmp while n6.x was running, but when i quit both
RealPlayer and n6.x, the temp files were removed, as expected.

winNT:
* tested with the Windows nightly build on http://mozilla.org/ [WinZip was the
default app].
* WinZip successfully launched.
* the temp file was saved to C:\TEMP while n6.x was running, but when quit both
WinZip and n6.x., the temp file was removed --again, as expected.

Mac OS X: see bug 112526 for details. :)