1290373 - Add tests for XMPPParser

Status: RESOLVED FIXED

(Chat Core :: XMPP, defect)

Comment 1

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: v1 - add tests for XMPPParser

Comment 2

[aleth [:aleth]]

Comment on attachment 8775958 [details] [diff] [review]
v1 - add tests for XMPPParser

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

::: chat/protocols/xmpp/xmpp-xml.jsm
@@ -276,5 @@
> -    if (!TOP_LEVEL_ELEMENTS.hasOwnProperty(this.qName))
> -      return false;
> -    let ns = TOP_LEVEL_ELEMENTS[this.qName];
> -    return ns == this.uri || (Array.isArray(ns) && ns.indexOf(this.uri) != -1);
> -  },

Why are you removing this check? It's important to filter out potential XML nodes that are invalid in XMPP.

@@ +401,5 @@
>                                  "No parent for node : " + aLocalName);
>        return;
>      }
>  
> +    if (--this._depth == 0) {

Aren't all stanzas child nodes of a stream element?

Is the depth value of the XMPP stanzas always the same?

Comment 3

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: v2 - add tests for XMPPParser

(In reply to aleth [:aleth] from comment #2)
> Why are you removing this check? It's important to filter out potential XML
> nodes that are invalid in XMPP.

Sorry for that, you are right.

> Aren't all stanzas child nodes of a stream element?

No, RFC 6120 section 4.1 and Figure 2

> An XML stanza is the basic unit of meaning
> in XMPP. A stanza is a first-level element (at depth=1 of the
> stream) whose element name is "message", "presence", or "iq" and
> whose qualifying namespace is ’jabber:client’ or ’jabber:server’.

> Is the depth value of the XMPP stanzas always the same?

The depth value is changing according to levels and must be 0 at the end of parsing of stanza, otherwise the stanza will have missing closing tag for some element.

An example
> <message xmlns="jabber:client" from="SENDER" to="RECEIVER" type="chat">
> 	<body>message text...</body>
> </message>

Steps of how the parser works:
<message>
startElement: depth is 1 (first time)

<body>
startElement: depth is 2 (increment)

</body>
endElement: depth is 1 (decrement)

</message>
endElement: depth is 0 (decrement)

Comment 4

[aleth [:aleth]]

Comment on attachment 8776105 [details] [diff] [review]
v2 - add tests for XMPPParser

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

::: chat/protocols/xmpp/xmpp-xml.jsm
@@ +361,5 @@
>    startDocument: function() { },
>    endDocument: function() { },
>  
> +  // Indicates the current depth while parsing received stanza element by
> +  // element.

This is not clear. You mean the depth relative to the <stream> element?

@@ +362,5 @@
>    endDocument: function() { },
>  
> +  // Indicates the current depth while parsing received stanza element by
> +  // element.
> +  _depth: 0,

Also initialise in the constructor.

@@ +391,5 @@
>        this._node.addChild(node);
> +      this._depth++;
> +    }
> +    else
> +      this._depth = 1;

This needs comments. When is this._node not set and how does it relate to _depth.

Why is this initialization of this._depth not in the constructor?

Are there other methods in the parser that should reset/change _depth?

@@ +422,5 @@
>                                  "No parent for node : " + aLocalName);
>        return;
>      }
>  
> +    if (--this._depth == 0 && this._node.isXmppStanza()) {

I doubt this is the right place to decrease _depth. What happens if we hit one of the early returns?

Add a comment saying what is being checked here. Maybe a comment at isXmppStanza describing it would be useful for the future too. 

Do you even need _depth as an integer, or would the following be enough:
if (!this._node._parentNode && this._node.isXmppStanza())

Comment 5

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: v3 - add tests for XMPPParser

(In reply to aleth [:aleth] from comment #4)
> Do you even need _depth as an integer, or would the following be enough:
> if (!this._node._parentNode && this._node.isXmppStanza())

Yes, this is enough (I didn't really notice that). The need for _depth is no longer needed.

Comment 6

[aleth [:aleth]]

Comment on attachment 8776421 [details] [diff] [review]
v3 - add tests for XMPPParser

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

Ah, that's much simpler. r+ with minor improvements.

::: chat/protocols/xmpp/test/test_xmppParser.js
@@ +63,5 @@
> +bescreen"d in night, so stumblest on my counsel?</body>\
> +</message>',
> +    isError: true,
> +    description: "No closing of body tag"
> +  },

You could add an invalid top-level element (not a valid stanza)

::: chat/protocols/xmpp/xmpp-xml.jsm
@@ +414,5 @@
>                                  "No parent for node : " + aLocalName);
>        return;
>      }
>  
> +    // Checks if the node is the root and it's valid.

Maybe add the RFC section for future reference.

@@ +415,5 @@
>        return;
>      }
>  
> +    // Checks if the node is the root and it's valid.
> +    if (!this._node._parentNode && this._node.isXmppStanza()) {

If there's no parent node and it's not an XmppStanza, you could WARN.

Comment 7

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: v4 - add tests for XMPPParser

Comment 8

[Abdelrahman Ahmed [:abdelrahman]]

https://hg.mozilla.org/comm-central/rev/f48a5c3d3e84