1146154 - Unhandled presence stanza after closing MUC

Status: RESOLVED FIXED

(Chat Core :: XMPP, defect)

Description

[aleth [:aleth]]

STR
Close a MUC, this sends a presence stanza to the server.
The server responds, and we don't handle this response as the MUC no longer exists ("Received presence stanza for unknown buddy").

Comment 1

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 1 - handle response of server closing muc

Comment 2

[aleth [:aleth]]

Comment on attachment 8584904 [details] [diff] [review]
rev 1 - handle response of server closing muc

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

::: chat/protocols/xmpp/xmpp.jsm
@@ +260,5 @@
> +        if (!x)
> +          return false;
> +        let codes = x.getElements(["status"]).map(elt => elt.attributes["code"]);
> +        if (codes.indexOf("110") == -1)
> +          return false;

I don't think you should return false in any of these cases. If the server responds to our stanza (as shown by the id), but not in a way given by the spec, we should this.WARN("Received unexpected server response on leaving a MUC") and return true, as we *have* handled that stanza as well as we can at the moment: since we don't know what such an unexpected response would look like, there is nothing more we can do here.

If we see any examples of this warning popping up in the future, we may be able to do better then.

Comment 3

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 2 - handle response of server closing muc

Comment 4

[aleth [:aleth]]

Comment on attachment 8585101 [details] [diff] [review]
rev 2 - handle response of server closing muc

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

::: chat/protocols/xmpp/xmpp.jsm
@@ +267,5 @@
> +          return true;
> +        }
> +        let codes = x.getElements(["status"]).map(elt => elt.attributes["code"]);
> +        if (codes.indexOf("110") == -1) {
> +          this.WARN(message + "code 110 does not exist in status element.");

Since I think we can do without the extra detail in the warning message in this case, how about shortening it to

if (aStanza.attributes["type"] == "unavailable")) {
  try {
    let x = aStanza.getElements(["x"]).find(e => e.uri == Stanza.NS.muc_user);
    let codes = x.getElements(["status"]).map(elt => elt.attributes["code"]);
    if (codes.indexOf("110") != -1)
      return true;
  } catch(e) {}
}
this.WARN("Received unexpected server response on leaving a MUC.");
return true;

@@ +271,5 @@
> +          this.WARN(message + "code 110 does not exist in status element.");
> +          return true;
> +        }
> +
> +        GenericConvChatPrototype.close.call(this);

Please move this back - closing a conversation is a synchronous API call, we have to do it immediately.

Comment 5

[Abdelrahman Ahmed [:abdelrahman]]

(In reply to aleth [:aleth] from comment #4)
> Please move this back - closing a conversation is a synchronous API call, we
> have to do it immediately.

Are you sure that we can use this.WARN in callback after removing this?

Comment 6

[aleth [:aleth]]

(In reply to Abdelrhman Ahmed [:abdelrhman] from comment #5)
> (In reply to aleth [:aleth] from comment #4)
> > Please move this back - closing a conversation is a synchronous API call, we
> > have to do it immediately.
> 
> Are you sure that we can use this.WARN in callback after removing this?

Good question. I think it would work, because arrow functions have lexical scope, but it does mean we keep a reference to the MUC object around, which we don't want. So it's better to do let account = this._account before the callback and then use account.WARN inside the callback.

Comment 7

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 3 - handle response of server closing muc

Comment 8

[aleth [:aleth]]

Comment on attachment 8586238 [details] [diff] [review]
rev 3 - handle response of server closing muc

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

Thanks!

Comment 9

[aleth [:aleth]]

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