Changes required to fix up the webrtc patch that aren't really related to webrtc/Jingle.

This improves the stanza handling at the XMPP socket and fixes some bugs: - JS-XMPP was using an integer for its stanza ids, counting up from zero. If you are talking to another IB instance, this leads to strange intermittent bugs as these ids may not be unique if the other end uses a particular integer first. - If an id-based callback handles an incoming stanza, we should not handle that stanza further.

This cleans up the sendStanza method on the account that did not forward all arguments correctly to the socket. There's also no reason not to use this method more consistently in the code, since it exists and shortens line lengths quite a bit.

This adds some methods to parse, handle, and send error stanzas correctly according to the spec. They aren't yet complete or perfect, but are enough to be used by the jingle code without introducing a lot of painful duplication there. JS-XMPP is still fairly underdeveloped at dealing with error responses to stanzas we send correctly (it's not just the webrtc code which should handle error responses better).

Missed a hunk when splitting up the patches.

Comment on attachment 8558194 [details] [diff] [review] Part 1: Stanza handling Review of attachment 8558194 [details] [diff] [review]: ----------------------------------------------------------------- ::: chat/protocols/xmpp/xmpp-session.jsm @@ +495,5 @@ > accountListening: function(aStanza) { > + if (aStanza.attributes.id) { > + if (this.execHandler(aStanza.attributes.id, aStanza)) > + return; > + } Nit: Combine these two if-statements. ::: chat/protocols/xmpp/xmpp.jsm @@ +844,5 @@ > > /* Called when a iq stanza is received */ > + onIQStanza: function(aStanza) { > + let type = aStanza.attributes["type"]; > + if (type == "set") { This is only used once, it seems.

Comment on attachment 8558206 [details] [diff] [review] Part 2: cleanup v2 Review of attachment 8558206 [details] [diff] [review]: ----------------------------------------------------------------- ::: chat/protocols/xmpp/xmpp.jsm @@ +863,5 @@ > + if (aStanza.attributes["from"] == this._jid.domain) { > + this.sendStanza(Stanza.iq("result", aStanza.attributes["id"], > + this._jid.domain)); > + } > + return; This return is gratuitous since this is the last statement, but I assume it is here just to match the other sections above? @@ +1180,5 @@ > }, > > /* Public methods */ > + > + sendStanza(aStanza, aCallback, aThis) { I'm forgetting the exact syntax (something with a ...?), but do we want to just forward all arguments here so we never have to update this or do we not care? @@ +1441,5 @@ > > // Send the vCard only if it has really changed. > + if (this._userVCard.getXML() != existingVCard) { > + this.sendStanza(Stanza.iq("set", null, null, this._userVCard), > + aStanza => aStanza.attributes.type == "result"); Do we want to add a comment here saying that this is here just to avoid the warning from the next patch?

Comment on attachment 8558207 [details] [diff] [review] Part 3: Error handling v2 Review of attachment 8558207 [details] [diff] [review]: ----------------------------------------------------------------- I have a few questions on this one before I decide whether to r+/r- this. :) ::: chat/protocols/xmpp/xmpp.jsm @@ +285,5 @@ > + aMsg = _("conversation.error.remoteServerNotFound"); > + else { > + // If we don't recognize the defined condition, try to use > + // the supplied descriptive error message if available. > + aMsg = error.text || _("conversation.error.unknownError"); Don't we generally never do this because it might not be localized? @@ +826,5 @@ > + parseError(aStanza) { > + if (aStanza.attributes["type"] != "error") > + return undefined; > + > + let retval = { stanza: aStanza }; Nit: No spaces around {}. @@ +829,5 @@ > + > + let retval = { stanza: aStanza }; > + let error = aStanza.getElement(["error"]); > + > + // rfc6120#section-8.3.2: Type must be one of Nit: Weird formatting here that doesn't match other places we refer to RFCs. @@ +838,5 @@ > + // wait -- retry after waiting (the error is temporary) > + retval.type = error.attributes["type"]; > + > + // rfc6120#section-8.3.3. > + const definedConditions = [ Nit: kDefinedConditions. @@ +882,5 @@ > + // Returns an error-handling callback for use with sendStanza. > + // If the stanza is an error stanza, and aHandlers contains a method > + // with the name of the defined condition of the error, that method > + // is called. It should return true if the error was handled. > + handleErrors(aHandlers, aThis) { I assume this is used in bug 1018060? I think this looks OK, but it's hard without knowing how it is used. @@ +907,5 @@ > + // optional plain-text description. > + sendErrorStanza(aStanza, aCondition, aType, aText) { > + // TODO: Support the other stanza types (message, presence). > + if (aStanza.qName != "iq") { > + this.ERROR("Trying to send an error stanza that's not supported yet."); Include what the qName is here. @@ +911,5 @@ > + this.ERROR("Trying to send an error stanza that's not supported yet."); > + return; > + } > + > + let error = Stanza.node("error", null, { type: aType }, Nit: No spaces around {}.

Nits fixed

> This return is gratuitous since this is the last statement, but I assume it > is here just to match the other sections above? Yes, and I think in later patches stuff gets added below. > I'm forgetting the exact syntax (something with a ...?), but do we want to > just forward all arguments here so we never have to update this or do we not > care? I'd prefer to keep it explicit as that acts as in-code documentation (the function being forwarded is in another file).

> Don't we generally never do this because it might not be localized? It was in the existing code, but good catch.

Comment on attachment 8559168 [details] [diff] [review] Part 1: Stanza handling v2 Review of attachment 8559168 [details] [diff] [review]: ----------------------------------------------------------------- ::: chat/protocols/xmpp/xmpp-session.jsm @@ +104,5 @@ > * Can set a callback if required, which will be called > * when the server responds to the stanza with > + * a stanza of the same id. The callback should return > + * true if the stanza was handled, false if not. > + * Note that an undefined return value is treated as true. This looks...oddly formatted. If you feel like being proactive, please reflow this before check-in. ::: chat/protocols/xmpp/xmpp.jsm @@ +643,5 @@ > > + /* Generate unique id for a stanza. Using id and unique sid is defined in > + * RFC 6120 (Section 8.2.3, 4.7.3). > + */ > + generateId: function() UuidGenerator.generateUUID().toString().slice(1, -1), We could check if there's already an ID with this in _handlers, but that's unlikely.

https://hg.mozilla.org/comm-central/rev/d5332956ab4e https://hg.mozilla.org/comm-central/rev/d08b59fa1fa1 https://hg.mozilla.org/comm-central/rev/803ea60c1fd0