964070 - Support Jabber/XMPP Message Carbons (XEP-280)

Status: RESOLVED FIXED

(Chat Core :: XMPP, defect)

Description

[Andreas Kloeckner]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:25.0) Gecko/20100101 Firefox/25.0 Iceweasel/25.0 (Beta/Release)
Build ID: 20131102022523

Steps to reproduce:

1. Log into my Jabber account from two devices (phone, laptop) simultaneously
2. Start chat conversation





Actual results:

3a. Only one logged-in client receives the conversation


Expected results:

3b. All logged-in clients should have received a copy of both ends of the conversation.

This is described in http://xmpp.org/extensions/xep-0280.html , and it's very useful if you'd like to use a Jabber account from multiple devices. Otherwise, the latest reply, on average, is always on the wrong device, at least in my experience.

Comment 1

[Andreas Kloeckner]

I should add that this should likely be optional.

Comment 2

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: v1 - support message carbons

Comment 3

[Abdelrahman Ahmed [:abdelrahman]]

I used |_isCarbonsEnabled| flag to indicate that we have enabled carbons successfully and to reduce the recursive checking of |getElement| to search for forwarded, sent and received.

Comment 4

[aleth [:aleth]]

Comment on attachment 8777569 [details] [diff] [review]
v1 - support message carbons

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

::: chat/protocols/xmpp/xmpp.jsm
@@ +99,5 @@
>  
>    return date;
>  }
>  
> +function _displaySendMsg(aConv, aMsg, aDate) {

nit: _displaySentMsg

Add a comment that aDate is optional and means the message was sent from another client.

@@ +1177,5 @@
>    // An array of jids for which we still need to request vCards.
>    _pendingVCardRequests: [],
>  
> +  // XEP-0280: Message Carbons.
> +  // It's not enabled by default.

This comment is confusing, instead say 
"If true, message carbons are currently enabled."

@@ +1685,5 @@
>                         Stanza.node("query", Stanza.NS.disco_items));
>      this.sendStanza(iq, this.onServiceDiscovery, this);
> +
> +    // XEP-0280: Message Carbons.
> +    // Enabling Carbons on server, as it's disabled by default on server.

Shouldn't all this be in onServiceDiscovery?

@@ +1704,5 @@
> +                    "while enabling message carbons.");
> +          return;
> +        }
> +
> +        this._isCarbonsEnabled = true;

How about a this.LOG("Message carbons enabled.") or something like that

@@ +1977,5 @@
> +    // XEP-0280: Message Carbons.
> +    // Sending and Receiving Messages.
> +    // Indicates that the forwarded message was sent or received.
> +    let isSent = false;
> +    if (this._isCarbonsEnabled) {

Check this later to ensure carbon message get handled correctly even if they arrive when you don't expect them.

@@ +1978,5 @@
> +    // Sending and Receiving Messages.
> +    // Indicates that the forwarded message was sent or received.
> +    let isSent = false;
> +    if (this._isCarbonsEnabled) {
> +      let messageStanza = aStanza.getElement(["sent", "forwarded", "message"]);

carbonStanza would be clearer

@@ +1986,5 @@
> +        messageStanza =
> +          aStanza.getElement(["received", "forwarded", "message"]);
> +      }
> +
> +      // If this is a forwarded message, escape the wrapping message.

escape?

@@ +1988,5 @@
> +      }
> +
> +      // If this is a forwarded message, escape the wrapping message.
> +      if (messageStanza)
> +        aStanza = messageStanza;

At this point it makes sense to check isCarbonsEnabled and WARN if we didn't expect this.

@@ +1993,5 @@
> +    }
> +
> +    // In case of sending in message carbons, we need to use "to" attribute to
> +    // get the right conversation as from in this case is this account.
> +    let from = isSent ? aStanza.attributes["to"] : aStanza.attributes["from"];

Don't use from for this as it is likely to cause accidental errors in the future due to the misleading name.

Instead, something like
let convJid = isSent? to : from 
(or norm(to) : norm(from) if that is what's needed)
and then replace "from" in the following code where "convJid" is needed now.

@@ +2085,5 @@
> +
> +      if (isSent) {
> +        _displaySendMsg(conv, body, date);
> +        return;
> +      }

Does everything in onMessageStanza make sense for
- sent stanzas?
- carbon stanzas?
You should probably be careful e.g. with invitations.

Comment 5

[aleth [:aleth]]

Comment on attachment 8777569 [details] [diff] [review]
v1 - support message carbons

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

::: chat/protocols/xmpp/xmpp.jsm
@@ +1983,5 @@
> +      if (messageStanza)
> +        isSent = true;
> +      else {
> +        messageStanza =
> +          aStanza.getElement(["received", "forwarded", "message"]);

Would be better and more performant to check just for "sent" or "received". If either one is present, it's a carbon message. Then if you can't getElement("forwarded", "message") it's an invalid message.

Comment 6

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: v2 - support message carbons

(In reply to aleth [:aleth] from comment #4)
> Shouldn't all this be in onServiceDiscovery?

No, it should use service discovery information features according to XEP-280.
BTW, I implemented it to be able to discover that.

> Does everything in onMessageStanza make sense for
> - sent stanzas?
> - carbon stanzas?
> You should probably be careful e.g. with invitations.

Actually, invitations are not forwarded.
Section 6 (Messages Eligible for Carbons Delivery) in XEP-280.

Comment 7

[aleth [:aleth]]

Comment on attachment 8777954 [details] [diff] [review]
v2 - support message carbons

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

Looks much better, thanks.

::: chat/protocols/xmpp/xmpp.jsm
@@ +1686,5 @@
>      let iq = Stanza.iq("get", null, this._jid.domain,
>                         Stanza.node("query", Stanza.NS.disco_items));
>      this.sendStanza(iq, this.onServiceDiscovery, this);
> +
> +    // XEP-0030: Service Discovery Information Features.

Ah, it's in Features, OK.

@@ +1848,5 @@
> +    }
> +
> +    let features = query.getElements(["feature"])
> +                        .map(elt => elt.attributes["var"]);
> +    if (features.indexOf(Stanza.NS.carbons) != -1) {

features.includes

@@ +1874,5 @@
> +          this._isCarbonsEnabled = true;
> +        });
> +      }
> +    }
> +    // TODO: Handle other features that are supported by the server.

Should you return true or false here? You're returning true at the top of the function.

@@ +2003,5 @@
> +    // Indicates that the forwarded message was sent or received.
> +    let isSent = false;
> +    let carbonStanza =
> +      aStanza.getElement(["sent"]) || aStanza.getElement(["received"]);
> +    if (carbonStanza) {

Maybe check the namespace for carbons?

@@ +2009,5 @@
> +      carbonStanza = carbonStanza.getElement(["forwarded", "message"]);
> +      if (this._isCarbonsEnabled)
> +        aStanza = carbonStanza;
> +      else {
> +        this.WARN("Received unexpected message carbons as it's not enabled.");

"Received an unexpected forwarded message while message carbons are not enabled."

@@ +2015,5 @@
> +      }
> +    }
> +
> +    // In case of sending in message carbons, we need to use "to" attribute to
> +    // get the right conversation as from in this case is this account.

English nit: "For forwarded sent messages, we need to use "to" ...

@@ +2018,5 @@
> +    // In case of sending in message carbons, we need to use "to" attribute to
> +    // get the right conversation as from in this case is this account.
> +    let convJid = isSent ? aStanza.attributes["to"] : aStanza.attributes["from"];
> +
> +    let norm = this.normalize(convJid);

Let's call this normConvJid to be explicit.

@@ +2123,5 @@
>        if (!muc) {
>          this.WARN("Received a groupchat message for unknown MUC " + norm);
>          return;
>        }
>        muc.onMessageStanza(aStanza);

Does muc.onMessageStanza handle sent carbons OK?

@@ +2128,5 @@
>        return;
>      }
>  
> +    // In case of sending in message carbons, we don't need to display the
> +    // typing notifications as the user is not typing on this client.

// If this is a sent message carbon, the user is typing on another client.

@@ +2154,5 @@
>          typingState = Ci.prplIConvIM.TYPED;
>      }
>      let convName = norm;
>      if (isMuc)
> +      convName = convJid;

It's not really part of this bug, but can we add a comment here explaining what this if clause is for?

Comment 8

[aleth [:aleth]]

Comment on attachment 8777954 [details] [diff] [review]
v2 - support message carbons

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

::: chat/protocols/xmpp/xmpp.jsm
@@ +2098,5 @@
>            this.WARN("Received a groupchat message for unknown MUC " + norm);
>            return;
>          }
>          let muc = this._mucs.get(norm);
>          muc.incomingMessage(body, aStanza, date);

Check this is all fine for message carbons.

Comment 9

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: v3 - support message carbons

(In reply to aleth [:aleth] from comment #7)
> Does muc.onMessageStanza handle sent carbons OK?


(In reply to aleth [:aleth] from comment #8)
> Comment on attachment 8777954 [details] [diff] [review]
> v2 - support message carbons
> 
> Review of attachment 8777954 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: chat/protocols/xmpp/xmpp.jsm
> @@ +2098,5 @@
> >            this.WARN("Received a groupchat message for unknown MUC " + norm);
> >            return;
> >          }
> >          let muc = this._mucs.get(norm);
> >          muc.incomingMessage(body, aStanza, date);
> 
> Check this is all fine for message carbons.

In XEP-0280:
> A <message/> is eligible for carbons delivery if it is of type ”chat”.

So any MUC messages will not be forwarded, but there is a potential issue.
The private message to a MUC participant (sending and receiving) is forwarded and when we process the received message we do not know that as we have not joined that muc, so all sent and received messages (private muc) will be written in the same conversation (room@domain) as one-to-one chat because of normalization.

Comment 10

[aleth [:aleth]]

(In reply to Abdelrhman Ahmed [:abdelrhman] from comment #9)
> In XEP-0280:
> > A <message/> is eligible for carbons delivery if it is of type ”chat”.
> 
> So any MUC messages will not be forwarded, but there is a potential issue.
> The private message to a MUC participant (sending and receiving) is
> forwarded and when we process the received message we do not know that as we
> have not joined that muc, so all sent and received messages (private muc)
> will be written in the same conversation (room@domain) as one-to-one chat
> because of normalization.

Have you seen this in testing? Because the spec says "A <message/> is not eligible for carbons delivery if it is determined to have been sent by a MUC room or service, even if it would be otherwise eligible (this also includes private messages from MUC participants)."

Comment 11

[Abdelrahman Ahmed [:abdelrahman]]

(In reply to aleth [:aleth] from comment #10)
> Have you seen this in testing? Because the spec says "A <message/> is not
> eligible for carbons delivery if it is determined to have been sent by a MUC
> room or service, even if it would be otherwise eligible (this also includes
> private messages from MUC participants)."

Yes.We can ignore that according to next sentences in the XEP.

> As this is a implementation detail of servers, clients MUST NOT rely on the server implement-
> ing a particular set of rules for which messages are eligible for Carbons delivery.

> Future specifications may have more precise requirements on which messages need to be
> eligible for carbons delivery

Comment 12

[aleth [:aleth]]

Comment on attachment 8778326 [details] [diff] [review]
v3 - support message carbons

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

Thanks! 

Issues noticed in testing that should be filed as other bugs:

- When another resource for the user is online, we receive presence stanzas for that resource. These are currently unhandled. We should probably handle them by ignoring them, as we don't have any use for the information I think?

- The result stanza response to an <iq type="set"><query xmlns="jabber:iq:roster"> (adding a contact) is unhandled.

Comment 13

[aleth [:aleth]]

(In reply to aleth [:aleth] from comment #10)
> > So any MUC messages will not be forwarded, but there is a potential issue.
> > The private message to a MUC participant (sending and receiving) is
> > forwarded and when we process the received message we do not know that as we
> > have not joined that muc, so all sent and received messages (private muc)
> > will be written in the same conversation (room@domain) as one-to-one chat
> > because of normalization.

It's annoyiung that MUC private messages don't have a different namespace or something to normal private messages...

Comment 14

[Abdelrahman Ahmed [:abdelrahman]]

https://hg.mozilla.org/comm-central/rev/51e168d7dc78