75317 - Forwarding msgs with attachments doesn't clean up temp files

Status: VERIFIED FIXED

(MailNews Core :: Backend, defect)

Description

[Cavin Song]

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.

Comment 1

[Cavin Song]

Attachment: Add a new list in class nsMsgCompFields to record temp files

Comment 2

[Jean-Francois Ducarroz]

+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

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

nsFileSpec is being depricated.  see nsFileSpec.h

what about using the code we found in edior that used nsIFileUrl and nsIFile?

Comment 4

[Cavin Song]

Attachment: Changed code based on comments

Comment 5

[Cavin Song]

Attachment: Changed code based on another comment

Comment 6

[Jean-Francois Ducarroz]

Looks good. R=ducarroz

Comment 7

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

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

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

> you should check and assert (but return)
I meant

> you should check and assert (but NOT return)

Comment 9

[Jean-Francois Ducarroz]

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.

Comment 10

[Cavin Song]

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?

Comment 11

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

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

[Jean-Francois Ducarroz]

I'll give it a shot on Mac. Personnaly I worry more about Linux :-)

Comment 13

[Cavin Song]

Attachment: Incorporated comments from sspitzer

Comment 14

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

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

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

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

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

Attachment: modified patch, ducarroz please test on mac

Comment 17

[Jean-Francois Ducarroz]

Works very well on Mac too... R=ducarroz

Comment 18

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

then I'll check it in.

again, nice work cavin!

Comment 19

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

fxied.

Comment 20

[Jean-Francois Ducarroz]

Sorry but I have to reopen it because sspitzer forget to checkin the mime part 
of this fix (mimedrft.cpp)!

Comment 21

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

damn it, thanks for catching that ducarroz.

I'm lucky the build didn't break.

Comment 22

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

ok, really fixed this time.

Comment 23

[Sheela Ravindran]

esther->sheelar

Comment 24

[Sheela Ravindran]

verified 
win98, mac, linux-2001-08-02-08

See also bug 91959 on mac where the attachments are not cleaned off of temp 
files