Bug 65835 - Need a way to selectively use hixie-76 for websocket connections depending on destination and/or origin
Summary: Need a way to selectively use hixie-76 for websocket connections depending on...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-08-07 17:59 PDT by Denis Lagno
Modified: 2011-09-06 15:40 PDT (History)
4 users (show)

See Also:


Attachments
Patch (7.36 KB, patch)
2011-08-07 18:06 PDT, Denis Lagno
no flags Details | Formatted Diff | Diff
this patch is against http://svn.webkit.org/repository/webkit/branches/chromium/835/ (4.80 KB, patch)
2011-08-10 11:02 PDT, Denis Lagno
no flags Details | Formatted Diff | Diff
Patch (7.01 KB, patch)
2011-08-11 04:54 PDT, Denis Lagno
no flags Details | Formatted Diff | Diff
Patch (7.04 KB, patch)
2011-08-11 17:01 PDT, Denis Lagno
no flags Details | Formatted Diff | Diff
Patch (7.04 KB, patch)
2011-08-11 17:05 PDT, Denis Lagno
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Denis Lagno 2011-08-07 17:59:45 PDT
Recently chromium switched to using HiBy-10 as its websocket protocol.
However there may be critical consumers still retarding at hixie-76.
Need a way to specify in settings that websocket connections destined to given urls should use hixie-76 instead of hiby-10 (as for all other destinations).
Comment 1 Denis Lagno 2011-08-07 18:06:15 PDT
Created attachment 103192 [details]
Patch
Comment 2 Yuta Kitamura 2011-08-07 19:42:06 PDT
Comment on attachment 103192 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

You need to justify the reason why you don't add a new test.

> Source/WebCore/page/Settings.cpp:810
> +    KURL tmp(url);

We usually avoid using abbreviated words. See "Names" section of <http://www.webkit.org/coding/coding-style.html>.

> Source/WebCore/page/Settings.h:566
> +        HashMap<AtomicString, bool> m_hixie76WebSocketProtocolExceptionList;

I think this line should be moved somewhere else because it divides the bit field into two sections.

Why AtomicString? I think String is better because we may compare it with any arbitrary URLs.

> Source/WebCore/websockets/WebSocketChannel.cpp:115
> +    ASSERT(m_handle); // Hixie76 vs HiBy decision is not made before connect() call.

This assumption conflicts with what I'm working on; the protocol decision must be available after WebSocketChannel construction (not after connect()), because we need to check the value of subprotocols (provided in JavaScript's WebSocket constructor) before the actual connection is attempted.

You probably need to move the "url" argument from connect() to WebSocketChannel constructor. See also: bug 65367

Typo: "HiBy" -> "HyBi"

> Source/WebKit/chromium/src/WebSettingsImpl.cpp:453
> +    UNUSED_PARAM(origin_list);

origin_list -> url
Comment 3 Alexey Proskuryakov 2011-08-08 11:06:34 PDT
Hi dilmah :-)

If these consumers are so critical that Chromium cannot ignore them, perhaps we should hardcode them in WebCore to benefit all other ports?
Comment 5 Denis Lagno 2011-08-10 11:03:46 PDT
(In reply to comment #3)
> Hi dilmah :-)
> 
> If these consumers are so critical that Chromium cannot ignore them, perhaps we should hardcode them in WebCore to benefit all other ports?

Hi Lex :)

The consumer I keep in mind when preparing this patch is experimental feature on ChromeOS which provides local websocket-to-TCP proxy.  Its address is 127.0.0.1 and hardcoding it benefits nothing to other ports.
Comment 6 Ian 'Hixie' Hickson 2011-08-10 11:35:21 PDT
Why can't we just fix the proxy?
Comment 7 Yuta Kitamura 2011-08-10 19:15:38 PDT
Comment on attachment 103506 [details]
this patch is against  http://svn.webkit.org/repository/webkit/branches/chromium/835/

Looks good to me, provided that this is temporary workaround and applies only to Chromium M14 branch. (note: I'm not a WebKit reviewer)
Comment 8 Alexey Proskuryakov 2011-08-10 22:11:42 PDT
Comment on attachment 103506 [details]
this patch is against  http://svn.webkit.org/repository/webkit/branches/chromium/835/

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

I'll make a review pass, but as this seems to not be landing on trunk, I'll leave it up to someone from Chromium land to give final review.

1. Normally, no patches can be landed without a ChangeLog: <http://www.webkit.org/coding/contributing.html>.
2. It's unclear to me if you could hardcode 127.0.0.1 for the branch.
3. I think that the patch has a wrong root (it should be above Source).

> WebCore/page/Settings.h:445
> +        void addHixie76WebSocketProtocolException(KURL url);

Should be "const KURL&", and argument should be unnamed, as it doesn't add any information.

> WebCore/page/Settings.h:447
> +        bool useHixie76WebSocketProtocol(KURL url);

Ditto.

> WebCore/page/Settings.h:575
> +        HashMap<String, bool> m_hixie76WebSocketProtocolExceptionList;

Why not HashSet?

> WebCore/page/Settings.cpp:802
> +    url.setPath(String());
> +    url.setQuery(String());
> +    url.setUser(String());
> +    url.setPass(String());
> +    url.removeFragmentIdentifier();

Perhaps you want SecurityOrigin(url).toString()? Not sure - this kind of exception is not pretty either way.
Comment 9 Denis Lagno 2011-08-11 04:54:24 PDT
Created attachment 103602 [details]
Patch
Comment 10 Denis Lagno 2011-08-11 05:04:03 PDT
(In reply to comment #8)
> (From update of attachment 103506 [details])
> 1. Normally, no patches can be landed without a ChangeLog: <http://www.webkit.org/coding/contributing.html>.

done

> 3. I think that the patch has a wrong root (it should be above Source).

done

> > WebCore/page/Settings.h:445
> > +        void addHixie76WebSocketProtocolException(KURL url);
> 
> Should be "const KURL&", and argument should be unnamed, as it doesn't add any information.

argument name fixed.  As for type: if passed as const reference then this method must copy its argument into temporary local immediately upon entering method.  It is equivalent to just passing by value -- and it is simpler and more straightforward.

> > WebCore/page/Settings.h:447
> > +        bool useHixie76WebSocketProtocol(KURL url);
> 
> Ditto.

done

> > WebCore/page/Settings.h:575
> > +        HashMap<String, bool> m_hixie76WebSocketProtocolExceptionList;
> 
> Why not HashSet?

done

> > WebCore/page/Settings.cpp:802
> > +    url.setPath(String());
> > +    url.setQuery(String());
> > +    url.setUser(String());
> > +    url.setPass(String());
> > +    url.removeFragmentIdentifier();
> 
> Perhaps you want SecurityOrigin(url).toString()? Not sure - this kind of exception is not pretty either way.

SecurityOrigin is too heavyweight for this purpose.  Also its usage here misleads reader because we do not really treat url as security origin (also I doubt if security origin can be of "ws://" scheme).
Comment 11 Denis Lagno 2011-08-11 05:23:41 PDT
(In reply to comment #6)
> Why can't we just fix the proxy?

it is fixed.  But that fix was written in a rush and it is not well-tested and it is too risky to land it on release branch.
Comment 12 Alexey Proskuryakov 2011-08-11 08:58:11 PDT
> As for type: if passed as const reference then this method must copy its argument into
> temporary local immediately upon entering method.  It is equivalent to just passing by
> value -- and it is simpler and more straightforward.

Yes, I know this argument. The costs of this approach are:
- frequent changing of function signatures when implementation changes;
- some code bloat, as caller becomes responsible for copying in the value;
- and confusing code - you start changing argument value inside function body, which can be unexpected by readers.

Expanding on the last cost, you lose an opportunity to use a speaking variable name. In this example, you just call both original and stripped url a "url", while you could name the stripped one in a way that explains what exactly remains in it, and/or what's the purpose of stripping.

For WebKit, it's pretty much universal coding style. We just pass strings and kurls by reference, and doing it differently without strong support in the form of performance numbers is frowned upon.

Not a big deal for temporary branch only code. CC'ing Adam, who might suggest a better alternative for stripping an URL like this, and would be a good reviewer for this patch in general.
Comment 13 Adam Barth 2011-08-11 09:55:54 PDT
Comment on attachment 103602 [details]
Patch

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

> Source/WebCore/page/Settings.cpp:797
> +void Settings::addHixie76WebSocketProtocolException(KURL url)

Yeah, we usually pass KURLs by const reference.  That makes it easier to read the code because it's consistent.
Comment 14 Denis Lagno 2011-08-11 17:01:14 PDT
Created attachment 103707 [details]
Patch
Comment 15 Denis Lagno 2011-08-11 17:05:01 PDT
Created attachment 103708 [details]
Patch
Comment 16 Denis Lagno 2011-08-11 17:05:43 PDT
> > Source/WebCore/page/Settings.cpp:797
> > +void Settings::addHixie76WebSocketProtocolException(KURL url)
> 
> Yeah, we usually pass KURLs by const reference.  That makes it easier to read the code because it's consistent.

done
Comment 17 Denis Lagno 2011-08-12 15:19:51 PDT
so what's the resolution on this?  Can it be committed?

I am not a webkit committer, can Adam or Kitamura land it on http://svn.webkit.org/repository/webkit/branches/chromium/835/ ?
Comment 18 Adam Barth 2011-08-12 15:28:17 PDT
> so what's the resolution on this?  Can it be committed?

To confirm my understanding, you only want this landed on the chromium/835 branch?  I can do that.
Comment 19 Denis Lagno 2011-08-12 15:40:04 PDT
> To confirm my understanding, you only want this landed on the chromium/835 branch?  I can do that.

yes.  It is for M14 branch
Comment 20 Adam Barth 2011-08-12 15:59:24 PDT
Done: http://trac.webkit.org/changeset/93008
Comment 21 Eric Seidel (no email) 2011-09-06 15:40:28 PDT
Comment on attachment 103708 [details]
Patch

Cleared review? from attachment 103708 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).