792402 - Cannot "close" a channel tab on a disconnected server

Status: RESOLVED FIXED

(Other Applications :: ChatZilla, defect)

Description

[Tony Mechelynck [:tonymec]]

Mozilla/5.0 (X11; Linux x86_64; rv:18.0) Gecko/18.0 Firefox/18.0 SeaMonkey/2.15a1 ID:20120919003007 c-c:f97bbaa649d5 m-c:80499f04e875
ChatZilla 0.9.88.2

Reproducible: Always

Steps to reproduce:
1. Start ChatZilla, connect to one or more servers, join one or more channels.
2. /disconnect from a server where you have joined at least one channel
3. Right-click a channel tab on the disconnected server, then "Close tab"

Actual result:
The tab doesn't close, but gives the following message:
[ERROR] dispatchCommand: Not connected. @ <chrome://chatzilla/content/commands.js> 747

Expected result:
Tab should close with no error.

Workaround:
"Hide" the tab instead of "closing" it.

Additional info:
* /delete-view (in a channel tab on a disconnected server) gives the same erroneous result as "Right-click → Close tab".
* No problem with the server tab itself, nor with a query tab.

Comment 1

[Steve Lis]

I can confirm what Tony is seeing; however, I experience this issue with the latest ChatZilla version only (0.9.89) and not with 0.9.88.2).

UserAgent that I *do* see the bug on:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:16.0) Gecko/20121026 Firefox/16.0 SeaMonkey/2.13.2
ChatZilla 0.9.89

UserAgent that I do *not* see the bug on:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:16.0) Gecko/20121026 Firefox/16.0 SeaMonkey/2.13.2
ChatZilla 0.9.88.2

I also had other people on an IRC network that I frequent test it out, and they experienced the bug in ChatZilla 0.9.89 running Firefox in both Windows 7 and GhostBSD.

Did you actually experience the bug in ChatZilla 0.9.88.2? I'm hoping that's a typo or copy-paste error because if you really did have the bug in that version, that would complicate narrowing down the cause of the bug. I have not experienced the bug in any ChatZilla versions other than the latest (0.9.89), and I've tested it in both Windows and OS X.

In my opinion, I think of this bug as being more than "minor." I've been running into this bug almost on a daily basis, and it's really annoying to constantly have to work around. I may just have to downgrade ChatZilla until it can be fixed.

Would it be a fair assumption that the issue lies in commands.js? I'm going to try to compare that file in 0.9.89 to the same file in 0.9.88.2. Hopefully there will be something obvious like a syntax error that's causing this bug, which would make for an easy fix.

Comment 2

[Jesse Maloney (jtmal0723 [at] gmail for email)]

Attachment: visual demonstration of bug Windows 7 ChatZilla 0.9.89

This is confirmed evident on Windows 7 Ultimate running ChatZilla 0.9.89.

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0 ID:20121113065533

Comment 3

[Zach Land]

Attachment: chatzilla bug confirmation

I am also able to confirm this issue with Chatzilla on version 0.9.89, however I use Chatzilla with XULRunner.

Comment 4

[Steve Lis]

This bug is caused by the change seen in attachment 657505 [details] [diff] [review] in bug 682682. In this change, which was checked in...

["leave",             cmdLeave,           CMD_NEED_CHAN | CMD_CONSOLE],

was changed to...

["leave",             cmdLeave,            CMD_NEED_SRV | CMD_CONSOLE],

I opened up the commands.js file that came with ChatZilla 0.9.89 and changed this single line back to CMD_NEED_CHAN, and sure enough, the bug reported here was fixed, but it eliminates the functionality added to ChatZilla in bug 682682.

As it turns out, having CMD_NEED_SRV associated with cmdLeave means that when you try to close a channel tab *after* disconnecting from the server, cmdLeave is executed as long as you didn't leave the channel prior to disconnecting, and due to the fact that CMD_NEED_SRV is meant for commands that can only be run while connected to the server, ChatZilla throws a fit when you try to close a tab while not connected to the server.

As I mentioned on IRC in #chatzilla, I think bug 682682 is quite valuable, but the way it was implemented caused a regression of previously working functionality. Here's hoping that the people involved with that bug can figure out a different way to implement that functionality so that everything can work correctly. The short-term fix, it would seem, would be to roll back bug 682682 to eliminate this regression, but in the long-term, we need to figure out a better way to get bug 682682 AND this bug fixed at the same time. I'm going to head over to that bug page and post some ideas of different ways to get both of these bugs fixed at once, but of course, the experts on ChatZilla's code know best, so their ideas on this will prove much more valuable than mine. :)

Due to this regression caused by bug 682682, I would think it best to roll back and REOPEN that bug so that this one can be automatically fixed; meanwhile, everyone involved with that bug could get back together and figure out a new way to implement it.

Comment 5

[Donald Sarratt]

Attachment: Fix for bug 792402

It appears that the CMD_NEED_CHAN flag isn't required anymore, since the "if (!e.channelName)" check on line 2394 already safeguards against running the command outside of a channel. I've removed the 'CMD_NEED_' flag entirely and it seems to fix this bug without causing any obvious issues, or affecting the functionality added in bug 682682.

Comment 6

[:Gijs (he/him)]

(In reply to Donald Sarratt from comment #5)
> Created attachment 682853 [details] [diff] [review]
> Fix for bug 792402
> 
> It appears that the CMD_NEED_CHAN flag isn't required anymore, since the "if
> (!e.channelName)" check on line 2394 already safeguards against running the
> command outside of a channel. I've removed the 'CMD_NEED_' flag entirely and
> it seems to fix this bug without causing any obvious issues, or affecting
> the functionality added in bug 682682.

For my sanity, can you or James explain why this shouldn't be CMD_NEED_NET ?

Comment 7

[James Ross]

(In reply to Gijs Kruitbosch from comment #6)
That would work too, and may be better, but it's not necessary as the first thing cmdLeave does is check for e.server (which implies e.network AFAIK).

I'll take it both ways.

Comment 8

[Steve Lis]

Actually, it would be better to use CMD_NEED_NET because if you leave it blank and then try to use the command /leave in the *client* tab or something like it, you get the ugly raw javascript error message of

[ERROR]	msg.err.improper.view

Whereas if you use the CMD_NEED_NET line, you get the pretty error message of

[ERROR]	Command “leave” must be run in the context of a network.

So unless there's a really, really good reason to leave it blank, we should definitely use CMD_NEED_NET instead.

Comment 9

[Donald Sarratt]

(In reply to Steve Lis from comment #8)
> Actually, it would be better to use CMD_NEED_NET because if you leave it
> blank and then try to use the command /leave in the *client* tab or
> something like it, you get the ugly raw javascript error message of
> 
> [ERROR]	msg.err.improper.view
> 
> Whereas if you use the CMD_NEED_NET line, you get the pretty error message of
> 
> [ERROR]	Command “leave” must be run in the context of a network.
> 
> So unless there's a really, really good reason to leave it blank, we should
> definitely use CMD_NEED_NET instead.

Makes sense to me. I vote for CMD_NEED_NET in that case.

Comment 10

[Donald Sarratt]

Attachment: Updated patch for bug 792402, doesn't cause an error when trying to /part from the client window.

Comment 11

[Donald Sarratt]

(In reply to Steve Lis from comment #8)
The 

 [ERROR]	msg.err.improper.view

message is caused by a funny method call on line 2388:

        display(MSG_ERR_IMPROPER_VIEW, MT_ERROR);

Every other error call I've seen looks like this:

display(getMsg(MSG_ERR_IMPROPER_VIEW, e.command.name), MT_ERROR);

And using that format produces the following message instead:

 [ERROR]	“leave” cannot be used from this view.

Definitely an improvement. Though as mentioned, if we have CMD_NEED_NET I'm not even sure it's possible for e.server to be false when /leave is called, so we could probably remove the whole "if (!e.server)" block.

Comment 12

[Donald Sarratt]

Attachment: Fixes bug 792402 and improves the error message given on line 2388

Well it couldn't hurt to leave the extra check in there. This patch should fix the original bug, and also make sure that if you do get the error message mentioned by Steve it's at least formatted properly.

Comment 13

[James Ross]

Comment on attachment 687363 [details] [diff] [review]
Fixes bug 792402 and improves the error message given on line 2388

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

Agreed, looks good.

Comment 14

[James Ross]

http://hg.mozilla.org/chatzilla/rev/df29297d13cc