Closed Bug 954732 Opened 8 years ago Closed 8 years ago

Server message when closing tab of an already parted MUC

Categories

(Chat Core :: IRC, defect)

x86
Windows Vista
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: benediktp, Assigned: clokep)

References

Details

Attachments

(1 file, 3 obsolete files)

*** Original post on bio 1300 at 2012-02-29 10:00:00 UTC ***

STR:
1. Join a channel.
2. /part the channel
3. Close the tab.


Result: I received a message "10:26:56 - concrete.mozilla.org: There is no channel: Mic."

Expected result: no message, the tab should just go away.



Here's the output on the error console, with logging level = 2 (blank lines mean that a new error message starts, time order is from earliest on top to latest at the end):

Sending:
JOIN :#mictest

onTransportStatus(STATUS_SENDING_TO)

onTransportStatus(STATUS_RECEIVING_FROM)

:Mic!Instantbir@<bla..blub> JOIN :#mictest

:concrete.mozilla.org MODE #mictest +n 

:concrete.mozilla.org 353 Mic = #mictest :@Mic 

:concrete.mozilla.org 366 Mic #mictest :End of /NAMES list.

Sending:
PART :#mictest

onTransportStatus(STATUS_SENDING_TO)

onTransportStatus(STATUS_RECEIVING_FROM)

:Mic!Instantbir@<bla..blub> PART #mictest

Sending:
PART :#mictest

onTransportStatus(STATUS_SENDING_TO)

onTransportStatus(STATUS_RECEIVING_FROM)

:concrete.mozilla.org 403 Mic #mictest :No such channel
Blocks: 953944
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1300 as attmnt 1210 at 2012-03-01 02:16:00 UTC ***

This checks now if we're in the room before parting! (It also makes a small whitespace change and sets the socket to log using DEBUG instead of LOG.)
Attachment #8352961 - Flags: review?(florian)
Assignee: nobody → clokep
Status: NEW → ASSIGNED
Attached patch Patch v2 (obsolete) — Splinter Review
*** Original post on bio 1300 as attmnt 1212 at 2012-03-01 11:45:00 UTC ***

flo pointed out on IRC that the log change would also affect how messages are logged, not just the socket status. We don't want this and I'll need to do more extensive changes to the socket code.

A revised patch is attached without that change.
Attachment #8352963 - Flags: review?(florian)
Comment on attachment 8352961 [details] [diff] [review]
Patch

*** Original change on bio 1300 attmnt 1210 at 2012-03-01 11:45:58 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352961 - Attachment is obsolete: true
Attachment #8352961 - Flags: review?(florian)
*** Original post on bio 1300 at 2012-03-01 12:39:31 UTC ***

Comment on attachment 8352963 [details] [diff] [review] (bio-attmnt 1212)
Patch v2

>diff --git a/chat/protocols/irc/irc.js b/chat/protocols/irc/irc.js
>--- a/chat/protocols/irc/irc.js
>+++ b/chat/protocols/irc/irc.js
>@@ -113,17 +113,17 @@ ircChannel.prototype = {
>     if (msg)
>       params.push(msg);
> 
>     this._account.sendMessage("PART", params);
>   },
> 
>   unInit: function() {
>     // Tell the server about the part if connected.
>-    if (this._account.connected)
>+    if (this._account.connected && !this.left)
>       this.part();

Shouldn't this be done in the close method instead of the unInit method?

The comment at http://lxr.instantbird.org/instantbird/source/chat/components/public/prplIConversation.idl#82 says that unInit is also called during shutdown, it may be the reason why we part channels instead of sending quit when disconnected because of shutdown.

(By the way, that comment is obsolete, as purpleCoreService most likely isn't relevant any more now that bug 954193 (bio 759) is fixed.)
*** Original post on bio 1300 at 2012-03-01 13:17:27 UTC ***

(In reply to comment #3)
> Comment on attachment 8352963 [details] [diff] [review] (bio-attmnt 1212) [details]
> Patch v2
> 
> >diff --git a/chat/protocols/irc/irc.js b/chat/protocols/irc/irc.js
> >--- a/chat/protocols/irc/irc.js
> >+++ b/chat/protocols/irc/irc.js
> >@@ -113,17 +113,17 @@ ircChannel.prototype = {
> >     if (msg)
> >       params.push(msg);
> > 
> >     this._account.sendMessage("PART", params);
> >   },
> > 
> >   unInit: function() {
> >     // Tell the server about the part if connected.
> >-    if (this._account.connected)
> >+    if (this._account.connected && !this.left)
> >       this.part();
> 
> Shouldn't this be done in the close method instead of the unInit method?
It probably should be. The close() method is called when the conversation is closed from the UI? The comment just says /* Close the conversation */ which doesn't really give any extra information.

> The comment at
> http://lxr.instantbird.org/instantbird/source/chat/components/public/prplIConversation.idl#82
> says that unInit is also called during shutdown, it may be the reason why we
> part channels instead of sending quit when disconnected because of shutdown.
That is probably the problem! I'll put up a new patch later that handles this better.

> (By the way, that comment is obsolete, as purpleCoreService most likely isn't
> relevant any more now that bug 954193 (bio 759) is fixed.)
We should fix that! :)
*** Original post on bio 1300 at 2012-03-01 14:58:25 UTC ***

(In reply to comment #4)
> The close() method is called when the conversation is
> closed from the UI?
I think so.

> The comment just says /* Close the conversation */ which
> doesn't really give any extra information.

Fix it? :)
Attached patch Patch v3 (obsolete) — Splinter Review
*** Original post on bio 1300 as attmnt 1213 at 2012-03-01 22:39:00 UTC ***

This replaces the unInit function with the close function and fixes the comment about the close method.
Attachment #8352964 - Flags: review?(florian)
Comment on attachment 8352963 [details] [diff] [review]
Patch v2

*** Original change on bio 1300 attmnt 1212 at 2012-03-01 22:39:47 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352963 - Attachment is obsolete: true
Attachment #8352963 - Flags: review?(florian)
Attached patch Patch v4Splinter Review
*** Original post on bio 1300 as attmnt 1214 at 2012-03-01 22:50:00 UTC ***

This keeps the unInit method and also removes an extra debug statement that I found. :(
Attachment #8352965 - Flags: review?(florian)
Comment on attachment 8352964 [details] [diff] [review]
Patch v3

*** Original change on bio 1300 attmnt 1213 at 2012-03-01 22:50:41 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352964 - Attachment is obsolete: true
Attachment #8352964 - Flags: review?(florian)
*** Original post on bio 1300 at 2012-03-02 00:17:38 UTC ***

A slightly modified patch was checked in as http://hg.instantbird.org/instantbird/rev/c525086f3a9c

It calls the GenericConvChatPrototype close and unInit methods.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.2
Comment on attachment 8352965 [details] [diff] [review]
Patch v4

*** Original change on bio 1300 attmnt 1214 at 2012-03-02 15:39:55 UTC ***

Updated patch given over IRC and checked in.
Attachment #8352965 - Flags: review?(florian) → review-
You need to log in before you can comment on or make changes to this bug.