974438 - JS-XMPP fails to set the resource correctly for XMPP accounts created with libpurple

Status: RESOLVED FIXED

(Chat Core :: XMPP, defect)

Description

[aleth [:aleth]]

This won't be an issue until we switch over to JS-XMPP, but there are (minor) issues when connecting already existing XMPP accounts after removing the override pref. For example, the resource in libpurple is part of the account name, and doesn't get set correctly when connected with JS-XMPP.

It might be worth checking other advanced prefs (e.g. encryption) migrate correctly.

Comment 1

[Florian Quèze [:florian]]

(In reply to aleth from comment #0)
> the resource in libpurple is part of the
> account name, and doesn't get set correctly when connected with JS-XMPP.

This is handled at http://mxr.mozilla.org/comm-central/source/chat/protocols/xmpp/xmpp.jsm#693
695     this._jid = this._parseJID(this.name);
696 
697     // For the resource, if the user has edited the option to a non
698     // empty value, use that.
699     if (this.prefs.prefHasUserValue("resource")) {
700       let resource = this.getString("resource");
701       if (resource)
702         this._jid.resource = resource;
703     }
704     // Otherwise, if the username doesn't contain a resource, use the
705     // value of the resource option (it will be the default value).
706     // If we set an empty resource, XMPPSession will fallback to
707     // XMPPDefaultResource (set to brandShortName).
708     if (!this._jid.resource)
709       this._jid.resource = this.getString("resource");

Comment 2

[aleth [:aleth]]

Attachment: Set resource pref for migrated libpurple XMPP accounts

Looks like this was assigned to me because I still had a patch for it in my queue.

Comment 3

[aleth [:aleth]]

Attachment: Set resource pref for migrated libpurple XMPP accounts

Reformatted to make better use of 80 chars...

Comment 4

[Florian Quèze [:florian]]

Comment on attachment 8757406 [details] [diff] [review]
Set resource pref for migrated libpurple XMPP accounts

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

::: chat/protocols/xmpp/xmpp.jsm
@@ +1193,3 @@
>        this._jid.resource = this.getString("resource");
> +    else if (this._jid.resource)
> +      this.prefs.setCharPref("resource", this._jid.resource);

Can this contain non-ascii characters?

Comment 5

[Patrick Cloke [:clokep]]

Comment on attachment 8757406 [details] [diff] [review]
Set resource pref for migrated libpurple XMPP accounts

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

::: chat/protocols/xmpp/xmpp.jsm
@@ +1193,3 @@
>        this._jid.resource = this.getString("resource");
> +    else if (this._jid.resource)
> +      this.prefs.setCharPref("resource", this._jid.resource);

getString uses getComplexValue, so we definitely want to set it using the same type of mechanism: https://dxr.mozilla.org/comm-central/source/chat/modules/jsProtoHelper.jsm#220

Comment 6

[aleth [:aleth]]

Attachment: Set resource pref for migrated libpurple XMPP accounts

Good catch! Unicode is probably allowed, but unlikely. (I also tweaked the comment, hopefully an improvement)

Comment 7

[aleth [:aleth]]

Attachment: Set resource pref for migrated libpurple XMPP accounts

Fixed incorrect use of nsISupportsString

Comment 8

[Patrick Cloke [:clokep]]

Comment on attachment 8764621 [details] [diff] [review]
Set resource pref for migrated libpurple XMPP accounts

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

r+ assuming my understanding is fixed and the nits are fixed.

::: chat/protocols/xmpp/xmpp.jsm
@@ +1183,5 @@
>      this._jid = this._parseJID(this.name);
>  
> +    // For the resource, if the user has edited the option, always use that.
> +    // If that would set an empty resource, XMPPSession will fallback to
> +    // XMPPDefaultResource (set to brandShortName).

To make sure I understand, at the end of this block of code, it's OK if this._jid.resource is False-y because it gets set to brandShortname later on?

@@ +1188,5 @@
> +    // If no resource is set by the user, the pref getter will return a
> +    // default value.
> +    if (this.prefs.prefHasUserValue("resource") || !this._jid.resource)
> +      this._jid.resource = this.getString("resource");
> +    else if (this._jid.resource) {

Isn't this just an else?

@@ +1193,5 @@
> +      // If there is a resource in the account name (inherited from libpurple),
> +      // migrate it to the pref so it appears correctly in the advanced account
> +      // options next time.
> +      let str = Cc["@mozilla.org/supports-string;1"]
> +                .createInstance(Ci.nsISupportsString);

Nit: Line up the. with the [.

Comment 9

[Patrick Cloke [:clokep]]

aleth, I think you need to upload a new patch.

Comment 10

[aleth [:aleth]]

Attachment: Set resource pref for migrated libpurple XMPP accounts

Nit fixed.

Comment 11

[aleth [:aleth]]

(In reply to Patrick Cloke [:clokep] from comment #8)

> > +    // For the resource, if the user has edited the option, always use that.
> > +    // If that would set an empty resource, XMPPSession will fallback to
> > +    // XMPPDefaultResource (set to brandShortName).
> 
> To make sure I understand, at the end of this block of code, it's OK if
> this._jid.resource is False-y because it gets set to brandShortname later on?

Yes.

> @@ +1188,5 @@
> > +    // If no resource is set by the user, the pref getter will return a
> > +    // default value.
> > +    if (this.prefs.prefHasUserValue("resource") || !this._jid.resource)
> > +      this._jid.resource = this.getString("resource");
> > +    else if (this._jid.resource) {
> 
> Isn't this just an else?

I preferred to keep it explicit as it's confusing enough as it is.

Comment 12

[aleth [:aleth]]

(In reply to Patrick Cloke [:clokep] from comment #9)
> aleth, I think you need to upload a new patch.
bzexport fail ;)

Comment 13

[Patrick Cloke [:clokep]]

Thanks! https://hg.mozilla.org/comm-central/rev/e98bbf436ca8