Closed Bug 955379 Opened 7 years ago Closed 7 years ago

Some characters lost when splitting messages

Categories

(Chat Core :: IRC, defect)

x86
Other
defect
Not set
major

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: florian, Assigned: aleth)

Details

(Whiteboard: [1.4-wanted])

Attachments

(2 files, 3 obsolete files)

*** Original post on bio 1941 at 2013-04-19 16:24:00 UTC ***

It happened at http://log.bezut.info/instantbird/130419#m294

I tried to reproduce, and could reproduce, although in my case only one character got lost.

I joined #testib with both the flo-retina and the flo2 nicks. I sent qlum's long message from flo-retina, and looked at what flo2 received.



Debug log, flo-retina:
[19/04/13 18:13:56] LOG   (@ prpl-irc: Socket.sendString resource:///modules/socket.jsm:242)
Sending:
PRIVMSG #testib :A few days back I talked about a problem I was having with my account disconnecting and then reconnecting quite a lot, this is still the case however now I know its not just the nightly builds as I tried reverting to the stable build and recreated the irc accounts the problem presisted. This was consistent with me switching isp and router. I don't have any connection errors elsewhere and as long as people keep talking it doesn't disconnect. I

[19/04/13 18:13:56] LOG   (@ prpl-irc: Socket.sendString resource:///modules/socket.jsm:242)
Sending:
PRIVMSG #testib :don't know if this information is useful but I thought to share it anyway.



Debug log, flo2:
[19/04/13 18:13:57] DEBUG (@ prpl-irc: ircSocket.prototype.onDataReceived jar:file:///Applications/Instantbird%20Nightly.app/Contents/MacOS/omni.ja!/components/irc.js:621)
:flo-retina!Instantbir@moz-87C33FDA.kimsufi.com PRIVMSG #testib :A few days back I talked about a problem I was having with my account disconnecting and then reconnecting quite a lot, this is still the case however now I know its not just the nightly builds as I tried reverting to the stable build and recreated the irc accounts the problem presisted. This was consistent with me switching isp and router. I don't have any connection errors elsewhere and as long as people keep talking it doesn't disconnect. 
[19/04/13 18:13:57] DEBUG (@ prpl-irc: ircHandlers._handleMessage resource:///modules/ircHandlers.jsm:104)
{"rawMessage":":flo-retina!Instantbir@moz-87C33FDA.kimsufi.com PRIVMSG #testib :A few days back I talked about a problem I was having with my account disconnecting and then reconnecting quite a lot, this is still the case however now I know its not just the nightly builds as I tried reverting to the stable build and recreated the irc accounts the problem presisted. This was consistent with me switching isp and router. I don't have any connection errors elsewhere and as long as people keep talking it doesn't disconnect. ","command":"PRIVMSG","params":["#testib","A few days back I talked about a problem I was having with my account disconnecting and then reconnecting quite a lot, this is still the case however now I know its not just the nightly builds as I tried reverting to the stable build and recreated the irc accounts the problem presisted. This was consistent with me switching isp and router. I don't have any connection errors elsewhere and as long as people keep talking it doesn't disconnect. "],"nickname":"flo-retina","user":"Instantbir","host":"moz-87C33FDA.kimsufi.com","source":"Instantbir@moz-87C33FDA.kimsufi.com"}
[19/04/13 18:13:57] DEBUG (@ prpl-irc: Socket.onTransportStatus resource:///modules/socket.jsm:457)
onTransportStatus(STATUS_RECEIVING_FROM)
[19/04/13 18:13:57] DEBUG (@ prpl-irc: ircSocket.prototype.onDataReceived jar:file:///Applications/Instantbird%20Nightly.app/Contents/MacOS/omni.ja!/components/irc.js:621)
:flo-retina!Instantbir@moz-87C33FDA.kimsufi.com PRIVMSG #testib :don't know if this information is useful but I thought to share it anyway.
[19/04/13 18:13:57] DEBUG (@ prpl-irc: ircHandlers._handleMessage resource:///modules/ircHandlers.jsm:104)
{"rawMessage":":flo-retina!Instantbir@moz-87C33FDA.kimsufi.com PRIVMSG #testib :don't know if this information is useful but I thought to share it anyway.","command":"PRIVMSG","params":["#testib","don't know if this information is useful but I thought to share it anyway."],"nickname":"flo-retina","user":"Instantbir","host":"moz-87C33FDA.kimsufi.com","source":"Instantbir@moz-87C33FDA.kimsufi.com"}
*** Original post on bio 1941 at 2013-04-19 20:37:14 UTC ***

This is due to two things:

1) We don't include the leading ":" here: http://lxr.instantbird.org/instantbird/source/chat/protocols/irc/irc.js#122

2) The host string used to calculate the max message length is the real host string of the sender, whereas on the receiving end it's the slightly obfuscated "moz-XXXXXXXX.whatever.com". The length of the real host string is often shorter.

1) is trivial, 2) I have no immediate server-independent idea for.
*** Original post on bio 1941 at 2013-04-19 21:08:55 UTC ***

We can solve 2) by using the whois response for our own nick.
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1941 as attmnt 2392 at 2013-04-19 21:41:00 UTC ***

Maybe the reviewer has a better idea ;)
Attachment #8354159 - Flags: review?(clokep)
Assignee: nobody → aleth
Status: NEW → ASSIGNED
*** Original post on bio 1941 at 2013-04-20 11:51:00 UTC ***

Should we "play safe" and only use the host as returned by the whois connectedFrom if it is longer than the previously stored string? I don't expect it's necessary, but I doubt there is a RFC on its behaviour (?).
Whiteboard: [1.4-wanted]
Comment on attachment 8354159 [details] [diff] [review]
Patch

*** Original change on bio 1941 attmnt 2392 at 2013-04-20 12:35:20 UTC ***

>diff --git a/chat/protocols/irc/irc.js b/chat/protocols/irc/irc.js
>@@ -114,17 +114,17 @@ const GenericIRCConversation = {
>   // This will calculate the maximum number of bytes that are left for a message
>   // typed by the user by calculate the amount of bytes that would be used by
>   // the IRC messaging.
>   getMaxMessageLength: function() {
>     // Build the shortest possible message that could be sent to other users.
>-    let baseMessage = this._account._nickname + this._account.prefix +
>+    let baseMessage = ":" + this._account._nickname + this._account.prefix +
>                       " " + this._account.buildMessage("PRIVMSG", this.name) +
>                       " :\r\n";
Good catch!


>diff --git a/chat/protocols/irc/ircBase.jsm b/chat/protocols/irc/ircBase.jsm
>@@ -365,16 +365,19 @@ var ircBase = {
>       // Get our full prefix.
>       this.prefix = aMessage.params[1].slice(
>         aMessage.params[1].lastIndexOf(" ") + 1);
>       // Remove the nick from the prefix.
>       this.prefix = this.prefix.slice(this.prefix.indexOf("!"));
>+      // Request our own whois entry to get the host as seen by others
>+      // to correct this prefix.
>+      this.requestBuddyInfo(this._nickname);
I wonder if we should always just do it this way, the current way parses a free text area of a message.

>     "311": function(aMessage) { // RPL_WHOISUSER
>       // <nick> <user> <host> * :<real name>
>       // <username>@<hostname>
>       let source = aMessage.params[2] + "@" + aMessage.params[3];
>+      if (aMessage.params[1] == this._nickname) {
We need to normalize.

>+        // Some servers obfuscate the host when sending messages. Therefore,
>+        // we correct the account prefix by using the host from this response.
>+        this.prefix = this.prefix.substr(0, this.prefix.indexOf("@") + 1 ) +
Isn't what we want just the source variable a few lines above this with a ! in front of it?
Attachment #8354159 - Flags: review?(clokep) → review-
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1941 as attmnt 2393 at 2013-04-20 13:06:00 UTC ***

(In reply to comment #5)
> >       // Get our full prefix.
> >       this.prefix = aMessage.params[1].slice(
> >         aMessage.params[1].lastIndexOf(" ") + 1);
> >       // Remove the nick from the prefix.
> >       this.prefix = this.prefix.slice(this.prefix.indexOf("!"));
> >+      // Request our own whois entry to get the host as seen by others
> >+      // to correct this prefix.
> >+      this.requestBuddyInfo(this._nickname);
> I wonder if we should always just do it this way, the current way parses a free
> text area of a message.
...
> >+        // Some servers obfuscate the host when sending messages. Therefore,
> >+        // we correct the account prefix by using the host from this response.
> >+        this.prefix = this.prefix.substr(0, this.prefix.indexOf("@") + 1 ) +
> Isn't what we want just the source variable a few lines above this with a ! in
> front of it?

The reason we still need the old method is that for some unknown reason the username (Instantbir) is not returned when requesting the whois for one's own nick. Instead, we just get our nickname. Since I think we don't automatically know what the username is (e.g. whether "Instantbird" (or whatever) got shortened or not) we need to grab it from the prefix as we did before, and only amend the host.
Attachment #8354160 - Flags: review?(clokep)
Comment on attachment 8354159 [details] [diff] [review]
Patch

*** Original change on bio 1941 attmnt 2392 at 2013-04-20 13:06:22 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354159 - Attachment is obsolete: true
Comment on attachment 8354160 [details] [diff] [review]
Patch

*** Original change on bio 1941 attmnt 2393 at 2013-04-20 13:15:51 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354160 - Flags: review?(clokep)
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1941 as attmnt 2394 at 2013-04-20 13:22:00 UTC ***

Turns out I still had a custom username set on that profile, causing the strange behaviour for my own nick.
Attachment #8354161 - Flags: review?(clokep)
Comment on attachment 8354160 [details] [diff] [review]
Patch

*** Original change on bio 1941 attmnt 2393 at 2013-04-20 13:22:12 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354160 - Attachment is obsolete: true
*** Original post on bio 1941 at 2013-04-20 13:30:44 UTC ***

Comment on attachment 8354161 [details] [diff] [review] (bio-attmnt 2394)
Patch

Do we need to store this separately or should we get it from our whois info and prepend the !?

Or maybe make this.prefix a getter?
*** Original post on bio 1941 at 2013-04-20 13:35:27 UTC ***

(In reply to comment #8)
> Do we need to store this separately or should we get it from our whois info and
> prepend the !?
I stored it separately because the whois entry will be (temporarily) deleted whenever the whois info for our own nick is requested again (eg via a tooltip).
Comment on attachment 8354161 [details] [diff] [review]
Patch

*** Original change on bio 1941 attmnt 2394 at 2013-04-20 14:49:30 UTC ***


>@@ -708,19 +705,25 @@ var ircBase = {
>     },
> 
>     /*
>      * WHOIS
>      */
>     "311": function(aMessage) { // RPL_WHOISUSER
>       // <nick> <user> <host> * :<real name>
>       // <username>@<hostname>
>+      let nick = aMessage.params[1];
>       let source = aMessage.params[2] + "@" + aMessage.params[3];
>-      return this.setWhois(aMessage.params[1], {realname: aMessage.params[5],
>-                                                connectedFrom: source});
>+      if (this.normalize(nick) == this.normalize(this._nickname)) {
>+        // Some servers obfuscate the host when sending messages. Therefore,
>+        // we set the account prefix by using the host from this response.
>+        this.prefix = "!" + source;
>+      }
Move this comment above the if statement and kill the {}.
Attachment #8354161 - Flags: review?(clokep) → review-
*** Original post on bio 1941 at 2013-04-20 14:51:37 UTC ***

(In reply to comment #9)
> (In reply to comment #8)
> > Do we need to store this separately or should we get it from our whois info and
> > prepend the !?
> I stored it separately because the whois entry will be (temporarily) deleted
> whenever the whois info for our own nick is requested again (eg via a tooltip).

I'm not super concerned about that, but ok. Add this in a comment and r+, assuming you tested it.
Attached patch PatchSplinter Review
*** Original post on bio 1941 as attmnt 2397 at 2013-04-21 14:16:00 UTC ***

(In reply to comment #10)
> >+      if (this.normalize(nick) == this.normalize(this._nickname)) {
> >+        // Some servers obfuscate the host when sending messages. Therefore,
> >+        // we set the account prefix by using the host from this response.
> >+        this.prefix = "!" + source;
> >+      }
> Move this comment above the if statement and kill the {}.
I don't really like this, but OK.

(In reply to comment #11)
> > I stored it separately because the whois entry will be (temporarily) deleted
> > whenever the whois info for our own nick is requested again (eg via a tooltip).
> I'm not super concerned about that, but ok. Add this in a comment and r+,
> assuming you tested it.
It's not a very likely scenario, but as we can avoid it, we should. Just on general principle ("don't rely on whois info unless you just requested it and got the notification")
Attachment #8354164 - Flags: review+
Comment on attachment 8354161 [details] [diff] [review]
Patch

*** Original change on bio 1941 attmnt 2394 at 2013-04-21 14:16:24 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354161 - Attachment is obsolete: true
Whiteboard: [1.4-wanted] → [1.4-wanted][checkin-needed]
*** Original post on bio 1941 at 2013-04-21 14:28:27 UTC ***

Looks good, thanks for fixing this. :-)
*** Original post on bio 1941 at 2013-04-21 21:37:35 UTC ***

http://hg.instantbird.org/instantbird/rev/11e56669140e

Thanks for fixing this so quickly! :-)
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [1.4-wanted][checkin-needed] → [1.4-wanted]
Target Milestone: --- → 1.4
*** Original post on bio 1941 as attmnt 2404 at 2013-04-23 13:12:00 UTC ***

The test did not incorporate the extra ":" added to the base message in getMaxMessageLength()

Untested test ;)
Attachment #8354171 - Flags: review?(clokep)
Comment on attachment 8354171 [details] [diff] [review]
Patch to fix broken test

*** Original change on bio 1941 attmnt 2404 at 2013-04-23 13:15:53 UTC ***

This looks like it will fix the problem.
Attachment #8354171 - Flags: review?(clokep) → review+
*** Original post on bio 1941 at 2013-04-23 13:30:46 UTC ***

(In reply to comment #15)
> Created attachment 8354171 [details] [diff] [review] (bio-attmnt 2404) [details]
> Patch to fix broken test
> 
> The test did not incorporate the extra ":" added to the base message in
> getMaxMessageLength()
> 
> Untested test ;)

http://hg.instantbird.org/instantbird/rev/1e24898d48bc
You need to log in before you can comment on or make changes to this bug.