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

RESOLVED FIXED in Instantbird 50

Status

defect
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: aleth, Assigned: aleth)

Tracking

trunk
Instantbird 50

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(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");
(Assignee)

Updated

5 years ago
QA Contact: aleth
(Assignee)

Updated

5 years ago
Assignee: nobody → aleth
(Assignee)

Updated

3 years ago
Assignee: aleth → nobody
(Assignee)

Comment 2

3 years ago
Looks like this was assigned to me because I still had a patch for it in my queue.
Attachment #8757353 - Flags: review?(florian)
(Assignee)

Updated

3 years ago
Assignee: nobody → aleth
Status: NEW → ASSIGNED
(Assignee)

Comment 3

3 years ago
Reformatted to make better use of 80 chars...
Attachment #8757406 - Flags: review?(florian)
(Assignee)

Updated

3 years ago
Attachment #8757353 - Attachment is obsolete: true
Attachment #8757353 - Flags: review?(florian)
(Assignee)

Updated

3 years ago
Attachment #8757406 - Flags: review?(florian) → review?(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);

Can this contain non-ascii characters?
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
Attachment #8757406 - Flags: review?(clokep) → review-
(Assignee)

Comment 6

3 years ago
Good catch! Unicode is probably allowed, but unlikely. (I also tweaked the comment, hopefully an improvement)
Attachment #8764614 - Flags: review?(clokep)
(Assignee)

Updated

3 years ago
Attachment #8757406 - Attachment is obsolete: true
(Assignee)

Comment 7

3 years ago
Fixed incorrect use of nsISupportsString
Attachment #8764621 - Flags: review?(clokep)
(Assignee)

Updated

3 years ago
Attachment #8764614 - Attachment is obsolete: true
Attachment #8764614 - Flags: review?(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 [.
Attachment #8764621 - Flags: review?(clokep) → review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
aleth, I think you need to upload a new patch.
Keywords: checkin-needed
(Assignee)

Updated

3 years ago
Attachment #8764621 - Attachment is obsolete: true
(Assignee)

Comment 11

3 years ago
(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.
(Assignee)

Comment 12

3 years ago
(In reply to Patrick Cloke [:clokep] from comment #9)
> aleth, I think you need to upload a new patch.
bzexport fail ;)
Thanks! https://hg.mozilla.org/comm-central/rev/e98bbf436ca8
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Instantbird 50
You need to log in before you can comment on or make changes to this bug.