Closed
Bug 114778
Opened 23 years ago
Closed 23 years ago
formpost temp files not cleaned up when browser exits
Categories
(Core :: Networking, defect, P2)
Tracking
()
VERIFIED
FIXED
mozilla0.9.8
People
(Reporter: darin.moz, Assigned: badami)
Details
(Whiteboard: [ready-to-land])
Attachments
(4 files)
(deleted),
patch
|
dougt
:
review+
mscott
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
rpotts
:
review+
darin.moz
:
superreview+
|
Details | Diff | Splinter Review |
formpost temp files not cleaned up when browser exits... seems like the patch
for bug 15320 didn't do the trick for Win9x systems.
Reporter | ||
Comment 1•23 years ago
|
||
-> 0.9.8
Severity: normal → major
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → mozilla0.9.8
Reporter | ||
Comment 2•23 years ago
|
||
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•23 years ago
|
||
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?
Attachment #61533 -
Flags: review+
Reporter | ||
Comment 4•23 years ago
|
||
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•23 years ago
|
||
Comment on attachment 61533 [details] [diff] [review]
v1.0 patch
sr=mscott
Attachment #61533 -
Flags: superreview+
The patch is already marked "has-review", but I didn't see an r= anywhere, so
here's mine: r=gordon.
Reporter | ||
Updated•23 years ago
|
Whiteboard: [ready-to-land]
Reporter | ||
Comment 7•23 years ago
|
||
fixed-on-trunk
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
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.
Reporter | ||
Comment 9•23 years ago
|
||
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•23 years ago
|
||
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
Reporter | ||
Comment 11•23 years ago
|
||
ok... guess i need to find myself a win98 system to test this on :-(
reopening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Reporter | ||
Comment 12•23 years ago
|
||
vinay: do you have access to a win98 build system (w/ a debug build) where you
can test this bug?
Assignee | ||
Comment 13•23 years ago
|
||
Will find out from the sysadmin here and let u know on Monday.
Comment 14•23 years ago
|
||
I've seen this bug for months on Windows NT4 as well. Still happening in
2002010803-trunk.
Reporter | ||
Comment 15•23 years ago
|
||
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!
Assignee | ||
Comment 16•23 years ago
|
||
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.
Assignee | ||
Comment 17•23 years ago
|
||
I'am wondering if the Close is not getting called on shutdown becuase th io
stuff is an XPCOM service ???
Comment 18•23 years ago
|
||
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•23 years ago
|
||
Comment 20•23 years ago
|
||
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•23 years ago
|
||
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.)
Reporter | ||
Comment 22•23 years ago
|
||
sounds like vinay is on top of this one.
-> badami
Assignee: darin → badami
Status: REOPENED → NEW
Comment 23•23 years ago
|
||
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.
Assignee | ||
Comment 24•23 years ago
|
||
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.
Assignee | ||
Comment 25•23 years ago
|
||
same problem in sLocalFileOS2.cpp as in the windows version.
The OS2 version is not tested and not sure if it compiles either.
Reporter | ||
Updated•23 years ago
|
Attachment #66280 -
Flags: superreview+
Reporter | ||
Comment 26•23 years ago
|
||
Comment on attachment 66280 [details] [diff] [review]
adding in fix for OS2 also
sr=darin (nice work nailing this one!)
Assignee | ||
Comment 27•23 years ago
|
||
rpotts
Can i get a r= on this please ?
Comment 28•23 years ago
|
||
Comment on attachment 66280 [details] [diff] [review]
adding in fix for OS2 also
r=rpotts@netscape.com
looks good!
Attachment #66280 -
Flags: review+
Reporter | ||
Comment 29•23 years ago
|
||
fixed-on-trunk
Status: NEW → RESOLVED
Closed: 23 years ago → 23 years ago
Resolution: --- → FIXED
Comment 30•23 years ago
|
||
win32 build 2002012608, win98se
Looks like it is indeed fixed. Thank you!
You need to log in
before you can comment on or make changes to this bug.
Description
•