Nick truncated if already in use during connection registration

RESOLVED FIXED in 1.6

Status

Chat Core
IRC
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: florian, Assigned: clokep)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

4 years ago
*** Original post on bio 2229 at 2013-10-24 16:02:00 UTC ***

clokep told me to file a bug ;)
(Reporter)

Updated

4 years ago
Summary: Nick flo-retina changed to flo-reti1 if flo-retina was in used → Nick flo-retina changed to flo-reti1 if flo-retina was in use
(Assignee)

Updated

4 years ago
(Assignee)

Comment 1

3 years ago
We fallback to the default maximum length of an IRC nick (8 characters) if we're told a nick is invalid during connection registration (I don't think we're told WHY it is, just that it is). E.g. "flo-retina changed to flo-reti1 if flo-retina was in use".
Summary: Nick flo-retina changed to flo-reti1 if flo-retina was in use → Nick truncated if already in use during connection registration
(Assignee)

Comment 2

3 years ago
Created attachment 8474141 [details] [diff] [review]
Patch v1

This changes from trying to anticipate when we'd send too long of a nick to just sending one and seeing when the nick returned to us is shorter than the nick that we sent.
Assignee: nobody → clokep
Status: NEW → ASSIGNED
Attachment #8474141 - Flags: review?(aleth)

Comment 3

3 years ago
Comment on attachment 8474141 [details] [diff] [review]
Patch v1

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

Thanks for fixing this!

I'm glad this has tests already.

::: chat/protocols/irc/irc.js
@@ +1235,5 @@
> +    // nickname with the returned nickname and check for truncation.
> +    if (aOldNick.length < this._sentNickname.length) {
> +      // The nick will be too long, overwrite the end of the nick instead of
> +      // appending.
> +      print("FOOBAR");

Possibly, but I suspect all these prints are superfluous to requirements ;)

::: chat/protocols/irc/test/test_tryNewNick.js
@@ +50,5 @@
> +}
> +
> +// This tests a bunch of cases near the max length by maintaining the state
> +// through a series of test nicks.
> +function test_maxLength() {

I don't think this test is being run, which is a good way to make it pass.
Attachment #8474141 - Flags: review?(aleth) → review-
(Assignee)

Comment 4

3 years ago
Created attachment 8475620 [details] [diff] [review]
Patch v2

I had to modify the tests a bit to pass, but I'm fairly certain they were mistakes in the tests though. Please make sure the tests make sense!
Attachment #8474141 - Attachment is obsolete: true
Attachment #8475620 - Flags: review?(aleth)

Comment 5

3 years ago
Comment on attachment 8475620 [details] [diff] [review]
Patch v2

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

Looks good!

::: chat/protocols/irc/test/test_tryNewNick.js
@@ +28,5 @@
>      "clo1kep01": "clo1kep02",
>      "clo1kep09": "clo1kep10",
>  
>      // Some to test the max length.
> +    //"abcdefgh9": "abcdefgh10",

With a maxlength of 9, naively this should not work. Comment? Or are these obsolete now due to the separate test?
Attachment #8475620 - Flags: review?(aleth) → review+
(Assignee)

Comment 6

3 years ago
Created attachment 8475832 [details] [diff] [review]
Patch v2.1

Correct, those tests were replaced. Forgot to remove them, sorry!
Attachment #8475620 - Attachment is obsolete: true
Attachment #8475832 - Flags: review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
(Assignee)

Comment 7

3 years ago
https://hg.mozilla.org/comm-central/rev/13eadd28a2db
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 1.6
You need to log in before you can comment on or make changes to this bug.