1350607 - Fix error handling after call to ReplyWithtemplate() during rule execution, add entry to log when not sending

Status: RESOLVED FIXED

(MailNews Core :: Filters, enhancement)

Description

[Jorg K (CEST = GMT+2)]

+++ This bug was initially created as a clone of Bug #1334980 +++

In bug 1334980 we noticed that nsMsgComposeService::ReplyWithTemplate() can return NS_ERROR_ABORT, see bug 1334980 comment #56 for details. Basically the reply is aborted under certain conditions implemented in bug 904478.

There are three call sites in rule execution:

"After the fact" (after junk classification or manually run):
https://dxr.mozilla.org/comm-central/rev/1df067640ceebf58b01ab1705b0496c43f732d57/mailnews/base/search/src/nsMsgFilterService.cpp#785

IMAP upon receipt:
https://dxr.mozilla.org/comm-central/rev/1df067640ceebf58b01ab1705b0496c43f732d57/mailnews/imap/src/nsImapMailFolder.cpp#3749

POP upon receipt:
https://dxr.mozilla.org/comm-central/rev/1df067640ceebf58b01ab1705b0496c43f732d57/mailnews/local/src/nsParseMailbox.cpp#2345

At least the IMAP case doesn't do any error checking or logging or anything. The filter log says: Reply sent, but it is never sent. Perhaps we should log "Reply cancelled, no matching identity found".

I also noticed that when manually run, clicking "Cancel" on the panel that says:
"Applying filter [name of filer] failed. Would you like to continue applying filters?"
crashes at least in a debug compile.

Comment 1

[Jorg K (CEST = GMT+2)]

We should also put something into the filter log when we abort the reply due to "no identity found" implemented in bug 904478.

Comment 2

[Jorg K (CEST = GMT+2)]

Attachment: 1350607-ReplyWithTemplate.patch (v1).

Comment 3

[Jorg K (CEST = GMT+2)]

I forgot to say: I needed to remove one overzealous MOZ_ASSERT since this got triggered when a manually-run rule failed and the I hit "OK" or "Cancel" on the panel.

Test patch to come.

Comment 4

[Jorg K (CEST = GMT+2)]

Attachment: 1350607-test.patch

Instead of mucking around setting this up to fail, I suggest to force it to fail in the code. Here shown for running rules manually.

You get for example:
[3/26/17, 10:34:22 PM] Applied filter "Reply" to message from Jörg Knobloch <[e-mail]> - HUHU 2 at 3/11/16, 9:39:50 AM replied

[3/26/17, 10:34:22 PM] Filter Action Failed: "Error sending reply" with error code=0x80004004 while attempting: Applied filter "Reply" to message from Jörg Knobloch <[e-mail]> - HUHU at 3/11/16, 9:36:24 AM replied

Comment 5

[Jorg K (CEST = GMT+2)]

Attachment: 1350607-test-pop.patch

Another test patch, this time testing reply upon receipt on POP. Result:

[3/26/17, 10:43:14 PM] Applied filter "Subject contains: HUHU" to message from Jörg Knobloch <[e-mail]> - HUHU at 3/26/17, 10:42:54 PM replied

[3/26/17, 10:43:14 PM] Filter Action Failed: "Error sending reply" with error code=0x80004005 while attempting: Applied filter "Subject contains: HUHU" to message from Jörg Knobloch <[e-mail]> - HUHU at 3/26/17, 10:42:54 PM replied 

I'll leave it to the reviewer to come up with an IMAP patch ;-)

Comment 6

[:aceman]

Comment on attachment 8851394 [details] [diff] [review]
1350607-ReplyWithTemplate.patch (v1).

Review of attachment 8851394 [details] [diff] [review]:
-----------------------------------------------------------------

I wanted that we tell the user when the reply wasn't sent intentionally (we decided the addresses didn't match). Can that be done?

Comment 7

[Jorg K (CEST = GMT+2)]

Yes, if instead of NS_ERROR_ABORT, ReplyWithTemplate() returns some other error: NS_ERROR_NO_MATCHING_IDENTITY or similar. Then we could test for that. Where are these error codes defined so I can add a new one?

Somewhere here?
http://searchfox.org/comm-central/search?q=define.*NS_ERROR_&case=false&regexp=true&path=

Anyway, the message will be in hard-coded in English in the filter log.

Comment 8

[Magnus Melin [:mkmelin]]

I don't think we want to add more error codes, rather less - and fix bug 783526

Comment 9

[Jorg K (CEST = GMT+2)]

OK, so if we don't want more error codes, I can test for NS_ERROR_ABORT
https://dxr.mozilla.org/comm-central/rev/2967d03cf5bc66717e3ff62ece9fc94faeae45e6/mailnews/compose/src/nsMsgComposeService.cpp#901
but that might come from somewhere else as well. So perhaps the current patch is best and not too ambitious.

Comment 10

[:aceman]

We need a way to help bug 1334980 so that the user can see TB is intentionally not sending the reply and then he can investigate further. I'd be for using the NS_ERROR_ABORT code. Yes, some other reasons can cause it. But due to the name, hopefully also all the other reasons mean intentional abort by TB so the string can be the same.

(In reply to Jorg K (GMT+1) from comment #7)
> Anyway, the message will be in hard-coded in English in the filter log.

That's bad. Can we make it localizable?
The aErrmsg argument to logRuleHitFial could be a stringID.

Comment 11

[Jorg K (CEST = GMT+2)]

Attachment: 1350607-ReplyWithTemplate.patch (v2).

The test patches most likely don't apply any more now.

(In reply to :aceman from comment #10)
> That's bad. Can we make it localizable?
No, not in this bug - you didn't give me a compact message that's actually legible since it will stay more then 10 milliseconds either :-( [bug 689992 comment #24].

Let's be reasonable and not spoil the fun of the contributor by widening the scope. Plus, I want to uplift this, so no string changes.

Comment 12

[:aceman]

Comment on attachment 8852659 [details] [diff] [review]
1350607-ReplyWithTemplate.patch (v2).

Review of attachment 8852659 [details] [diff] [review]:
-----------------------------------------------------------------

This looks better now.

::: mailnews/base/search/src/nsMsgFilterService.cpp
@@ +787,5 @@
> +            if (NS_FAILED(rv)) {
> +              if (rv == NS_ERROR_ABORT)
> +                m_curFilter->LogRuleHitFail(filterAction, msgHdr, rv, "Sending reply aborted");
> +              else
> +                m_curFilter->LogRuleHitFail(filterAction, msgHdr, rv, "Error sending reply");

I would output this at the end of the block if any failure happened.

::: mailnews/imap/src/nsImapMailFolder.cpp
@@ +3751,5 @@
> +                NS_WARNING("ReplyWithtemplate failed");
> +                if (rv == NS_ERROR_ABORT)
> +                  filter->LogRuleHitFail(filterAction, msgHdr, rv, "Sending reply aborted");
> +                else
> +                  filter->LogRuleHitFail(filterAction, msgHdr, rv, "Error sending reply");

Could you also output this error when !compService?

::: mailnews/local/src/nsParseMailbox.cpp
@@ +2350,5 @@
> +          if (NS_FAILED(rv)) {
> +            NS_WARNING("ReplyWithtemplate failed");
> +            if (rv == NS_ERROR_ABORT)
> +              m_filter->LogRuleHitFail(m_ruleAction, m_msgToForwardOrReply, rv,
> +                                       "Sending reply aborted");

Please add some {} .

@@ +2353,5 @@
> +              m_filter->LogRuleHitFail(m_ruleAction, m_msgToForwardOrReply, rv,
> +                                       "Sending reply aborted");
> +            else
> +              m_filter->LogRuleHitFail(m_ruleAction, m_msgToForwardOrReply, rv,
> +                                       "Error sending reply");

Could you also output this error when !compService?

Comment 13

[Jorg K (CEST = GMT+2)]

Comment on attachment 8852659 [details] [diff] [review]
1350607-ReplyWithTemplate.patch (v2).

Review of attachment 8852659 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/base/search/src/nsMsgFilterService.cpp
@@ +793,1 @@
>              CONTINUE_IF_FAILURE(rv, "ReplyWithtemplate failed");

Your suggestion doesn't make sense, since this macro contains a |continue;|.

::: mailnews/imap/src/nsImapMailFolder.cpp
@@ +3751,5 @@
> +                NS_WARNING("ReplyWithtemplate failed");
> +                if (rv == NS_ERROR_ABORT)
> +                  filter->LogRuleHitFail(filterAction, msgHdr, rv, "Sending reply aborted");
> +                else
> +                  filter->LogRuleHitFail(filterAction, msgHdr, rv, "Error sending reply");

Not necessary, why would that fail?

::: mailnews/local/src/nsParseMailbox.cpp
@@ +2350,5 @@
> +          if (NS_FAILED(rv)) {
> +            NS_WARNING("ReplyWithtemplate failed");
> +            if (rv == NS_ERROR_ABORT)
> +              m_filter->LogRuleHitFail(m_ruleAction, m_msgToForwardOrReply, rv,
> +                                       "Sending reply aborted");

Above you didn't ask for {}. I can add them everywhere.

@@ +2353,5 @@
> +              m_filter->LogRuleHitFail(m_ruleAction, m_msgToForwardOrReply, rv,
> +                                       "Sending reply aborted");
> +            else
> +              m_filter->LogRuleHitFail(m_ruleAction, m_msgToForwardOrReply, rv,
> +                                       "Error sending reply");

Not necessary, really. The application would have to be build incorrectly for this not to work.

Comment 14

[Jorg K (CEST = GMT+2)]

Attachment: 1350607-ReplyWithTemplate.patch (v2b).

Added braces. The other issues weren't issues ;-)

Comment 15

[:aceman]

Comment on attachment 8852690 [details] [diff] [review]
1350607-ReplyWithTemplate.patch (v2b).

Review of attachment 8852690 [details] [diff] [review]:
-----------------------------------------------------------------

Yes, I wanted the braces everywhere :)

But if compService can't fail why do we check for it?

I still wonder why only 'copy' and 'move' and now 'reply' actions call LogRuleHitFail. Do the others report error to the user in any way? Console warnings do not count. But that is not for this bug.

::: mailnews/base/search/src/nsMsgFilterService.cpp
@@ +794,1 @@
>              CONTINUE_IF_FAILURE(rv, "ReplyWithtemplate failed");

ReplyWithTemplate?

::: mailnews/imap/src/nsImapMailFolder.cpp
@@ +3749,3 @@
>                rv = compService->ReplyWithTemplate(msgHdr, replyTemplateUri.get(), msgWindow, server);
> +              if (NS_FAILED(rv)) {
> +                NS_WARNING("ReplyWithtemplate failed");

Did you mean ReplyWithTemplate?

::: mailnews/local/src/nsParseMailbox.cpp
@@ +2347,5 @@
>            rv = compService->ReplyWithTemplate(m_msgToForwardOrReply,
>                                                m_replyTemplateUri[i].get(),
>                                                msgWindow, server);
> +          if (NS_FAILED(rv)) {
> +            NS_WARNING("ReplyWithtemplate failed");

Did you mean ReplyWithTemplate?

Comment 16

[Jorg K (CEST = GMT+2)]

Thanks.

(In reply to :aceman from comment #15)
> >              CONTINUE_IF_FAILURE(rv, "ReplyWithtemplate failed");
> 
> ReplyWithTemplate?
Yes, I copied wrong a few times from this existing code. Will fix before landing.

Comment 17

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/9aed92a645ac93ab32623887f1995d15dac2f7d4

Comment 18

[Jorg K (CEST = GMT+2)]

Comment on attachment 8852690 [details] [diff] [review]
1350607-ReplyWithTemplate.patch (v2b).

Let's uplift this so TB 52 users get at least some feedback.

Comment 19

[eicker]

Hi Jörg,

looks like it is going to be fixed soon - very nice!
There is just one thing I don't understand in the first place: When I trigger the action ReplyWithTemplate, I got a valid identity in the from - field of my template. If TB filter would pick up the identity from there instead of trying to make a guess from the recipient of incoming mail, all would be well, wouldn't it?

Since I'm to dumb to follow most of this conversation, I assume you did the right stuff already ;-)

Regards,

Andreas Eicker

Comment 20

[Jorg K (CEST = GMT+2)]

We are not changing any behaviour, we're just logging that sending was aborted in the filter log. So nothing is "fixed" here to make your use case work.

You need to understand bug 904478 comment #0. We're not allowed to send a reply with the identity that was used to create the template if the e-mail we're replying to wasn't sent to that address.

Looking at bug 1334980 comment #63: If the e-mail was sent to [e-mail] you can't auto-reply with [e-mail]. Can't you create an identity that has [e-mail] as its e-mail?
Tools > Account Settings, click on the account, "Manage identities" button.

Comment 21

[eicker]

Hi Jörg,

that (almost) does the trick - and I have learned a formerly unknown feature.
When I add a second identity [e-mail] to account [e-mail], filter action reply with template works immediately - but I get two responses. Better than none, but still not right - and the replies originate from [e-mail], although the template says to send it from [e-mail].

From what I understand, the initial request in bug 904478 would save all this hassle (or withdrawing the patch made then).
Well, subsriptions for our MTB-club are done for this season - let's see how this feature works next year...

Comment 22

[Jorg K (CEST = GMT+2)]

You get two responses? I don't understand. If the message went to [e-mail] then one auto-reply will be send with that address, regardless of what the template was created with.

Bug 904478 is about this scenario: Mailing list member Hans sends a message to a mailing list [e-mail]. My magic of mailing list handling, you get a copy of that message:
From: Hans
To: [e-mail]
Delivered to Eicker.
Without fixing bug 904478, TB would now auto-reply To: Hans with From: Eicker. So although Hans has no access to the mailing list members, he now gets your address. That needed to be fixed.

So repeating: TB will only auto-reply if the message was sent directly to one of your e-mail addresses as configured in the entities that you have.

Comment 23

[eicker]

Yep, I get two responses. I have set up a second identity within one account: [e-mail] is the default and an additional one is [e-mail]. When I now send an email to [e-mail], the ReplyWithTemplate Filter responds with two Auto: responses from [e-mail]. I can see it in the filter log too: Obviously the filter gets executed twice when you have two identities connected to the same IMAP account.

 [30.03.2017 13:52:22] Filter "Anmeldungen" auf die Nachricht "EI Systems, Andreas Eicker" <...> - Test3 am 30.03.2017 13:52:16 angewendet beantwortet

[30.03.2017 13:52:22] Filter "Anmeldungen" auf die Nachricht "EI Systems, Andreas Eicker" <...> - Test3 am 30.03.2017 13:52:16 angewendet Nachricht (ID= [e-mail]) verschoben nach imap://info%40bike-.../INBOX/Anmeldung 2017

[30.03.2017 13:52:24] Filter "Anmeldungen" auf die Nachricht "EI Systems, Andreas Eicker" <...> - Test3 am 30.03.2017 13:52:16 angewendet beantwortet

[30.03.2017 13:52:24] Filter "Anmeldungen" auf die Nachricht "EI Systems, Andreas Eicker" <...> - Test3 am 30.03.2017 13:52:16 angewendet Nachricht (ID= [e-mail]) verschoben nach imap://info%40bike-

That's new behaviour since I set up a second identity - but it really sends emails as opposed to the scenario before, where there was only one entry in the filter log but no email actually sent...

Comment 24

[Jorg K (CEST = GMT+2)]

(In reply to eicker from comment #23)
> Obviously the filter gets executed twice when you have two
> identities connected to the same IMAP account.
Sorry, that's not obvious to me at all. For one incoming message you should generate one reply.

> it really sends emails as opposed to the scenario before, where there was only
> one entry in the filter log but no email actually sent.
The failure to send will now be registered in the log, that's what this bug is about.

Comment 25

[eicker]

Ok, sorry, it isn't obvious and it should not happen in the first place.
What I meant to say was the one incoming mail (see the identical ID in the filter log (I'm still using TB 45.8.0)) triggers the filter action twice since I added a second identity to my IMAP account.
Just checked: It has nothing to do with ReplyWithTemplate, I see two entries in the filter log even if I just move the message into a folder. It doesn't matter wether I set up the filter for the default identity or the second identity.
Shall I open another bug for this?

Comment 26

[Jorg K (CEST = GMT+2)]

(In reply to eicker from comment #25)
> It doesn't matter wether I set up the filter for the default identity
> or the second identity.
There must be some sort of misunderstanding here. Filters are set up for an account, not identities in the account.
I just tested this. One IMAP account, two identities, even with identical e-mail, sent an e-mail there, filter was hit, *one* reply was sent.

> Shall I open another bug for this?
I'm not convinced there is a bug really. You should check what's going on on your machine.

Comment 27

[Jorg K (CEST = GMT+2)]

I filed bug 1352391 for the two replies being generated.

Comment 28

[Jorg K (CEST = GMT+2)]

Aurora (TB 54):
https://hg.mozilla.org/releases/comm-aurora/rev/2294382e95803d5dcbe9b66933c6295eb3bcab12

Comment 29

[Jorg K (CEST = GMT+2)]

Beta (TB 53):
https://hg.mozilla.org/releases/comm-beta/rev/b74bf497e3d852bede66ef84c8c8545348822f9f

Comment 30

[Jorg K (CEST = GMT+2)]

TB 53 ESR:
https://hg.mozilla.org/releases/comm-esr52/rev/31c45068f1556c74c1ccca567e213447689bbbaf