115307 - send mail fails from some apps in MAPI_NEW_DIR_TRUNK branch

Status: VERIFIED FIXED

(MailNews Core :: Simple MAPI, defect)

Description

[Rajiv Dayal]

Some apps like Wordpad donot delete the temp file it creates to send an unsaved
doc since this temp file is specifically created for send, whereas most others
like Excel do, immediately on return. Thus after doing the changes to comply
with changes for nsIMsgCompose (which were to avoid the copying of the temp file
created for send) send mail fails for some apps.

Comment 1

[Rajiv Dayal]

The solution for this is either make a copy of this temp file or block the
MAPISendMail call to return after opening the Compose Window. The second
solution is definitely what we should go with, however there is a problem with
blocking the MAPI SendMail thread in Mozilla since it is the main thread and
since UI too runs on the main thread, when we block that thread, even using
event loop, the UI thread also gets blocked and the app freezes. 

Comment 2

[Rajiv Dayal]

I have a solution for this using Windows named events which works across process
spaces. We block the MAPISendMail call in the mapi32.dll on a named event which
is then signalled by the MAPISendListener::OnStopSending and thus we donot need
to block the mozilla's MAPI SendMail thread which is also Mozilla's main thread.
Using Wndows named events should not pose any problems as MAPI support is
Windows only.

Please find below a patch attached for the same.

Hi JF and Scott, can u please take a look at this.

thanks, - rajiv.

Comment 3

[Rajiv Dayal]

Attachment: fix for the problem using named events

Comment 4

[Rajiv Dayal]

Comment on attachment 61874 [details] [diff] [review]
fix for the problem using named events

new patch with changes for both MAPI SendMail and SendDocuments coming.

Comment 5

[Rajiv Dayal]

Attachment: fix for both SendMail and SendDocs using named events

Hi Scott and JF,

Can u please sr and r this patch.

thanks, - Rajiv.

Comment 6

[Jean-Francois Ducarroz]

Comment on attachment 62312 [details] [diff] [review]
fix for both SendMail and SendDocs using named events

Looks good. R=ducarroz

Comment 7

[Scott MacGregor]

Comment on attachment 62312 [details] [diff] [review]
fix for both SendMail and SendDocs using named events

sr=mscott

Comment 8

[Rajiv Dayal]

checked in to the MAPI_NEW_DIR_TRUNK

Comment 9

[Rajiv Dayal]

Closing the compose window without doing a send blocks the MAPISend call from
the MAPI apps. This is happening because the SendListener is not called in this
case. Below is the patch which calls the SendListener::OnStopSending when the
compose window closes even without an actual send done.

Comment 10

[Rajiv Dayal]

Attachment: calling SendListener::OnStopSending when compose window closed without actual send

Hi JF and David,

Can u please r and sr this patch.

thanks, - rajiv.

Comment 11

[David :Bienvenu]

I really don't like the idea of sending this notification when no actual send
happened. For example, in this case, we'd send an OnStopSending without ever
sending an OnStartSending notification, which seems wrong, and could confuse
some listeners. I'd prefer some other more specific notification to the send
listener for this rather special case, or do this some other way.
OnSendNotPerformed or something like that. Or, don't do this with a send
listener at all - you're really interested in the compose window closing than
the send operation completing, right? I believe you can listen for a window
closing just as easily.

Comment 12

[Rajiv Dayal]

As far as MAPI is concerned it is a Send operation (MAPISend), although the user
may decide to close the compose window without really sending which is more a
Mozilla sub-operation. I am interested in the MAPISend call completion whether
it is done after the send is done successfully or send is aborted. So for the
fix here we are calling OnStopSending with an error representing aborting send.

Comment 13

[David :Bienvenu]

I think it's wrong to hard code mozilla notifications to fit the mapi model of
the world. 

Comment 14

[Rajiv Dayal]

I dont think we are hardcoding the way mozilla notifications are made, we are
calling OnStopSending with an error for send being stopped on being aborted by
the user. The Send functionality clients which use Send listeners are/would not
interested in window operations, they are high level clients interested only in
how send in going on and when it is done irrespective of whether it completes
successfully or aborted.

Comment 15

[David :Bienvenu]

Well, I disagree. If you can convince mscott or sspitzer for an sr, that's fine,
but I simply don't like it.

Comment 16

[Jean-Francois Ducarroz]

I had this discussion with Radjiv last week as I wasn't in favor of doing this
either but mscott told him he was ok to call onstopsending even when we did not
start a send operation. The other solution would have been to create a new
listener class with more appropriate member functions.

Comment 17

[Jean-Francois Ducarroz]

Comment on attachment 62781 [details] [diff] [review]
calling SendListener::OnStopSending when compose window closed without actual send

R=ducarroz

Comment 18

[Rajiv Dayal]

Seth, since mscott is not here can u please sr this one. thanks, - rajiv.

Also creating another listener for window operations is not what a send client
would be interested in .. a send client is more interested in send operations
and hence we decided to go this way. As far as send client is concerned the send
operation has begun but stopped by user intervention and thus we call
OnStopSending with an error code.

Comment 19

[(not reading, please use [e-mail] instead)]

bienvenu has some valid concerns.

looking at the interface for nsIMsgSendListener:

http://lxr.mozilla.org/mozilla/source/mailnews/compose/public/nsIMsgSendListener
.idl#64

calling onStopSending() is a hack, it just happens to do what you need.

what does onStopSending() do that you need?  do you call it because it closes 
the window, do you call it because it notifies external listeners?

if you could explain why calling onStopSending() does what you need, we can 
think of a better solution.


 

Comment 20

[(not reading, please use [e-mail] instead)]

fyi,  

externalListener.onStopSending(null, -1, null, null);

if we go this way, that should be:

externalListener.onStopSending(null, Components.results.NS_ERROR_FAILURE, null, 
null);

or something, instead of -1.

Comment 21

[Jean-Francois Ducarroz]

Components.results.NS_ERROR_ABORT should be better

Comment 22

[(not reading, please use [e-mail] instead)]

according to rajiv, he calls onStopSending() because it notifies the external 
listeners.  he says that's the only reason he calls it.

he says that OnStartSending() is called only when the user clicks the send btn 
on compose window.  in this case, the send never starts.

I think bienvenu was on the right track, this code would affect other send 
listeners, not just your mapi one.  calling OnStopSending() like that seems bad.

I'd add OnSendNotPerformed(), call it instead, and have all the other 
implementations do nothing (just return NS_OK;) and have your mapi 
implementation (in mozilla\mailnews\mapi\mapihook\src\msgMapiHook.cpp) call 
your OnStopSending(), since that's what you want.

curious, why isn't this scenario a problem for mapi, but not for other external 
listeners?

Comment 23

[Jean-Francois Ducarroz]

MAPI is the only one to setup an external listener!

Comment 24

[Rajiv Dayal]

With Compose Window yes, but with nsIMsgSend::CreateAndSendMessage import from
Outlook and Eudora also uses external sendListeners .. but this case of Compose
Window being closed will not effect them.

If you really see the OnStopSending is called for these send clients even when
send fails .. it is called with an error code .. this one is a similiar case ..
send failed because of user intervention. Thus calling OnStopSending with an
error code in this case would be similiar to calling another function.

Anyway, I have done the changes for this new function .. if everyone still feels
that having this new function will not behave differently for MAPI than for
other clients (as i mentioned above) i can put the updated patch. 

I feel that this would be a different behaviour than what already exist, the
other client depend on OnStopSending irrespective of send success or fail and
with no relation to OnStartSending. Thus having this new function may infact
effect them negatively if later it is called from other places besides compose
window being closed.

Comment 25

[David :Bienvenu]

we can all come up with scenarios where implementers of notifications can get
things wrong because they make wrong assumptions about the notifications. Our
protection from that is enforcing a contract between listeners and notifiers.
The best way of documenting that contract is through the method names. the name
OnStopSending implies two things: that we started a send, and that the send
operation has finished, neither of which are true in this case, from the mozilla
point of view - I know that from the mapi point of view, a send operation has
been started, but this is a mozilla notification, not a mapi notification. From
the mozilla point of view, a compose window has been brought up and closed,
without a send taking place. If we wanted to have the method name truly indicate
what happens so implementors don't get confused, the method would be called
"OnComposeWindowClosedWithoutSending".

Comment 26

[Rajiv Dayal]

I believe the notifications to the clients should be based on what the clients
would need. But anyway to avoid the confusion of the OnStartSending i have
implemented another function in nsIMsgSendListener called OnSendNotPerformed as
suggested by David.

Please find the patch below.

Comment 27

[Rajiv Dayal]

Attachment: new fn for notification when user aborts send

Hi JF, Can u please review this patch. thanks, - Rajiv.

Comment 28

[Jean-Francois Ducarroz]

Comment on attachment 62962 [details] [diff] [review]
new fn for notification when user aborts send

Please remove the dump in the DoCOmmandClose function. Also, as mentioned by
sspitzer, you should pass Components.results.NS_ERROR_<something> instead of
passing -1 when calling onSendNotPerformed. NS_ERROR_ABORT seems to be the most
appropriate in that case. Appart that, R=ducarroz

Comment 29

[Rajiv Dayal]

Attachment: above patch updated with review comments

Comment 30

[Rajiv Dayal]

Comment on attachment 62976 [details] [diff] [review]
above patch updated with review comments

r=ducarroz as per comments above.

Comment 31

[David :Bienvenu]

please remove those printfs - is there any reason everyone needs to see them?
You can #ifdef DEBUG_rdayal if you want.

Comment 32

[Rajiv Dayal]

those will be displayed only in debug mode, but sure I will remove them.

Comment 33

[Rajiv Dayal]

Attachment: above patch to take care of comments, without debug prints

Hi Seth / David,
Can u please sr this patch. thanks, - Rajiv.

Comment 34

[David :Bienvenu]

Comment on attachment 62991 [details] [diff] [review]
above patch to take care of comments, without debug prints 

sr=bienvenu

Comment 35

[Rajiv Dayal]

Comment on attachment 62991 [details] [diff] [review]
above patch to take care of comments, without debug prints 

r=ducarroz as per comments above.

Comment 36

[Rajiv Dayal]

thanks very much David for super reviewing the patch. - rajiv. 

Comment 37

[Rajiv Dayal]

checked into the trunk.

Comment 38

[Trix Supremo]

verified on trunk build 2002042410