1245325 - Better error reporting when outgoing messages fail

Status: RESOLVED FIXED

(Chat Core :: XMPP, defect)

Description

[arlolra]

Attachment: failed.patch

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/48.0.2564.97 Safari/537.36

Steps to reproduce:

Sent a message to an offline user when (presumably) the server doesn't support offline messages.

See https://trac.torproject.org/projects/tor/ticket/17494


Actual results:

"Unknown error"


Expected results:

Something more descriptive.

Comment 1

[aleth [:aleth]]

Comment on attachment 8715075 [details] [diff] [review]
failed.patch

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

Thanks for adding the missing error condition!

::: chat/locales/en-US/xmpp.properties
@@ +66,5 @@
>  #   %2$S is the text of the message that wasn't delivered.
>  conversation.error.sendFailedAsRecipientNotInRoom=Message could not be sent to %1$S as the recipient is no longer in the room: %2$S
>  #   These are displayed in a conversation as a system error message.
> +conversation.error.remoteServerNotFound=Could not reach the recipient's server.
> +conversation.error.serviceUnavailable=Service unavailable. Contact was likely offline and server doesn't support offline messages.

We likely don't need to guess whether the contact was offline, as we have presence info. (or is this unreliable?)

So we could use two strings here, 'Service unavailable' and 'Could not send this message as %S is offline and this server does not support offline messages.'

Comment 2

[arlolra]

Comment on attachment 8715075 [details] [diff] [review]
failed.patch

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

Thanks for taking a look.

::: chat/locales/en-US/xmpp.properties
@@ +66,5 @@
>  #   %2$S is the text of the message that wasn't delivered.
>  conversation.error.sendFailedAsRecipientNotInRoom=Message could not be sent to %1$S as the recipient is no longer in the room: %2$S
>  #   These are displayed in a conversation as a system error message.
> +conversation.error.remoteServerNotFound=Could not reach the recipient's server.
> +conversation.error.serviceUnavailable=Service unavailable. Contact was likely offline and server doesn't support offline messages.

Hmm, the "likely" here was trying to qualify that the "unavailable service" was "offline messaging". I guess I imagined that there could be other services that would return this same error message, and the stanza isn't being helpful in describing which one.

I'm going to just shorten this to "Service unavailable" and leave it at that for now, unless you have any bright ideas.

Comment 3

[arlolra]

Attachment: failed.patch from comment 2

Comment 4

[arlolra]

On second thought, since this is an error in response to an outgoing message, it's probably safe to say it's because the server couldn't store it. Updated the patch with that.

Comment 5

[aleth [:aleth]]

Probably I wasn't clear in comment 1. My suggestion was that we know if the recipient is offline or not (stored in statusType). If he is offline, this message makes sense. If not, a generic "Service unavailable" is probably the best we can do.

Comment 6

[arlolra]

Attachment: failed.patch from comment 5

Comment 7

[aleth [:aleth]]

Potentially relevant, from the spec (rfc 6120):
      Security Warning: An application MUST return a <service-
      unavailable/> stanza error (Section 8.3.3.19) instead of <item-
      not-found/> (Section 8.3.3.7) or <recipient-unavailable/>
      (Section 8.3.3.13) if sending one of the latter errors would
      provide information about the intended recipient's network
      availability to an entity that is not authorized to know such
      information (for a more detailed discussion of presence
      authorization, refer to [XMPP-IM]).

Comment 8

[arlolra]

Hmm, maybe we should just unconditionally put: "Service for the recipient is currently unavailable."

Comment 9

[arlolra]

Attachment: failed.patch from comment 8

Comment 10

[aleth [:aleth]]

Attachment: Better error reporting for failed outgoing messages

I modified the strings a little to try to better reflect comment 7. Note we shouldn't show the error-condition directly to the user, it is not localiseable. Advanced users can look at the debug log.

Comment 11

[Patrick Cloke [:clokep]]

Comment on attachment 8717678 [details] [diff] [review]
Better error reporting for failed outgoing messages

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

::: chat/protocols/xmpp/xmpp.jsm
@@ +681,5 @@
>        if (!aMsg) {
> +        // Failed outgoing message.
> +        switch (error.condition) {
> +          case "remote-server-not-found":
> +            aMsg = _("conversation.error.remoteServerNotFound");

This is weird that we're setting aMsg, but we were doing it previously too.

Comment 12

[aleth [:aleth]]

arlolra: these changes seem OK to you too?

Comment 13

[arlolra]

(In reply to aleth [:aleth] from comment #12)
> arlolra: these changes seem OK to you too?

yup, lgtm

Comment 14

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/4cfc2a04ebe02f53d789c7c27f8c3cd2a40b6483
Bug 1245325 - Better error reporting for failed outgoing messages. r=clokep