Bug 65835 – Need a way to selectively use hixie-76 for websocket connections depending on destination and/or origin

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Denis Lagno]

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).

[Denis Lagno]

Created attachment 103192 [details]
Patch

[Yuta Kitamura]

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

[Alexey Proskuryakov]

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?

[Denis Lagno]

Created attachment 103506 [details]
this patch is against  http://svn.webkit.org/repository/webkit/branches/chromium/835/

This patch is against branch http://svn.webkit.org/repository/webkit/branches/chromium/835/

[Denis Lagno]

(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.

[Ian 'Hixie' Hickson]

Why can't we just fix the proxy?

[Yuta Kitamura]

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)

[Alexey Proskuryakov]

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.

[Denis Lagno]

Created attachment 103602 [details]
Patch

[Denis Lagno]

(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).

[Denis Lagno]

(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.

[Alexey Proskuryakov]

> 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.

[Adam Barth]

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.

[Denis Lagno]

Created attachment 103707 [details]
Patch

[Denis Lagno]

Created attachment 103708 [details]
Patch

[Denis Lagno]

> > 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

[Denis Lagno]

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/ ?

[Adam Barth]

> 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.

[Denis Lagno]

> To confirm my understanding, you only want this landed on the chromium/835 branch?  I can do that.

yes.  It is for M14 branch

[Adam Barth]

Done: http://trac.webkit.org/changeset/93008

[Eric Seidel (no email)]

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).