Closed Bug 955678 Opened 6 years ago Closed 5 years ago

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

Categories

(Chat Core :: IRC, defect)

x86
Other
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: florian, Assigned: clokep)

Details

Attachments

(2 files, 3 obsolete files)

*** 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
*** 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.
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".)
Flags: needinfo?(aleth)
Summary: str is undefined in irc.js:839 → message.servername not set when hostname does not have . or : in it (e.g. localhost) [str is undefined in irc.js:839]
We discussed this idea on IRC, I also think removing the nickname/servername differentiation when parsing should work for this.
Flags: needinfo?(aleth)
I intend to take a look at this soon (/ I think I have a partial patch somewhere...)
Assignee: nobody → clokep
Status: NEW → ASSIGNED
Attached patch Patch v1 (obsolete) — Splinter Review
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.
Attachment #8507634 - Flags: feedback?(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?
Attachment #8507634 - Flags: feedback?(aleth) → feedback+
(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!
(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.
Attached patch Patch v2 (obsolete) — Splinter Review
I still want to bang on this more before checking it in, but I think it's ready for a review.
Attachment #8507634 - Attachment is obsolete: true
Attachment #8509404 - Flags: review?(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?
(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 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 ;)
Attachment #8509404 - Flags: review?(aleth) → review-
Attached patch Patch v3 (obsolete) — Splinter Review
I think this is ready for review now. I did some testing already, but this is a pretty invasive change and probably requires more.
Attachment #8509404 - Attachment is obsolete: true
Attachment #8511026 - Flags: review?(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.
Attachment #8511026 - Flags: review?(aleth) → review-
Attached patch Patch v4Splinter Review
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.
Attachment #8511026 - Attachment is obsolete: true
Attachment #8511085 - Flags: review?(aleth)
Comment on attachment 8511085 [details] [diff] [review]
Patch v4

Thanks!
Attachment #8511085 - Flags: review?(aleth) → review+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/0ac236b0be38
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 1.6
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.
Attachment #8514219 - Flags: review?(aleth)
Attachment #8514219 - Flags: review?(aleth) → review+
Keywords: checkin-needed
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.