Bug 54811 - REGRESSION(r78383): Failure to connect on websocketstest.com
Summary: REGRESSION(r78383): Failure to connect on websocketstest.com
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.6
: P1 Normal
Assignee: Yuta Kitamura
URL: http://websocketstest.com/
Keywords: InRadar, Regression
Depends on:
Reported: 2011-02-19 11:44 PST by Alexey Proskuryakov
Modified: 2011-03-01 22:06 PST (History)
4 users (show)

See Also:

Patch (3.48 KB, patch)
2011-02-28 20:23 PST, Yuta Kitamura
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 2011-02-19 11:44:47 PST
According <http://websocketstest.com/>, connecting on port 80 is broken on ToT. It works with Safari 5.0.3
Comment 1 Alexey Proskuryakov 2011-02-19 11:45:27 PST
Comment 2 Yuta Kitamura 2011-02-25 05:43:13 PST
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...
Comment 3 Yuta Kitamura 2011-02-28 02:33:00 PST
I did bisect this regression and found that the revision caused this regression is r78383.

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.
Comment 4 Yuta Kitamura 2011-02-28 20:23:11 PST
Created attachment 84183 [details]
Comment 5 Yuta Kitamura 2011-02-28 21:19:38 PST
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.
Comment 6 Fumitoshi Ukai 2011-02-28 21:53:54 PST
Comment on attachment 84183 [details]

Comment 7 Alexey Proskuryakov 2011-02-28 22:37:08 PST
Another possible fix would be to remove WebSocket ports from KURL default list. Eric, what do you think?
Comment 8 Eric Seidel (no email) 2011-02-28 23:51:24 PST
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.
Comment 10 Yuta Kitamura 2011-03-01 02:47:00 PST
KURL (after r78383) and KURLGoogle are aligned in terms of setPort() behavior. So, I think fixing SocketStreamHandleCFNet is sufficient...
Comment 11 Darin Adler 2011-03-01 09:57:07 PST
Comment on attachment 84183 [details]

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.
Comment 12 Yuta Kitamura 2011-03-01 22:03:25 PST
Thanks, I will rename the function and commit manually.
Comment 13 Yuta Kitamura 2011-03-01 22:06:25 PST
Committed r80092: <http://trac.webkit.org/changeset/80092>