Unhandled presence stanza after closing MUC

RESOLVED FIXED in 1.6

Status

defect
--
minor
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: aleth, Assigned: abdelrahman)

Tracking

trunk
x86
macOS

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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").
Assignee: nobody → a.ahmed1026
Status: NEW → ASSIGNED
Attachment #8584904 - Flags: review?(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.
Attachment #8584904 - Flags: review?(aleth) → review-
Attachment #8584904 - Attachment is obsolete: true
Attachment #8585101 - Flags: review?(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.
Attachment #8585101 - Flags: review?(aleth) → review-
(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?
(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.
Attachment #8585101 - Attachment is obsolete: true
Attachment #8586238 - Flags: review?(aleth)
Comment on attachment 8586238 [details] [diff] [review]
rev 3 - handle response of server closing muc

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

Thanks!
Attachment #8586238 - Flags: review?(aleth) → review+
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.6
You need to log in before you can comment on or make changes to this bug.