Closed Bug 100669 Opened 23 years ago Closed 23 years ago

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

Categories

(MailNews Core :: Simple MAPI, defect, P1)

Product:
MailNews Core
Component:
Simple MAPI
Platform:
x86
Windows 2000
Type:
defect

Tracking

(Not tracked)

Status:
VERIFIED FIXED

People

(Reporter: tiantian, Assigned: rdayal)

References

Details

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

Attachments

(4 files, 7 obsolete files)

Proposed fix for nsMsgCompFields::CleanUpTempFiles
1.97 KB, patch
cavin
: review+
mscott
: superreview+
Details | Diff | Splinter Review
updated patch for Powerpoint (Excel too) unable to send attachments correctly
9.36 KB, patch
bugzilla
: review+
mscott
: superreview+
Details | Diff | Splinter Review
updated patch for Excel unable to call send in mozilla
1.82 KB, patch
bugzilla
: review+
mscott
: superreview+
Details | Diff | Splinter Review
patch for blocker 2) : Word unable to call mapi32.dll
7.79 KB, patch
bugzilla
: review+
mscott
: superreview+
Details | Diff | Splinter Review 
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.
Blocks: 95113, 95116
Keywords: nsbranch+
Priority: -- → P1
QA Contact: sheelar → trix
Whiteboard: PDT
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.

        Attachment #50707 -
        Flags: review+

    
    

    
  
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+

        Attachment #50673 -
        Flags: needs-work+

    
    

    
  
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

        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+

        Attachment #50816 -
        Attachment is obsolete: true

        Attachment #50815 -
        Attachment is obsolete: true

        Attachment #50673 -
        Attachment is obsolete: true

        Attachment #50670 -
        Attachment is obsolete: true

        Attachment #50553 -
        Attachment is obsolete: true

        Attachment #50560 -
        Attachment is obsolete: true

    
    

    
  
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.


    
    

    
  
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!

        Attachment #50963 -
        Flags: needs-work+

    
    

    
  
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.


        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 on attachment 50707 [details] [diff] [review]
Proposed fix for nsMsgCompFields::CleanUpTempFiles

sr=mscott

        Attachment #50707 -
        Flags: superreview+

    
    

    
  
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 on attachment 50910 [details] [diff] [review]
updated patch for Excel unable to call send in mozilla

sr=mscott

        Attachment #50910 -
        Flags: superreview+

    
  
Whiteboard: PDT → [PDT], [ETA ?]

    
    

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

sr=mscott

        Attachment #50983 -
        Flags: superreview+

    
    

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

    
    

    
  
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 ?]

    
    

    
  
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.

    
  
Component: Composition → Simple MAPI

    
    

    
  
thanks Scott. - rajiv.



    
    

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

    
    

    
  
verified on 2001-10-12-05-0.9.4
Status: RESOLVED → VERIFIED
Product: MailNews → Core
Product: Core → MailNews Core

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: