1205769 - Roster fetch can trigger disconnects due to vcard request flood

Status: VERIFIED FIXED

(Chat Core :: XMPP, defect)

Description

[aleth [:aleth]]

" On connect, the XMPP code fetches the roster.  This seems to trigger downloading the vcard if we're subscribed to their presence.
<Mook> ... this server seems to be kicking me off since that means I just requested the vCard of >1000 people."

Comment 1

[aleth [:aleth]]

Mook: do you have more details about this? Error stanzas? Test server? ;)

Comment 2

[:Mook]

Sadly, no. There was no error message, the connection was just closed (after a _lot_ of correct-looking vCard responses).

Sorry, test server isn't available either :( It's a private HipChat server, though.

Comment 3

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 1 - reduce flood of vcard requests

Comment 4

[aleth [:aleth]]

Comment on attachment 8669821 [details] [diff] [review]
rev 1 - reduce flood of vcard requests

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

_pendingvCardRequests needs to be cleared on disconnect.

::: chat/protocols/xmpp/xmpp.jsm
@@ +1001,5 @@
>    // discovery.
>    _mucService: null,
>  
> +  // Contains a set of jids that we need to request their vcards.
> +  _pendingvCardRequests: new Set(),

_pendingVCardRequests for consistency.

@@ +1276,5 @@
>    },
>  
> +  // Sends vcard request for the first item in _pendingvCardRequests set and
> +  // deletes it from set.
> +  _sendNextVCard: function() {

_requestNextVCard would be better as this doesn't actually send vCards.

@@ +1505,5 @@
>          for (let item of query.getChildren("item"))
>            this._onRosterItem(item, true);
>          return;
>        }
> +      this._sendNextVCard();

why?

@@ +1814,5 @@
>        buddy._saveIcon(vCardInfo.photo);
>      if (!foundFormattedName && buddy._vCardFormattedName)
>        buddy.vCardFormattedName = "";
>      buddy._vCardReceived = true;
> +    this._sendNextVCard();

You always sendNextVcard, so just call it first.

@@ +1910,5 @@
>      // subscribed to presence for that contact.
> +    if ((subscription == "both" || subscription == "to") &&
> +        !buddy._vCardReceived &&
> +        jid != this._jid.node + "@" + this._jid.domain)
> +      this._pendingvCardRequests.add(jid);

Use a method for this instead that automatically calls sendNextVCard if needed.

@@ +1979,5 @@
> +
> +    // Add vcard request for current account.
> +    this._pendingvCardRequests.add(this._jid.node + "@" + this._jid.domain);
> +
> +    this._sendNextVCard();

I don't understand this change.

Comment 5

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 2 - reduce flood of vcard requests

(In reply to aleth [:aleth] from comment #4)
> why?
The last patch, I was just adding jids to set in onRosterItem and after all additions, start a series of requests (This way is no more used in this patch)

> I don't understand this change.
Sorry, that change seems not to be correct.

Comment 6

[aleth [:aleth]]

Comment on attachment 8670509 [details] [diff] [review]
rev 2 - reduce flood of vcard requests

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

Still missing the reset on disconnect.

::: chat/protocols/xmpp/xmpp.jsm
@@ +1286,5 @@
> +      this._vCardRequested = false;
> +      return;
> +    }
> +    else
> +      this._vCardRequested = true;

You don't really need this boolean, as it simply tracks !this._pendingVCardRequests.size, so you can use that instead.

@@ +1919,5 @@
>      // We request the vCard only if we haven't received it yet and are
>      // subscribed to presence for that contact.
> +    if ((subscription == "both" || subscription == "to") &&
> +        !buddy._vCardReceived &&
> +        jid != this._jid.node + "@" + this._jid.domain)

What's the jid test for?

Comment 7

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 3 - reduce flood of vcard requests

(In reply to aleth [:aleth] from comment #6)
> What's the jid test for?

I think I have noticed that some servers provide the jid of user's account as an item in roster query, so we will not request it (I'm not sure).
BTW, let it in a separate bug if it's reproducible.

Comment 8

[aleth [:aleth]]

Comment on attachment 8670553 [details] [diff] [review]
rev 3 - reduce flood of vcard requests

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

::: chat/protocols/xmpp/xmpp.jsm
@@ +1041,5 @@
>    // Contains the domain of MUC service which is obtained using service
>    // discovery.
>    _mucService: null,
>  
> +  // Contains a set of jids that we need to request their vcards.

Nit: "A set of jids for which we still need to request vCards."

@@ +1215,5 @@
>    },
>  
>    /* Disconnect from the server */
>    disconnect: function() {
> +    this._pendingVCardRequests.clear();

That's not the right place to put this. Move it to _disconnect, which also gets called (by onError) when the connection is terminated unexpectedly.

@@ +1320,5 @@
>      });
>    },
>  
> +  // Sends vcard request for the first item in _pendingVCardRequests set and
> +  // deletes it from set.

I don't think you need this comment, it's self-explanatory.

@@ +1326,5 @@
> +    if (!this._pendingVCardRequests.size)
> +      return;
> +    let jid = this._pendingVCardRequests.values().next().value;
> +    this._requestVCard(jid);
> +    this._pendingVCardRequests.delete(jid);

Shouldn't you only delete this when the vcard arrives? Otherwise the next addVCardRequest call will think there is no request pending and send another one.

@@ +1330,5 @@
> +    this._pendingVCardRequests.delete(jid);
> +  },
> +
> +  _addVCardRequest: function(aJID) {
> +    let requestNextVCard = this._pendingVCardRequests.size == 0;

Let's call this boolean requestPending, i.e. let requestPending = !!this._pendingVcardrequests.size

then you can do
if (!requestPending)
  requestNext...()

That's a bit more readable.

Comment 9

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 4 - reduce flood of vcard requests

Comment 10

[aleth [:aleth]]

Comment on attachment 8670847 [details] [diff] [review]
rev 4 - reduce flood of vcard requests

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

I don't think you've converted all the calls of _requestVCard() to addVCardRequest.

::: chat/protocols/xmpp/xmpp.jsm
@@ +1330,5 @@
> +    let requestPending = !!this._pendingVCardRequests.size;
> +    this._pendingVCardRequests.add(aJID);
> +    if (!requestPending)
> +      this._requestNextVCard();
> +  },

Nit: move these two functions closer to _requestVCard, which is more closely related.

@@ +1848,5 @@
>    /* When a vCard is received */
>    _vCardReceived: false,
>    onVCard: function(aStanza) {
>      let jid = this.normalize(aStanza.attributes["from"]);
> +    if (!jid || !this._buddies.has(jid)) {

Turns out from Example 10 in XEP-0054 error responses are allowed to not have a jid (how annoying). This complicates things as it's OK for the server to respond with item-not-found if someone doesn't have a vcard. 

And I was wrong about "if there's no jid, we don't know who the stanza is about" as this is always a callback, so the id does identify who it refers to, indirectly.

So it's probably best to change pendingVCardRequests from a Set to an Array (i.e. to make it an ordered list). Then you can use .push() to add items to the stack and .shift() to remove the first item, which is more natural than using values() on Set.

@@ +1849,5 @@
>    _vCardReceived: false,
>    onVCard: function(aStanza) {
>      let jid = this.normalize(aStanza.attributes["from"]);
> +    if (!jid || !this._buddies.has(jid)) {
> +      this.ERROR("received a vCard for unknown buddy or jid may be broken " + jid);

You don't need the !jid check any more then as it's replaced by proper error handling.

!this._buddies.has(jid) you should still check for of course. That would mean some error has occurred (or maybe the buddy was deleted while we were waiting).

@@ +1861,5 @@
> +    if ((error && (error.condition == "item-not-found" ||
> +        error.condition == "service-unavailable")) || !vCard ||
> +        !vCard.children.length) {
> +      this.WARN("No vCard exists for buddy " + jid + " or this user does not" +
> +                "exist");

If it's item-not-found, service-unavailable, or if there is no vcard or an empty vcard, that's all OK according to the spec. We don't need to warn in that case. this.LOG would be fine.

You should warn if it's an error but with a different condition.

@@ +2119,5 @@
>      // Also clear the cached user vCard, as we will want to redownload it
>      // after reconnecting.
>      delete this._userVCard;
>  
> +    this._pendingVCardRequests.clear();

Nit: add a blank line after this

Comment 11

[aleth [:aleth]]

(In reply to Abdelrhman Ahmed [:abdelrhman] from comment #7)
> I think I have noticed that some servers provide the jid of user's account
> as an item in roster query, so we will not request it (I'm not sure).
> BTW, let it in a separate bug if it's reproducible.

That's interesting, I wonder why that happens. (Unless you have added yourself to the roster?)

Comment 12

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 5 - reduce flood of vcard requests

Comment 13

[aleth [:aleth]]

Comment on attachment 8670921 [details] [diff] [review]
rev 5 - reduce flood of vcard requests

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

::: chat/protocols/xmpp/xmpp.jsm
@@ +1829,5 @@
>        aError = Ci.prplIAccount.ERROR_OTHER_ERROR;
>      this._disconnect(aError, aException.toString());
>    },
>  
>    /* Callbacks for Query stanzas */

Let's remove this comment, as there's only one of these callbacks and now also a bunch of functions that aren't callbacks...

@@ +1841,2 @@
>        return;
> +    }

As you're checking this using the jid we stored ourselves, the right place to ERROR would be in requestVCard (though I think we can live without it).

At this point you are only checking the buddy wasn't removed in the interim, I think. So just this.WARN here as that's not actually an error.

@@ +1843,5 @@
>  
>      let vCard = aStanza.getElement(["vCard"]);
> +    let error = this.parseError(aStanza);
> +    if ((error && (error.condition == "item-not-found" ||
> +        error.condition == "service-unavailable")) || !vCard ||

Nit: indentation (align the 'error's or 'error.condition's)
Also it's more readable if you move the ||vCard||s so it's together with the other one on the next line.

@@ +1846,5 @@
> +    if ((error && (error.condition == "item-not-found" ||
> +        error.condition == "service-unavailable")) || !vCard ||
> +        !vCard.children.length) {
> +      this.LOG("No vCard exists for buddy " + jid + " or this user does not" +
> +                "exist");

Nit: indentation

@@ +1857,3 @@
>  
>      let foundFormattedName = false;
> +    let buddy = this._buddies.get(jid);

Would be good to check jid matches the one in "from" at this point (this.ERROR if it doesn't I suppose).

@@ +1872,5 @@
> +  _requestNextVCard: function() {
> +    if (!this._pendingVCardRequests.length)
> +      return;
> +    this._requestVCard(this._pendingVCardRequests[0]);
> +  },

Can we move _requestVCard here too then? Maybe even inline it if this is the only caller?

Comment 14

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 6 - reduce flood of vcard requests

Comment 15

[Abdelrahman Ahmed [:abdelrahman]]

(In reply to aleth [:aleth] from comment #11)
> That's interesting, I wonder why that happens. (Unless you have added
> yourself to the roster?)

May be you are right. I'll try to reproduce that in next few days and I'll tell you if I discover something strange ;)

Comment 16

[aleth [:aleth]]

Comment on attachment 8670978 [details] [diff] [review]
rev 6 - reduce flood of vcard requests

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

This works well!

::: chat/protocols/xmpp/xmpp.jsm
@@ +1833,1 @@
>    /* When a vCard is received */

Let's either improve this comment by making it more specific, or remove it. This doesn't tell us more than the name "onVCard" does ;)

@@ +1833,2 @@
>    /* When a vCard is received */
>    _vCardReceived: false,

This boolean is defined in the wrong place: it should be on the accountbuddy prototype.

@@ +1845,5 @@
> +    if ((error && (error.condition == "item-not-found" ||
> +         error.condition == "service-unavailable")) ||
> +        !vCard || !vCard.children.length) {
> +      this.LOG("No vCard exists for buddy " + jid + " or this user does not" +
> +               "exist");

"No vCard exists (or the user does not exist) for " + jid

@@ +1857,5 @@
> +    let buddy = this._buddies.get(jid);
> +    let stanzaJid = this.normalize(aStanza.attributes["from"]);
> +    if (jid && jid != stanzaJid) {
> +      this.ERROR("Received vCard for different jid (" + stanzaJid + ") from " +
> +                 jid);

Nit: from -> than the requested

@@ +1874,5 @@
>      buddy._vCardReceived = true;
>    },
>  
> +  _requestNextVCard: function() {
> +    if (!!this._pendingVCardRequests.length)

Nit: you don't need the !! here

@@ +1875,5 @@
>    },
>  
> +  _requestNextVCard: function() {
> +    if (!!this._pendingVCardRequests.length)
> +      this._requestVCard(this._pendingVCardRequests[0]);

Inline _requestVCard.

Comment 17

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 7 - reduce flood of vcard requests

Comment 18

[aleth [:aleth]]

Comment on attachment 8671033 [details] [diff] [review]
rev 7 - reduce flood of vcard requests

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

Thanks!

Comment 19

[aleth [:aleth]]

Attachment: Reduce flood of vcard requests after fetching roster

Comment 20

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/2011f180de9a8c6eb5dc1f823e6f22b7d2d0ab7f
Bug 1205769 - Reduce flood of vcard requests after fetching roster. r=aleth

Comment 21

[:Mook]

This does appear to fix my problems with the server disconnecting me from the vCard flood (i.e. I can now connect to the server just fine without it booting me off), thanks.