This is discovered in the integration testing. MAPI call from MS Word, EXCEL can get through to log on, but not to send mail/send doc yet. Krishna is working together with Rajiv on this blocker issue right now.

I have tracked down the problem seen when a MAPI send call is made from Excel. The call made from Excel is with null originator and recipients strings since it expects some kind of mail compose window to be displayed to do so. However the type library (proxy dll for MAPI support MS COM interface in Mozilla) generated by MIDL makes a check for the method parameters to be non null. Since this check in the MS MIDL generated proxy lib fails it does not call the mapi support dll in mozilla. I have hacked this down and have made a fix to make sure that a null value is not passed down to the MSCOM type lib. The send call from Excel to our mapi32.dll to mozilla/mailnews works fine now !

The send call from PowerPoint also seems to go through to the mailnews interface. However I saw there is some problem with the attachment that Powerpoint sends. When Powerpoint is used to send the currently open attachment it creates a temp file and sends the path name pointing to this temp file rather than the pathname for the file currently open. This could be to take care of the case if the currently open file is not saved and also maybe to avoid any file locking problem that the messaging app may face since the file is already open in Powerpoint. Whatever maybe the reason but when the file is sent the path points to a temp file and also the extension of the file is not correct, the mail compose component uses the extension to determine the mime type and doesnot take any inputs for the mime type of the attachments. I am looking into this to figure out what could be the solution.

I have a solution to take care of the temp file problem and the .tmp extension for the attachment pathnames passed for an open file by Powerpoint, for the MAPI send. I do get the real filename too along with the temporary pathname, i copy the temp file to a file with this real name and then send this new temp filename to the mailnews/compose function. This temp file i delete on send completion. However, i believe it will be good if the fix for this is put into the mailnews/compose component. There should be a way i can pass in the real filename to the compose interface. This way the compose interface can use the real filename to find the extension/mime type and then can read the data from the temporary file created by the calling application. Also the real filename can be sent to specify the attachment sent. At this moment though with the above solution of changing filenames and creating another temporary file the send with attachments from Excel and Powerpoint works fine !

hey rajiv, what does your code look like which copies the temp file to a file with the real file name? Are we opening ourselves up to some data loss if I already have a file with that name in the same directory? i.e. you rename myresume.tmp to myresume.doc in my temp directory which is where I'm currently storing the original myresume.doc file. Just wondering.

I know you want to get something working without waiting for JF to change the compose service, but making another temp file is less than ideal - you could run out of disk space, there's a performance hit, you make it more possible to leak a temp file (if we crash, for example) etc. So I'd really like to see us not create an extra temp file.

no i am not in a hurry to have the thing working .. i just put this quick solution to make sure that things will work if we deal with the temp file created by Excel/Powerpoint and use the real file name passed in to the MAPI call. I too have mentioned above that this is not the ideal solution. JF can u deal with this in ur compose code ..

The way compose fields store attachment is very limited as we store only the file path. We have been already beten by this problem (during reply, forward, intl, etc) and I had planed to do some work in the future but maybe I should do it sooner. The solution is to replace the list of file path in the compose field by an array of object. This object will let us store the following information: -the real attachment file name -the file path -the mime type -does the file must be deleted when done with it -maybe the character set! In order to implement that, I will have to modify nsMsgCompField, mime and msg compose front end and back end. This is not a small change and therefore is unlikely that will append for 0.9.4. Kind of risky

Well .. on second thoughts I think the solution of creating another temp file using the real file name will work quite ok for now. The problem of running out of disk space ... will the nsIFile/nsILocalFile not return me an error if there is no disk space ? Performance hit ..some ..but maybe not very noticeable .. And i check that this creation of new temp file business is done Only if the real file name passed is not same as the one in the full path name passed. Also i check that the dir path in the full path name is same as the WindowsTempPath before doing the new temp file creation. To answer Scott's Q i do check if a file with the new file name exists in the temp dir, if it does, currently i am just returning an error.

I have also observed that Excel and Powerpoint creates the temp file only for the send operation and after the MAPI send call is completed it deletes the file. Thus instead of copying the temp file to a new temp file with the real file name we can just rename the temp file with the real file name as far as Excel and Powerpoint is concerned. This will also take care of the performance and disk full issue with creation of another temp file. We will however need to do some more testing with some other apps too.

Another interesting thing i observed is that when send is called from Powerpoint it creates a temp file in temp dir which is removed once the send returns. We return the send call once we bring up the Compose Window since we donot want to hold up the calling application till the user hits the send button. So in case of Powerpoint it goes ahead and deletes the temp file as soon as send return and the send from Compose window will fail if we dont deal with the temp file. Thus it will become necessary that we either rename the temp file or make a copy (i guess rename is better, Powerpoint does not crash if it doesnot file this temp file to delete) even when we deal with this temp file situation in the mailnews/compose component.

The problem with Word is also fixed now ! Word calls Logon twice (as was also noticed during N4 release) and the second call made on another thread was blocking. We use a Mutex in Logon to handle multiple calls. In the second call it was blocking on this mutex since the mutex creation was done with bInitialOwner = TRUE, the second thread was not able to get the ownership of the mutex and the call to WaitForSingleObject was blocking. I have fixed this to create the mutex with this flag set to FALSE so that other threads using the mutex can get its ownership.

Rajiv - keep testin this and get it up somewhere where it can be tested.

I have noticed a similiar behaviour in Netscape 4.7 while dealing with MAPI send from Powerpoint / Excel which creates a temp file for the attachment to be sent. N4.7 creates a file with the real file name in the temp dir for it. This issue was discussed in PDT and a decision has been made to handle the temp file created for attachments during MAPI send from MS applications like Powerpoint / Excel in the MAPI support code itself rather than mailnews / compose for now.

This bug deals with 3 blockers we had : 1) Excel not able to call mozilla from mapi32.dll 2) Word not able to call MAPISend defined in mapi32.dll 3) Powerpoint (also Excel) MAPI Send not able to send attachments correctly. The third problem (3) above was because of the temp file that these apps create for sending the open document as an attachment. Below are the patches for each of these problems. Since the 3rd problem required the maximum code change, that is attached for review first.

Hi Rajiv, Can you attach your diffs using cvs diff -uw? (-u means unified). It makes the diffs much more legible and easier to understand. I can't read the regular old contextual diffs =).

The patch above to deal with the temp file creation done by Powerpoint and Excel to send currenlty open documents as attachments has changes done to the SendMail code. The original Send Mail code is attached as the latest patch to the bug # 95113.

Rajiv, I am so sorry! nsMsgIComposeFields already knows how to deal with temp file that need to be deleted when done with it. All you have to do is to set the the "temporaryFiles" field the same way you set the "attachment" field. When the nsMsgIComposeFields is destroyed, it will automatically delete any file specified into "temporaryFiles".

Oh !! ... but i still need to deal with the temp file creation using the real filename, right ?

Right. Another question is do you still have to delete the temp file if the send failed or is aborted? if no, you must clear the temporaryfiles field before the compose fields is destroyed.

Yes i still need to delete the temp files i renamed even if send failed or was aborted, since the calling MAPI app wouldn't know about it. So i will go ahead and remove the code dealing with deletion of temp file in the SendListener code, test it and put up the new patch. Meanwhile can u review the other parts of the code. The first 25-30 lines of the patch deal with the deletion of temp files and the remaining code deals with temp file creation, etc, can u please review that. Thanks, - rajiv.

Above is the patch updated to not delete the temp files in the send listener, it calls the nsICompFields::SetTemporaryFiles instead. I tested the code and i saw a problem in "nsMsgCompFields::CleanUpTempFiles()", the rv for 'urlFile->IsDirectory(&isDir)' fails, it displays : "the NS_ASSERTION(0, "IsDirectory() call failed!");" and goes into in an infinite loop. This is because strtok() on var 'rest' is not called in this case and the value of var 'token' does not change and hence it never gets out of the while loop. Surprising .. IsDirectory is returning a failure !! I guess it should return PR_FALSE for var 'isDir' and return NS_OK ! Anyway, if I comment out the check for the return value (below) for the 'urlFile->IsDirectory(&isDir)' call the code works perfectly fine and the temp files are cleaned up appropriately. /** commenting out if (NS_FAILED(rv)) { NS_ASSERTION(0, "IsDirectory() call failed!"); continue; } **/ Hi JF, Can u please take care of this in 'nsMsgCompFields.cpp' as well as review the above attached patch. thanks, - rajiv.

Please find below the code that fixes the problem seen with Excel, blocker (1) as mentioned in comments above (2001-09-24 11:24) MAPI Send call from Excel was reaching the exported MAPI function in mapi32.dll but not reaching the MAPI support MS COM interface exported from Mozilla. The fix is briefly described in above comments (2001-09-19 19:53).

I have rewrote nsMsgCompFields::CleanUpTempFiles, this time we should not loop in case an error occurs. Rajiv, can you check if this patch solve your problem. Thanks.

Comment on attachment 50673 [details] [diff] [review] patch for fix for blocker (1), Excel unable to call SendMail in Mozilla This fix seems wrong and could lead to a crash. Basically, if I correctly understand, you are passing the address of a structure allocated on the stack to a function. I see 2 major problems: 1) when you declare a struct on the stack, the structure is not initialized unless it has a constructor and therefore it could hold random value. Some compiler has the bad idea to always initialize the memory to 0 in debug mode! 2) You are passing the address of a variable that went out of scoop. Therefore you cannot count on it anymore

Comment on attachment 50707 [details] [diff] [review] Proposed fix for nsMsgCompFields::CleanUpTempFiles r=cavin.

We are passing the address of struct that is created on stack here to a function which is a blocking function. The struct goes out of scope only after the called function returns which returns only after either completing the usage of the struct or after copying the values in the struct to another memory location. As far as initializing the struct is concerned, it is initialized with the values passed to the calling function or is just ignored. The count variables passed to the function which also takes the struct pointer as a parameter is used to determine if the struct is used or not.

Comment on attachment 50670 [details] [diff] [review] patch dealing with temp file creation for Powerpoint/Excel attachments with CompFields::SetTemporaryFiles Couple comments: 1) instead of using GetTempPath, you can use the Mozilla way: nsSpecialSystemDirectory tmpFile(nsSpecialSystemDirectory::OS_TemporaryDirectory); 2) please, avoid duplicating code, especially when it's a big chunk. You should be able to create a function that set the native attachment into the compose fields. 3) Also, use nsILocalFile member function GetLeafName or GetUnicodeLeafName to extract the leaf name. NsILocalFile provide you all sort of function to manipulate files path.

1) i will change it to use nsSpecialSystemDirectory 2) both the chunks of code for setting comp fields native attachment are somewhat different, one is for Unicode data and another for non-Unicode data. However, i will try to see if it is still possible to merge them together without nearly having the same amount of code in the common function. 3) i am using GetLeafName and GetUnicodeLeafName for getting the filename from the file / path wherever possible, eg : // filename of the file attachment nsXPIDLString pLeafName ; pFile->GetUnicodeLeafName (getter_Copies(pLeafName)) ; nsAutoString LeafName ; LeafName.Assign (pLeafName.get());

Hi JF and Scott, Can u please review / super review the above updated patches (id=50815 and id=50816) for the blockers a) Excel unable to call send in mozilla and b) the attachment problem with Powerpoint and Excel. thanks, - Rajiv.

Comment on attachment 50815 [details] [diff] [review] Updated Patch (B1_v2) for blocker (1) : Excel unable to call send in mozilla You clearly fix the problem with variable getting used despite they were out of scope. About the initialization of Message, Recipient and Files, you said it does not matter as they are protected by a counter which should be zero. That's seems wrong for me when lpMessage = &Message. In that case, should you initialize all the member variable of Message to o or null!

Comment on attachment 50816 [details] [diff] [review] Updated Patch for blocker 3) : Powerpoint/Excel unable to send attachments correctly Good, no more code duplication. However, I am pretty sure you can reduce your code by using more Mozilla API (nsILocalFile, etc) when you manipulate path.

Please find below the updates patches for 1) Excel unable to call send in mozilla, and 3) Powerpoint/Excel unable to send attachments correctly. The changes are the following : 1) initialized the structs in MapiDll.cpp (although this was not necessary since MS apps donot pass lpMessage as null but it is a good idea just in case any unlikely custom apps that might). 2) changed msgMapiHook.cpp to use nsSpecialSystemDirectory instead of GetTempPath. 3) used LeafName instead of dir path Delim for path manipulation.

Comment on attachment 50909 [details] [diff] [review] updated patch for Powerpoint (Excel too) unable to send attachments correctly Looks good this time R=ducarroz

Comment on attachment 50910 [details] [diff] [review] updated patch for Excel unable to call send in mozilla R=ducarroz

Hi Scott, Can u please super review the latest two patches (attachment 50909 [details] [diff] [review]) and (attachment 50910 [details] [diff] [review]) for the two problems Powerpoint unable to send attachments correctly and Excel unable to call send in mozilla respectively. Both these patches have an r=. thanks. - rajiv.

we need also a SR for attachment 50707 [details] [diff] [review]

Please find below the patch for the problem seen with Word, blocker 2) Word unable to call Send in mapi32.dll. This was happening because it calls Logon twice, the first time it calls on the same thread as Dllmain and the second time on another thread. Thus this resulted in the MS COM marshalling failure. Since CoInitialize was not called on the second thread as well as the interface object was not created on the second thread, the marshalling for the interface pointer on the second thread failed with an MS COM error for this. The fix in the patch is to call CoInitialize on each new thread appartment and another interface object is created for the new thread and released after its usage. This solves the marshalling failure and the send call succeeds.

Hi JF, Thanks for your r= (has review) for the patches for blocker 1) Excel problem and blocker 3) Powerpoint attachment problem. Can u please review the patch (id=50963) attached for the blocker 2) Word problem, described in comments just above the patch (id=50963). Thanks, - Rajiv.

Comment on attachment 50963 [details] [diff] [review] patch for blocker 2) Word unable to call send in mapi32.dll @@ -262,11 +345,18 @@ return MAPI_E_LOGIN_FAILURE ; HRESULT hr ; + nsIMapi *pNsMapi = NULL; + BOOL bComInitialized = FALSE; + hr = pNsMapi->SendDocuments(lhSession, (LPTSTR) lpszDelimChar, (LPTSTR) lpszFilePaths, (LPTSTR) lpszFileNames, ulReserved) ; MAPILogoff (lhSession, ulUIParam, 0,0) ; + pNsMapi->Release(); + if (bComInitialized) + ::CoUninitialize(); + return hr ; } You forget to call GetMozillaReference, therefore pNsMapi is NULL which will cause a crash!

Please ignore the patch (attachment 50963 [details] [diff] [review]) it is a wrong patch. I had tested the code and it worked fine, with this null it would never. In fact the patch i created had lines for the Excel bug which i deleted from the patch so as not to confuse you and by mistake removed the other lines. Please use the patch attached below. Also we are in process of creating a separate branch for the MAPI feature which will help avoid these problems of creating patches (we ourselves presently maintian all versions which can cause all these mistakes) as well as help everyone to test the feature too.

Comment on attachment 50983 [details] [diff] [review] patch for blocker 2) : Word unable to call mapi32.dll Remove bCoUnInit which is not used and you have my R=ducarroz

Comment on attachment 50963 [details] [diff] [review] patch for blocker 2) Word unable to call send in mapi32.dll wrong patch

Comment on attachment 50707 [details] [diff] [review] Proposed fix for nsMsgCompFields::CleanUpTempFiles sr=mscott

Comment on attachment 50909 [details] [diff] [review] updated patch for Powerpoint (Excel too) unable to send attachments correctly sr=mscott

Comment on attachment 50910 [details] [diff] [review] updated patch for Excel unable to call send in mozilla sr=mscott

Comment on attachment 50983 [details] [diff] [review] patch for blocker 2) : Word unable to call mapi32.dll sr=mscott

pls check it into branch and trunk - PDT+

removing the plus. Jaime just meant to plus the mailnews compose patch which is one of many in this bug report. JF and I are about to check that into the branch and the trunk right now.

doh! Thanks for the catch Scott. :-)

FYI, tiantian and Rajiv, I just checked in the compose patch into the branch and the trunk so you can admit that from your steps when you email those out.

thanks Scott. - rajiv.

all the problems for Word, PP, Excel have been fixed, patches have r and sr.

verified on 2001-10-12-05-0.9.4