114778 - formpost temp files not cleaned up when browser exits

Status: VERIFIED FIXED

(Core :: Networking, defect, P2)

Description

[Darin Fisher]

formpost temp files not cleaned up when browser exits... seems like the patch
for bug 15320 didn't do the trick for Win9x systems.

Comment 1

[Darin Fisher]

-> 0.9.8

Comment 2

[Darin Fisher]

Attachment: v1.0 patch

this patch cleans up the "delete-on-close" impl... it's now an XP impl that
first attempts to unlink the file after opening a file descriptor.  if that
fails, the file input stream remembers the nsIFile passed via
nsIFileInputStream::Init, and calls Remove on the nsIFile after closing the
file descriptor.

i think it was wrong to assume that, for example, linux systems would always
operate on POSIX compatible file systems.  afterall, it is possible to mount a
Fat filesystem under linux... not that i can think of any reason why one would
use one as their temp directory =)

Comment 3

[Doug Turner (:dougt)]

Comment on attachment 61533 [details] [diff] [review]
v1.0 patch

a.  why even bother with trying to delete the file prior to your done with it?

b.  what happens if you the remove file fails?

Comment 4

[Darin Fisher]

a) because if the file can be removed in the Init() method, then i don't have to
hold onto the nsLocalFile object for the lifetime of the nsFileInputStream.  for
multipart formpost data, this would correspond to the lifetime of a browser window.

b) if Remove fails, then we'll leave/leak a file in the users temp directory :(

Comment 5

[Scott MacGregor]

Comment on attachment 61533 [details] [diff] [review]
v1.0 patch

sr=mscott

Comment 6

[gordon]

The patch is already marked "has-review", but I didn't see an r= anywhere, so
here's mine:  r=gordon.

Comment 7

[Darin Fisher]

fixed-on-trunk

Comment 8

[K Chayka]

build 2001122003, win98se

Ran some html files through http://validator.w3.org/file-upload.html

formpost files are still left in the temp dir after the browser closes. :-(
In case it matters, I override the default c:\windows\temp in my autoexec.bat file:

SET temp=e:\tmpdir
SET tmp=e:\tmpdir

The formpost files get created in e:\tmpdir OK, but don't get removed.

Comment 9

[Darin Fisher]

K Chayka: could you try to reproduce the problem using a more recent build?  i
suspect the 2001122003 build might not of picked up this fix.... though i'm not
really certain how much latency there is between code changes and nightly builds.

Comment 10

[K Chayka]

build 2001122103 - same thing

1. Start mozilla
2. Run one html file through the W3C validator upload service - the formpost
file gets created in the temp dir
3. Close browser window - mozilla shuts down and the formpost file sits there

Comment 11

[Darin Fisher]

ok...  guess i need to find myself a win98 system to test this on :-(

reopening.

Comment 12

[Darin Fisher]

vinay: do you have access to a win98 build system (w/ a debug build) where you
can test this bug?

Comment 13

[Vinay Badami]

Will find out from the sysadmin here and let u know on Monday.

Comment 14

[David Gerard]

I've seen this bug for months on Windows NT4 as well. Still happening in
2002010803-trunk.

Comment 15

[Darin Fisher]

david: any chance you can supply a testcase or example of this bug somewhere on
the internet?  my own tests with WINNT all passed.  thx!

Comment 16

[Vinay Badami]

Darin
I'am able to reproduce this on W2K using the url mentioned in the bug and
uploading a html page to that url.
ie  http://validator.w3.org/file-upload.html

One bug i do see is that rv always comes back as NS_OK from
mozilla/xpcom/io/nsLocalFileWin.cpp. This is because the return from remove is
being ignored. So we need this patch.
Index: nsLocalFileWin.cpp
===================================================================
RCS file: /cvsroot/mozilla/xpcom/io/nsLocalFileWin.cpp,v
retrieving revision 1.71
diff -u -r1.71 nsLocalFileWin.cpp
--- nsLocalFileWin.cpp  2002/01/09 20:03:45     1.71
+++ nsLocalFileWin.cpp  2002/01/14 11:41:36
@@ -1223,15 +1223,15 @@
                 iterator->HasMoreElements(&more);
             }
         }
-        rmdir(filePath);  // todo: save return value?
+        rv = rmdir(filePath);  // todo: save return value?
     }
     else
     {
-        remove(filePath); // todo: save return value?
+        rv = remove(filePath); // todo: save return value?
     }

     MakeDirty();
-    return NS_OK;
+    return rv;
 }

However, even after this patch the file does not get removed. We are now
remembering mFileToDelete.
However, looks like the Close is never getting called.
Looking into this.

Comment 17

[Vinay Badami]

I'am wondering if the Close is not getting called on shutdown becuase th io
stuff is an XPCOM service ???

Comment 18

[David Gerard]

Er, testcase? I will attach a formpost file that this build
(2002010803) left behind when I posted a comment at
http://www.rocknerd.org/rocknerd/1010725277/index_html .

Comment 19

[David Gerard]

Attachment: formpost file left behind when posting comment on rocknerd.org

Comment 20

[Grey Hodge (jX)]

This is also present on WinXP as well, but it seems only to manifest it self on
Win32 funning on FAT32, not NTFS. Which is why testing on NT passes (no FAT32
support (and WinInternals FAT32 for NT driver doesn't count)). From a cursory
examination of the problem (not the code, yet), I would say that the FAT32 file
system services is not deleting the file because: A) Moz isn't properly asking
for the right API call, and/or B) FAT32 FS services suck, and need to be led by
the hand into deleting the exact file in a very basic manner.

Now, the FAT32 support for @k and XP are based on the IFS services component for
Windows 98/98SE. 98/98SE introduced the WDM system, which the file system
services plugged into as well. this made it more portable to 2k (if you examine
the FS services files you'll see references to NTFS, bearing out the shared
codebase). This is why the bug manifests itself on both 9x/ME and 2k/XP; the FSS
code is the shared on both platforms.

I'll have time in the next day or two and I'll look into it too. The more eyes
the better. :)

Comment 21

[David Gerard]

Grey: hrmmm. I'm seeing this on NT4, and both partitions on this box
are NTFS, not FAT16 or anything else (no FAT32 support on this thing).

I suppose I should get a more recent nightly and keep trying.

Is there anything in particular I can do to test for this one? Anyone
got a particular debugging build they'd like me to try? (I have no
compiler here.)

Comment 22

[Darin Fisher]

sounds like vinay is on top of this one.

-> badami

Comment 23

[Grey Hodge (jX)]

I was making that assumption based on hearing that it does not manifest itself
on NTFS disks. If it does, then it's not related to the Win32 FAT32 FSS code.

Comment 24

[Vinay Badami]

Attachment: To cause file removal

1. mozilla/xpcom/io/nsLocalFileWin.cpp
Return proper return value as opposed to NS_OK always.

2. netwerk/base/src/nsBufferedStreams.cpp
Should release reference on mstream when buffered stream is being closed.

3.mozilla/netwerk/base/src/nsFileStreams.h
 ~nsFileInputStream() should call it's close.

Comment 25

[Vinay Badami]

Attachment: adding in fix for OS2 also

same problem in sLocalFileOS2.cpp as in the windows version.
The OS2 version is not tested and not sure if it compiles either.

Comment 26

[Darin Fisher]

Comment on attachment 66280 [details] [diff] [review]
adding in fix for OS2 also

sr=darin (nice work nailing this one!)

Comment 27

[Vinay Badami]

rpotts
Can i get a r= on this please ?

Comment 28

[rpotts (gone)]

Comment on attachment 66280 [details] [diff] [review]
adding in fix for OS2 also

r=[e-mail]

looks good!

Comment 29

[Darin Fisher]

fixed-on-trunk

Comment 30

[K Chayka]

win32 build 2002012608, win98se

Looks like it is indeed fixed.  Thank you!

Comment 31

[Grey Hodge (jX)]

vrfy