Closed
Bug 75317
Opened 24 years ago
Closed 24 years ago
Forwarding msgs with attachments doesn't clean up temp files
Categories
(MailNews Core :: Backend, defect)
MailNews Core
Backend
Tracking
(Not tracked)
VERIFIED
FIXED
People
(Reporter: cavin, Assigned: cavin)
References
Details
Attachments
(5 files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
Select a message with at least one attachment (in INBOX or any folders) and hit
the Forward button. Either you send (ie, forward) the message or cancel it some
temp files in C:\TEMP are not cleaned up. The temp files are those created for
the attachments.
Assignee | ||
Comment 1•24 years ago
|
||
Comment 2•24 years ago
|
||
+void nsMsgCompFields::CleanUpTempFiles()
+{
+
char *attachmentList = (char *)GetAttachmentsToDelete();
+
+
if ((!attachmentList) || (!*attachmentList))
+ return;
+
+ // Make our local copy...
+ attachmentList = PL_strdup(GetAttachments());
in the line above, I presume you mean:
attachmentList = PL_strdup(GetAttachmentsToDelete());
Comment 3•24 years ago
|
||
nsFileSpec is being depricated. see nsFileSpec.h
what about using the code we found in edior that used nsIFileUrl and nsIFile?
Assignee | ||
Comment 4•24 years ago
|
||
Assignee | ||
Comment 5•24 years ago
|
||
Comment 6•24 years ago
|
||
Looks good. R=ducarroz
Comment 7•24 years ago
|
||
comments:
+ char *attachmentList = (char *)GetTemporaryFiles();
+ if ((!attachmentList) || (!*attachmentList))
+ return NS_ERROR_NULL_POINTER;
is that really an error? what happens if I send mail without any attachments
and no vcard?
+ // Make our local copy...
+ attachmentList = nsCRT::strdup(GetTemporaryFiles());
You should check if that allocation failed:
+ if (!attachmentList) return NS_ERROR_OUT_OF_MEMORY;
+ urlFile->Delete(PR_TRUE); // remove it
you should check and assert (but return) on error if the delete fails.
why pass in PR_TRUE (for recursive delete)? attachments can't be folders,
right? In fact, I'd add code that double checks that the file isn't a
directory, and if it is, I'd assert and skip it.
just imagine if somehow we got "/" or "/tmp" (for unix) or "" (for cwd on the
mac). deleting a directory could be very, very bad.
+ nsCRT::free(attachmentList);
that will crash if the memory allocation fails, since attachmentList will be
null.
how did you test this code? make sure it works properly when posting a news
message with an attachment, and with posting and sending in the same message.
you should get someone to test on the mac and linux. (especially mac). what
happens if the attachment's name is "foo,bar.txt"? does that get escaped
properly?
try this on linux:
cd ~
touch \ \ # create a file named " "
can you attach it?
I assume since he reviewed it, ducarroz knows he'll have to add add
GetTemporaryFiles() to the nsIMsgCompFields interface.
Comment 8•24 years ago
|
||
> you should check and assert (but return)
I meant
> you should check and assert (but NOT return)
Comment 9•24 years ago
|
||
To answer Seth concerns:
1) GetTemporaryFiles() will never return an null string. If we don't have
temp attachment, it will return an valid empty string. Anyway, there is another
way to do the allocation and initialization of attachmentList by using the top
level function GetTemporaryFiles(char*) which take care of the allocation:
char* attachmentList;
GetTemporaryFiles(&attachmentList)
if (attachmentList && *attachmentList)
{
...do your job here using directly attachment list, don't need to do an extra
copy...
}
NSCRTFREE(attachmentList);
2) Right, temp file or attachment are not directory. Don't do a recursive
delete. Sorry to have missed this point durung my review!
3) Attachment URL and therefore tempFile URL should never have a raw comma in it
as it's use as separator in the world of message compose. Mime knows this rule
therefore it should do the right job else the front end will not work correctly.
Therfore don't need to worry about this case at this point.
Assignee | ||
Comment 10•24 years ago
|
||
1. Should just reurn NS_OK since there's nothing to delete and check NULL
pointer here. Good catch.
2. Agree. I did not pay attention to the usage of the parameter.
3. Agree with ducarroz's point because otherwise all layers below the front end
will have to do the same checking when dealing with file delete. But agree with
the point that it's always safe to check file type.
How do I get someone to test the fix on Mac and Unix? Should I make the release
build then?
Updated•24 years ago
|
OS: Windows NT → All
Hardware: PC → All
Comment 11•24 years ago
|
||
once you provide the final patch, I'll test on linux for you.
I'm most worried about mac. perhaps ducarroz or another mailnews developer with
a mac can test it out for you.
Comment 12•24 years ago
|
||
I'll give it a shot on Mac. Personnaly I worry more about Linux :-)
Assignee | ||
Comment 13•24 years ago
|
||
Comment 14•24 years ago
|
||
a good rule is to follow the braces style of the rest of the code that you
change.
if () {
}
or
if ()
{
}
are the most common styles.
How about something like this:
rv = urlFile->IsDirectory(&isDir);
NS_ASSERTION(NS_SUCCEEDED(rv), "IsDirectory() call failed!");
NS_ASSERTION(!isDir, "temp file is a directory");
if (NS_SUCCEEDED(rv) && !isDir) {
// remove the file. PR_FALSE for "non recursive delete"
urlFile->Delete(PR_FALSE);
}
Comment 15•24 years ago
|
||
this patch works for me.
I slightly changed it (for style), I'll attach what I got in my local tree.
note to cavin:
#ifdef NS_DEBUG
printf("foo");
#endif
will get executed for anyone who builds debug.
to make it so only you get it, do this:
#ifdef DEBUG_cavin
#endif
that will get turned on for you by default.
Comment 16•24 years ago
|
||
Comment 17•24 years ago
|
||
Works very well on Mac too... R=ducarroz
Comment 18•24 years ago
|
||
then I'll check it in.
again, nice work cavin!
Updated•24 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 20•24 years ago
|
||
Sorry but I have to reopen it because sspitzer forget to checkin the mime part
of this fix (mimedrft.cpp)!
Comment 21•24 years ago
|
||
damn it, thanks for catching that ducarroz.
I'm lucky the build didn't break.
Comment 22•24 years ago
|
||
ok, really fixed this time.
Status: REOPENED → RESOLVED
Closed: 24 years ago → 24 years ago
Resolution: --- → FIXED
Comment 24•24 years ago
|
||
verified
win98, mac, linux-2001-08-02-08
See also bug 91959 on mac where the attachments are not cleaned off of temp
files
Status: RESOLVED → VERIFIED
Updated•20 years ago
|
Product: MailNews → Core
Updated•16 years ago
|
Product: Core → MailNews Core
You need to log in
before you can comment on or make changes to this bug.
Description
•