Better error reporting when outgoing messages fail

RESOLVED FIXED in Instantbird 47

Status

Chat Core
XMPP
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: arlolra, Assigned: aleth)

Tracking

trunk
Instantbird 47

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

2 years ago
Created attachment 8715075 [details] [diff] [review]
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.
(Reporter)

Updated

2 years ago
Attachment #8715075 - Attachment is patch: true
Attachment #8715075 - Attachment mime type: text/x-patch → text/plain
Attachment #8715075 - Flags: review?(aleth)
Assignee: nobody → arlolra
Component: Conversation → XMPP
Product: Instantbird → Chat Core
(Assignee)

Comment 1

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

Comment 2

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

Comment 3

2 years ago
Created attachment 8716072 [details] [diff] [review]
failed.patch from comment 2
Attachment #8715075 - Attachment is obsolete: true
Attachment #8715075 - Flags: review?(aleth)
Attachment #8716072 - Flags: review?(aleth)
(Reporter)

Comment 4

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

Comment 5

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

Comment 6

2 years ago
Created attachment 8716664 [details] [diff] [review]
failed.patch from comment 5
Attachment #8716072 - Attachment is obsolete: true
Attachment #8716072 - Flags: review?(aleth)
Attachment #8716664 - Flags: review?(aleth)
(Assignee)

Comment 7

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

Comment 8

2 years ago
Hmm, maybe we should just unconditionally put: "Service for the recipient is currently unavailable."
(Reporter)

Comment 9

2 years ago
Created attachment 8717254 [details] [diff] [review]
failed.patch from comment 8
Attachment #8716664 - Attachment is obsolete: true
Attachment #8716664 - Flags: review?(aleth)
Attachment #8717254 - Flags: review?(aleth)
(Assignee)

Updated

2 years ago
Attachment #8716664 - Attachment is obsolete: false
(Assignee)

Comment 10

2 years ago
Created attachment 8717678 [details] [diff] [review]
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.
Attachment #8717678 - Flags: review?(clokep)
(Assignee)

Updated

2 years ago
Assignee: arlolra → aleth
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
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.
Attachment #8717678 - Flags: review?(clokep) → review+
(Assignee)

Comment 12

2 years ago
arlolra: these changes seem OK to you too?
Flags: needinfo?(arlolra)
(Reporter)

Comment 13

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

yup, lgtm
Flags: needinfo?(arlolra)
(Assignee)

Comment 14

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

Updated

2 years ago
Attachment #8716664 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8717254 - Attachment is obsolete: true
Attachment #8717254 - Flags: review?(aleth)
(Assignee)

Updated

2 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Instantbird 47
You need to log in before you can comment on or make changes to this bug.