MAPI call from MS word, EXCEL does not call send mail/send doc

VERIFIED FIXED

Status

MailNews Core
Simple MAPI
P1
blocker
VERIFIED FIXED
17 years ago
10 years ago

People

(Reporter: Tiantian Kong, Assigned: Rajiv Dayal)

Tracking

Trunk
x86
Windows 2000
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [PDT], [ETA ?])

Attachments

(4 attachments, 7 obsolete attachments)

1.97 KB, patch
Cavin Song
: review+
Scott MacGregor
: superreview+
Details | Diff | Splinter Review
9.36 KB, patch
Jean-Francois Ducarroz
: review+
Scott MacGregor
: superreview+
Details | Diff | Splinter Review
1.82 KB, patch
Jean-Francois Ducarroz
: review+
Scott MacGregor
: superreview+
Details | Diff | Splinter Review
7.79 KB, patch
Jean-Francois Ducarroz
: review+
Scott MacGregor
: superreview+
Details | Diff | Splinter Review
(Reporter)

Description

17 years ago
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.
(Reporter)

Updated

17 years ago
Blocks: 95113, 95116
Keywords: nsbranch+
Priority: -- → P1
QA Contact: sheelar → trix
Whiteboard: PDT
(Assignee)

Comment 1

17 years ago
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 !
(Assignee)

Comment 2

17 years ago
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.



(Assignee)

Comment 3

17 years ago
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 !

Comment 4

17 years ago
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.

Comment 5

17 years ago
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.
(Assignee)

Comment 6

17 years ago
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
(Assignee)

Comment 8

17 years ago
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.
(Assignee)

Comment 9

17 years ago
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.
(Assignee)

Comment 10

17 years ago
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. 
(Assignee)

Comment 11

17 years ago
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.

Comment 12

17 years ago
Rajiv - keep testin this and get it up somewhere where it can be tested.
(Assignee)

Comment 13

17 years ago
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.
(Assignee)

Comment 14

17 years ago
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.
(Assignee)

Comment 15

17 years ago
Created attachment 50553 [details] [diff] [review]
patch for dealing with temp file creation by Powerpoint, Excel for attachments.

Comment 16

17 years ago
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 =).
(Assignee)

Comment 17

17 years ago
Created attachment 50560 [details] [diff] [review]
above patch recreated with -u option
(Assignee)

Comment 18

17 years ago
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".
(Assignee)

Comment 20

17 years ago
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.
(Assignee)

Comment 22

17 years ago
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.
(Assignee)

Comment 23

16 years ago
Created attachment 50670 [details] [diff] [review]
patch dealing with temp file creation for Powerpoint/Excel attachments with CompFields::SetTemporaryFiles
(Assignee)

Comment 24

16 years ago
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.

(Assignee)

Comment 25

16 years ago
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).

(Assignee)

Comment 26

16 years ago
Created attachment 50673 [details] [diff] [review]
patch for fix for blocker (1), Excel unable to call SendMail in Mozilla
Created attachment 50707 [details] [diff] [review]
Proposed fix for nsMsgCompFields::CleanUpTempFiles
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 30

16 years ago
Comment on attachment 50707 [details] [diff] [review]
Proposed fix for nsMsgCompFields::CleanUpTempFiles

r=cavin.
Attachment #50707 - Flags: review+
(Assignee)

Comment 31

16 years ago
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.
Attachment #50670 - Flags: needs-work+

Updated

16 years ago
Attachment #50673 - Flags: needs-work+
(Assignee)

Comment 33

16 years ago
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());
(Assignee)

Comment 34

16 years ago
Created attachment 50815 [details] [diff] [review]
Updated Patch (B1_v2) for blocker (1) : Excel unable to call send in mozilla
(Assignee)

Comment 35

16 years ago
Created attachment 50816 [details] [diff] [review]
Updated Patch for blocker 3) : Powerpoint/Excel unable to send attachments correctly
(Assignee)

Comment 36

16 years ago
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.
(Assignee)

Comment 39

16 years ago
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.
(Assignee)

Comment 40

16 years ago
Created attachment 50909 [details] [diff] [review]
updated patch for Powerpoint (Excel too) unable to send attachments correctly
(Assignee)

Comment 41

16 years ago
Created attachment 50910 [details] [diff] [review]
updated patch for Excel unable to call send in mozilla
Comment on attachment 50909 [details] [diff] [review]
updated patch for Powerpoint (Excel too) unable to send attachments correctly

Looks good this time R=ducarroz
Attachment #50909 - Flags: review+
Comment on attachment 50910 [details] [diff] [review]
updated patch for Excel unable to call send in mozilla

R=ducarroz
Attachment #50910 - Flags: review+

Updated

16 years ago
Attachment #50816 - Attachment is obsolete: true

Updated

16 years ago
Attachment #50815 - Attachment is obsolete: true

Updated

16 years ago
Attachment #50673 - Attachment is obsolete: true

Updated

16 years ago
Attachment #50670 - Attachment is obsolete: true

Updated

16 years ago
Attachment #50553 - Attachment is obsolete: true

Updated

16 years ago
Attachment #50560 - Attachment is obsolete: true
(Assignee)

Comment 44

16 years ago
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]
(Assignee)

Comment 46

16 years ago
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.
(Assignee)

Comment 47

16 years ago
Created attachment 50963 [details] [diff] [review]
patch for blocker 2) Word unable to call send in mapi32.dll
(Assignee)

Comment 48

16 years ago
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!
Attachment #50963 - Flags: needs-work+
(Assignee)

Comment 50

16 years ago
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.
(Assignee)

Comment 51

16 years ago
Created attachment 50983 [details] [diff] [review]
patch for blocker 2) : Word unable to call mapi32.dll

Updated

16 years ago
Attachment #50963 - Attachment is obsolete: true
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
Attachment #50983 - Flags: review+
Comment on attachment 50963 [details] [diff] [review]
patch for blocker 2) Word unable to call send in mapi32.dll

wrong patch

Comment 54

16 years ago
Comment on attachment 50707 [details] [diff] [review]
Proposed fix for nsMsgCompFields::CleanUpTempFiles

sr=mscott
Attachment #50707 - Flags: superreview+

Comment 55

16 years ago
Comment on attachment 50909 [details] [diff] [review]
updated patch for Powerpoint (Excel too) unable to send attachments correctly

sr=mscott
Attachment #50909 - Flags: superreview+

Comment 56

16 years ago
Comment on attachment 50910 [details] [diff] [review]
updated patch for Excel unable to call send in mozilla

sr=mscott
Attachment #50910 - Flags: superreview+

Updated

16 years ago
Whiteboard: PDT → [PDT], [ETA ?]

Comment 57

16 years ago
Comment on attachment 50983 [details] [diff] [review]
patch for blocker 2) : Word unable to call mapi32.dll

sr=mscott
Attachment #50983 - Flags: superreview+

Comment 58

16 years ago
pls check it into branch and trunk - PDT+
Whiteboard: [PDT], [ETA ?] → [PDT+], [ETA ?]

Comment 59

16 years ago
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.
Whiteboard: [PDT+], [ETA ?] → [PDT], [ETA ?]

Comment 60

16 years ago
doh! Thanks for the catch Scott. :-)

Comment 61

16 years ago
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.
(Reporter)

Updated

16 years ago
Component: Composition → Simple MAPI
(Assignee)

Comment 62

16 years ago
thanks Scott. - rajiv.

(Assignee)

Comment 63

16 years ago
all the problems for Word, PP, Excel have been fixed, patches have r and sr.
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 64

16 years ago
verified on 2001-10-12-05-0.9.4
Status: RESOLVED → VERIFIED
Blocks: 91397
Product: MailNews → Core
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.