REGRESSION(r78383): Failure to connect on websocketstest.com

[Alexey Proskuryakov]

According <http://websocketstest.com/>, connecting on port 80 is broken on ToT. It works with Safari 5.0.3

[Alexey Proskuryakov]

<rdar://problem/9027701>

[Yuta Kitamura]

It seems like CFSocket reports EADDRNOTAVAIL. Unfortunately there was no useful infomation left in the stack and I could not see what was happening under CF.

I will try bisect...

[Yuta Kitamura]

I did bisect this regression and found that the revision caused this regression is r78383.
http://trac.webkit.org/changeset/78383

SocketStreamHandle for CF (WebCore/platform/network/SocketStreamHandleCFNet.cpp) calls KURL::setPort() in its constructor (line 73) and tries to make sure the port number (":80" or ":443") is always included in the host part.

After r78383, KURL::setPort() no longer appends the port part if you specify the default port number for a given scheme. That's why SocketStreamHandle has regressed.

I'm not sure if the current usage of setPort() in SocketStreamHandle is valid or not. I'd like to hear Eric and Adam's opinion about this.

If SocketStreamHandle needs a fix, I think I can do it.

[Yuta Kitamura]

Created attachment 84183 [details]
Patch

[Yuta Kitamura]

This patch is an attempt to fix SocketStreamHandleCFNet so that it does not rely on KURL::setPort() appending ":port" part.

This regression makes us unable to connect to almost all WebSocket servers (listening at the default port), which sounds rather serious to me.

[Fumitoshi Ukai]

Comment on attachment 84183 [details]
Patch

LGTM

[Alexey Proskuryakov]

Another possible fix would be to remove WebSocket ports from KURL default list. Eric, what do you think?

[Eric Seidel (no email)]

What does GKURL do in this case? I'm not familiar enough with KURL::port() but I wonder if it shouldn't know how to return the defualt ports.

[Yuta Kitamura]

Seems like GoogleURL also strips ":port" part if |port| is equal to the default port for the URL scheme.

KURLGoogle::setPort() will eventually reach the following code:

http://codesearch.google.com/codesearch/p?hl=ja#OAMlx_jo-ck/src/googleurl/src/url_canon_etc.cc&d=3&l=226&gs=cpp:url_canon::::DoPort(const%2520%230%2520*,%2520const%2520url_parse::Component%2520&,%2520int,%2520url_canon::CanonOutputT%253Cchar%253E%2520*,%2520url_parse::Component%2520*)@chrome/trunk/src/googleurl/src/url_canon_etc.cc%257Cdef&gsn=DoPort

[Yuta Kitamura]

KURL (after r78383) and KURLGoogle are aligned in terms of setPort() behavior. So, I think fixing SocketStreamHandleCFNet is sufficient...

[Darin Adler]

Comment on attachment 84183 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=84183&action=review

I’m going to say r=me, but I’d prefer that the function name be changed.

> Source/WebCore/platform/network/cf/SocketStreamHandle.h:75
> +    unsigned short getPort() const;

WebKit style naming for these functions does not include the word “get” -- this is mentioned in the Names section of <http://www.webkit.org/coding/coding-style.html>.

This function should just be named port. The patch is otherwise fine.

[Yuta Kitamura]

Thanks, I will rename the function and commit manually.

[Yuta Kitamura]

Committed r80092: <http://trac.webkit.org/changeset/80092>