955678 - message.servername not set when hostname does not have . or : in it (e.g. localhost) [str is undefined in irc.js:839]

Status: RESOLVED FIXED

(Chat Core :: IRC, defect)

Description

[Florian Quèze [:florian]]

*** Original post on bio 2230 at 2013-10-24 16:04:00 UTC ***

Error: Error running command 001 with handler RFC 2812:
{"rawMessage":":localhost 001 alexis :Welcome to the BitlBee gateway, alexis","command":"001","params":["alexis","Welcome to the BitlBee gateway, alexis"],"nickname":"localhost","user":null,"host":null,"source":null}
str is undefined
Source File: resource://gre/components/irc.js
Line: 839

Comment 1

[Patrick Cloke [:clokep]]

*** Original post on bio 2230 at 2013-10-24 16:07:08 UTC ***

My first guess for this is that the nickname being set to localhost (and source being null) is messing things up, the logic at http://lxr.instantbird.org/instantbird/source/chat/protocols/irc/irc.js#66 is broken specifically in this case.

Comment 2

[Patrick Cloke [:clokep]]

I've confirmed that this is the issue, we actually have a commented out test case about this. aleth, do you have any ideas of what we could do for this? (Note that it seems we use whether servername is set in some cases to tell if a message is "from the server".)

Comment 3

[aleth [:aleth]]

We discussed this idea on IRC, I also think removing the nickname/servername differentiation when parsing should work for this.

Comment 4

[Patrick Cloke [:clokep]]

I intend to take a look at this soon (/ I think I have a partial patch somewhere...)

Comment 5

[Patrick Cloke [:clokep]]

Attachment: Patch v1

I ran the tests on this and everything seemed to pass, but I have not yet vetted this beyond that. I'm requesting some feedback about how this looks in general before continuing on, however.

This changes the parsing function of IRC messages to more often generate the "servername" field of messages and greatly simplifies the parsing of the prefix. I tested to ensure that the characters @ and ! are not valid in nicknames or usernames, so splitting on those is OK. During this testing I also fixed the error you get when using invalid usernames:
> Warning: Unhandled IRC message:
> :levin.mozilla.org 461 clokep_js USER :Your username is not valid
> Source File: file:///c:/Users/clokep/comm-central/obj-i686-pc-mingw32/dist/bin/components/irc.js Line: 693

Requesting feedback until I can do more extensive testing.

Comment 6

[aleth [:aleth]]

Comment on attachment 8507634 [details] [diff] [review]
Patch v1

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

This looks like a nice simplification!

How about, additionally, going through all the occurences of message.servername and getting rid of the message.nickname || message.servername pattern we have all over the place while you are at it. Also, in most cases where it is actually needed you can probably replace message.servername with this._currentServerName. This might clean up the code further.

::: chat/protocols/irc/irc.js
@@ +65,5 @@
>    if (temp[4] != undefined)
>      message.params.push(temp[4]);
>  
> +  // The prefix can be split into multiple parts as:
> +  //   :(servername|nickname[[!user]@host])

:(servername|(nickname[[!user]@host])) to be clear.

@@ +87,1 @@
>    }

I really like these changes.

@@ +819,5 @@
>    // The nickname that was last requested. This can differ from
>    // _requestedNickname when a new nick is automatically generated (e.g. by
>    // adding digits).
>    _sentNickname: null,
> +  get user() {

Why did you call this user and not username?

Comment 7

[Patrick Cloke [:clokep]]

(In reply to aleth [:aleth] from comment #6)
> How about, additionally, going through all the occurences of
> message.servername and getting rid of the message.nickname ||
> message.servername pattern we have all over the place while you are at it.

I thought about doing this but wanted to keep the patch as small as possible. If we're going to do that...what would I replace it by? If servername is set, nickname is always identical.

> Also, in most cases where it is actually needed you can probably replace
> message.servername with this._currentServerName. This might clean up the
> code further.

No, it purposefully pulls the name from the *current* message. And in many of those places it is ambiguous whether the message is from a server or another user.

> ::: chat/protocols/irc/irc.js
> @@ +819,5 @@
> >    // The nickname that was last requested. This can differ from
> >    // _requestedNickname when a new nick is automatically generated (e.g. by
> >    // adding digits).
> >    _sentNickname: null,
> > +  get user() {
> 
> Why did you call this user and not username?

No particular reason, I'll change it. Thanks for taking a look!

Comment 8

[aleth [:aleth]]

(In reply to Patrick Cloke [:clokep] from comment #7)
> (In reply to aleth [:aleth] from comment #6)
> > How about, additionally, going through all the occurences of
> > message.servername and getting rid of the message.nickname ||
> > message.servername pattern we have all over the place while you are at it.
> 
> I thought about doing this but wanted to keep the patch as small as
> possible. If we're going to do that...what would I replace it by? If
> servername is set, nickname is always identical.

The alternative would be to just use message.nickname.

> > Also, in most cases where it is actually needed you can probably replace
> > message.servername with this._currentServerName. This might clean up the
> > code further.
> 
> No, it purposefully pulls the name from the *current* message. And in many
> of those places it is ambiguous whether the message is from a server or
> another user.

But is it possible that message.servername != this._currentServerName? i.e. can we receive messages from other servers (due to syndication or whatever) without that being a bug? That's what I wanted to discuss.

If it's not possible, we have the option of getting rid of message.servername altogether. Not sure it would be a good idea though.

Comment 9

[Patrick Cloke [:clokep]]

Attachment: Patch v2

I still want to bang on this more before checking it in, but I think it's ready for a review.

Comment 10

[aleth [:aleth]]

(In reply to aleth [:aleth] from comment #8)
> But is it possible that message.servername != this._currentServerName? i.e.
> can we receive messages from other servers (due to syndication or whatever)
> without that being a bug?

Out of interest, do you know the answer to this?

Comment 11

[Patrick Cloke [:clokep]]

(In reply to aleth [:aleth] from comment #10)
> (In reply to aleth [:aleth] from comment #8)
> > But is it possible that message.servername != this._currentServerName? i.e.
> > can we receive messages from other servers (due to syndication or whatever)
> > without that being a bug?
> 
> Out of interest, do you know the answer to this?

I'm fairly certain that another server could send us a message if they want. I'm not sure *why* it would do that, however.

Interestingly, received messages without any prefix are supposed to be treated as if they're from the current server.

Comment 12

[aleth [:aleth]]

Comment on attachment 8509404 [details] [diff] [review]
Patch v2

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

::: chat/protocols/irc/irc.js
@@ +85,1 @@
>    }

Do we want an |else message.nickname = this._currentServerName;| here?

::: chat/protocols/irc/ircBase.jsm
@@ +351,5 @@
>        // Welcome to the Internet Relay Network <nick>!<user>@<host>
>        this._socket.resetPingTimer();
> +      // This seems a little strange, but we don't differentiate between a
> +      // nickname and the servername since it can be ambiguous.
> +      this._currentServerName = aMessage.nickname;

Would it help to do s/message.nickname/message.name (in a separate patch)? I don't have a strong opinion about this.

::: chat/protocols/irc/ircServices.jsm
@@ +160,5 @@
>        }
>        else if (text == "*** \u0002End of Message(s) of the Day\u0002 ***") {
>          if (this._showServerTab && this._infoServMotd) {
>            this._infoServMotd.push(text);
> +          this.getConversation(aMessage.nickname || this._currentServerName)

See my comment in irc.js.

::: chat/protocols/irc/test/test_ircMessage.js
@@ +136,5 @@
>  
>      // Let's do a little dance here...we don't rebuild the "source" of the
>      // message (the server does that), so when comparing our output message, we
>      // need to avoid comparing to that part.
> +    if (message.servername || message.nickname) {

Tests need updating ;)

Comment 13

[Patrick Cloke [:clokep]]

Attachment: Patch v3

I think this is ready for review now. I did some testing already, but this is a pretty invasive change and probably requires more.

Comment 14

[aleth [:aleth]]

Comment on attachment 8511026 [details] [diff] [review]
Patch v3

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

Looking good already.

::: chat/protocols/irc/irc.js
@@ +32,5 @@
>   *   host       The user's hostname, note that this can be undefined.
>   *   source     A "nicely" formatted combination of user & host, which is
>   *              <user>@<host> or <user> if host is undefined.
> + *
> + * There are cases (e.g. localhost) where it cannot be easily calculate if a

typo: calculated (or determined?)

@@ +689,5 @@
>        function(aStr) lowDequote[aStr[1]] || aStr[1]);
>  
>      try {
> +      let message = new ircMessage(dequotedMessage,
> +                                   this._account._currentServerName);

Better change the currentServername default in the proto to "" (or possibly "unknown server" or something like that) instead of null now.

@@ +790,5 @@
>    let splitter = this.name.lastIndexOf("@");
>    this._accountNickname = this.name.slice(0, splitter);
>    this._server = this.name.slice(splitter + 1);
> +  // To avoid _currentServerName being null, initialize it to the server being
> +  // connected to, this should get replaced once connected.

and if you agree, update the comment ;)

::: chat/protocols/irc/ircBase.jsm
@@ +270,5 @@
>      "NOTICE": function(aMessage) {
>        // NOTICE <msgtarget> <text>
>        // If the message doesn't have a nickname, it's from the server, don't
>        // show it unless the user wants to see it.
>        if (!aMessage.hasOwnProperty("nickname"))

This needs to be changed. Please do a search for "\"nickname\"" too.

Comment 15

[Patrick Cloke [:clokep]]

Attachment: Patch v4

Thanks for the quick review. I searched over "servername" and "nickname" in this patch. I also expanded the comment above setting _currentServerName to _server in the account constructor and fixed the comment you identified.

Comment 16

[aleth [:aleth]]

Comment on attachment 8511085 [details] [diff] [review]
Patch v4

Thanks!

Comment 17

[Patrick Cloke [:clokep]]

https://hg.mozilla.org/comm-central/rev/0ac236b0be38

Comment 18

[Patrick Cloke [:clokep]]

Attachment: Follow-up for sign-on messages v1

This follow-up addresses an issue where we check in a couple of places if aMessage has the field "origin", this used to check if "nickname" (vs. "servername") existed, but "origin" always exists. This reworks a bit of the logic here.

Comment 19

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/62292be63178