189174 - MAPISendMail does not copy file attachment to message before returning (WinZip 'Zip and e-mail' doesn't work)

Status: RESOLVED FIXED

(MailNews Core :: Simple MAPI, defect)

Description

[Manish Jethani]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.2.1) Gecko/20021130
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.2.1) Gecko/20021130

My code calls MAPISendMail with the MAPI_DIALOG flag set.  After the call
returns, my code deletes the attachment file from disk.  Meanwhile, when the
user clicks "Send" in the compose window, the program is unable to find the
attachment (obviously, because my code just deleted it!).

This was working fine with Outlook and Netscape 4.x because they make a separate
copy of each attachment file before returning from MAPISendMail.  The spec is
clear on this:  "File attachments are copied to the message before MAPISendMail
returns; therefore, later changes to the files do not affect the contents of the
message."

http://msdn.microsoft.com/library/en-us/mapi/html/_mapi1book_c_mapisendmail.asp

1) The spec is not being followed here.
2) This is not compatible with other implementations (Netscape 4.x, Outlook, etc.).


Reproducible: Always

Steps to Reproduce:
Write a program that:

1.  Loads MAPI DLL, etc.
2.  Calls MAPISendMail with MAPI_DIALOG set.
    The MapiMessage object should have at least one attachment.
    (When the call returns, the Compose window will have come up.)
3.  Deletes the attachment from disk.

In the Compose window, click "Send".

Actual Results:  
Error:  "Can't find attachment."


Expected Results:  
Send the message.  MAPISendMail should "copy the attachment to the message
before returning."

Comment 1

[Manish Jethani]

Attachment: Test case: C source file; compile and run to reproduce problem

Compile this source file in VC++.  Then run the program.  Pass the path of the
mozMapi32.dll file (C:\PROGRA~1\MOZILLA.ORG\MOZILLA\mozMapi32.dll) as the
command line argument.	When the Compose windows comes up, enter a recipient
address and hit "Send".  You should get an error: "Attachment not found."

Comment 2

[WD]

Bug 157566 looks to be a specific example of this bug.
So I'm not sure if one should be marked a duplicate of the other or what....
(this one definately has good info)

Comment 3

[Manish Jethani]

This is the base bug.  Mark the other one duplicate of this.

Comment 4

[Cyril Chaboisseau]

I work with a document management software integrated with the MS Office Suite
and the sending of attachements via the interface doesn't work as it used to
with Netscape 4.7x (and as is does with Outlook that I also tried)

This bug is clearly triggered by applications that passes attachement to the
mail client when the file is just deleted afterwards

Comment 5

[parish]

Re comment 3, no-one marked bug 157566 a dupe of this and Cyril has just posted
a patch to it so I guess this bug should now be marked a dupe of bug 157566

Comment 6

[Cyril Chaboisseau]

Hi,

Has anyone the "power" to commit/resolve this bug along with #157566 before the
final release of 1.4 and/or the (maybe) upcoming 1.3.2 ?

please find patch here :
http://bugzilla.mozilla.org/attachment.cgi?id=124311&action=view

That would be great not only for me but also for all of those who depends on
file attachment on the fly.


thanks

Comment 7

[Asa Dotzler [:asa]]

Too late for 1.4.1.

Comment 8

[Asa Dotzler [:asa]]

Seth, can you take a look at the patch from bug 157566 and see if it's good?
Scott, do you think this is needed for Thunderbird?

Comment 9

[WD]

*** Bug 224095 has been marked as a duplicate of this bug. ***

Comment 10

[Wayne Mery (:wsmwk)]

Would think this would be an issue for many people.  After significant digging I
find this problem also affects my users and I'm suprised that a fix might
actually be available.  

Anyway, it's definitely well described in bug 157566 and even has the patch!
Patch author has requested review per comment 6

Would be a nice addition to 1.7

Comment 11

[Steven Davies]

Bug 157566 has been marked as fixed - does this mean that this bug's also resolved?

Comment 12

[Cyril Chaboisseau]

Attachment: correction of MAPI function (nsMapiHook::PopulateCompFieldsForSendDocs)

this patch has first been developped against msgMapi.cpp from Mozilla v 1.7.1
so I had to "re-adapt" it a little bit the differences with the actual version
(1.7.3)

Comment 13

[Cyril Chaboisseau]

here is another round of correction which will hopefully nail this bug down

so, just to explain the problem : the bug is still not resolved (at least for me)

the problem has partially been solved for a lot (most) applications that use the
MAPI (nsMapiHook) functions namely winzip/winrar et al. that are using the
nsMapiHook::HandleAttachments
see bug 157566 for a big part of the discussion


the application I use here calls nsMapiHook::PopulateCompFieldsForSendDocs which
is still buggy (it doesn't copy anything in the moz_mapi_doc directory)

so I relaunch the correction with a company that has more knowledge than I do
wrt Windows developement and they provided me with a patch that works fine with
version 1.7.3 (they have provided me with a replacement of the msgMapi.dll)

so, if someone could take a look and maybe include the patch above in the
official release (for Mozilla and Thunderbird) that would be great

I'm willing to do all the necessary tests but unfortunately I don't know any
application that use the buggy MAPI call (PopulateCompFieldsForSendDocs) instead
of the one that is more often used (HandleAttachments)

thanks a _lot_ for your help

Comment 14

[timeless]

Comment on attachment 161200 [details] [diff] [review]
correction of MAPI function (nsMapiHook::PopulateCompFieldsForSendDocs)

>Index: msgMapiHook.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/mailnews/mapi/mapihook/src/msgMapiHook.cpp,v
>retrieving revision 1.27
>diff -u -r1.27 msgMapiHook.cpp
>--- msgMapiHook.cpp	2 Sep 2004 22:46:21 -0000	1.27
>+++ msgMapiHook.cpp	5 Oct 2004 21:53:06 -0000
>@@ -750,12 +750,81 @@
>+    nsSpecialSystemDirectory tmpDir(nsSpecialSystemDirectory::OS_TemporaryDirectory) ;
>+    // get nsIFile for nsFileSpec, why use a obsolete class if not required!

*don't*

here's js which does what you're supposed to do:

var
pTempDir=Components.classes["@mozilla.org/file/directory_service;1"].getService
(Components.interfaces.nsIProperties).get("TmpD",
Components.interfaces.nsILocalFile);
pTempDir.append("moz_mapi_doc");

>+    pTempDir->Exists (&bExist) ;
>+    if (!bExist)
>+    {
>+        rv = pTempDir->Create(nsIFile::DIRECTORY_TYPE, 777) ;
>+        if (NS_FAILED(rv)) return rv ;
>+    }
>+
>+	//Default Body String

don't use tabs

>+	nsCOMPtr <nsILocalFile> pFile = do_CreateInstance (NS_LOCAL_FILE_CONTRACTID, &rv) ;
>+        pFile->InitWithPath (strFilePaths) ;

don't forget to check for rv from xpcom apis (e.g. InitWithPath).

>+        rv = pFile->Exists(&bExist) ;
>+        if (NS_FAILED(rv) || (!bExist) ) return NS_ERROR_FILE_TARGET_DOES_NOT_EXIST ;

don't do this. if NS_FAILED() was because ACCESS_DENIED then you should not
return FILE_DOES_NOT_EXIST. handle the cases individually.

>+		nsCOMPtr<nsIMsgAttachment> attachment = do_CreateInstance(NS_MSGATTACHMENT_CONTRACTID, &rv);
>+		if (NS_FAILED(rv) || (!attachment) ) return rv ;

no need to check !attachment, doCI guarantees attachment if NS_SUCEEDED(rv), if
it didn't you'd be returning a success code in a failure case which would be
bad...

>+		printf ("File fileNameNative: %S \n", fileNameNative) ;

don't leave bare prints, seek nspr logging or wrap in ifdef DEBUG_yourname

Comment 15

[Cyril Chaboisseau]

here is another try
please be aware that I'm not a developper but just a middle man between the
company who did the work and us (the customers) who just happen to have a
problem with Mozilla and our document management system who cannot handle
attachement of files correctly

so please forgive my mistakes !


(In reply to comment #14)
>
> >+    // get nsIFile for nsFileSpec, why use a obsolete class if not required!
> *don't*
> here's js which does what you're supposed to do:


ok, I tried to correct it but I'm not sure it's the right way

> >+	nsCOMPtr <nsILocalFile> pFile = do_CreateInstance
(NS_LOCAL_FILE_CONTRACTID, &rv) ;
> >+        pFile->InitWithPath (strFilePaths) ;
> 
> don't forget to check for rv from xpcom apis (e.g. InitWithPath).

I couldn't figure out how

please help


> don't do this. if NS_FAILED() was because ACCESS_DENIED then you should not
> return FILE_DOES_NOT_EXIST. handle the cases individually.

?!

> >+		if (NS_FAILED(rv) || (!attachment) ) return rv ;
> 
> no need to check !attachment, doCI guarantees attachment if NS_SUCEEDED(rv), if
> it didn't you'd be returning a success code in a failure case which would be
> bad...

replaced it with NS_ENSURE_SUCCESS(rv, rv);


can someone competent have a look at it and maybe do the necessary corrections


thanks

Comment 16

[Cyril Chaboisseau]

Attachment: corrections to the previous patch (#161200) wrt comment #13

Comment 17

[Cyril Chaboisseau]

Hi,

Can someone help to close the bug of the MAPI function.


The problem is the following :

there is 2 ways to call a MAPI function under Mozilla/Firefox :

either through HandleAttachments or via PopulateCompFieldsForSendDocs

the first one which is used by WinZIP et al. and has been corrected for some
time now but the 2nd one (PopulateCompFieldsForSendDocs) is seldom used but does
not handle file attachement correctly :

whenever a file is to be send by mail trough MAPI, it needs to be copied to the
directory "C:\Documents and Settings\<user>\Local Settings\Temp\moz_mapi_doc\"
(and C:\...\moz_mapi\ if HandleAttachments is called)

the problem is that no files are copied to moz_mapi_doc and this is exactly what
patch #161401 tries to correct

now, it only needs to be acknowledged, tested and included in the official source

is this possible and if not, what needs to be done for it's inclusion

Comment 18

[David :Bienvenu]

I'll look into this.

Comment 19

[David :Bienvenu]

I'm seeing a slightly different behaviour - we do copy the file to a temp dir,
but the name of the attachment is munged - it includes the name of the temp dir
in it, i.e., moz_mapi.

Comment 20

[David :Bienvenu]

Attachment: proposed fix

we were copying the file, but the logic for creating the attachmen t name was
all wrong if the user didn't specify a pretty name

Comment 21

[David :Bienvenu]

Comment on attachment 166809 [details] [diff] [review]
proposed fix

Jungshik, I was hoping for your input on this code - it was clearly wrong
before because they were putting the whole path in leafName, which won't
work...but I don't know if this code will do the right thing with unicode or
non 7-bit ascii file names (my guess is that it didn't before and won't now
either)

Comment 22

[Jungshik Shin]

Comment on attachment 166809 [details] [diff] [review]
proposed fix

r=jshin
Sorry for the long delay. 
As long as the following works, it seems to be fine. Sometime soon, we need to
kinda 'audit' MAPI code, but for now, this should be fine, I guess.


 if (aIsUnicode)
		pFile->InitWithPath
(nsDependentString(aFiles[i].lpszPathName)); 
	    else
		pFile->InitWithNativePath (nsDependentCString((const
char*)aFiles[i].lpszPathName));

Comment 23

[Cyril Chaboisseau]

(In reply to comment #22)
> As long as the following works, it seems to be fine. Sometime soon, we need to
> kinda 'audit' MAPI code, but for now, this should be fine, I guess.

any chance that it gets into the nightly build in order for me to test it ?

Comment 24

[David :Bienvenu]

have you tried a recent nightly build? I think I checked this in already...or
rather, a variant of it, becase the initial patch caused a regression in tbird 1.0. 

Comment 25

[Cyril Chaboisseau]

(In reply to comment #24)
> have you tried a recent nightly build? I think I checked this in already...or
> rather, a variant of it, becase the initial patch caused a regression in tbird
1.0. 

no, I didn't (yet)
-> I'll do that tomorrow when I arrive at work (this is where I can test it with
the Document Management system)


but I tried Mozilla 1.7.5 and the outcome is even worse :

just like the previous version it doesn't call the MAPI function but now even
when I put in the 'component' directory the patched DLL build against a previous
version it doesn't work anymore (it used to until 1.7.3)

the point is that it's difficult for me to rebuild it without the company that
did it in the first place

Comment 26

[Cyril Chaboisseau]

(In reply to comment #24)
> have you tried a recent nightly build?

yes !

it works perfectly
tested on Mozilla 1.8a6 (build id 2004122106)


now the next step is to have the patch going through the regression test in
order to finalize it either in the upcoming 1.8 or 1.7.x to close the bug completely

great work...

Comment 27

[David :Bienvenu]

it will automatically be in 1.8 final. Re 1.7.x, I can go request appoval for
that, except I'm not sure what other mapi fixes are in 1.7.x already, and which
aren't...since this is fixed on the trunk, I'm going to mark this fixed. I'll go
find the bug that contained the patch that was checked into the trunk to fix
this...bug 273849

Comment 28

[David :Bienvenu]

we'd need the fix for bug 157566 and the patch in bug 273849 for the 1.7.x
branch - that's a little scary change to check into 1.7.x at this point...

Comment 29

[Cyril Chaboisseau]

(In reply to comment #28)
> we'd need the fix for bug 157566 and the patch in bug 273849 for the 1.7.x
> branch - that's a little scary change to check into 1.7.x at this point...

hi,

I still have pending problems with this bug/patch :

first it has not been included in the lattest 1.7.8 which I understand is due to
the side effect it might cause

but since 1.8 it not planned to be released except for a "community" version,
I'm a little bit worry
http://www.mozillazine.org/talkback.html?article=6206

now, just to add insult to injury, 1.7.8 is out but the DLL that helped me to
have the bug corrected is not "compatible" with this lattest version while it
worked fine up until 1.7.7 !!
(don't ask me why although I'm fully ready to help anyone to find out the
technical reasons...)

Now, I have some questions :

Has the MAPI API been changed in some way since 1.7.8 ?

Would it be possible to include the patch in the next 1.7 ?

Is 1.8 going to be out sometime ?


...I'm hopeless