355008 - Convert mailnews/compose to nsIFile

Status: RESOLVED FIXED

(MailNews Core :: Composition, defect)

Description

[Andrew Schultz]

This bug is to track removing nsIFile from mailnews/compose.  I have a patch to convert nsMsgSend's own internal usage and a few bits in related files.

Comment 1

[Andrew Schultz]

Attachment: convert most of nsMsgSend

Comment 2

[David :Bienvenu]

 I will look at this soon, probably after the 2.0 code freeze.

Comment 3

[David :Bienvenu]

I've applied this patch, and I'll build and run with it. Thx for the patch!

Comment 4

[David :Bienvenu]

OK, the patch doesn't seem to work - perhaps it bit-rotted, or my configuration is different. It fails to create a temp file for sending, or save as draft. I'll look into it a little. 

Comment 5

[Andrew Schultz]

Hmm.  That's working for me here.  I get an nscopy-1.tmp and an nsemail.eml.

I've had the attached patch in my debug tree since I attached it and occasionally use the bulid with my default profile and haven't noticed any problems.

Comment 6

[David :Bienvenu]

What platform are you running on, Andrew? I've found the problem, and a fix, at least for my windows build...I think it's really a bug in nsLocalFileWin.cpp, but we can work around it in nsMsgSend.cpp

Comment 7

[Andrew Schultz]

Ah, that makes sense.  I just have linux here.

Comment 8

[Karsten Düsterloh]

> I think it's really a bug in nsLocalFileWin.cpp,
> but we can work around it in nsMsgSend.cpp

Shouldn't the problem rather be fixed in nsLocalFileWin.cpp?

Comment 9

[David :Bienvenu]

The issue I see on Windows is that nsLocalFileWin::GetFileSize thinks that the cached stat info it has is correct - it doesn't seem to have noticed that we've written some data to it. We create an output stream on the temp file using NS_NewLocalFileOutputStream, and then write data to it, and close the output stream. But mTempFile doesn't know about this.

Linux nsLocalFileUnix does similar stat caching, so I'm not sure why it doesn't have this issue, and I don't have a linux machine to figure out why.

The little hack I added to nsMsgSend.cpp is to add these two lines:

    PRBool exists;
    mTempFile->Exists(&exists);

before calling

    rv = mTempFile->GetFileSize(&fileSize);

because Exists invalidates the stat cache, which makes GetFileSize work.

I'm hesitant to change nsLocalFileWin.cpp GetFileSize to just ignore the cached stat because of the potential performance impact. Perhaps the local file output stream code should force the nsIFile object to invalidate its stat cache somehow, on close/flush.

Again, it would be useful to know why this works on Linux.

Comment 10

[David :Bienvenu]

Comment on attachment 240786 [details] [diff] [review]
convert most of nsMsgSend

thx a lot for taking this on!

Can you add a comment saying this isn't add'reffed? Which is why it's [noscript], I think. Or make it addref its result, and fix the callers.

-    [noscript] nsOutputFileStream getOutputStream();
+    [noscript] nsIOutputStream getOutputStream();

+  nsresult rv;
   PRBool shouldDeleteDeliveryState = PR_TRUE;
   PRInt32 status;
   PRUint32    i;
   char *headers = 0;
   PRFileDesc  *in_file = 0;
   PRBool multipart_p = PR_FALSE;
   PRBool plaintext_is_mainbody_p = PR_FALSE; // only using text converted from HTML?
   char *buffer = 0;
@@ -703,72 +693,73 @@ nsMsgComposeAndSend::GatherMimeAttachmen
        m_attachment1_body && PL_strcmp(m_attachment1_type, TEXT_HTML) == 0)
   {
     //
     // If we get here, we have an HTML body, but we really need to send
     // a text/plain message, so we will write the HTML out to a disk file,
     // fire off another URL request for this local disk file and that will
     // take care of the conversion...
     //
-    mHTMLFileSpec = nsMsgCreateTempFileSpec("nsmail.html");
-    if (!mHTMLFileSpec)
-      goto FAILMEM;
+    rv = nsMsgCreateTempFile("nsemail.html", getter_AddRefs(mHTMLFile));


you could move the declaration of nsresult rv to where it's first used.

using a comptr here:

+    nsIOutputStream *tempfile;
+    rv = NS_NewLocalFileOutputStream(&tempfile, mHTMLFile, -1, 00600);
+    if (NS_FAILED(rv))

would help with leaks on errors...

again, a comptr would help here:

+  nsIOutputStream *tempOutfile;
+  rv = NS_NewLocalFileOutputStream(&tempOutfile, mCopyFile, -1, 00600);
+  if (NS_FAILED(rv))

and we also have to deal with the nsLocalFileWin issue I mentioned earlier.

Comment 11

[Andrew Schultz]

> Again, it would be useful to know why this works on Linux.

AFAICT, it "works" because none of the methods that get called before GetFileSize fills the cache with data.  On Windows, this happens when nsFileOutputStream::Open calls OpenNSPRFileDesc, but nsLocalFileUnix doesn't fill the cache in that method.

From looking at the file, it doesn't appear that there's any contract that the any of the getter methods will work after being modified via anything but nsI(Local)File methods.  It doesn't say it /won't/ work, but there's really no way it can know when something else changes the file, so to make it happen, it could never cache anything.

So... perhaps it would be best to have nsMsgSend just get a new instance after closing the output stream.  Beyond that, we'd need a explicit refresh() method in nsI(Local)File.

Comment 12

[David :Bienvenu]

Andrew, I think you're right. It's unfortunate that we're probably going to have to do this in a lot of places when we get rid of nsFileSpec. nsIFile and nsILocalFile are frozen, so adding a refresh method is probably a non-starter.

Comment 13

[Andrew Schultz]

Attachment: convert nsMsgSend v2

Comment 14

[David :Bienvenu]

Comment on attachment 258910 [details] [diff] [review]
convert nsMsgSend v2

nsresult
 mime_write_message_body(nsIMsgSend *state, const char *buf, PRInt32 size)
 {
   NS_ENSURE_ARG_POINTER(state);
 
-  nsOutputFileStream * output;
+  nsIOutputStream * output;
   nsCOMPtr<nsIMsgComposeSecure> crypto_closure;
 
   state->GetOutputStream(&output);

This needs to be a comptr, since GetOutputStream now addrefs. I think you fixed the other instances of this...

Other than that, this looks good. I haven't tried the patch on windows yet, though...If you'll attach a new patch with that one change, for sr (I'd suggest mscott), I'll test that patch.

Thx again for working on this!

Comment 15

[Andrew Schultz]

Attachment: convert nsMsgSend v3

> This needs to be a comptr, since GetOutputStream now addrefs. I think 
> you fixed the other instances of this...

Actually, the other caller was calling nsMsgComposeSecure::GetOutputStream which was not addrefing.  So I fixed the site you noted to use an nsCOMPtr and made nsMsgComposeSecure's version addref.

Comment 16

[Scott MacGregor]

Comment on attachment 259405 [details] [diff] [review]
convert nsMsgSend v3

nice!

Comment 17

[David :Bienvenu]

Andrew, this patch works for me. Do you want to land it, or do you want me to land it? I'm afraid I've caused one small conflict for you in my recent checkins, but it's easily resolved.

I'd love this to get checked in soon, because I have an other patch that relies on this patch, and I need this patch to land before I can make progress.

Comment 18

[Andrew Schultz]

OK, I've landed the patch.

Comment 19

[walter sheets]

I just started having trouble posting to newsgroups.  When I
click Send, no packets are ever sent to the server, but tb
thinks it is actually sending and waits until the timeout
occurs.  Mail is not affected.

Any chance your patch could be doing this?

Comment 20

[Andrew Schultz]

walter: not likely to be this patch.  attachment 259358 [details] [diff] [review] from bug 33451 did touch news, but I wouldn't expect it to cause that kind of behavior.  Probably best to file a new bug.

Comment 21

[Mark Banner (:standard8)]

Attachment: smime no longer needs xpcom_obsolete (checked in)

I don't know if anyone spotted it, but with the commit of the previous patch we no longer need mailnews/extensions/smime to depend on xpcom_obsolete. So one less directory to have to search when compiling in there ;-)

Note I'm not removing the lib dependency in mailnews/extensions/smime/build as I did that before with address book and mac failed to build because of things like msgbaseutl linking against xpcom_obsolete still.

Comment 22

[Masatoshi Kimura [:emk]]

integrated into bug 33451.

Comment 23

[Andrew Schultz]

this had patches, reviews and checkins....

Comment 24

[Andrew Schultz]

resolving FIXED

Comment 25

[Masatoshi Kimura [:emk]]

Adding dependency again. Sorry for the bugspam.
BTW, you don't have to reopen to changing resolution.