284149 - A lack of secure servers on a network when using ircs:// urls should give a better error message

Status: RESOLVED FIXED

(Other Applications :: ChatZilla, defect)

Description

[:Gijs (he/him)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7.6) Gecko/20050223 Firefox/1.0.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7.6) Gecko/20050223 Firefox/1.0.1

When connecting to a network using /attach ircs://foonet/, while the network
does not have any servers capable of making a secure connection returns
"Connection attempts exhausted, giving up", which is non-descriptive of the
actual problem (there's not an ssl-enabled server in the network, as far as
ChatZilla knows)

Reproducible: Always

Steps to Reproduce:
1. /attach ircs://moznet/
2. Get a weird message about connection attempts being exhausted, without any
notice of any connection attempts being actually tried / made.
3. Get confused

Actual Results:  
I did not know what happened to cause the connection to fail, and had to resort
to pulling apart the javascript code behind ChatZilla to find out ;-).

Expected Results:  
It should have displayed a message that the network did not have any servers
capable of making a secure connection.

Comment 1

[:Gijs (he/him)]

Attachment: Patch v1

Fixes:

- "DISCONNECT" message type. Uses that instead of "INFO" for when the user
disconnects (instead of a non-intended disconnect). 
- Updated all motifs in mozilla/extensions/irc/xul/skin to style "DISCONNECT"
in the same way they style "ERROR", to make it stand out as it used to (since
originally the message type was actually "ERROR").
- Adds the secure version of the irc.mozilla.org server to the moznet irc
network, so ircs://moznet will actually work :-). (also fixes a 80-character a
line style break in the definition of the networks)

When trying to connect to a network using a secure connection:
- A better error message is used when ChatZilla doesn't know any servers in the
network that support a secure connection
- "Connection Attempts" to non-secure servers are no longer counted as real
connection attempts, so when ChatZilla uses up all 5 connection attempts it
actually tried to connect to secure servers 5 times, not just to all servers
(of which some may NOT have supported a secure connection, thus failing
instantly).

This should solve all problems previously reported (see comment 15 on bug
281706)

Requesting review from rginda.

Comment 2

[Robert Ginda]

Comment on attachment 175945 [details] [diff] [review]
Patch v1

I'd suggest factoring out the "has secure server" check from
CIRCNetwork.prototype.connect into a new hasSecureServer() method.  Someone
will probably find a reason to use it later.

This is a bit awkward:
  A secure connection could not be established 
  because there are no known secure servers to
  connect to.

How about:
  The network %S has no secure servers defined.

In the properties file:
  msg.rsp.disc	= [QUIT]

"disc" is a bit ambiguous, spelling it out isn't so bad.


Other than that it looks good to me.

Comment 3

[:Gijs (he/him)]

Attachment: Patch incorporating the changes requested

I also fixed a strict warning now thrown for net_connect, which did not always
return a value :-).

Comment 4

[Robert Ginda]

Comment on attachment 176139 [details] [diff] [review]
Patch incorporating the changes requested

Ah, now it's easier to see that you don't need to finish the loop after you
find the first secure server.  You can safely return true the first time you
find one that is secure.  You can even replace the loacal "hasSecure" with a
constant "return false" at the end.

Comment 5

[:Gijs (he/him)]

Attachment: patch v3 (Bail out upon finding a secure server)

I'll just keep trying ;).

Done the change you wanted, I believe :).

Comment 6

[Robert Ginda]

Comment on attachment 176169 [details] [diff] [review]
patch v3 (Bail out upon finding a secure server)

r=rginda

Comment 7

[Asa Dotzler [:asa]]

Comment on attachment 176169 [details] [diff] [review]
patch v3 (Bail out upon finding a secure server)

a=asa

Comment 8

[James Ross]

Checked in --> FIXED.

Automated builds at http://twpol.dyndns.org/mozilla/chatzilla/nightly/ will
include this shortly.